You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/06/27 20:26:37 UTC
Review Request 67761: Added a new registry field
`minimum_capabilities`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Joseph Wu.
Bugs: MESOS-8880
https://issues.apache.org/jira/browse/MESOS-8880
Repository: mesos
Description
-------
This patch adds a new registry field `minimum_capabilities`
to faciliatate safe downgrade. See MESOS-8878.
Also added two accompanying registry operations
`AddMinimumCapability` and `RemoveMinimumCapability`.
Also added a test for the new operations.
Diffs
-----
src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
src/master/registry_operations.hpp c07268ea0a2dd5c95399d860fcbe8ab804ecef92
src/master/registry_operations.cpp c417c4d6c4272eeab15f5313ca5160d4a0aa4da4
src/tests/registrar_tests.cpp d3e4ee0164b8730b39ab05f4880fd0aa034ec8b0
Diff: https://reviews.apache.org/r/67761/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On June 29, 2018, 12:28 p.m., Joseph Wu wrote:
> > src/master/registry.proto
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67761/diff/1/?file=2046454#file2046454line88>
> >
> > Could use a comment along the lines of:
> > ```
> > The minimum set of `MasterInfo::Capability` that the master reading
> > this registry should be capable of. If the master is upgraded
> > or downgraded and no longer satisfies these capabilities, the
> > master is expected to exit upon recovery.
> >
> > We do not use `MasterInfo::Capability` in this field directly
> > because the enumeration may be extended in later Mesos versions.
> > If an earlier version is given an enum value from a later version,
> > the protobuf parser will interpret this as an `UNKNOWN` capability,
> > and we will be unable to provide remediation text to the operator.
> > ```
Great comment, thanks!
> On June 29, 2018, 12:28 p.m., Joseph Wu wrote:
> > src/master/registry_operations.hpp
> > Lines 140-163 (patched)
> > <https://reviews.apache.org/r/67761/diff/1/?file=2046455#file2046455line140>
> >
> > Instead of Add/Remove, how about an `UpdateMinimumCapability` operation instead?
> >
> > It would just take the whole list of capabilities and save it. An "update" would be less prone to dev error, as we wouldn't need to account for the current set of capabilities in the registry.
It is intentional that we only support add/remove minimum capability entries one by one. The idea is that we should only add a minimum capability only if the capability is actually required and we will remove that once it is deemed no longer necessary.
For example, in the future, we will only add `QUOTA_LIMIT` as minimum capability iff Mesos needs to set up a quota limit that is different from the quota guarantee. And it will be removed upon the removal of the last quota entry.
We are hoping devs will have intimate knowledge about the minimum capability entry that relates to their features and fine-tune the entries as needed.
What do you think?
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/#review205579
-----------------------------------------------------------
On June 27, 2018, 1:26 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67761/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 1:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Joseph Wu.
>
>
> Bugs: MESOS-8880
> https://issues.apache.org/jira/browse/MESOS-8880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new registry field `minimum_capabilities`
> to faciliatate safe downgrade. See MESOS-8878.
>
> Also added two accompanying registry operations
> `AddMinimumCapability` and `RemoveMinimumCapability`.
>
> Also added a test for the new operations.
>
>
> Diffs
> -----
>
> src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
> src/master/registry_operations.hpp c07268ea0a2dd5c95399d860fcbe8ab804ecef92
> src/master/registry_operations.cpp c417c4d6c4272eeab15f5313ca5160d4a0aa4da4
> src/tests/registrar_tests.cpp d3e4ee0164b8730b39ab05f4880fd0aa034ec8b0
>
>
> Diff: https://reviews.apache.org/r/67761/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On June 29, 2018, 12:28 p.m., Joseph Wu wrote:
> > src/master/registry_operations.hpp
> > Lines 140-163 (patched)
> > <https://reviews.apache.org/r/67761/diff/1/?file=2046455#file2046455line140>
> >
> > Instead of Add/Remove, how about an `UpdateMinimumCapability` operation instead?
> >
> > It would just take the whole list of capabilities and save it. An "update" would be less prone to dev error, as we wouldn't need to account for the current set of capabilities in the registry.
>
> Meng Zhu wrote:
> It is intentional that we only support add/remove minimum capability entries one by one. The idea is that we should only add a minimum capability only if the capability is actually required and we will remove that once it is deemed no longer necessary.
>
> For example, in the future, we will only add `QUOTA_LIMIT` as minimum capability iff Mesos needs to set up a quota limit that is different from the quota guarantee. And it will be removed upon the removal of the last quota entry.
>
> We are hoping devs will have intimate knowledge about the minimum capability entry that relates to their features and fine-tune the entries as needed.
>
> What do you think?
Hmm... let's take Quota Limits as an example:
1) Suppose we have a Master that supports Quota Limits. The Registry entry for MinimalCapabilities is initially blank because no quotas have been set.
2) The operator uses a V1 API to set a Quota Limit.
---> The master first checks its in-memory state to see what MinimumCapabilities it has set in the Registry. Finding no minimum, it runs an `AddMinimumCapability` operation. Then master persists the quota in the registry in a separate operation.
---> If we have an `UpdateMinimumCapability` operation, the master still needs to check its in-memory state regardless.
3) The operator deletes the Quota Limit.
---> The master must check if all Quota Limits are disabled, and then run `RemoveMinimumCapability`. After that, the master still needs to remove the Quota itself.
---> Very similar picture for an `UpdateMinimumCapability` operation.
So it looks like having one or two operations won't change much.
---
However, it looks like we might run into problems with the non-atomicity of `Add/Remove-MinimumCapability` with their associated Quota operations. What if the process exits between an `AddMinimumCapability` and an `UpdateQuota`? Or between `RemoveQuota` and `RemoveMinimumCapability`?
What if the modifications to minimum capabilities take place inside `UpdateQuota` and `RemoveQuota` instead? i.e. The operation itself will update the required capabilities according to the current state of the registry, rather than the Master doing so directly.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/#review205579
-----------------------------------------------------------
On June 29, 2018, 1:25 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67761/
> -----------------------------------------------------------
>
> (Updated June 29, 2018, 1:25 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Joseph Wu.
>
>
> Bugs: MESOS-8880
> https://issues.apache.org/jira/browse/MESOS-8880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new registry field `minimum_capabilities`
> to faciliatate safe downgrade. See MESOS-8878.
>
> Also added two accompanying registry operations
> `AddMinimumCapability` and `RemoveMinimumCapability`.
>
> Also added a test for the new operations.
>
>
> Diffs
> -----
>
> src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
> src/master/registry_operations.hpp c07268ea0a2dd5c95399d860fcbe8ab804ecef92
> src/master/registry_operations.cpp c417c4d6c4272eeab15f5313ca5160d4a0aa4da4
> src/tests/registrar_tests.cpp d3e4ee0164b8730b39ab05f4880fd0aa034ec8b0
>
>
> Diff: https://reviews.apache.org/r/67761/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On June 29, 2018, 12:28 p.m., Joseph Wu wrote:
> > src/master/registry_operations.hpp
> > Lines 140-163 (patched)
> > <https://reviews.apache.org/r/67761/diff/1/?file=2046455#file2046455line140>
> >
> > Instead of Add/Remove, how about an `UpdateMinimumCapability` operation instead?
> >
> > It would just take the whole list of capabilities and save it. An "update" would be less prone to dev error, as we wouldn't need to account for the current set of capabilities in the registry.
>
> Meng Zhu wrote:
> It is intentional that we only support add/remove minimum capability entries one by one. The idea is that we should only add a minimum capability only if the capability is actually required and we will remove that once it is deemed no longer necessary.
>
> For example, in the future, we will only add `QUOTA_LIMIT` as minimum capability iff Mesos needs to set up a quota limit that is different from the quota guarantee. And it will be removed upon the removal of the last quota entry.
>
> We are hoping devs will have intimate knowledge about the minimum capability entry that relates to their features and fine-tune the entries as needed.
>
> What do you think?
>
> Joseph Wu wrote:
> Hmm... let's take Quota Limits as an example:
>
> 1) Suppose we have a Master that supports Quota Limits. The Registry entry for MinimalCapabilities is initially blank because no quotas have been set.
> 2) The operator uses a V1 API to set a Quota Limit.
> ---> The master first checks its in-memory state to see what MinimumCapabilities it has set in the Registry. Finding no minimum, it runs an `AddMinimumCapability` operation. Then master persists the quota in the registry in a separate operation.
> ---> If we have an `UpdateMinimumCapability` operation, the master still needs to check its in-memory state regardless.
> 3) The operator deletes the Quota Limit.
> ---> The master must check if all Quota Limits are disabled, and then run `RemoveMinimumCapability`. After that, the master still needs to remove the Quota itself.
> ---> Very similar picture for an `UpdateMinimumCapability` operation.
>
> So it looks like having one or two operations won't change much.
>
> ---
>
> However, it looks like we might run into problems with the non-atomicity of `Add/Remove-MinimumCapability` with their associated Quota operations. What if the process exits between an `AddMinimumCapability` and an `UpdateQuota`? Or between `RemoveQuota` and `RemoveMinimumCapability`?
>
> What if the modifications to minimum capabilities take place inside `UpdateQuota` and `RemoveQuota` instead? i.e. The operation itself will update the required capabilities according to the current state of the registry, rather than the Master doing so directly.
Good point regarding atomicity, we should embed the capability update in the respective feature's operations such as update_quota. Removed the operations.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/#review205579
-----------------------------------------------------------
On June 29, 2018, 3:48 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67761/
> -----------------------------------------------------------
>
> (Updated June 29, 2018, 3:48 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Joseph Wu.
>
>
> Bugs: MESOS-8880
> https://issues.apache.org/jira/browse/MESOS-8880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new registry field `minimum_capabilities`
> to faciliatate safe downgrade. See MESOS-8878.
>
>
> Diffs
> -----
>
> src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
>
>
> Diff: https://reviews.apache.org/r/67761/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/#review205579
-----------------------------------------------------------
src/master/registry.proto
Lines 88 (patched)
<https://reviews.apache.org/r/67761/#comment288453>
Could use a comment along the lines of:
```
The minimum set of `MasterInfo::Capability` that the master reading
this registry should be capable of. If the master is upgraded
or downgraded and no longer satisfies these capabilities, the
master is expected to exit upon recovery.
We do not use `MasterInfo::Capability` in this field directly
because the enumeration may be extended in later Mesos versions.
If an earlier version is given an enum value from a later version,
the protobuf parser will interpret this as an `UNKNOWN` capability,
and we will be unable to provide remediation text to the operator.
```
src/master/registry_operations.hpp
Lines 140-163 (patched)
<https://reviews.apache.org/r/67761/#comment288454>
Instead of Add/Remove, how about an `UpdateMinimumCapability` operation instead?
It would just take the whole list of capabilities and save it. An "update" would be less prone to dev error, as we wouldn't need to account for the current set of capabilities in the registry.
- Joseph Wu
On June 27, 2018, 1:26 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67761/
> -----------------------------------------------------------
>
> (Updated June 27, 2018, 1:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Joseph Wu.
>
>
> Bugs: MESOS-8880
> https://issues.apache.org/jira/browse/MESOS-8880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new registry field `minimum_capabilities`
> to faciliatate safe downgrade. See MESOS-8878.
>
> Also added two accompanying registry operations
> `AddMinimumCapability` and `RemoveMinimumCapability`.
>
> Also added a test for the new operations.
>
>
> Diffs
> -----
>
> src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
> src/master/registry_operations.hpp c07268ea0a2dd5c95399d860fcbe8ab804ecef92
> src/master/registry_operations.cpp c417c4d6c4272eeab15f5313ca5160d4a0aa4da4
> src/tests/registrar_tests.cpp d3e4ee0164b8730b39ab05f4880fd0aa034ec8b0
>
>
> Diff: https://reviews.apache.org/r/67761/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/#review213238
-----------------------------------------------------------
Ship it!
I feel the patch description is a bit lacking. Refering to the JIRA is fine, but it does not hurt to summarize the JIRA's purpose too.
Suggestion:
```
This patch adds a new registry field `minimum_capabilities`
to faciliatate safe downgrade of masters. This is a list of strings
which will be written whenever the master uses a feature that may
be backwards incompatible. If an older master sees an unknown feature
in the list, the older master will refuse to recover and try to provide
remediation steps instead.
Also see: MESOS-8878.
```
- Joseph Wu
On June 29, 2018, 3:48 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67761/
> -----------------------------------------------------------
>
> (Updated June 29, 2018, 3:48 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Joseph Wu.
>
>
> Bugs: MESOS-8880
> https://issues.apache.org/jira/browse/MESOS-8880
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a new registry field `minimum_capabilities`
> to faciliatate safe downgrade. See MESOS-8878.
>
>
> Diffs
> -----
>
> src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
>
>
> Diff: https://reviews.apache.org/r/67761/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/
-----------------------------------------------------------
(Updated June 29, 2018, 3:48 p.m.)
Review request for mesos, Benjamin Mahler and Joseph Wu.
Changes
-------
Operations removed.
Bugs: MESOS-8880
https://issues.apache.org/jira/browse/MESOS-8880
Repository: mesos
Description (updated)
-------
This patch adds a new registry field `minimum_capabilities`
to faciliatate safe downgrade. See MESOS-8878.
Diffs (updated)
-----
src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
Diff: https://reviews.apache.org/r/67761/diff/3/
Changes: https://reviews.apache.org/r/67761/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 67761: Added a new registry field
`minimum_capabilities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67761/
-----------------------------------------------------------
(Updated June 29, 2018, 1:25 p.m.)
Review request for mesos, Benjamin Mahler and Joseph Wu.
Changes
-------
Thanks for the review! See my comments below.
Bugs: MESOS-8880
https://issues.apache.org/jira/browse/MESOS-8880
Repository: mesos
Description
-------
This patch adds a new registry field `minimum_capabilities`
to faciliatate safe downgrade. See MESOS-8878.
Also added two accompanying registry operations
`AddMinimumCapability` and `RemoveMinimumCapability`.
Also added a test for the new operations.
Diffs (updated)
-----
src/master/registry.proto cac0c3196aed601ba41c337e8d746819ce42f8d9
src/master/registry_operations.hpp c07268ea0a2dd5c95399d860fcbe8ab804ecef92
src/master/registry_operations.cpp c417c4d6c4272eeab15f5313ca5160d4a0aa4da4
src/tests/registrar_tests.cpp d3e4ee0164b8730b39ab05f4880fd0aa034ec8b0
Diff: https://reviews.apache.org/r/67761/diff/2/
Changes: https://reviews.apache.org/r/67761/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu