You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/03/27 23:27:08 UTC
Review Request 66318: Made agent resource provider config API calls
idempotent.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
Bugs: MESOS-8742
https://issues.apache.org/jira/browse/MESOS-8742
Repository: mesos
Description
-------
This patch adds descriptions to declare the following agent API calls
idempotent:
- `ADD_RESOURCE_PROVIDER_CONFIG`
- `UPDATE_RESOURCE_PROVIDER_CONFIG`
- `REMOVE_RESOURCE_PROVIDER_CONFIG`
Diffs
-----
include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
Diff: https://reviews.apache.org/r/66318/diff/1/
Testing
-------
N/A
The actual implementation will be in the next patch.
Thanks,
Chun-Hung Hsiao
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/#review200151
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On March 28, 2018, 3:22 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> -----------------------------------------------------------
>
> (Updated March 28, 2018, 3:22 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
>
>
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds descriptions to declare the following agent API calls
> idempotent:
> - `ADD_RESOURCE_PROVIDER_CONFIG`
> - `UPDATE_RESOURCE_PROVIDER_CONFIG`
> - `REMOVE_RESOURCE_PROVIDER_CONFIG`
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
> include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
>
>
> Diff: https://reviews.apache.org/r/66318/diff/2/
>
>
> Testing
> -------
>
> N/A
>
> The actual implementation will be in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/
-----------------------------------------------------------
(Updated March 28, 2018, 3:22 a.m.)
Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
Changes
-------
Addressed James' comments.
Bugs: MESOS-8742
https://issues.apache.org/jira/browse/MESOS-8742
Repository: mesos
Description
-------
This patch adds descriptions to declare the following agent API calls
idempotent:
- `ADD_RESOURCE_PROVIDER_CONFIG`
- `UPDATE_RESOURCE_PROVIDER_CONFIG`
- `REMOVE_RESOURCE_PROVIDER_CONFIG`
Diffs (updated)
-----
include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
Diff: https://reviews.apache.org/r/66318/diff/2/
Changes: https://reviews.apache.org/r/66318/diff/1-2/
Testing
-------
N/A
The actual implementation will be in the next patch.
Thanks,
Chun-Hung Hsiao
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/#review200081
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66318]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 27, 2018, 4:27 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> -----------------------------------------------------------
>
> (Updated March 27, 2018, 4:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
>
>
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds descriptions to declare the following agent API calls
> idempotent:
> - `ADD_RESOURCE_PROVIDER_CONFIG`
> - `UPDATE_RESOURCE_PROVIDER_CONFIG`
> - `REMOVE_RESOURCE_PROVIDER_CONFIG`
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
> include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
>
>
> Diff: https://reviews.apache.org/r/66318/diff/1/
>
>
> Testing
> -------
>
> N/A
>
> The actual implementation will be in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by Jie Yu <yu...@gmail.com>.
> On March 28, 2018, 2:36 a.m., James DeFelice wrote:
> > include/mesos/agent/agent.proto
> > Line 332 (original), 338 (patched)
> > <https://reviews.apache.org/r/66318/diff/1/?file=1989161#file1989161line338>
> >
> > Maybe return 409 instead of 404 here, if the caller tries to update a config that does not exist? Because, you know, 404s can be misleading given the convo we had earlier...
>
> Chun-Hung Hsiao wrote:
> Yeah I was aware of this issue and discussed with Jie but we still thought 404 is semantically more correct here. Will probably raise a general question related to this on the next API WG.
I think 409 is not the right code for this case. The best code is 404. I think we probably need to fix libprocess to return 50x during initialization.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/#review200089
-----------------------------------------------------------
On March 28, 2018, 3:22 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> -----------------------------------------------------------
>
> (Updated March 28, 2018, 3:22 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
>
>
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds descriptions to declare the following agent API calls
> idempotent:
> - `ADD_RESOURCE_PROVIDER_CONFIG`
> - `UPDATE_RESOURCE_PROVIDER_CONFIG`
> - `REMOVE_RESOURCE_PROVIDER_CONFIG`
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
> include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
>
>
> Diff: https://reviews.apache.org/r/66318/diff/2/
>
>
> Testing
> -------
>
> N/A
>
> The actual implementation will be in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On March 28, 2018, 2:36 a.m., James DeFelice wrote:
> > include/mesos/agent/agent.proto
> > Line 327 (original), 331 (patched)
> > <https://reviews.apache.org/r/66318/diff/1/?file=1989161#file1989161line331>
> >
> > "Note that only..."
> >
> > I don't think this statement is true. I have e2e tests that add a provider and then update it. The provider DID NOT exist at agent startup.
You are right. I described the statement in a wrong way. It should be:
"Note that if a config file is placed into the resource provider config directory out-of-band after the starts up, it will not be checked against this call."
> On March 28, 2018, 2:36 a.m., James DeFelice wrote:
> > include/mesos/agent/agent.proto
> > Line 332 (original), 338 (patched)
> > <https://reviews.apache.org/r/66318/diff/1/?file=1989161#file1989161line338>
> >
> > Maybe return 409 instead of 404 here, if the caller tries to update a config that does not exist? Because, you know, 404s can be misleading given the convo we had earlier...
Yeah I was aware of this issue and discussed with Jie but we still thought 404 is semantically more correct here. Will probably raise a general question related to this on the next API WG.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/#review200089
-----------------------------------------------------------
On March 27, 2018, 11:27 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> -----------------------------------------------------------
>
> (Updated March 27, 2018, 11:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
>
>
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds descriptions to declare the following agent API calls
> idempotent:
> - `ADD_RESOURCE_PROVIDER_CONFIG`
> - `UPDATE_RESOURCE_PROVIDER_CONFIG`
> - `REMOVE_RESOURCE_PROVIDER_CONFIG`
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
> include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
>
>
> Diff: https://reviews.apache.org/r/66318/diff/1/
>
>
> Testing
> -------
>
> N/A
>
> The actual implementation will be in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/#review200089
-----------------------------------------------------------
include/mesos/agent/agent.proto
Line 321 (original), 325 (patched)
<https://reviews.apache.org/r/66318/#comment280736>
nit: there are two spaces between "field" and "will"
include/mesos/agent/agent.proto
Line 327 (original), 331 (patched)
<https://reviews.apache.org/r/66318/#comment280737>
"Note that only..."
I don't think this statement is true. I have e2e tests that add a provider and then update it. The provider DID NOT exist at agent startup.
include/mesos/agent/agent.proto
Line 332 (original), 338 (patched)
<https://reviews.apache.org/r/66318/#comment280738>
Maybe return 409 instead of 404 here, if the caller tries to update a config that does not exist? Because, you know, 404s can be misleading given the convo we had earlier...
include/mesos/agent/agent.proto
Lines 351 (patched)
<https://reviews.apache.org/r/66318/#comment280739>
"Note that only..."
again, I have e2e tests that prove otherwise
- James DeFelice
On March 27, 2018, 11:27 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> -----------------------------------------------------------
>
> (Updated March 27, 2018, 11:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
>
>
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds descriptions to declare the following agent API calls
> idempotent:
> - `ADD_RESOURCE_PROVIDER_CONFIG`
> - `UPDATE_RESOURCE_PROVIDER_CONFIG`
> - `REMOVE_RESOURCE_PROVIDER_CONFIG`
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
> include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
>
>
> Diff: https://reviews.apache.org/r/66318/diff/1/
>
>
> Testing
> -------
>
> N/A
>
> The actual implementation will be in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 66318: Made agent resource provider config API
calls idempotent.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66318/#review200080
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['66318']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66318
Relevant logs:
- [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66318/logs/mesos-tests-stdout.log):
```
[ OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (106 ms)
[----------] 9 tests from Endpoint/SlaveEndpointTest (1034 ms total)
[----------] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[ OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (31 ms)
[ RUN ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[ OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (49 ms)
[----------] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (83 ms total)
[----------] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[ OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (744 ms)
[----------] 1 test from IsolationFlag/CpuIsolatorTest (771 ms total)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[ OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (730 ms)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest (755 ms total)
[----------] Global test environment tear-down
[==========] 949 tests from 94 test cases ran. (424151 ms total)
[ PASSED ] 948 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] CommandExecutorCheckTest.CommandCheckTimeout
1 FAILED TEST
YOU HAVE 214 DISABLED TESTS
```
- [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66318/logs/mesos-tests-stderr.log):
```
I0328 00:52:22.522769 10496 master.cpp:10446] Updating the state of task 38ba006b-fb82-47a8-8792-0fba6f99ae4a of framework 2982a78e-810d-4839-8d2e-ee618792c50a-0000 (latest state: TASK_KILLED, status update state: TASK_KILLED)
I0328 00:52:22.522769 12672 slave.cpp:3873] Shutting down framework 2982a78e-810d-4839-8d2e-ee618792c50a-0000
I0328 00:52:22.522769 12672 slave.cpp:6566] Shutting down executor '38ba006b-fb82-47a8-8792-0fba6f99ae4a' of framework 2982a78e-810d-4839-8d2e-ee618792c50a-0000 at executor(1)@10.3.1.8:53140
I0328 00:52:22.523767 14360 slave.cpp:919] Agent terminating
W0328 00:52:22.523767 14360 slave.cpp:3869] Ignoring shutdown framework 2982a78e-810d-4839-8d2e-ee618792c50a-I0328 00:52:22.365787 2380 exec.cpp:162] Version: 1.6.0
I0328 00:52:22.390769 8796 exec.cpp:236] Executor registered on agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0
I0328 00:52:22.394785 5828 executor.cpp:176] Received SUBSCRIBED event
I0328 00:52:22.400787 5828 executor.cpp:180] Subscribed executor on winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0328 00:52:22.400787 5828 executor.cpp:176] Received LAUNCH event
I0328 00:52:22.404789 5828 executor.cpp:648] Starting task 38ba006b-fb82-47a8-8792-0fba6f99ae4a
I0328 00:52:22.484781 5828 executor.cpp:483] Running 'D:\DCOS\mesos\src\mesos-containerizer.exe launch <POSSIBLY-SENSITIVE-DATA>'
I0328 00:52:22.496755 5828 executor.cpp:661] Forked command at 120
I0328 00:52:22.523767 13756 exec.cpp:445] Executor asked to shutdown
I0328 00:52:22.524767 14148 executor.cpp:176] Received SHUTDOWN event
I0328 00:52:22.524767 14148 executor.cpp:758] Shutting down
I0328 00:52:22.524767 14148 executor.cpp:868] Sending SIGTERM to process tree at pid 120000 because it is terminating
I0328 00:52:22.524767 10496 master.cpp:10545] Removing task 38ba006b-fb82-47a8-8792-0fba6f99ae4a with resources cpus(allocated: *):4; mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: *):[31000-32000] of framework 2982a78e-810d-4839-8d2e-ee618792c50a-0000 on agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 (winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 00:52:22.527761 10496 master.cpp:1295] Agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 (winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0328 00:52:22.527761 10496 master.cpp:3283] Disconnecting agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 (winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 00:52:22.527761 10496 master.cpp:3302] Deactivating agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 (winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 00:52:22.527761 5560 containerizer.cpp:2338] Destroying container f62e00e8-faa2-4bcc-aad5-74a7918cd9cb in RUNNING state
I0328 00:52:22.528767 5560 containerizer.cpp:2952] Transitioning the state of container f62e00e8-faa2-4bcc-aad5-74a7918cd9cb from RUNNING to DESTROYING
I0328 00:52:22.528767 7956 hierarchical.cpp:344] Removed framework 2982a78e-810d-4839-8d2e-ee618792c50a-0000
I0328 00:52:22.529754 7956 hierarchical.cpp:766] Agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0 deactivated
I0328 00:52:22.529754 5560 launcher.cpp:156] Asked to destroy container f62e00e8-faa2-4bcc-aad5-74a7918cd9cb
I0328 00:52:22.561785 12672 containerizer.cpp:2791] Container f62e00e8-faa2-4bcc-aad5-74a7918cd9cb has exited
I0328 00:52:22.592789 14360 master.cpp:1137] Master terminating
I0328 00:52:22.595784 15564 hierarchical.cpp:609] Removed agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0
I0328 00:52:23.008795 14008 process.cpp:929] Stopped the socket accept loop
```
- Mesos Reviewbot Windows
On March 27, 2018, 11:27 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> -----------------------------------------------------------
>
> (Updated March 27, 2018, 11:27 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
>
>
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds descriptions to declare the following agent API calls
> idempotent:
> - `ADD_RESOURCE_PROVIDER_CONFIG`
> - `UPDATE_RESOURCE_PROVIDER_CONFIG`
> - `REMOVE_RESOURCE_PROVIDER_CONFIG`
>
>
> Diffs
> -----
>
> include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d
> include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7
>
>
> Diff: https://reviews.apache.org/r/66318/diff/1/
>
>
> Testing
> -------
>
> N/A
>
> The actual implementation will be in the next patch.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>