You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/04/24 03:28:42 UTC
Review Request 70538: WIP: Fixed upgrade path for tasks with invalid
protobuf unions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70538/
-----------------------------------------------------------
Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann.
Repository: mesos
Description
-------
As part of MESOS-6874, the master now validates protobuf unions
passed as part of an ExecutorInfo::ContainerInfo. This prevents a
task from specifying, for example, a ContainerInfo::MESOS, but filling
out the `docker` field (which is then ignored by the agent).
However, if a task was already launched with an invalid protobuf
union, the same validation will happen when the agent tries to
reregister with the master. In this case, if the master is upgraded
to validate protobuf unions, the agent reregistration will be rejected.
This adds a hack to wipe invalid fields from the agent's reregistration
message before sending it to the master.
Diffs
-----
src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682
Diff: https://reviews.apache.org/r/70538/diff/1/
Testing
-------
WIP!!!
Thanks,
Joseph Wu
Re: Review Request 70538: WIP: Fixed upgrade path for tasks with
invalid protobuf unions.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On April 25, 2019, 11:05 a.m., Vinod Kone wrote:
> > I would suggest we tweak the validation for 1.13.0 to log a warning instead of throwing an error and add a deprecation warning that "union" protobufs will be validated in a future release in the CHANGELOG. In the meantime, we can give times to frameworks to fix their bug, as well as figure what's the best way to handle existing tasks with invalid protos.
That is probably a better approach. This patch only deals with reregistration, and does not give time for frameworks to fix their protobufs.
Here's the validation tweak: https://reviews.apache.org/r/70546/
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70538/#review214888
-----------------------------------------------------------
On April 23, 2019, 8:28 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70538/
> -----------------------------------------------------------
>
> (Updated April 23, 2019, 8:28 p.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann.
>
>
> Bugs: MESOS-9740
> https://issues.apache.org/jira/browse/MESOS-9740
>
>
> Repository: mesos
>
>
> Description
> -------
>
> As part of MESOS-6874, the master now validates protobuf unions
> passed as part of an ExecutorInfo::ContainerInfo. This prevents a
> task from specifying, for example, a ContainerInfo::MESOS, but filling
> out the `docker` field (which is then ignored by the agent).
>
> However, if a task was already launched with an invalid protobuf
> union, the same validation will happen when the agent tries to
> reregister with the master. In this case, if the master is upgraded
> to validate protobuf unions, the agent reregistration will be rejected.
>
> This adds a hack to wipe invalid fields from the agent's reregistration
> message before sending it to the master.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682
>
>
> Diff: https://reviews.apache.org/r/70538/diff/1/
>
>
> Testing
> -------
>
> WIP!!!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70538: WIP: Fixed upgrade path for tasks with
invalid protobuf unions.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70538/#review214888
-----------------------------------------------------------
I would suggest we tweak the validation for 1.13.0 to log a warning instead of throwing an error and add a deprecation warning that "union" protobufs will be validated in a future release in the CHANGELOG. In the meantime, we can give times to frameworks to fix their bug, as well as figure what's the best way to handle existing tasks with invalid protos.
- Vinod Kone
On April 24, 2019, 3:28 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70538/
> -----------------------------------------------------------
>
> (Updated April 24, 2019, 3:28 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann.
>
>
> Bugs: MESOS-9740
> https://issues.apache.org/jira/browse/MESOS-9740
>
>
> Repository: mesos
>
>
> Description
> -------
>
> As part of MESOS-6874, the master now validates protobuf unions
> passed as part of an ExecutorInfo::ContainerInfo. This prevents a
> task from specifying, for example, a ContainerInfo::MESOS, but filling
> out the `docker` field (which is then ignored by the agent).
>
> However, if a task was already launched with an invalid protobuf
> union, the same validation will happen when the agent tries to
> reregister with the master. In this case, if the master is upgraded
> to validate protobuf unions, the agent reregistration will be rejected.
>
> This adds a hack to wipe invalid fields from the agent's reregistration
> message before sending it to the master.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682
>
>
> Diff: https://reviews.apache.org/r/70538/diff/1/
>
>
> Testing
> -------
>
> WIP!!!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70538: WIP: Fixed upgrade path for tasks with
invalid protobuf unions.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70538/#review214851
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70538]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On April 24, 2019, 3:28 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70538/
> -----------------------------------------------------------
>
> (Updated April 24, 2019, 3:28 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann.
>
>
> Bugs: MESOS-9740
> https://issues.apache.org/jira/browse/MESOS-9740
>
>
> Repository: mesos
>
>
> Description
> -------
>
> As part of MESOS-6874, the master now validates protobuf unions
> passed as part of an ExecutorInfo::ContainerInfo. This prevents a
> task from specifying, for example, a ContainerInfo::MESOS, but filling
> out the `docker` field (which is then ignored by the agent).
>
> However, if a task was already launched with an invalid protobuf
> union, the same validation will happen when the agent tries to
> reregister with the master. In this case, if the master is upgraded
> to validate protobuf unions, the agent reregistration will be rejected.
>
> This adds a hack to wipe invalid fields from the agent's reregistration
> message before sending it to the master.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682
>
>
> Diff: https://reviews.apache.org/r/70538/diff/1/
>
>
> Testing
> -------
>
> WIP!!!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70538: WIP: Fixed upgrade path for tasks with
invalid protobuf unions.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70538/#review214849
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['70538']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3252/mesos-review-70538
Relevant logs:
- [mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3252/mesos-review-70538/logs/mesos-tests.log):
```
I0424 09:31:02.051659 73824 master.cpp:3312] Disconnecting agent d7b1510a-c334-4e0e-b29f-4e608ed96fc4-S0 at slave(504)@192.10.1.4:52521 (windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0424 09:31:02.051659 73824 master.cpp:3331] Deactivating agent d7b1510a-c334-4e0e-b29f-4e608ed96fc4-S0 at slave(504)@192.10.1.4:52521 (windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0424 09:31:02.051659 71552 hierarchical.cpp:392] Removed framework d7b1510a-c334-4e0e-b29f-4e608ed96fc4-0000
I0424 09:31:02.052666 71552 hierarchical.cpp:829] Agent d7b1510a-c334-4e0e-b29f-4e608ed96fc4-S0 deactivated
I0424 09:31:02.053719 73276 containerizer.cpp:2576] Destroying container 5b37002c-0a30-4822-bd86-bf909080a220 in RUNNING state
I0424 09:31:02.053719 73276 containerizer.cpp:3278] Transitioning the state of container 5b37002c-0a30-4822-bd86-bf909080a220 from RUNNING to DESTROYING
I0424 09:31:02.053719 73276 launcher.cpp:161] Asked to destroy container 5b37002c-0a30-4822-bd86-bf909080a220
W0424 09:31:02.055660 69164 pro[ OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (876 ms)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest (898 ms total)
[----------] Global test environment tear-down
[==========] 1166 tests from 109 test cases ran. (596766 ms total)
[ PASSED ] 1163 tests.
[ FAILED ] 3 tests, listed below:
[ FAILED ] DockerFetcherPluginTest.INTERNET_CURL_FetchManifest
[ FAILED ] DockerFetcherPluginTest.INTERNET_CURL_FetchImage
[ FAILED ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName
3 FAILED TESTS
YOU HAVE 233 DISABLED TESTS
cess.cpp:1423] Failed to recv on socket WindowsFD::Type::SOCKET=6276 to peer '192.10.1.4:54931': IO failed with error code: The specified network name is no longer available.
W0424 09:31:02.055660 69164 process.cpp:838] Failed to recv on socket WindowsFD::Type::SOCKET=6268 to peer '192.10.1.4:54932': IO failed with error code: The specified network name is no longer available.
I0424 09:31:02.154880 71552 containerizer.cpp:3117] Container 5b37002c-0a30-4822-bd86-bf909080a220 has exited
I0424 09:31:02.189864 77420 master.cpp:1135] Master terminating
I0424 09:31:02.190870 73276 hierarchical.cpp:680] Removed agent d7b1510a-c334-4e0e-b29f-4e608ed96fc4-S0
I0424 09:31:03.654887 69164 process.cpp:927] Stopped the socket accept loop
```
- Mesos Reviewbot Windows
On April 24, 2019, 3:28 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70538/
> -----------------------------------------------------------
>
> (Updated April 24, 2019, 3:28 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann.
>
>
> Bugs: MESOS-9740
> https://issues.apache.org/jira/browse/MESOS-9740
>
>
> Repository: mesos
>
>
> Description
> -------
>
> As part of MESOS-6874, the master now validates protobuf unions
> passed as part of an ExecutorInfo::ContainerInfo. This prevents a
> task from specifying, for example, a ContainerInfo::MESOS, but filling
> out the `docker` field (which is then ignored by the agent).
>
> However, if a task was already launched with an invalid protobuf
> union, the same validation will happen when the agent tries to
> reregister with the master. In this case, if the master is upgraded
> to validate protobuf unions, the agent reregistration will be rejected.
>
> This adds a hack to wipe invalid fields from the agent's reregistration
> message before sending it to the master.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682
>
>
> Diff: https://reviews.apache.org/r/70538/diff/1/
>
>
> Testing
> -------
>
> WIP!!!
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 70538: WIP: Fixed upgrade path for tasks with
invalid protobuf unions.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70538/#review214848
-----------------------------------------------------------
Probably clearing the incorrect protobufs should be done in the master, otherwise we get the same problem when running 1.7 agents with an 1.8 master.
So maybe an alternative approach would be to remove the check from the general `ContainerInfo` validation and only do it for incoming `LaunchTasks` messages.
However, I think the bigger problem is the following: If we have reasonable suspicion that this validation will trigger often, we're going to break a lot of users' frameworks that generated these incorrect protobuf unions in the first place. So we should think about either removing the validation completely, with a comment explaining why it can't be done, or at least provide a master flag to turn off this behaviour for operators who are not in a position to easily fix their frameworks.
- Benno Evers
On April 24, 2019, 3:28 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70538/
> -----------------------------------------------------------
>
> (Updated April 24, 2019, 3:28 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann.
>
>
> Bugs: MESOS-9740
> https://issues.apache.org/jira/browse/MESOS-9740
>
>
> Repository: mesos
>
>
> Description
> -------
>
> As part of MESOS-6874, the master now validates protobuf unions
> passed as part of an ExecutorInfo::ContainerInfo. This prevents a
> task from specifying, for example, a ContainerInfo::MESOS, but filling
> out the `docker` field (which is then ignored by the agent).
>
> However, if a task was already launched with an invalid protobuf
> union, the same validation will happen when the agent tries to
> reregister with the master. In this case, if the master is upgraded
> to validate protobuf unions, the agent reregistration will be rejected.
>
> This adds a hack to wipe invalid fields from the agent's reregistration
> message before sending it to the master.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682
>
>
> Diff: https://reviews.apache.org/r/70538/diff/1/
>
>
> Testing
> -------
>
> WIP!!!
>
>
> Thanks,
>
> Joseph Wu
>
>