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 2015/08/24 20:54:03 UTC
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 11:54 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Rebased and updated diff. No other changes.
Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069
Repository: mesos
Description
-------
Endpoint: /maintenance.start
Registry operation = maintenance::StartMaintenance
Sets the list of machines as Deactivated.
Diffs (updated)
-----
src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
src/master/maintenance.hpp PRE-CREATION
src/master/maintenance.cpp PRE-CREATION
src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
src/tests/master_maintenance_tests.cpp PRE-CREATION
src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
Diff: https://reviews.apache.org/r/37358/diff/
Testing
-------
`make check`
New Tests:
RegistrarTest.StartMaintenance
Schedules some machines. Deactivates some.
MasterMaintenanceTest.StartMaintenance
Tests some invalid lists.
Schedules some machines. Tests some valid and invalid lists.
Thanks,
Joseph Wu
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 25, 2015, 1:36 a.m., Guangya Liu wrote:
> > src/master/maintenance.cpp, line 121
> > <https://reviews.apache.org/r/37358/diff/5/?file=1048624#file1048624line121>
> >
> > What does strict means here? I see it was not used
This is explained in the previous review: https://reviews.apache.org/r/37325/diff/11#6
Essentially, this is the `registry_strict` flag, which only applies to operations which affect masters/agents infos in the registry. Hence, we do not use it for maintenance.
> On Aug. 25, 2015, 1:36 a.m., Guangya Liu wrote:
> > src/master/maintenance.cpp, line 133
> > <https://reviews.apache.org/r/37358/diff/5/?file=1048624#file1048624line133>
> >
> > Not quite understand the logic here, what does "changed" means? If one machine put into DEACTIVATED mode, then the changed will be translated to true and it will be true for the whole life cycle of this function.
This is to satisfy the prototype of an Operation. `Operation::perform` returns a bool indicating if the registry was changed as a result of the operation.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/#review96309
-----------------------------------------------------------
On Aug. 24, 2015, 11:54 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 11:54 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Endpoint: /maintenance.start
>
> Registry operation = maintenance::StartMaintenance
> Sets the list of machines as Deactivated.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
> src/master/maintenance.hpp PRE-CREATION
> src/master/maintenance.cpp PRE-CREATION
> src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
> src/tests/master_maintenance_tests.cpp PRE-CREATION
> src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
>
> Diff: https://reviews.apache.org/r/37358/diff/
>
>
> Testing
> -------
>
> `make check`
>
> New Tests:
> RegistrarTest.StartMaintenance
> Schedules some machines. Deactivates some.
> MasterMaintenanceTest.StartMaintenance
> Tests some invalid lists.
> Schedules some machines. Tests some valid and invalid lists.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/#review96309
-----------------------------------------------------------
src/master/maintenance.cpp (line 121)
<https://reviews.apache.org/r/37358/#comment151628>
What does strict means here? I see it was not used
src/master/maintenance.cpp (line 133)
<https://reviews.apache.org/r/37358/#comment151627>
Not quite understand the logic here, what does "changed" means? If one machine put into DEACTIVATED mode, then the changed will be translated to true and it will be true for the whole life cycle of this function.
- Guangya Liu
On Aug. 24, 2015, 6:54 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 6:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Endpoint: /maintenance.start
>
> Registry operation = maintenance::StartMaintenance
> Sets the list of machines as Deactivated.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
> src/master/maintenance.hpp PRE-CREATION
> src/master/maintenance.cpp PRE-CREATION
> src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
> src/tests/master_maintenance_tests.cpp PRE-CREATION
> src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
>
> Diff: https://reviews.apache.org/r/37358/diff/
>
>
> Testing
> -------
>
> `make check`
>
> New Tests:
> RegistrarTest.StartMaintenance
> Schedules some machines. Deactivates some.
> MasterMaintenanceTest.StartMaintenance
> Tests some invalid lists.
> Schedules some machines. Tests some valid and invalid lists.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the DOWN mode.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 27, 2015, 10:02 p.m., Benjamin Hindman wrote:
> > src/master/maintenance.cpp, lines 237-239
> > <https://reviews.apache.org/r/37358/diff/7/?file=1055414#file1055414line237>
> >
> > It seems weird to make this an error ... why is it a prerequisite that validation of a MachineInfos requires more than one machine in the list? Is this somehow tied to your expectation in the code?
We have this check because a no-op doesn't make much sense, with regards to maintenance; and also to match how we don't allow empty Maintenance Windows (i.e. no machines, but with an unavailability).
It's easy to change this behavior though, so I'll leave this issue open for now.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/#review96832
-----------------------------------------------------------
On Aug. 28, 2015, 3:32 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 3:32 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Endpoint: /machine/down
>
> Registry operation = maintenance::StartMaintenance
> Sets the list of machines as Down.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
> src/master/maintenance.hpp PRE-CREATION
> src/master/maintenance.cpp PRE-CREATION
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
> src/tests/master_maintenance_tests.cpp PRE-CREATION
> src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
>
> Diff: https://reviews.apache.org/r/37358/diff/
>
>
> Testing
> -------
>
> `make check`
>
> New Tests:
> RegistrarTest.StartMaintenance
> Schedules some machines. Downs some.
> MasterMaintenanceTest.BringDownMachines
> Tests some invalid lists.
> Schedules some machines. Tests some valid and invalid lists.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/#review96832
-----------------------------------------------------------
Ship it!
src/master/http.cpp (line 1477)
<https://reviews.apache.org/r/37358/#comment152511>
Need to s/Deactivated/DOWN/ mode here and elsewhere please.
src/master/http.cpp (line 1503)
<https://reviews.apache.org/r/37358/#comment152512>
See previous review comments.
src/master/http.cpp (line 1514)
<https://reviews.apache.org/r/37358/#comment152513>
Why 'DebugString' instead of JSON representation? I might have missed this on previous reviews as well ... the advantage of the JSON representation is that folks can map back what they submitted to what's wrong. If we print out in this format that won't be as obvious.
Here and below in this entire review as well as elsewhere we should clean this up please, thanks!
src/master/http.cpp (line 1527)
<https://reviews.apache.org/r/37358/#comment152514>
s/operationResult/result/ here and other reviews please.
src/master/maintenance.cpp (lines 237 - 239)
<https://reviews.apache.org/r/37358/#comment152515>
It seems weird to make this an error ... why is it a prerequisite that validation of a MachineInfos requires more than one machine in the list? Is this somehow tied to your expectation in the code?
src/tests/registrar_tests.cpp (line 532)
<https://reviews.apache.org/r/37358/#comment152517>
s/Schdule/Schedule/
- Benjamin Hindman
On Aug. 26, 2015, 9:59 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 9:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Endpoint: /maintenance.start
>
> Registry operation = maintenance::StartMaintenance
> Sets the list of machines as Deactivated.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
> src/master/maintenance.hpp PRE-CREATION
> src/master/maintenance.cpp PRE-CREATION
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
> src/tests/master_maintenance_tests.cpp PRE-CREATION
> src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
>
> Diff: https://reviews.apache.org/r/37358/diff/
>
>
> Testing
> -------
>
> `make check`
>
> New Tests:
> RegistrarTest.StartMaintenance
> Schedules some machines. Deactivates some.
> MasterMaintenanceTest.StartMaintenance
> Tests some invalid lists.
> Schedules some machines. Tests some valid and invalid lists.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the DOWN mode.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 3:32 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Rebase + changes to match previous reviews in chain.
Renamed endpoint to match the renamed `DOWN` mode.
Summary (updated)
-----------------
Maintenance Primitives: Adds an endpoint for transitioning agents into the DOWN mode.
Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069
Repository: mesos
Description (updated)
-------
Endpoint: /machine/down
Registry operation = maintenance::StartMaintenance
Sets the list of machines as Down.
Diffs (updated)
-----
src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
src/master/maintenance.hpp PRE-CREATION
src/master/maintenance.cpp PRE-CREATION
src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
src/tests/master_maintenance_tests.cpp PRE-CREATION
src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
Diff: https://reviews.apache.org/r/37358/diff/
Testing (updated)
-------
`make check`
New Tests:
RegistrarTest.StartMaintenance
Schedules some machines. Downs some.
MasterMaintenanceTest.BringDownMachines
Tests some invalid lists.
Schedules some machines. Tests some valid and invalid lists.
Thanks,
Joseph Wu
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/
-----------------------------------------------------------
(Updated Aug. 26, 2015, 2:59 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Update to match comments and changes in previous reviews.
Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069
Repository: mesos
Description
-------
Endpoint: /maintenance.start
Registry operation = maintenance::StartMaintenance
Sets the list of machines as Deactivated.
Diffs (updated)
-----
src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
src/master/maintenance.hpp PRE-CREATION
src/master/maintenance.cpp PRE-CREATION
src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
src/tests/master_maintenance_tests.cpp PRE-CREATION
src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
Diff: https://reviews.apache.org/r/37358/diff/
Testing
-------
`make check`
New Tests:
RegistrarTest.StartMaintenance
Schedules some machines. Deactivates some.
MasterMaintenanceTest.StartMaintenance
Tests some invalid lists.
Schedules some machines. Tests some valid and invalid lists.
Thanks,
Joseph Wu
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/#review96369
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On Aug. 25, 2015, 5:03 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 5:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Endpoint: /maintenance.start
>
> Registry operation = maintenance::StartMaintenance
> Sets the list of machines as Deactivated.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
> src/master/maintenance.hpp PRE-CREATION
> src/master/maintenance.cpp PRE-CREATION
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
> src/tests/master_maintenance_tests.cpp PRE-CREATION
> src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
>
> Diff: https://reviews.apache.org/r/37358/diff/
>
>
> Testing
> -------
>
> `make check`
>
> New Tests:
> RegistrarTest.StartMaintenance
> Schedules some machines. Deactivates some.
> MasterMaintenanceTest.StartMaintenance
> Tests some invalid lists.
> Schedules some machines. Tests some valid and invalid lists.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/#review96368
-----------------------------------------------------------
src/master/maintenance.cpp (line 121)
<https://reviews.apache.org/r/37358/#comment151699>
Got it
- Guangya Liu
On Aug. 25, 2015, 5:03 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 5:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Endpoint: /maintenance.start
>
> Registry operation = maintenance::StartMaintenance
> Sets the list of machines as Deactivated.
>
>
> Diffs
> -----
>
> src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
> src/master/maintenance.hpp PRE-CREATION
> src/master/maintenance.cpp PRE-CREATION
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
> src/tests/master_maintenance_tests.cpp PRE-CREATION
> src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
>
> Diff: https://reviews.apache.org/r/37358/diff/
>
>
> Testing
> -------
>
> `make check`
>
> New Tests:
> RegistrarTest.StartMaintenance
> Schedules some machines. Deactivates some.
> MasterMaintenanceTest.StartMaintenance
> Tests some invalid lists.
> Schedules some machines. Tests some valid and invalid lists.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for
transitioning agents into the Deactivated maintenance mode.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37358/
-----------------------------------------------------------
(Updated Aug. 25, 2015, 10:03 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Adjust some spacing in the tests (correcting the style).
Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069
Repository: mesos
Description
-------
Endpoint: /maintenance.start
Registry operation = maintenance::StartMaintenance
Sets the list of machines as Deactivated.
Diffs (updated)
-----
src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44
src/master/maintenance.hpp PRE-CREATION
src/master/maintenance.cpp PRE-CREATION
src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
src/tests/master_maintenance_tests.cpp PRE-CREATION
src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166
Diff: https://reviews.apache.org/r/37358/diff/
Testing
-------
`make check`
New Tests:
RegistrarTest.StartMaintenance
Schedules some machines. Deactivates some.
MasterMaintenanceTest.StartMaintenance
Tests some invalid lists.
Schedules some machines. Tests some valid and invalid lists.
Thanks,
Joseph Wu