Re: Review Request 60784: Fixed a bug in operator== for DiskInfo::Source.
> On July 13, 2017, 12:35 a.m., Greg Mann wrote: > > One question: if we were to enforce in our validation code that the > > relevant member of `Resource::DiskInfo::Source` must be set when its > > corresponding type is set (in this case, the `path` or `mount` fields), > > would we still want this patch for `operator==`? Should we add that to the > > validation code, or is it a valid state for `type` to be set and the > > corresponding optional member to be unset? I think that would be another option. It might be more robust to allow `operator==` to return sensible results for _any_ legal `Resource::DiskInfo::Source` object, rather than working only for validated objects (and misbehaving silently for invalid objects); on the other hand, this makes the equality routines a little harder to write. Given that we're already checking for `has_path()` (we're just doing it incorrectly), I'd argue for making this fix, and then potentially revisiting the relationship between validation and equality/etc. routines separately. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60784/#review180384 --- On July 11, 2017, 10:29 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60784/ > --- > > (Updated July 11, 2017, 10:29 p.m.) > > > Review request for mesos and Greg Mann. > > > Repository: mesos > > > Description > --- > > The previous implementation was incorrect for the case when either > `right.path` or `right.mount` is set but the corresponding field in > `left` is unset. > > > Diffs > - > > src/common/resources.cpp 3f4fff4f1dbe9c8acc55e3077212fe329e53e4d9 > src/v1/resources.cpp 55d493e89114acc94b1524f3f94a47ccea20469a > > > Diff: https://reviews.apache.org/r/60784/diff/1/ > > > Testing > --- > > `make check` > > I tried to write a unit test for this specific problem but wasn't able to > repro :-\ Current coding seems wrong / inconsistent in any case, though. > > > Thanks, > > Neil Conway > >
Review Request 60784: Fixed a bug in operator== for DiskInfo::Source.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60784/ --- Review request for mesos and Greg Mann. Repository: mesos Description --- The previous implementation was incorrect for the case when either `right.path` or `right.mount` is set but the corresponding field in `left` is unset. Diffs - src/common/resources.cpp 3f4fff4f1dbe9c8acc55e3077212fe329e53e4d9 src/v1/resources.cpp 55d493e89114acc94b1524f3f94a47ccea20469a Diff: https://reviews.apache.org/r/60784/diff/1/ Testing --- `make check` I tried to write a unit test for this specific problem but wasn't able to repro :-\ Current coding seems wrong / inconsistent in any case, though. Thanks, Neil Conway
Re: Review Request 59764: Ignore registration attempts by agents with misconfigured domain.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59764/ --- (Updated July 11, 2017, 5:14 p.m.) Review request for mesos and Vinod Kone. Changes --- Address review comment. Bugs: MESOS-7612 https://issues.apache.org/jira/browse/MESOS-7612 Repository: mesos Description --- We expect the master's domain to be configured first, then the domain of the agents to be configured. Therefore, if an agent with configured domain attempts to register or re-register with a master that does not have a configured domain, the registration attempt should be ignored. This is important, because the master would have no way of determining whether the agent is "remote" or not, which might result in placing inappropriate workloads on it. Diffs (updated) - src/master/master.cpp 7668749cc4658627ed3bdd6cdcf7e837daece8d6 src/tests/master_tests.cpp 9cfa510f1e5742b47a44cb640a0d6ba85a2880ae Diff: https://reviews.apache.org/r/59764/diff/5/ Changes: https://reviews.apache.org/r/59764/diff/4-5/ Testing --- `make check` Manual testing. Thanks, Neil Conway
Re: Review Request 59766: Changed allocator to offer remote resources to region-aware frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59766/ --- (Updated July 11, 2017, 5:18 p.m.) Review request for mesos and Vinod Kone. Changes --- Address review comments. Bugs: MESOS-7614 https://issues.apache.org/jira/browse/MESOS-7614 Repository: mesos Description --- Changed allocator to offer remote resources to region-aware frameworks. Diffs (updated) - include/mesos/allocator/allocator.hpp 9d116c68fe7225178bba85414cb88d97fa0dbed5 src/master/allocator/mesos/allocator.hpp 725ec7c3348b3f5c66bb6530983ee212480c5974 src/master/allocator/mesos/hierarchical.hpp 81d1b964800ef69e25bc874dbadcdfcfd47392b2 src/master/allocator/mesos/hierarchical.cpp fad9330dc371728e72eb2af808e993c4c97543af src/master/master.cpp 7668749cc4658627ed3bdd6cdcf7e837daece8d6 src/tests/allocator.hpp a990788d5218bbcac499613783750ade022811a7 src/tests/api_tests.cpp cdaa72462eac1687185115771265539d8c2b09a9 src/tests/master_allocator_tests.cpp f83ca66e64fe6d2d10d104ff24f31afd84c50a3a src/tests/master_quota_tests.cpp bbdbfbe9d9960cdc9fa7b2d56e9da9122d25db6a src/tests/master_tests.cpp 9cfa510f1e5742b47a44cb640a0d6ba85a2880ae src/tests/reservation_tests.cpp 3b4884be21a0b4f15bcbc1a58fa4a4ad7858a7fb src/tests/resource_offers_tests.cpp 427a6520034a537caf748a126001d131e196fedd src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 Diff: https://reviews.apache.org/r/59766/diff/6/ Changes: https://reviews.apache.org/r/59766/diff/5-6/ Testing --- `make check`. Thanks, Neil Conway
Re: Review Request 59762: Added domain to MasterInfo and SlaveInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59762/ --- (Updated July 10, 2017, 11:12 p.m.) Review request for mesos and Vinod Kone. Changes --- Address review comments. Bugs: MESOS-7610 https://issues.apache.org/jira/browse/MESOS-7610 Repository: mesos Description --- This means that each master's domain is stored in ZooKeeper, along with the rest of the MasterInfo protobuf message. Each agent's domain is stored as part of its checkpointed resources. Changing the agent's domain requires a full drain of the agent; this behavior might be relaxed in the future. Diffs (updated) - include/mesos/mesos.proto d70ac9e0d5d09919d0a474a48199f3cb5747b310 include/mesos/v1/mesos.proto fee8b0cca8ff3c6de2dd795dc58588a4b087c711 src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 src/common/type_utils.cpp aeb16238e9c3fd71a8e9e57dbb8098ddc004e1f2 src/internal/evolve.hpp 9db5fe6155243576f186a8b974e81068505b9fcb src/internal/evolve.cpp 93196f301e820b99572ee008b98a124ddafe9697 src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a src/master/master.cpp 7668749cc4658627ed3bdd6cdcf7e837daece8d6 src/slave/http.cpp 700871e1502a65b0bb1fc31219e09219dbdb5340 src/slave/slave.cpp a1a6b64b26cf5036e2e6ca010027e4e5457480dd src/tests/api_tests.cpp cdaa72462eac1687185115771265539d8c2b09a9 src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f src/tests/mesos.hpp 06b22f97149c9644ec5007b885134016019f322e src/tests/slave_tests.cpp 8a69cc2ede0b2f17a31986e9142aa2081691eb5e src/v1/mesos.cpp 423510ef14025dba208ef85edf5305c2ce58f01d Diff: https://reviews.apache.org/r/59762/diff/5/ Changes: https://reviews.apache.org/r/59762/diff/4-5/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 59766: Changed allocator to offer remote resources to region-aware frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59766/ --- (Updated July 10, 2017, 5:26 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase, added comment. Bugs: MESOS-7614 https://issues.apache.org/jira/browse/MESOS-7614 Repository: mesos Description --- Changed allocator to offer remote resources to region-aware frameworks. Diffs (updated) - include/mesos/allocator/allocator.hpp 9d116c68fe7225178bba85414cb88d97fa0dbed5 src/master/allocator/mesos/allocator.hpp 725ec7c3348b3f5c66bb6530983ee212480c5974 src/master/allocator/mesos/hierarchical.hpp 81d1b964800ef69e25bc874dbadcdfcfd47392b2 src/master/allocator/mesos/hierarchical.cpp fad9330dc371728e72eb2af808e993c4c97543af src/master/master.cpp 7668749cc4658627ed3bdd6cdcf7e837daece8d6 src/tests/allocator.hpp a990788d5218bbcac499613783750ade022811a7 src/tests/api_tests.cpp cdaa72462eac1687185115771265539d8c2b09a9 src/tests/master_allocator_tests.cpp f83ca66e64fe6d2d10d104ff24f31afd84c50a3a src/tests/master_quota_tests.cpp bbdbfbe9d9960cdc9fa7b2d56e9da9122d25db6a src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f src/tests/reservation_tests.cpp 3b4884be21a0b4f15bcbc1a58fa4a4ad7858a7fb src/tests/resource_offers_tests.cpp 427a6520034a537caf748a126001d131e196fedd src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 Diff: https://reviews.apache.org/r/59766/diff/5/ Changes: https://reviews.apache.org/r/59766/diff/4-5/ Testing --- `make check`. Thanks, Neil Conway
Re: Review Request 59761: Added master and agent flags to specify domain.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59761/ --- (Updated July 7, 2017, 4:20 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7610 https://issues.apache.org/jira/browse/MESOS-7610 Repository: mesos Description --- Added master and agent flags to specify domain. Diffs (updated) - docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 src/master/flags.hpp 8a9bd2dfa513f1de85dab2cc3cc1d78e80293837 src/master/flags.cpp ca5f02c728a9f39abbbee59fa169f95d20c69d3d src/slave/flags.hpp 858876fe86cc50cb3655b06071db6450916cb814 src/slave/flags.cpp 74df647cc4f1161d061f0349011ce070db3a9789 Diff: https://reviews.apache.org/r/59761/diff/4/ Changes: https://reviews.apache.org/r/59761/diff/3-4/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60346: Improved the documentation of 'TASK_LOST'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60346/#review179725 --- Ship it! include/mesos/mesos.proto Lines 1940 (patched) <https://reviews.apache.org/r/60346/#comment254563> We don't support the "strict-registry" flag anymore, so I removed the reference to this before committing. - Neil Conway On June 21, 2017, 10:17 p.m., Gastón Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60346/ > --- > > (Updated June 21, 2017, 10:17 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7662 > https://issues.apache.org/jira/browse/MESOS-7662 > > > Repository: mesos > > > Description > --- > > Updated the protobuf comments to clarify that 'TASK_LOST' is not > always a terminal state. > > > Diffs > - > > include/mesos/mesos.proto 38449da44a5288afc7e2ef17ef5e1380ecabd5d2 > include/mesos/v1/mesos.proto 81eca47ce1074f5e522a0410f01ab2b2cfc76aa2 > > > Diff: https://reviews.apache.org/r/60346/diff/1/ > > > Testing > --- > > None, this is not functional change. > > > Thanks, > > Gastón Kleiman > >
Re: Review Request 59762: Added domain to MasterInfo and SlaveInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59762/ --- (Updated July 5, 2017, 10:32 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7610 https://issues.apache.org/jira/browse/MESOS-7610 Repository: mesos Description --- This means that each master's domain is stored in ZooKeeper, along with the rest of the MasterInfo protobuf message. Each agent's domain is stored as part of its checkpointed resources. Changing the agent's domain requires a full drain of the agent; this behavior might be relaxed in the future. Diffs (updated) - include/mesos/mesos.proto 4e4b2790615e8bbbf262ba809cbfd67e24368952 include/mesos/v1/mesos.proto b8625d363388bc707bc2458abd47a218cc7b7888 src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 src/common/type_utils.cpp aeb16238e9c3fd71a8e9e57dbb8098ddc004e1f2 src/internal/evolve.hpp 9db5fe6155243576f186a8b974e81068505b9fcb src/internal/evolve.cpp 93196f301e820b99572ee008b98a124ddafe9697 src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 src/slave/http.cpp 700871e1502a65b0bb1fc31219e09219dbdb5340 src/slave/slave.cpp 0e24b8cb8d1020af515e3d1862e121e1daf82ce9 src/tests/api_tests.cpp cdaa72462eac1687185115771265539d8c2b09a9 src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f src/tests/mesos.hpp eac2c96985cdbbf1a50cfc054862eae2d44fbfcd src/tests/slave_tests.cpp 8a69cc2ede0b2f17a31986e9142aa2081691eb5e src/v1/mesos.cpp 423510ef14025dba208ef85edf5305c2ce58f01d Diff: https://reviews.apache.org/r/59762/diff/4/ Changes: https://reviews.apache.org/r/59762/diff/3-4/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 59921: Added agent domain to Offer message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59921/ --- (Updated July 5, 2017, 10:32 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7644 https://issues.apache.org/jira/browse/MESOS-7644 Repository: mesos Description --- This is a convenience mechanism to allow frameworks to determine the domain of an agent when they receive a resource offer. Diffs (updated) - include/mesos/mesos.proto 4e4b2790615e8bbbf262ba809cbfd67e24368952 include/mesos/v1/mesos.proto b8625d363388bc707bc2458abd47a218cc7b7888 src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f Diff: https://reviews.apache.org/r/59921/diff/2/ Changes: https://reviews.apache.org/r/59921/diff/1-2/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 59764: Ignore registration attempts by agents with misconfigured domain.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59764/ --- (Updated July 5, 2017, 10:32 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7612 https://issues.apache.org/jira/browse/MESOS-7612 Repository: mesos Description --- We expect the master's domain to be configured first, then the domain of the agents to be configured. Therefore, if an agent with configured domain attempts to register or re-register with a master that does not have a configured domain, the registration attempt should be ignored. This is important, because the master would have no way of determining whether the agent is "remote" or not, which might result in placing inappropriate workloads on it. Diffs (updated) - src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f Diff: https://reviews.apache.org/r/59764/diff/4/ Changes: https://reviews.apache.org/r/59764/diff/3-4/ Testing --- `make check` Manual testing. Thanks, Neil Conway
Re: Review Request 59766: Changed allocator to offer remote resources to region-aware frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59766/ --- (Updated July 5, 2017, 10:32 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7614 https://issues.apache.org/jira/browse/MESOS-7614 Repository: mesos Description --- Changed allocator to offer remote resources to region-aware frameworks. Diffs (updated) - include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37 src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541 src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 src/tests/allocator.hpp a990788d5218bbcac499613783750ade022811a7 src/tests/api_tests.cpp cdaa72462eac1687185115771265539d8c2b09a9 src/tests/master_allocator_tests.cpp f83ca66e64fe6d2d10d104ff24f31afd84c50a3a src/tests/master_quota_tests.cpp bbdbfbe9d9960cdc9fa7b2d56e9da9122d25db6a src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f src/tests/reservation_tests.cpp 3b4884be21a0b4f15bcbc1a58fa4a4ad7858a7fb src/tests/resource_offers_tests.cpp 427a6520034a537caf748a126001d131e196fedd src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 Diff: https://reviews.apache.org/r/59766/diff/4/ Changes: https://reviews.apache.org/r/59766/diff/3-4/ Testing --- `make check`. Thanks, Neil Conway
Re: Review Request 59760: Added REGION_AWARE framework capability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59760/ --- (Updated July 5, 2017, 10:31 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7609 https://issues.apache.org/jira/browse/MESOS-7609 Repository: mesos Description --- Added REGION_AWARE framework capability. Diffs (updated) - include/mesos/mesos.proto 4e4b2790615e8bbbf262ba809cbfd67e24368952 include/mesos/v1/mesos.proto b8625d363388bc707bc2458abd47a218cc7b7888 src/common/protobuf_utils.hpp 5476d2e429cebc649405504f02169de318f65fd6 src/tests/protobuf_utils_tests.cpp 6ec3a8e606e86406a0f1c9d5de4ed09d0811e49a Diff: https://reviews.apache.org/r/59760/diff/4/ Changes: https://reviews.apache.org/r/59760/diff/3-4/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 59763: Caused master to abort when joining a mixed-region cluster.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59763/ --- (Updated July 5, 2017, 10:32 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7611 https://issues.apache.org/jira/browse/MESOS-7611 Repository: mesos Description --- That is, if a standby master is configured to use region X but it learns that the current master has region Y, the standby master will abort with an error message. This enforces the requirement that all masters in a single Mesos cluster are configured to use the same region (they can be configured to use different zones in that region, however). To allow graceful upgrades, we only abort the standby master if both the standby master and the leading master have a configured domain; if either master has the unset (default) domain, the standby master does not abort. NOTE: It would be nice to have unit tests to validate this behavior, but the current unit test infrastructure does not support starting multiple masters (MESOS-2976). Diffs (updated) - include/mesos/type_utils.hpp d2a6591b65b95b4e37e6a87dd3b11377834daf85 include/mesos/v1/mesos.hpp 2479b004f342ffcb05279e3b12b6de6949190a93 src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 Diff: https://reviews.apache.org/r/59763/diff/3/ Changes: https://reviews.apache.org/r/59763/diff/2-3/ Testing --- `make check` Manual testing. Thanks, Neil Conway
Re: Review Request 59759: Added protobuf definitions for fault domains.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59759/ --- (Updated July 5, 2017, 10:31 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7608 https://issues.apache.org/jira/browse/MESOS-7608 Repository: mesos Description --- This introduces a first-class notion of a "domain", which is a set of hosts that have similar characteristics. Mesos will initially only support "fault domains", which identify groups of hosts with similar failure characteristics. Diffs (updated) - include/mesos/mesos.proto 4e4b2790615e8bbbf262ba809cbfd67e24368952 include/mesos/type_utils.hpp d2a6591b65b95b4e37e6a87dd3b11377834daf85 include/mesos/v1/mesos.hpp 2479b004f342ffcb05279e3b12b6de6949190a93 include/mesos/v1/mesos.proto b8625d363388bc707bc2458abd47a218cc7b7888 src/common/type_utils.cpp aeb16238e9c3fd71a8e9e57dbb8098ddc004e1f2 src/v1/mesos.cpp 423510ef14025dba208ef85edf5305c2ce58f01d Diff: https://reviews.apache.org/r/59759/diff/2/ Changes: https://reviews.apache.org/r/59759/diff/1-2/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 59761: Added master and agent flags to specify domain.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59761/ --- (Updated July 5, 2017, 10:31 p.m.) Review request for mesos and Vinod Kone. Changes --- Rebase. Bugs: MESOS-7610 https://issues.apache.org/jira/browse/MESOS-7610 Repository: mesos Description --- Added master and agent flags to specify domain. Diffs (updated) - docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 src/master/flags.hpp 8a9bd2dfa513f1de85dab2cc3cc1d78e80293837 src/master/flags.cpp ca5f02c728a9f39abbbee59fa169f95d20c69d3d src/slave/flags.hpp c6803eb54e09a5497755e1e5fef2872193eacba6 src/slave/flags.cpp 398768656b5fa3b7c85474de2b4b008bf7b85cb3 Diff: https://reviews.apache.org/r/59761/diff/3/ Changes: https://reviews.apache.org/r/59761/diff/2-3/ Testing --- `make check` Thanks, Neil Conway
Review Request 60485: Fixed flaky PersistentVolumeEndpointsTest.ReserveAndSlaveRemoval.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60485/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-7725 https://issues.apache.org/jira/browse/MESOS-7725 Repository: mesos Description --- Fixed flaky PersistentVolumeEndpointsTest.ReserveAndSlaveRemoval. Diffs - src/tests/persistent_volume_endpoints_tests.cpp 518bdf858096ec9bcfa4f899ead5a6c3d103c521 Diff: https://reviews.apache.org/r/60485/diff/1/ Testing --- `make check` Thanks, Neil Conway
Review Request 60417: Updated comments and help text in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60417/ --- Review request for mesos and Adam B. Repository: mesos Description --- Updated comments and help text in mesos-execute. Diffs - src/cli/execute.cpp 58eaa47bf8388424fd42f7830fdbb7cdecbebb7b Diff: https://reviews.apache.org/r/60417/diff/1/ Testing --- Visual inspection of help output. Thanks, Neil Conway
Review Request 60416: Avoided master crash on re-registration of old agents.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60416/ --- Review request for mesos and Adam B. Repository: mesos Description --- At present, agents that are refinement-capable send their task and executor resources in the post-refinement format, whereas older agents use the pre-refinement format. However, the master code only converted agent resources to post-refinement format if the agent was not multi-role capable. This leads to a subsequent `CHECK` failure for agents that have multi-role but not reservation-refinement capabilities. We should instead convert to post-refinement format for all non-refinement capable agents. Diffs - src/master/master.cpp 33eca0d17459781fdc2ea915e8f40c78dd306962 Diff: https://reviews.apache.org/r/60416/diff/1/ Testing --- `make check`, manual testing. Thanks, Neil Conway
Review Request 60415: Added comment to master logic for downgrading checkpointed resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60415/ --- Review request for mesos and Adam B. Repository: mesos Description --- Added comment to master logic for downgrading checkpointed resources. Diffs - src/master/master.cpp 33eca0d17459781fdc2ea915e8f40c78dd306962 Diff: https://reviews.apache.org/r/60415/diff/1/ Testing --- Thanks, Neil Conway
Re: Review Request 60407: Avoided master crash on agent re-registration.
> On June 24, 2017, 2:35 a.m., Adam B wrote: > > src/master/validation.cpp > > Line 370 (original), 370-372 (patched) > > <https://reviews.apache.org/r/60407/diff/2/?file=1761693#file1761693line370> > > > > Even if we can't `validateDynamicReservationInfo` (or > > `validateDiskInfo`?), would it be worthwhile to `validateGpus`? Maybe > > clone/parameterize `resource::validate` to validate what we can? We could certainly do better, but since all this validation is best-effort anyway, I think the current approach is okay for now. I added `TODO`s to note this for future improvement. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60407/#review178836 --- On June 24, 2017, 7:28 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60407/ > --- > > (Updated June 24, 2017, 7:28 p.m.) > > > Review request for mesos, Adam B and Michael Park. > > > Repository: mesos > > > Description > --- > > When validating the agent's ReregisterSlaveMessage, the master's > validation code neglected to account for the fact that the task > resources might not be in post-refinement format (e.g., if the agent > does not support reservation refinement). This lead to a `CHECK` failure > during validation. > > Fix this by relaxing the validation of ReregisterSlaveMessage so that we > do not depend on the task resources being in post-refinement > format. This means validation of ReregisterSlaveMessage will be less > effective, but since it is best-effort anyway, this seems tolerable. > > > Diffs > - > > src/master/validation.cpp 33e9ff7db9e2789cbb2d6dfd015288dfa1faa7c5 > > > Diff: https://reviews.apache.org/r/60407/diff/3/ > > > Testing > --- > > `make check` > > > Thanks, > > Neil Conway > >
Re: Review Request 60407: Avoided master crash on agent re-registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60407/ --- (Updated June 24, 2017, 7:28 p.m.) Review request for mesos, Adam B and Michael Park. Changes --- Add comments / TODOs. Repository: mesos Description --- When validating the agent's ReregisterSlaveMessage, the master's validation code neglected to account for the fact that the task resources might not be in post-refinement format (e.g., if the agent does not support reservation refinement). This lead to a `CHECK` failure during validation. Fix this by relaxing the validation of ReregisterSlaveMessage so that we do not depend on the task resources being in post-refinement format. This means validation of ReregisterSlaveMessage will be less effective, but since it is best-effort anyway, this seems tolerable. Diffs (updated) - src/master/validation.cpp 33e9ff7db9e2789cbb2d6dfd015288dfa1faa7c5 Diff: https://reviews.apache.org/r/60407/diff/3/ Changes: https://reviews.apache.org/r/60407/diff/2-3/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60405: Documented resource format in agent <-> master protocol.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60405/ --- (Updated June 24, 2017, 5:30 p.m.) Review request for mesos, Adam B and Michael Park. Changes --- Clarify comment. Repository: mesos Description --- Documented resource format in agent <-> master protocol. Diffs (updated) - src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c Diff: https://reviews.apache.org/r/60405/diff/3/ Changes: https://reviews.apache.org/r/60405/diff/2-3/ Testing --- Thanks, Neil Conway
Re: Review Request 60405: Documented resource format in agent <-> master protocol.
> On June 24, 2017, 2:21 a.m., Adam B wrote: > > src/slave/slave.cpp > > Lines 1405-1407 (original), 1405-1408 (patched) > > <https://reviews.apache.org/r/60405/diff/2/?file=1761692#file1761692line1405> > > > > If this agent has refinements, and we send post format to an old > > master, will the old master safely reject the registration, crash and burn, > > or something in between? The master will basically consider the resources to be unreserved; because the master and agent will have inconsistent views of the resource state at the agent, this will cause problems. Since you need a new master to create reservation refinements in the first place, you can only arrive in this situation by: Upgrading master Upgrading agent Creating res refinement Downgrading master Which arguably falls under the "don't downgrade if you are using new featues" bucket. But yes, this is certainly unfortunate. Hard to prevent without introducing something similar to master capabilities, which we definitely need (MESOS-5675). I'll drop this issue for now, since AFAIK there's not much we can do to improve this in the short term. > On June 24, 2017, 2:21 a.m., Adam B wrote: > > src/slave/slave.cpp > > Line 1408 (original), 1412-1414 (patched) > > <https://reviews.apache.org/r/60405/diff/2/?file=1761692#file1761692line1412> > > > > We could at least log an INFO/WARN if we aren't able to downgrade, and > > still send it anyway. Hmm, not sure a warning/log is warranted here. In the common case (refined reservation, >= 1.4 master, >= 1.4 agent), we will fail to downgrade the resources, but that is fine and expected. Should we really be cluttering the logs with this information? - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60405/#review178834 --- On June 24, 2017, 1:48 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60405/ > --- > > (Updated June 24, 2017, 1:48 a.m.) > > > Review request for mesos, Adam B and Michael Park. > > > Repository: mesos > > > Description > --- > > Documented resource format in agent <-> master protocol. > > > Diffs > - > > src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 > src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c > > > Diff: https://reviews.apache.org/r/60405/diff/2/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 60405: Documented resource format in agent <-> master protocol.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60405/#review178844 --- src/slave/slave.cpp Lines 1405-1407 (original), 1405-1408 (patched) <https://reviews.apache.org/r/60405/#comment253098> The master will basically consider the resources to be unreserved; because the master and agent will have inconsistent views of the resource state at the agent, this will cause problems. Since you need a new master to create reservation refinements in the first place, you can only arrive in this situation by: 1. Upgrading master 2. Upgrading agent 3. Creating res refinement 4. Downgrading master Which arguably falls under the "don't downgrade if you are using new featues" bucket. But yes, this is certainly unfortunate. Hard to prevent without introducing something similar to master capabilities, which we definitely need (MESOS-5675). I'll drop this issue for now, since AFAIK there's not much we can do to improve this in the short term. src/slave/slave.cpp Line 1408 (original), 1412-1414 (patched) <https://reviews.apache.org/r/60405/#comment253100> Hmm, not sure a warning/log is warranted here. In the common case (refined reservation, >= 1.4 master, >= 1.4 agent), we will fail to downgrade the resources, but that is fine and expected. Should we really be cluttering the logs with this information? - Neil Conway On June 24, 2017, 1:48 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60405/ > --- > > (Updated June 24, 2017, 1:48 a.m.) > > > Review request for mesos, Adam B and Michael Park. > > > Repository: mesos > > > Description > --- > > Documented resource format in agent <-> master protocol. > > > Diffs > - > > src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 > src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c > > > Diff: https://reviews.apache.org/r/60405/diff/2/ > > > Testing > --- > > > Thanks, > > Neil Conway > >
Re: Review Request 60407: Avoided master crash on agent re-registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60407/ --- (Updated June 24, 2017, 1:47 a.m.) Review request for mesos and Michael Park. Changes --- Tweak comment. Repository: mesos Description --- When validating the agent's ReregisterSlaveMessage, the master's validation code neglected to account for the fact that the task resources might not be in post-refinement format (e.g., if the agent does not support reservation refinement). This lead to a `CHECK` failure during validation. Fix this by relaxing the validation of ReregisterSlaveMessage so that we do not depend on the task resources being in post-refinement format. This means validation of ReregisterSlaveMessage will be less effective, but since it is best-effort anyway, this seems tolerable. Diffs (updated) - src/master/validation.cpp 33e9ff7db9e2789cbb2d6dfd015288dfa1faa7c5 Diff: https://reviews.apache.org/r/60407/diff/2/ Changes: https://reviews.apache.org/r/60407/diff/1-2/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60404: Documented the content of the `SlaveInfo.resources` field.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60404/ --- (Updated June 24, 2017, 12:32 a.m.) Review request for mesos and Michael Park. Changes --- Rebase. Repository: mesos Description --- Documented the content of the `SlaveInfo.resources` field. Diffs (updated) - include/mesos/mesos.proto 38449da44a5288afc7e2ef17ef5e1380ecabd5d2 include/mesos/v1/mesos.proto 81eca47ce1074f5e522a0410f01ab2b2cfc76aa2 Diff: https://reviews.apache.org/r/60404/diff/2/ Changes: https://reviews.apache.org/r/60404/diff/1-2/ Testing --- Thanks, Neil Conway
Re: Review Request 60405: Documented resource format in agent <-> master protocol.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60405/ --- (Updated June 24, 2017, 12:31 a.m.) Review request for mesos and Michael Park. Changes --- Tweak comment. Repository: mesos Description --- Documented resource format in agent <-> master protocol. Diffs (updated) - src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c Diff: https://reviews.apache.org/r/60405/diff/2/ Changes: https://reviews.apache.org/r/60405/diff/1-2/ Testing --- Thanks, Neil Conway
Review Request 60407: Avoided master crash on agent re-registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60407/ --- Review request for mesos and Michael Park. Repository: mesos Description --- When validating the agent's ReregisterSlaveMessage, the master's validation code neglected to account for the fact that the task resources might not be in post-refinement format (e.g., if the agent does not support reservation refinement). This lead to a `CHECK` failure during validation. Fix this by relaxing the validation of ReregisterSlaveMessage so that we do not depend on the task resources being in post-refinement format. This means validation of ReregisterSlaveMessage will be less effective, but since it is best-effort anyway, this seems tolerable. Diffs - src/master/validation.cpp 33e9ff7db9e2789cbb2d6dfd015288dfa1faa7c5 Diff: https://reviews.apache.org/r/60407/diff/1/ Testing --- `make check` Thanks, Neil Conway
Review Request 60406: Added sanity check to resource downgrade on agent registration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60406/ --- Review request for mesos and Michael Park. Repository: mesos Description --- `SlaveInfo.resources` should always be representable in the "pre-reservation-refinement" format, so `downgradeResources` should always succeed. Diffs - src/slave/slave.cpp f808458849bb9667a91abe18868751d377d36e0c Diff: https://reviews.apache.org/r/60406/diff/1/ Testing --- Thanks, Neil Conway
Review Request 60405: Documented resource format in agent <-> master protocol.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60405/ --- Review request for mesos and Michael Park. Repository: mesos Description --- Documented resource format in agent <-> master protocol. Diffs - src/messages/messages.proto 2c086263fdcee4d54a76a61379c2d4dba5271d23 Diff: https://reviews.apache.org/r/60405/diff/1/ Testing --- Thanks, Neil Conway
Review Request 60404: Documented the content of the `SlaveInfo.resources` field.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60404/ --- Review request for mesos and Michael Park. Repository: mesos Description --- Documented the content of the `SlaveInfo.resources` field. Diffs - include/mesos/mesos.proto 38449da44a5288afc7e2ef17ef5e1380ecabd5d2 include/mesos/v1/mesos.proto 81eca47ce1074f5e522a0410f01ab2b2cfc76aa2 src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 Diff: https://reviews.apache.org/r/60404/diff/1/ Testing --- Thanks, Neil Conway
Re: Review Request 60351: Updated the `operator<<` for repeated resources to use `JSON::protobuf`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60351/#review178758 --- Ship it! Ship It! - Neil Conway On June 22, 2017, 12:08 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60351/ > --- > > (Updated June 22, 2017, 12:08 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > The `Resource` objects in a `RepeatedPtrField` may not be > validated and/or not converted to the "post-reservation-refinement" > format. Since `Resources` requires the resources to have been validated > and converted, we cannot rely on the conversion to be meaningful. > We opt to print the resources in their JSON representation instead. > > > Diffs > - > > src/common/resources.cpp 1d9170a9836799cf3764fbb37cd2e2aebd3e75b3 > src/v1/resources.cpp 58a00e933ad16481d3bdec6e273154ea93221830 > > > Diff: https://reviews.apache.org/r/60351/diff/3/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60352: Relaxed the resource format restriction for frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60352/#review178757 --- Ship it! Per offline discussion, add more on rationale to commit message. - Neil Conway On June 22, 2017, 12:32 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60352/ > --- > > (Updated June 22, 2017, 12:32 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > This patch relaxes the validation such that a framework with > RESERVATION_REFINEMENT capability can send the old resources format as > well. This is more of a temporary solution due to the complexity of > the validation code, and we will revisit whether we want to restrict > RESERVATION_REFINEMENT-capable frameworks to the new format. MESOS-7705. > > > Diffs > - > > src/master/validation.hpp 129505948c0fad28743e290f24df55918c8d601c > src/master/validation.cpp 4d0af26fedb6ef3039536cb42d2ac9112c9f4bdd > > > Diff: https://reviews.apache.org/r/60352/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60283/#review178755 --- Fix it, then Ship it! src/tests/master_tests.cpp Lines 7367 (patched) <https://reviews.apache.org/r/60283/#comment252927> We use `EXPECT_EQ` in the other tests here. - Neil Conway On June 22, 2017, 10:22 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60283/ > --- > > (Updated June 22, 2017, 10:22 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Initially, it seemed like calling `convertResourceFormat` after > operation validation seemed safe since the operation validation > themselves performed `Resources::validate` within them. > > However, the rest of the operation validation code relies on > the fact that the resources have been validated, and uses > functions such as `isDynamicallyReserved`. Since functions such as > `isDynamicallyReserved` now requires "post-reservation-refinement" > format, we must perform this conversion earlier. > > In this patch, we use `upgradeResources` to perform resources > validation __and__ convert the resources before going into the > operation and task validation. > > We really need a better plan for this going forward. MESOS-7702. > > > Diffs > - > > src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf > src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 > src/tests/resource_offers_tests.cpp > c2bbf834c1d46079af492887b9dd40e57f3f2ac7 > > > Diff: https://reviews.apache.org/r/60283/diff/9/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60284: Removed unused `convertResourceFormat` for `Operation`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60284/#review178752 --- Ship it! Ship It! - Neil Conway On June 21, 2017, 7:08 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60284/ > --- > > (Updated June 21, 2017, 7:08 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Removed unused `convertResourceFormat` for `Operation`. > > > Diffs > - > > src/common/resources_utils.hpp 4b033687fa7224ffa36523cb3ca366f4384fd4f8 > src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 > > > Diff: https://reviews.apache.org/r/60284/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60255: Prevented reserve/create with refined reservation on non-capable agents.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60255/#review178751 --- Ship it! We can do this later, but would be nice to have unit tests for this -- similar to `CreateOperationValidationTest.AgentHierarchicalRoleCapability` - Neil Conway On June 20, 2017, 11:58 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60255/ > --- > > (Updated June 20, 2017, 11:58 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7700 > https://issues.apache.org/jira/browse/MESOS-7700 > > > Repository: mesos > > > Description > --- > > This patch makes it such that reserve / create operations involving > resources with refined reservations are invalid if they are attempted > on an agent without a RESERVATION_REFINEMENT capability. Such attempts > from an operator endpoint will result in a `BadRequest` response. > On the framework side, the operation will be dropped by the master. > > > Diffs > - > > src/master/validation.cpp 4d0af26fedb6ef3039536cb42d2ac9112c9f4bdd > > > Diff: https://reviews.apache.org/r/60255/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60283/#review178746 --- Fix it, then Ship it! src/master/master.cpp Line 4222 (original), 4222 (patched) <https://reviews.apache.org/r/60283/#comment252914> remove "after validation" src/master/master.cpp Lines 4760 (patched) <https://reviews.apache.org/r/60283/#comment252915> Per discussion, would be good to consider whether to validate/upgrade a task and its resources in a single operation. src/tests/master_tests.cpp Lines 7130 (patched) <https://reviews.apache.org/r/60283/#comment252916> `using` for `google::protobuf::RepeatedPtrField` ? src/tests/master_tests.cpp Lines 7132 (patched) <https://reviews.apache.org/r/60283/#comment252917> Can we add a comment here? ``` // If reservation refinement is enabled, inbound // resources should already be in the "post-refinement" format and should not need to be upgraded. ``` src/tests/master_tests.cpp Lines 7143 (patched) <https://reviews.apache.org/r/60283/#comment252924> Comment would be helpful here also. src/tests/master_tests.cpp Lines 7201 (patched) <https://reviews.apache.org/r/60283/#comment252918> Can we remove the `url` stuff? src/tests/master_tests.cpp Lines 7249 (patched) <https://reviews.apache.org/r/60283/#comment252919> Update or remove this comment. src/tests/master_tests.cpp Lines 7403 (patched) <https://reviews.apache.org/r/60283/#comment252921> "a RESERVE" src/tests/master_tests.cpp Lines 7406 (patched) <https://reviews.apache.org/r/60283/#comment252920> "an UNRESERVE" src/tests/master_tests.cpp Lines 7407 (patched) <https://reviews.apache.org/r/60283/#comment252922> "we test" - Neil Conway On June 21, 2017, 7:08 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60283/ > --- > > (Updated June 21, 2017, 7:08 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Initially, it seemed like calling `convertResourceFormat` after > operation validation seemed safe since the operation validation > themselves performed `Resources::validate` within them. > > However, the rest of the operation validation code relies on > the fact that the resources have been validated, and uses > functions such as `isDynamicallyReserved`. Since functions such as > `isDynamicallyReserved` now requires "post-reservation-refinement" > format, we must perform this conversion earlier. > > In this patch, we use `upgradeResources` to perform resources > validation __and__ convert the resources before going into the > operation and task validation. > > We really need a better plan for this going forward. MESOS-7702. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 > src/tests/resource_offers_tests.cpp > c2bbf834c1d46079af492887b9dd40e57f3f2ac7 > > > Diff: https://reviews.apache.org/r/60283/diff/8/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60374: Required that resources are always downgraded for a non-capable agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60374/#review178743 --- Ship it! Ship It! - Neil Conway On June 22, 2017, 7:09 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60374/ > --- > > (Updated June 22, 2017, 7:09 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7575 > https://issues.apache.org/jira/browse/MESOS-7575 > > > Repository: mesos > > > Description > --- > > Since the master prevents refined reservations from being created on > non-capable agents, we cannot be in a situation in which the resources > are not downgradable for a non-capable agent. > > > Diffs > - > > src/master/master.cpp b0818e1e59ce9804cd9592aa2a7ec8f80ba5bddf > > > Diff: https://reviews.apache.org/r/60374/diff/2/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Review Request 60377: Added test for reservation refinements and old agents.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60377/ --- Review request for mesos and Michael Park. Repository: mesos Description --- If the agent does not support reservation refinements, attempts to create a reservation refinement on the agent should be dropped by the master. Diffs - src/tests/upgrade_tests.cpp eef81018ab3fae2e91077d4314cdaba274111401 Diff: https://reviews.apache.org/r/60377/diff/1/ Testing --- `make check`, ran ~5k iterations w/o error. Thanks, Neil Conway
Re: Review Request 60281: Avoided sending an inconsistent `TaskInfo` to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60281/#review178740 --- src/master/master.cpp Line 4483 (original), 4483 (patched) <https://reviews.apache.org/r/60281/#comment252901> This comment needs updating. - Neil Conway On June 21, 2017, 7:06 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60281/ > --- > > (Updated June 21, 2017, 7:06 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Since we drop invalid tasks on a `LAUNCH` operation, we send a new > `Operation` with only the valid tasks appended. At the same time, > we adjust the `TaskInfo` a little bit to perform validation, and > to maintain backwards compatibility. However, the unmodified > `TaskInfo` was sent to the allocator. Since the allocator does not > look at the modified fields, this was inconsequential, but we should > avoid sending inconsistent `TaskInfo`s in general. > > We also avoided creating an unnecessary copy of the `TaskInfo`. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60281/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60374: Required that resources are always downgraded for a non-capable agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60374/#review178737 --- src/master/master.cpp Line 4919 (original), 4907 (patched) <https://reviews.apache.org/r/60374/#comment252897> Can we add a comment explaining why these are `CHECK`s? - Neil Conway On June 22, 2017, 7:09 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60374/ > --- > > (Updated June 22, 2017, 7:09 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7575 > https://issues.apache.org/jira/browse/MESOS-7575 > > > Repository: mesos > > > Description > --- > > Since the master prevents refined reservations from being created on > non-capable agents, we cannot be in a situation in which the resources > are not downgradable for a non-capable agent. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60374/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60283/#review178707 --- src/master/master.cpp Line 4680 (original), 4698 (patched) <https://reviews.apache.org/r/60283/#comment252862> Not in this RR, but this seems dubious to me: if we're not able to launch a task with refined reservations on an old agent, we skip sending `RunTaskMessage` but we _do_ add the task to `operations`, which is later passed to the allocator. Seems wrong. Also, do we have a test case for this situation? src/master/master.cpp Lines 4754 (patched) <https://reviews.apache.org/r/60283/#comment252863> I wonder if we want `validateAndUpgradeResources` of a `TaskInfo`. src/tests/master_tests.cpp Lines 7200 (patched) <https://reviews.apache.org/r/60283/#comment252856> Whitespace. src/tests/master_tests.cpp Lines 7202 (patched) <https://reviews.apache.org/r/60283/#comment252857> Can we remove this? Seems irrelevant to the test's purpose. - Neil Conway On June 21, 2017, 7:08 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60283/ > --- > > (Updated June 21, 2017, 7:08 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Initially, it seemed like calling `convertResourceFormat` after > operation validation seemed safe since the operation validation > themselves performed `Resources::validate` within them. > > However, the rest of the operation validation code relies on > the fact that the resources have been validated, and uses > functions such as `isDynamicallyReserved`. Since functions such as > `isDynamicallyReserved` now requires "post-reservation-refinement" > format, we must perform this conversion earlier. > > In this patch, we use `upgradeResources` to perform resources > validation __and__ convert the resources before going into the > operation and task validation. > > We really need a better plan for this going forward. MESOS-7702. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 > src/tests/resource_offers_tests.cpp > c2bbf834c1d46079af492887b9dd40e57f3f2ac7 > > > Diff: https://reviews.apache.org/r/60283/diff/5/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60351: Used `JSON::protobuf` to print resources not validated nor converted.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60351/#review178702 --- I wonder if we should just change the definition of `operator<<` for `RepeatedPtrField`. The current behavior is to silently omit printing invalid resources, which seems very misleading. This would also avoid the risk of random `CHECK` failures if a code path attempts to print a resource before upgrading it. - Neil Conway On June 22, 2017, 12:08 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60351/ > --- > > (Updated June 22, 2017, 12:08 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > During the authorization phase, the resources have not been validated > not converted to the "post-reservation-refinement" format. > We can't rely on any operations that require valid resources and/or > "post-reservation-refinement" format. `operator<<` is one of those > functions, and here we print out the JSON representation of the > resources instead. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60351/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60282: Introduced `validateAndUpgradeResources`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60282/#review178701 --- Fix it, then Ship it! "Validation code for operations, and tasks" in commit message: remove the comma. "a `upgradeResources`" in commit message: "a `validateAndUpgradeResources`" src/common/resources_utils.hpp Lines 141 (patched) <https://reviews.apache.org/r/60282/#comment252854> "in their previous format" => "unchanged". - Neil Conway On June 21, 2017, 7:07 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60282/ > --- > > (Updated June 21, 2017, 7:07 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Validation code for operations, and tasks validated the resources as > the first step, and assumed valid resources from then on. This means > that it started to use functions such as `isDynamicallyReserved`. > > However, since `isDynamicallyReserved` requires the resources to be > in the "post-reservation-refinement" format, we must convert the > resources before using those functions. For now, we introduce > a `upgradeResources` abstraction which is called to validate and > convert the resources before invoking operation / task validation. > > > Diffs > - > > src/common/resources_utils.hpp 4b033687fa7224ffa36523cb3ca366f4384fd4f8 > src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 > > > Diff: https://reviews.apache.org/r/60282/diff/3/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60281: Added the adjusted `TaskInfo` into the `Operation` to be sent out.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60281/#review178700 --- Ship it! Can we clarify the commit message that the incorrect `TaskInfo` is actually being sent to the allocator? The agent gets the correct `TaskInfo`. - Neil Conway On June 21, 2017, 7:06 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60281/ > --- > > (Updated June 21, 2017, 7:06 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Since we drop invalid tasks on a `LAUNCH` operation, we send a new > `Operation` with only the valid tasks appended. At the same time, > we adjust the `TaskInfo` a little bit to perform validation, and > to maintain backwards compatibility. In the end, we were appending > the unadjusted task to the new `Operation` rather than the adjusted > task. This patch changes this to append the adjusted task instead. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60281/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60281: Added the adjusted `TaskInfo` into the `Operation` to be sent out.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60281/#review178623 --- src/master/master.cpp Line 4558 (original), 4558 (patched) <https://reviews.apache.org/r/60281/#comment252729> If we're already copying the operation (see `foreach` a few lines up), do we need to make an additional copy of the `TaskInfo`? If we mutate w/o copying, we'd avoid the need to introduce a second variable. - Neil Conway On June 21, 2017, 7:06 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60281/ > --- > > (Updated June 21, 2017, 7:06 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Since we drop invalid tasks on a `LAUNCH` operation, we send a new > `Operation` with only the valid tasks appended. At the same time, > we adjust the `TaskInfo` a little bit to perform validation, and > to maintain backwards compatibility. In the end, we were appending > the unadjusted task to the new `Operation` rather than the adjusted > task. This patch changes this to append the adjusted task instead. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60281/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60283: Fixed `convertResourceFormat` uses with `validateAndUpgradeResources`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60283/#review178622 --- src/master/master.cpp Line 4221 (original), 4221 (patched) <https://reviews.apache.org/r/60283/#comment252728> Update this comment. - Neil Conway On June 21, 2017, 7:08 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60283/ > --- > > (Updated June 21, 2017, 7:08 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Initially, it seemed like calling `convertResourceFormat` after > operation validation seemed safe since the operation validation > themselves performed `Resources::validate` within them. > > However, the rest of the operation validation code relies on > the fact that the resources have been validated, and uses > functions such as `isDynamicallyReserved`. Since functions such as > `isDynamicallyReserved` now requires "post-reservation-refinement" > format, we must perform this conversion earlier. > > In this patch, we use `upgradeResources` to perform resources > validation __and__ convert the resources before going into the > operation and task validation. > > We really need a better plan for this going forward. MESOS-7702. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > src/tests/master_tests.cpp 652f37ab6f640ac87c48e419680501b705e11394 > src/tests/resource_offers_tests.cpp > c2bbf834c1d46079af492887b9dd40e57f3f2ac7 > > > Diff: https://reviews.apache.org/r/60283/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60284: Removed unused `convertResourceFormat` for `Operation`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60284/#review178537 --- src/common/resources_utils.cpp Line 198 (original) <https://reviews.apache.org/r/60284/#comment252588> Also need to update the header file. - Neil Conway On June 21, 2017, 7:08 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60284/ > --- > > (Updated June 21, 2017, 7:08 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Removed unused `convertResourceFormat` for `Operation`. > > > Diffs > - > > src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 > > > Diff: https://reviews.apache.org/r/60284/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60283: Fixed the uses of `convertResourceFormat` with `upgradeResources`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60283/#review178536 --- src/master/master.cpp Lines 4234 (patched) <https://reviews.apache.org/r/60283/#comment252584> Per offline discussion, naming this `upgradeResources_` vs validation errors as `error` is confusing. Call them both `error` and change the upgrade API? src/master/master.cpp Line 4302 (original), 4316 (patched) <https://reviews.apache.org/r/60283/#comment252577> Can we revert this formatting change? src/master/master.cpp Line 4592 (original), 4647 (patched) <https://reviews.apache.org/r/60283/#comment252585> Not yours, but using a reference here is strange. src/master/master.cpp Lines 4784 (patched) <https://reviews.apache.org/r/60283/#comment252587> If we can use `Option` for `upgradeResources`, can we avoid duplicating all this error handling code? - Neil Conway On June 21, 2017, 7:08 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60283/ > --- > > (Updated June 21, 2017, 7:08 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Initially, it seemed like calling `convertResourceFormat` after > operation validation seemed safe since the operation validation > themselves performed `Resources::validate` within them. > > However, the rest of the operation validation code relies on > the fact that the resources have been validated, and uses > functions such as `isDynamicallyReserved`. Since functions such as > `isDynamicallyReserved` now requires "post-reservation-refinement" > format, we must perform this conversion earlier. > > In this patch, we use `upgradeResources` to perform resources > validation __and__ convert the resources before going into the > operation and task validation. > > We really need a better plan for this going forward. MESOS-7702. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60283/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60282: Introduced `upgradeResources` which also validates the resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60282/#review178535 --- src/common/resources_utils.cpp Lines 271 (patched) <https://reviews.apache.org/r/60282/#comment252576> Should we put `validate` in the name, e.g., `validateAndUpgradeResources`? As currently named, the fact that it does validation is not obvious. Also, the current name suggests it is symmetric with `downgradeResources`, which is not quite true. src/common/resources_utils.cpp Lines 274 (patched) <https://reviews.apache.org/r/60282/#comment252583> Hmmm -- this means that we're going to validate most resources twice in most code paths, right? That seems unfortunate, although I guess it isn't easy to avoid. - Neil Conway On June 21, 2017, 7:07 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60282/ > --- > > (Updated June 21, 2017, 7:07 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Validation code for operations, and tasks validated the resources as > the first step, and assumed valid resources from then on. This means > that it started to use functions such as `isDynamicallyReserved`. > > However, since `isDynamicallyReserved` requires the resources to be > in the "post-reservation-refinement" format, we must convert the > resources before using those functions. For now, we introduce > a `upgradeResources` abstraction which is called to validate and > convert the resources before invoking operation / task validation. > > > Diffs > - > > src/common/resources_utils.hpp 4b033687fa7224ffa36523cb3ca366f4384fd4f8 > src/common/resources_utils.cpp 751194c5ff5c794c77be3bcf16892af39de6d4d9 > > > Diff: https://reviews.apache.org/r/60282/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60281: Added the adjusted `TaskInfo` into the `Operation` to be sent out.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60281/#review178533 --- Can we write a unit test for this change? - Neil Conway On June 21, 2017, 7:06 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60281/ > --- > > (Updated June 21, 2017, 7:06 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > Since we drop invalid tasks on a `LAUNCH` operation, we send a new > `Operation` with only the valid tasks appended. At the same time, > we adjust the `TaskInfo` a little bit to perform validation, and > to maintain backwards compatibility. In the end, we were appending > the unadjusted task to the new `Operation` rather than the adjusted > task. This patch changes this to append the adjusted task instead. > > > Diffs > - > > src/master/master.cpp ec594a8f4fa95e77fc38103c5561d1797fe2b133 > > > Diff: https://reviews.apache.org/r/60281/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60254: Added missing `RESERVATION_REFINEMENT` capability to examples and tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60254/#review178446 --- Ship it! Ship It! - Neil Conway On June 20, 2017, 11:35 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60254/ > --- > > (Updated June 20, 2017, 11:35 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > This is a follow-up to https://reviews.apache.org/r/60073/. > We missed a few places in root tests and examples which aren't part of > the test suite. > > > Diffs > - > > src/cli/execute.cpp 6ab0cb48c9226e66b49580c7c2a2c9af3019d77d > src/examples/balloon_framework.cpp 51b01cfd52d7b353e48e81c7a7deaff6920e > src/examples/docker_no_executor_framework.cpp > 657a02a22c457543b39a2eb9df42a2aa70dea756 > src/examples/load_generator_framework.cpp > 7f1ada507eaa010d22b4611744fff89109c42065 > src/examples/long_lived_framework.cpp > bdf20a79a49c987bf34240bb7cb3a53a58097105 > src/tests/containerizer/docker_containerizer_tests.cpp > 8ac56ed947ad49c46d8a7313bbc568f8fae1d656 > > > Diff: https://reviews.apache.org/r/60254/diff/1/ > > > Testing > --- > > ```bash > (1) sudo make distcheck > (2) ./src/mesos-execute --role="ads" --master=127.0.0.1:5050 --name=blah > --command="while true; do : ; done" --resources="mem:1100;cpus:9" > (3) ./src/long-lived-framework --master=127.0.0.1:5050 > ``` > > > Thanks, > > Michael Park > >
Re: Review Request 60253: Placed the `convertResourceFormat` calls after resource validation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60253/#review178445
---
Ship it!
These particular fixes seem fine (and I couldn't spot any places we are
neglecting to get validation/conversion wrong, from a quick look), but the
current approach to input validation and conversion seems very error-prone to
me. We need to separately:
(a) get one or more `Resource` objects (e.g., via `protobuf::parse`)
(b) validate that the `Resource` is valid
(c) convert `Resource` to post-refinement format
(d) convert/aggregate `Resource` into `Resources`
(e) in many cases, add the `Resources` into an operation, which is itself
validated for a different set of criteria.
Doing all 5 things at each call-site seems regrettable. For example, we could
consider combining validation and format conversion into a single helper; or
perhaps have an "try adding `Resource` to `Resources`" that validates the
`Resource`, converts it to the right format, and then either returns the
updated `Resources` or an error if validation.
- Neil Conway
On June 21, 2017, 12:47 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60253/
> ---
>
> (Updated June 21, 2017, 12:47 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> `Resources` requires that the `Resource` objects being constructed with
> be validated and are in the "post-reservation-refinement" format.
> The `convertResourceFormat` was previously placed after the validation
> of operations, but we construct `Resources` prior to operation
> validation. This patch moves the conversion logic earlier to be done
> right after resource validation.
>
>
> Diffs
> -
>
> src/master/http.cpp 801b80933985a95d58f6b3b9973558d0c5a4410e
>
>
> Diff: https://reviews.apache.org/r/60253/diff/2/
>
>
> Testing
> ---
>
> ```bash
> curl -i -d slaveId=9091bf1d-58cb-4f54-b804-fe3eef85facd-S0 -d
> resources='[{"name":"cpus","type":"SCALAR","scalar":{"value":1},"role":"ads","reservation":{}},{"name":"mem","type":"SCALAR","scalar":{"value":1024},"role":"ads","reservation":{}}]'
> -X POST http://127.0.0.1:5050/master/reserve
> ```
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60038: Prevented allocating reservation refinements to non-capable frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60038/#review178444 --- FWIW, I find the commit message a bit confusing: "it's possible to translate the refined reservations into the old format" -- it seems to me it is explicitly _not_ possible to accurately translate refined reservations into the old format, which is the reason for this change. - Neil Conway On June 19, 2017, 9:52 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60038/ > --- > > (Updated June 19, 2017, 9:52 p.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Repository: mesos > > > Description > --- > > When reservation refinements are present, old frameworks without the > RESERVATION_REFINEMENT capability won't be able to understand the new > format. While it's possible to translate the refined reservations into > the old format by "hiding" the intermediate reservations in the "stack", > this leads to ambiguity when processing RESERVE / UNRESERVE operations. > This is due to the loss of information when we drop the intermediate > reservations. Therefore, for now we simply filter out resources with > refined reservations if the framework does not have the capability. > > > Diffs > - > > src/master/allocator/mesos/hierarchical.cpp > 775fe4115d5f55be1e8542fa4f94699396d9e2a1 > > > Diff: https://reviews.apache.org/r/60038/diff/5/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60244: Tidied up lambda capture list.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60244/#review178401 --- Ship it! Ship It! - Neil Conway On June 20, 2017, 2:22 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60244/ > --- > > (Updated June 20, 2017, 2:22 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > This commit removes capture of 'DISK_SIZE' which is neither used in > the lambda nor captured to extend its lifetime. > > > Diffs > - > > src/tests/role_tests.cpp aeef9a7b4062c62efe5036c0c2824c310be00f05 > > > Diff: https://reviews.apache.org/r/60244/diff/1/ > > > Testing > --- > > make check (clang-5) > > Without this patch this emits a fatal error, with this patch the code > compiles successfully. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 60218: Added a couple of missing validation logic + minor improvements.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60218/#review178335 --- Fix it, then Ship it! src/common/resources.cpp Lines 877 (patched) <https://reviews.apache.org/r/60218/#comment252238> Also need to update v1 - Neil Conway On June 20, 2017, 12:57 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60218/ > --- > > (Updated June 20, 2017, 12:57 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > The `ReservationInfo`s in `Resource.reservations` are required to have > the `type` and `role` fields. This patch adds validation logic to > enforce that. It also makes a few improvements in the error messages > by checking conditions more fine-grained. > > > Diffs > - > > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/master/validation.cpp 1436002b5e30670166c1564b0e354299cc3a25fd > > > Diff: https://reviews.apache.org/r/60218/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60219: Introduced `downgradeResources` to facilitate out-bound resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60219/#review178334 --- Ship it! - Neil Conway On June 20, 2017, 12:59 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60219/ > --- > > (Updated June 20, 2017, 12:59 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > This patch introduces `downgradeResources` utility which converts > resources in the "post-reservation-refinement" format to the > "pre-reservation-refinement" format. The function has all-or-nothing > semantics in that all are converted if none of the resources have > refined reservations and none are converted if any of the resources > have refined reservations. > > > Diffs > - > > src/common/resources_utils.hpp c7ec3bdc804026b01b5897a7aa1e4599be95ae3a > src/common/resources_utils.cpp aca2a0c630164af718007c70197461cebcd10d3e > > > Diff: https://reviews.apache.org/r/60219/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60070: Adjusted the agent to the new resources format [11/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60070/#review178316 --- src/common/resources_utils.hpp Lines 140 (patched) <https://reviews.apache.org/r/60070/#comment252199> ", and" -> "; in this case, " src/slave/slave.cpp Lines 1398 (patched) <https://reviews.apache.org/r/60070/#comment252200> "an" => "a" src/slave/state.hpp Lines 111 (patched) <https://reviews.apache.org/r/60070/#comment252202> is "send the result" accurate here? "checkpoint the result" instead? - Neil Conway On June 14, 2017, 9:56 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60070/ > ------- > > (Updated June 14, 2017, 9:56 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7666 > https://issues.apache.org/jira/browse/MESOS-7666 > > > Repository: mesos > > > Description > --- > > Adjusted the agent to the new resources format [11/N]. > > > Diffs > - > > src/common/resources_utils.hpp c7ec3bdc804026b01b5897a7aa1e4599be95ae3a > src/common/resources_utils.cpp aca2a0c630164af718007c70197461cebcd10d3e > src/slave/paths.cpp b2709ad3b111f102ff223182a30c993a0c643230 > src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d > src/slave/slave.cpp 91a1bf330c3d5e91ef1b5ed016f9b3d0ad78400f > src/slave/state.hpp a497ce1f58fb8dc7718ee5bb10bc62dd7479efa5 > src/slave/state.cpp 18b790d2cc4f537cc9b0c3cca59b9cbaac0eda10 > > > Diff: https://reviews.apache.org/r/60070/diff/6/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60190: Added the agent `RESERVATION_REFINEMENT` capability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60190/#review178315 --- Ship it! Ship It! - Neil Conway On June 19, 2017, 6:43 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60190/ > --- > > (Updated June 19, 2017, 6:43 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/slave/constants.cpp d3668e4dca515f42d88d01b0852a543bfdb360f6 > src/tests/master_tests.cpp 543c4a3afab818578c3124331a092cb1057776af > src/tests/slave_tests.cpp 26901f4294ee74e9f8ac57f2b506d369ea540a15 > > > Diff: https://reviews.apache.org/r/60190/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60210: Disabled emitting unset fields with default values in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60210/#review178304 --- Can we add a bit more context on the motivation here? i.e., are we always omitting default values to ensure we roundtrip, or is it just good practice in these situations, etc.? - Neil Conway On June 19, 2017, 8:43 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60210/ > --- > > (Updated June 19, 2017, 8:43 p.m.) > > > Review request for mesos, Neil Conway and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Used the new `JSON::protobuf` overload to avoid emitting unset fields. > > > Diffs > - > > src/tests/master_quota_tests.cpp 6ca45706f47b7d75db519f98af64e6a52091eb84 > src/tests/persistent_volume_endpoints_tests.cpp > 0cb969cf06a123504720440f436ecb027d1e138a > src/tests/persistent_volume_tests.cpp > 787141cdbe0188ba839d3bbd6e5cbb97ff78ce4b > src/tests/reservation_endpoints_tests.cpp > ba283ee74fe27998a01c43e1ab6a8ce0089181fa > src/tests/resources_tests.cpp a44490a4f4fbd3af3b95c9e9a167a96c6e0f40f9 > src/tests/role_tests.cpp 850dfa79646562bd3653a5fd0f079d0226b95970 > > > Diff: https://reviews.apache.org/r/60210/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60207: Added `emit_default_value` option to `JSON::protobuf`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60207/#review178303 --- I wonder if an `enum` would be clearer at call-sites than a boolean. e.g., `OMIT_DEFAULT_VALUES`, `INCLUDE_DEFAULT_VALUES`. - Neil Conway On June 19, 2017, 8:40 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60207/ > --- > > (Updated June 19, 2017, 8:40 p.m.) > > > Review request for mesos, Neil Conway and Vinod Kone. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > 3rdparty/stout/include/stout/protobuf.hpp > 2735354256f35d2792c3690bcc8fc61c3f6d9524 > > > Diff: https://reviews.apache.org/r/60207/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60209: Fixed the `serialize` function to respect the protobuf format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60209/#review178300 --- I'm a bit confused on when code should be using `modelProtobufJSON` vs. `JSON::protobuf(..., false)`. I guess `JSON::protobuf(..., false)` does omits default values in strictly _more_ situations than `modelProtobufJSON` does? src/common/http.cpp Line 115 (original), 115 (patched) <https://reviews.apache.org/r/60209/#comment252192> Can we add a comment explaining what is going on here? - Neil Conway On June 19, 2017, 8:40 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60209/ > --- > > (Updated June 19, 2017, 8:40 p.m.) > > > Review request for mesos, Neil Conway and Vinod Kone. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/http.cpp 98899a789610add924a7d2c0af943230cf914d2d > > > Diff: https://reviews.apache.org/r/60209/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60211: Added RESERVATION_REFINEMENT capability for example frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60211/#review178298 --- Ship it! Ship It! - Neil Conway On June 19, 2017, 8:45 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60211/ > --- > > (Updated June 19, 2017, 8:45 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/examples/disk_full_framework.cpp > a73e6cf285b9a74492d476a624915235e079051f > src/examples/dynamic_reservation_framework.cpp > 41228145cf2249e383166a47a3ac33fb2bee27c9 > src/examples/no_executor_framework.cpp > 77b7408391c9d3fe7bc103e363913b816e439447 > src/examples/persistent_volume_framework.cpp > ab4597d1fb61f631cd3f52479ae68dcc5e4cd394 > src/examples/test_framework.cpp 05ddc89d953d9b968b78f98fec819aeda7f79e26 > src/examples/test_http_framework.cpp > 471835c349e0da031a540ed48881227a25887ba7 > > > Diff: https://reviews.apache.org/r/60211/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60207: Added `emit_default_value` option to `JSON::protobuf`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60207/#review178294 --- Fix it, then Ship it! Needs test coverage. 3rdparty/stout/include/stout/protobuf.hpp Line 835 (original), 835 (patched) <https://reviews.apache.org/r/60207/#comment252183> Update this comment. - Neil Conway On June 19, 2017, 8:40 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60207/ > --- > > (Updated June 19, 2017, 8:40 p.m.) > > > Review request for mesos, Neil Conway and Vinod Kone. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > 3rdparty/stout/include/stout/protobuf.hpp > 2735354256f35d2792c3690bcc8fc61c3f6d9524 > > > Diff: https://reviews.apache.org/r/60207/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 59760: Added REGION_AWARE framework capability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59760/ --- (Updated June 19, 2017, 5:56 p.m.) Review request for mesos and Vinod Kone. Changes --- Tweak text. Bugs: MESOS-7609 https://issues.apache.org/jira/browse/MESOS-7609 Repository: mesos Description --- Added REGION_AWARE framework capability. Diffs (updated) - include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e include/mesos/v1/mesos.proto b9a9cc353342ab0ca613675a6246fed207efe42c src/common/protobuf_utils.hpp 5476d2e429cebc649405504f02169de318f65fd6 src/tests/protobuf_utils_tests.cpp 6ec3a8e606e86406a0f1c9d5de4ed09d0811e49a Diff: https://reviews.apache.org/r/59760/diff/3/ Changes: https://reviews.apache.org/r/59760/diff/2-3/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 59760: Added REGION_AWARE framework capability.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59760/ --- (Updated June 19, 2017, 5:51 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-7609 https://issues.apache.org/jira/browse/MESOS-7609 Repository: mesos Description --- Added REGION_AWARE framework capability. Diffs (updated) - include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e include/mesos/v1/mesos.proto b9a9cc353342ab0ca613675a6246fed207efe42c src/common/protobuf_utils.hpp 5476d2e429cebc649405504f02169de318f65fd6 src/tests/protobuf_utils_tests.cpp 6ec3a8e606e86406a0f1c9d5de4ed09d0811e49a Diff: https://reviews.apache.org/r/59760/diff/2/ Changes: https://reviews.apache.org/r/59760/diff/1-2/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60070: Adjusted the agent to the new resources format [11/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60070/#review178194 --- src/slave/slave.cpp Lines 1395 (patched) <https://reviews.apache.org/r/60070/#comment252060> Fix this TODO? src/slave/slave.cpp Lines 1401 (patched) <https://reviews.apache.org/r/60070/#comment252061> Fix this TODO? src/slave/state.hpp Line 106 (original), 107 (patched) <https://reviews.apache.org/r/60070/#comment252062> Fix this TODO? src/slave/state.cpp Lines 175 (patched) <https://reviews.apache.org/r/60070/#comment252066> Fix enum name. src/slave/state.cpp Lines 419 (patched) <https://reviews.apache.org/r/60070/#comment252067> Fix enum name. src/slave/state.cpp Lines 619 (patched) <https://reviews.apache.org/r/60070/#comment252063> Wrong enum name? src/slave/state.cpp Lines 785 (patched) <https://reviews.apache.org/r/60070/#comment252064> Fix enum name - Neil Conway On June 14, 2017, 9:56 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60070/ > --- > > (Updated June 14, 2017, 9:56 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7666 > https://issues.apache.org/jira/browse/MESOS-7666 > > > Repository: mesos > > > Description > --- > > Adjusted the agent to the new resources format [11/N]. > > > Diffs > - > > src/slave/paths.cpp b2709ad3b111f102ff223182a30c993a0c643230 > src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d > src/slave/slave.cpp 91a1bf330c3d5e91ef1b5ed016f9b3d0ad78400f > src/slave/state.hpp a497ce1f58fb8dc7718ee5bb10bc62dd7479efa5 > src/slave/state.cpp 18b790d2cc4f537cc9b0c3cca59b9cbaac0eda10 > > > Diff: https://reviews.apache.org/r/60070/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60189: Prevented allocating non-capable agent resources to capable frameworks.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60189/#review178193
---
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.cpp
Line 2112 (original), 2112 (patched)
<https://reviews.apache.org/r/60189/#comment252059>
Not specific to this RR, but if we're going to check non-framework-filters
in `isFiltered`, various comments should be updated. e.g.,
```
// If the framework filters these resources, ignore. The unallocated
// part of the quota will not be allocated to other roles.
if (isFiltered(frameworkId, role, slaveId, resources)) {
continue;
}
```
The new semantics of `isFiltered` covers more than "if the framework
filters these resources".
Similar with the comment for `isFiltered` in hierarchical.hpp
- Neil Conway
On June 19, 2017, 6:43 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60189/
> ---
>
> (Updated June 19, 2017, 6:43 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch prevents allocating resources to frameworks with
> `RESERVATION_REFINEMENT` capability with resources from the agents
> without the `RESERVATION_REFINEMENT` capability.
>
>
> Diffs
> -
>
> src/master/allocator/mesos/hierarchical.cpp
> 775fe4115d5f55be1e8542fa4f94699396d9e2a1
>
>
> Diff: https://reviews.apache.org/r/60189/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60041: Converted the resources to the "endpoint" format for V0 endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60041/#review178191 --- Ship it! Ship It! - Neil Conway On June 13, 2017, 8:43 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60041/ > --- > > (Updated June 13, 2017, 8:43 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7665 > https://issues.apache.org/jira/browse/MESOS-7665 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/http.cpp 4f838493104146e1a6b68eb64d8ec122cd54b7f7 > src/slave/http.cpp e03b8869d2b16a8503030642a96847beaddcc326 > > > Diff: https://reviews.apache.org/r/60041/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60187: Converted the resources to the "endpoint" format for V1 endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60187/#review178192 --- Ship it! Ship It! - Neil Conway On June 19, 2017, 6:39 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60187/ > --- > > (Updated June 19, 2017, 6:39 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7679 > https://issues.apache.org/jira/browse/MESOS-7679 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/protobuf_utils.cpp cb500632879beab1ff469246a0ea3a94633fa8a6 > src/master/http.cpp 4f838493104146e1a6b68eb64d8ec122cd54b7f7 > > > Diff: https://reviews.apache.org/r/60187/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60070: Adjusted the agent to the new resources format [11/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60070/#review178190 --- src/master/master.cpp Line 7308 (original), 7308 (patched) <https://reviews.apache.org/r/60070/#comment252058> Seems like this is in the wrong RR? - Neil Conway On June 14, 2017, 9:56 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60070/ > --- > > (Updated June 14, 2017, 9:56 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7666 > https://issues.apache.org/jira/browse/MESOS-7666 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/master.cpp 287a5b71bc61949648ac0edff7668f217357a054 > src/slave/paths.cpp b2709ad3b111f102ff223182a30c993a0c643230 > src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d > src/slave/slave.cpp 91a1bf330c3d5e91ef1b5ed016f9b3d0ad78400f > src/slave/state.hpp a497ce1f58fb8dc7718ee5bb10bc62dd7479efa5 > src/slave/state.cpp 18b790d2cc4f537cc9b0c3cca59b9cbaac0eda10 > > > Diff: https://reviews.apache.org/r/60070/diff/3/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60069: Adjusted the master to the new resources format [10/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60069/#review178189 --- Ship it! Ship It! - Neil Conway On June 14, 2017, 9:56 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60069/ > --- > > (Updated June 14, 2017, 9:56 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7667 > https://issues.apache.org/jira/browse/MESOS-7667 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/http.cpp 4f838493104146e1a6b68eb64d8ec122cd54b7f7 > src/master/master.hpp 9c1e95c8ba4c649680c8b5dd3bf26aeea1cfcdc0 > src/master/master.cpp 287a5b71bc61949648ac0edff7668f217357a054 > src/master/quota.cpp 3e0d280efff69904b74e5d4c71985ab44ffa34a7 > src/master/quota_handler.cpp 7fe55808d4561ae5a8850ad904d09a7c65e014ce > > > Diff: https://reviews.apache.org/r/60069/diff/3/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60040: Resources: Adjusted `parse` to produce the new resource format [6/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60040/#review178188 --- Ship it! Ship It! - Neil Conway On June 13, 2017, 8:42 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60040/ > --- > > (Updated June 13, 2017, 8:42 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414 > > > Diff: https://reviews.apache.org/r/60040/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60017: Resources: Adjusted the utilities to the new resource format [5/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60017/#review178187 --- Fix it, then Ship it! src/common/resources.cpp Line 1501 (original), 1501 (patched) <https://reviews.apache.org/r/60017/#comment252057> Per discussion, should instead result in a statically reserved resource whose role is the last role in the reservation stack. - Neil Conway On June 13, 2017, 8:35 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60017/ > --- > > (Updated June 13, 2017, 8:35 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/common/resources_utils.cpp aca2a0c630164af718007c70197461cebcd10d3e > src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414 > > > Diff: https://reviews.apache.org/r/60017/diff/5/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60185: Adjusted the master validation to the new resource format [9/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60185/#review178186 --- Ship it! Ship It! - Neil Conway On June 19, 2017, 6:19 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60185/ > --- > > (Updated June 19, 2017, 6:19 a.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/validation.cpp 1436002b5e30670166c1564b0e354299cc3a25fd > > > Diff: https://reviews.apache.org/r/60185/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60182: Resources: Adjusted the predicates to the new resource format [2/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60182/#review178184 --- Fix it, then Ship it! Ship It! include/mesos/resources.hpp Line 267 (original), 267 (patched) <https://reviews.apache.org/r/60182/#comment252056> "is validated, and is in the ..." - Neil Conway On June 19, 2017, 5:23 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60182/ > --- > > (Updated June 19, 2017, 5:23 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/resources.hpp be2e377ab9a7332adf373c585e5fafd47ce79f7e > include/mesos/v1/resources.hpp fe62166f20088504bdf90a178d3e3b0a94cdc1ac > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414 > > > Diff: https://reviews.apache.org/r/60182/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60184: Resources: Adjusted the `operator<<` to the new resource format [4/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60184/#review178185 --- Ship it! Ship It! - Neil Conway On June 19, 2017, 5:26 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60184/ > --- > > (Updated June 19, 2017, 5:26 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414 > > > Diff: https://reviews.apache.org/r/60184/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60016: Resources: Adjusted the comparators to new resource format [3/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60016/#review178183 --- Ship it! Ship It! - Neil Conway On June 13, 2017, 8:35 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60016/ > --- > > (Updated June 13, 2017, 8:35 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414 > > > Diff: https://reviews.apache.org/r/60016/diff/5/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60017: Resources: Adjusted the utilities to the new resource format [5/N].
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60017/#review178182
---
src/common/resources.cpp
Line 1551 (original), 1556 (patched)
<https://reviews.apache.org/r/60017/#comment252054>
We should be careful about the user experience here: the error message
("Resource must be dynamically reserved") might be interpreted as saying the
resource is statically reserved, not that it is unreserved. I'd prefer two
separate checks with different error messages, for `Resources::isReserved` and
`Resources::isDynamicallyReserved`.
src/common/resources_utils.cpp
Line 56 (original), 56 (patched)
<https://reviews.apache.org/r/60017/#comment252055>
I'm confused. Seems like this is a behavioral change for reservation
refinements, not a change to just adapt to the new format? In any case, seems
like a comment would be helpful.
- Neil Conway
On June 13, 2017, 8:35 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60017/
> ---
>
> (Updated June 13, 2017, 8:35 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Neil Conway.
>
>
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306
> src/common/resources_utils.cpp aca2a0c630164af718007c70197461cebcd10d3e
> src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414
>
>
> Diff: https://reviews.apache.org/r/60017/diff/5/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60040: Resources: Adjusted `parse` to produce the new resource format [6/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60040/#review178181 --- src/common/resources.cpp Line 683 (original), 693 (patched) <https://reviews.apache.org/r/60040/#comment252050> Can we clarify that the return value of this function is always (?) going to be validated by the caller, so it is okay to use the pre-refinement format here? src/common/resources.cpp Line 687 (original), 697 (patched) <https://reviews.apache.org/r/60040/#comment252051> btw, the fact that `Resources::fromString` is called separately (e.g., by containerizer) makes me a little nervous, because the caller of `fromString` needs to ensure they validate/convert the resource appropriately. I guess that was always the case though. src/v1/resources.cpp Lines 663 (patched) <https://reviews.apache.org/r/60040/#comment252053> Should the `!resource.has_reservation()` be a `CHECK` instead? src/v1/resources.cpp Lines 672 (patched) <https://reviews.apache.org/r/60040/#comment252052> `CopyFrom`, I'd think. - Neil Conway On June 13, 2017, 8:42 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60040/ > --- > > (Updated June 13, 2017, 8:42 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306 > src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414 > > > Diff: https://reviews.apache.org/r/60040/diff/4/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60181: Resources: Updated the comment to mention the resource format [1/N].
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60181/#review178178 --- Ship it! Ship It! - Neil Conway On June 19, 2017, 5:17 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60181/ > --- > > (Updated June 19, 2017, 5:17 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/resources.hpp be2e377ab9a7332adf373c585e5fafd47ce79f7e > include/mesos/v1/resources.hpp fe62166f20088504bdf90a178d3e3b0a94cdc1ac > > > Diff: https://reviews.apache.org/r/60181/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60015: Introduced a utility function `Resources::reservationRole`.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60015/#review178177
---
Ship it!
src/common/resources.cpp
Line 1041 (original), 1041 (patched)
<https://reviews.apache.org/r/60015/#comment252046>
Personally, I think this is a bit more readable:
```
if (isUnreserved(resource)) {
return false;
}
return role.isNone() || role.get() == reservationRole(resource);
```
- Neil Conway
On June 13, 2017, 8:34 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60015/
> ---
>
> (Updated June 13, 2017, 8:34 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Introduced a utility function `Resources::reservationRole`.
>
>
> Diffs
> -
>
> include/mesos/resources.hpp be2e377ab9a7332adf373c585e5fafd47ce79f7e
> include/mesos/v1/resources.hpp fe62166f20088504bdf90a178d3e3b0a94cdc1ac
> src/common/resources.cpp cc305e64cadc9fab913ede987625fc1839646306
> src/v1/resources.cpp 03d6c8098a24a2751617d14f2664de1d88318414
>
>
> Diff: https://reviews.apache.org/r/60015/diff/9/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60178: Added test for endpoint resource format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60178/ --- (Updated June 19, 2017, 1:40 a.m.) Review request for mesos and Michael Park. Repository: mesos Description --- Added test for endpoint resource format. Diffs (updated) - src/tests/master_validation_tests.cpp 83370fa5653fe5da666ec577b05001c4a5499848 Diff: https://reviews.apache.org/r/60178/diff/3/ Changes: https://reviews.apache.org/r/60178/diff/2-3/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60036: Avoided exposing `role` in JSON for resources with refined reservations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60036/#review178172 --- Tests, can do later. src/common/http.hpp Lines 131 (patched) <https://reviews.apache.org/r/60036/#comment252037> Note that it doesn't roundtrip. src/common/http.hpp Lines 135 (patched) <https://reviews.apache.org/r/60036/#comment252036> Indentation is non-idiomatic src/common/http.cpp Lines 519 (patched) <https://reviews.apache.org/r/60036/#comment252038> Backticks not double quotes? src/common/http.cpp Lines 526 (patched) <https://reviews.apache.org/r/60036/#comment252039> Can we add `is` checks here, for safety's sake? - Neil Conway On June 15, 2017, 7:39 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60036/ > --- > > (Updated June 15, 2017, 7:39 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > With reservation refinement, we use the `Resource.reservations` field to > express the reservation state. Due to the fact that `Resource.role` is > an optional field __and__ has a default value, our generic Protobuf to > JSON code injects the `role` field with the default value even if it's > not set. Since this will likely confuse users, here we manually remove > the `Resource.role` field if the resource has refined reservations. > > > Diffs > - > > src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e > src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 > src/master/http.cpp e01c3fcdc0df63934fc82ee4c9bf081f54cf7ea2 > src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 > > > Diff: https://reviews.apache.org/r/60036/diff/9/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60036: Avoided exposing `role` in JSON for resources with refined reservations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60036/#review178173 --- Ship it! Ship It! - Neil Conway On June 15, 2017, 7:39 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60036/ > --- > > (Updated June 15, 2017, 7:39 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > With reservation refinement, we use the `Resource.reservations` field to > express the reservation state. Due to the fact that `Resource.role` is > an optional field __and__ has a default value, our generic Protobuf to > JSON code injects the `role` field with the default value even if it's > not set. Since this will likely confuse users, here we manually remove > the `Resource.role` field if the resource has refined reservations. > > > Diffs > - > > src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e > src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 > src/master/http.cpp e01c3fcdc0df63934fc82ee4c9bf081f54cf7ea2 > src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 > > > Diff: https://reviews.apache.org/r/60036/diff/9/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60022: Introduced `convertResourceFormat` to convert between resource formats.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60022/#review178170
---
src/common/resources_utils.hpp
Lines 61 (patched)
<https://reviews.apache.org/r/60022/#comment252024>
Typo
src/common/resources_utils.hpp
Lines 63 (patched)
<https://reviews.apache.org/r/60022/#comment252025>
Can we remove the 3x repitition of this? Seems sufficient to have the
header comment.
src/common/resources_utils.hpp
Lines 75 (patched)
<https://reviews.apache.org/r/60022/#comment252026>
Typo.
src/common/resources_utils.hpp
Lines 76 (patched)
<https://reviews.apache.org/r/60022/#comment252027>
We also do this for frameworks that send old resources, right? i.e.,
clarify that the old-agent conversion discussion is an example.
src/common/resources_utils.hpp
Lines 96 (patched)
<https://reviews.apache.org/r/60022/#comment252029>
Can we clarify this slightly?
"We inject the resources with the "pre-reservation-refinement" format to
enable backward compatibility with external tooling. If the master has been
upgraded to a version that supports reservation refinement but no refined
reservations have been made, the endpoints will return the data in both the new
and old formats to maximize backward compatibility. However, once a reservation
refinement is made to a resource, that resource is only returned in the new
format."
src/common/resources_utils.hpp
Lines 103 (patched)
<https://reviews.apache.org/r/60022/#comment252030>
"Converts the given `Resource` to the specified `ResourceFormat`."
src/common/resources_utils.hpp
Lines 105 (patched)
<https://reviews.apache.org/r/60022/#comment252031>
I'd remove these comments, they seem confusing/redundant with the
discussion above.
src/common/resources_utils.cpp
Lines 87 (patched)
<https://reviews.apache.org/r/60022/#comment252035>
I'm confused by this function's precondition. When converting to
`PRE_RESERVATION_REFINEMENT`, the resources _cannot_ be in the pre-refined
format; but when converting to `POST_RESERVATION_REFINEMENT`, the resources
_can_ be in the post-refined format. Seems inconsistent.
src/common/resources_utils.cpp
Lines 125 (patched)
<https://reviews.apache.org/r/60022/#comment252032>
Typo
src/common/resources_utils.cpp
Lines 135 (patched)
<https://reviews.apache.org/r/60022/#comment252034>
"we're in"
src/common/resources_utils.cpp
Lines 158 (patched)
<https://reviews.apache.org/r/60022/#comment252033>
I feel like `reservation->CopyFrom(resource->reservation())` would be more
idiomatic...?
- Neil Conway
On June 15, 2017, 8:48 a.m., Michael Park wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60022/
> -------
>
> (Updated June 15, 2017, 8:48 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Neil Conway.
>
>
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
>
>
> Repository: mesos
>
>
> Description
> ---
>
> With reservation refinement, we introduced a new resource format to
> enable creating a refined reservation on top of an existing reservation.
> We still have the "pre-reservation-refinement" format which uses
> the `Resource.role` and `Resource.reservation` fields, and we now also
> have the "post-reservation-refinement" format which uses
> the `Resource.reservations` field to represent the reservation state.
>
> In order to simplify our code, we use `convertResourceFormat` to
> canonicalize the resources into the "post-reservation-refinement" in
> memory, and convert them back as necessary.
>
>
> Diffs
> -
>
> src/common/resources_utils.hpp 2840f459288bbe8440eda08119d4f86a8be5a291
> src/common/resources_utils.cpp c3088433d8c147b06dbef6f310fbe4059c3dc0ba
>
>
> Diff: https://reviews.apache.org/r/60022/diff/8/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 60178: Added test for endpoint resource format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60178/ --- (Updated June 19, 2017, 1:12 a.m.) Review request for mesos and Michael Park. Changes --- Avoid using resource utility functions. Repository: mesos Description --- Added test for endpoint resource format. Diffs (updated) - src/tests/master_validation_tests.cpp 83370fa5653fe5da666ec577b05001c4a5499848 Diff: https://reviews.apache.org/r/60178/diff/2/ Changes: https://reviews.apache.org/r/60178/diff/1-2/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60174: Updated the resources validation logic to allow for the endpoint format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60174/#review178168 --- Ship it! Ship It! - Neil Conway On June 18, 2017, 10:02 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60174/ > --- > > (Updated June 18, 2017, 10:02 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > For backwards compatibility of external tools consuming endpoints, > We'll inject the "pre-reservation-refinement" format for resources > without refined reservations. That is, the `Resource.role` and > `Resource.reservation` fields will be present as well as > the in with the single `Resource.reservations` field. > > Since the users of the endpoints are likely to feed the result back into > Mesos, we need to update the validation logic to allow these resources > to be considered valid. > > > Diffs > - > > src/common/resources.cpp f12ab970fbce2815a5a22e5080af9fb70c6aef9f > src/v1/resources.cpp 30644ee0bfeb8498d7b10daa79c0da369201abf3 > > > Diff: https://reviews.apache.org/r/60174/diff/2/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Review Request 60178: Added test for endpoint resource format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60178/ --- Review request for mesos and Michael Park. Repository: mesos Description --- Added test for endpoint resource format. Diffs - src/tests/master_validation_tests.cpp 83370fa5653fe5da666ec577b05001c4a5499848 Diff: https://reviews.apache.org/r/60178/diff/1/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60174: Updated the resources validation logic to allow for the endpoint format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60174/#review178165 --- In the commit message, can we explain why we need to update validation to support this new format? i.e., the commit message says the master is going to _emit_ this weird format, but it doesn't necessarily follow why the master needs to _accept_ inputs in this format. Seems like we should update the tests for this change, right? - Neil Conway On June 18, 2017, 10:02 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60174/ > --- > > (Updated June 18, 2017, 10:02 p.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7655 > https://issues.apache.org/jira/browse/MESOS-7655 > > > Repository: mesos > > > Description > --- > > For backwards compatibility of external tools consuming endpoints, > We'll inject the "pre-reservation-refinement" format for resources > without refined reservations. That is, the `Resource.role` and > `Resource.reservation` fields will be filled in with the single > reservation inside `Resource.reservations`. > > We update the validation logic to allow these resources to be valid. > > > Diffs > - > > src/common/resources.cpp f12ab970fbce2815a5a22e5080af9fb70c6aef9f > src/v1/resources.cpp 30644ee0bfeb8498d7b10daa79c0da369201abf3 > > > Diff: https://reviews.apache.org/r/60174/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 60175: Introduced a utility function `Resources::hasRefinedReservations`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60175/#review178164 --- Ship it! Is it worth adding coverage of this to the tests? - Neil Conway On June 18, 2017, 10:03 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60175/ > --- > > (Updated June 18, 2017, 10:03 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > include/mesos/resources.hpp d380a590404f119cdb1f715c43bbb0052422d1ec > include/mesos/v1/resources.hpp 66c2b215761c488a1554f925726d53035d40708a > src/common/resources.cpp f12ab970fbce2815a5a22e5080af9fb70c6aef9f > src/v1/resources.cpp 30644ee0bfeb8498d7b10daa79c0da369201abf3 > > > Diff: https://reviews.apache.org/r/60175/diff/1/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 59454: Adjust tests to account for GCC 7.1 fix in bytes.hpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59454/#review178163 --- Ship it! lgtm, although I'll change the commit message -- there isn't anything specific about GCC 7.1 in this fix. - Neil Conway On May 24, 2017, 5:53 p.m., Aaron Wood wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59454/ > --- > > (Updated May 24, 2017, 5:53 p.m.) > > > Review request for mesos, Benjamin Bannier, James Peach, Michael Park, and > Neil Conway. > > > Bugs: MESOS-7520 > https://issues.apache.org/jira/browse/MESOS-7520 > > > Repository: mesos > > > Description > --- > > This fixes tests that uses the old inheritance types from `bytes.hpp`. > > > Diffs > - > > src/tests/persistent_volume_tests.cpp ef0085316 > src/tests/role_tests.cpp 03b6f7bd6 > > > Diff: https://reviews.apache.org/r/59454/diff/3/ > > > Testing > --- > > `./bootstrap && mkdir build && cd build && ../configure --disable-python > --disable-java --disable-optimize --disable-hardening && make check > -j$(nproc)` > > > Thanks, > > Aaron Wood > >
Re: Review Request 59861: Added protobuf changes for reservation refinement.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59861/#review178133 --- Fix it, then Ship it! include/mesos/mesos.proto Lines 349 (patched) <https://reviews.apache.org/r/59861/#comment251971> "The framework is offered resources in a new format." include/mesos/mesos.proto Lines 359 (patched) <https://reviews.apache.org/r/59861/#comment251968> "The framework can create refined reservations." include/mesos/mesos.proto Lines 368 (patched) <https://reviews.apache.org/r/59861/#comment251964> Typo: "capability" Tweak: "A resource is said to have" include/mesos/mesos.proto Lines 980 (patched) <https://reviews.apache.org/r/59861/#comment251965> Is this consistent with the "set both old and new fields for endpoints" approach? include/mesos/mesos.proto Lines 1005 (patched) <https://reviews.apache.org/r/59861/#comment251966> "a framework or agent"? Seems technically correct but maybe confusing to mention here. include/mesos/mesos.proto Lines 1101 (patched) <https://reviews.apache.org/r/59861/#comment251970> Can we add a note here that the contents of the `reservations` field are ordered from "least refined" to "most refined"? include/mesos/v1/mesos.proto Lines 367 (patched) <https://reviews.apache.org/r/59861/#comment251967> Typo include/mesos/v1/mesos.proto Lines 1151 (patched) <https://reviews.apache.org/r/59861/#comment251969> Remove comma: "reservation's role," - Neil Conway On June 16, 2017, 10:51 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59861/ > ------- > > (Updated June 16, 2017, 10:51 p.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7575 > https://issues.apache.org/jira/browse/MESOS-7575 > > > Repository: mesos > > > Description > --- > > With reservation refinement, we now represent the reservation state in > \`Resource.reservations\` rather than with \`Resource.role\` and > \`Resource.reservation\`. We also keep track of the type of reservation, > and the role the reservation is for in the \`Resource.ReservationInfo\`. > This patch introduces these changes and explains the different formats > in detail. > > > Diffs > - > > include/mesos/mesos.proto e9a8b1513150b394f4986a1f34483cfca32b35e3 > include/mesos/v1/mesos.proto 5e8c15c07a02999e3376d2104c1c642f00511969 > src/common/protobuf_utils.hpp 7b1a1289c16edde44143432b645ecce68576 > src/common/protobuf_utils.cpp 46331b379c732edc597b46bb10665c95c9640e96 > > > Diff: https://reviews.apache.org/r/59861/diff/12/ > > > Testing > --- > > Updated tests + `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 60062: Prevented reserve/create with hierarchical roles on a non-capable agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60062/#review178122 --- Ship it! Ship It! - Neil Conway On June 16, 2017, 7:48 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60062/ > --- > > (Updated June 16, 2017, 7:48 p.m.) > > > Review request for mesos and Neil Conway. > > > Repository: mesos > > > Description > --- > > This patch makes it such that reserve / create operations involving > resources with a hierarchical role are invalid if they are attempted on > an agent without a HIERARCHICAL_ROLE capability. Such attempts from an > operator endpoint will result in a `BadRequest` response. On the > frameworks side, we've opted to not offer resources from > a non-HIERARCHICAL_ROLE agent to hierarchical roles (ca8d33ab). > > If we were to undo ca8d33ab, an attempt from a framework to perform such > an operation would be considered invalid due to this patch and the > operation would be dropped by the master. > > > Diffs > - > > src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 > src/master/master.cpp 47a76ba9f549f2a170b5f424ac351e120ba75fb2 > src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 > src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 > src/tests/master_validation_tests.cpp > 4e7ce74edf0069b9317f869b299694a45e2a62f2 > > > Diff: https://reviews.apache.org/r/60062/diff/10/ > > > Testing > --- > > Added new tests to `master_validation_tests.cpp` + `make check` > > > Thanks, > > Michael Park > >
Re: Review Request 60139: Avoided needless copy of `Owned` in libprocess.
> On June 16, 2017, 11:25 a.m., Benjamin Bannier wrote: > > I do not agree with this change. > > > > An `Owned` cannot be copied semantically. If we pass an `Owned` at all, it > > should be by value, not by reference, so potential copies happen on > > interface boundaries and we do not need to perform potentially unneeded > > copies inside the consuming functions. See e.g., > > https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/ > > for a more detailed write-up. > > > > I believe the existing code is still broken since it performs copies of an > > `Owned` (which is currently allowed syntactically, > > https://issues.apache.org/jira/browse/MESOS-5122). IMHO the correct fix > > would be to keep the existing signatures, but to `std::move` when passing > > the `authenticator` value on, both when used as function argument and when > > ultimatly stored in `AuthenticatorManagerProcess::authenticators_`. This > > would both prevent copies and express ownership semantics correctly. In principle, I agree with you 100%. `Owned` should have `unique_ptr` semantics (or we should just remove `Owned` and use `unique_ptr`), and a function that takes over the ownership of an `Owned` pointer should use `std::move`. But until `Owned` actually enforces `unique_ptr` semantics and disallows copies, do we actually want to encourage people using `std::move`? We use `std::move` relatively rarely, and in the places where we use it, we often get it wrong :) In contrast, we use `const Owned&` parameters reasonably often right now... At least in this case, we'd need to add quite a few `std::move`s to account for all the call-sites. I guess for now I'll just give up on this, and we can try to clean it up properly as part of fixing `Owned`. - Neil --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60139/#review178098 --- On June 15, 2017, 11:16 p.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60139/ > --- > > (Updated June 15, 2017, 11:16 p.m.) > > > Review request for mesos and Alexander Rojas. > > > Repository: mesos > > > Description > --- > > Spotted via clang-tidy. > > > Diffs > - > > 3rdparty/libprocess/src/authenticator_manager.hpp > 0dc8fd24b411d649bcc62208bde5784cac4ea997 > 3rdparty/libprocess/src/authenticator_manager.cpp > 5cbed53e7085f227d90679e1b56ad803d9b74a47 > > > Diff: https://reviews.apache.org/r/60139/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Neil Conway > >
Review Request 60162: Fixed bug in GroupTest.ConnectTimer, GroupTest.TimerCleanup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60162/ --- Review request for mesos and Andrei Budnik. Repository: mesos Description --- These tests looked for dispatches to `GroupProces::expired` as a way to determine when the current Zk session has expired. However, the previous implementation of the connect timer (`GroupProcess::timedout`) invoked `GroupProcess::expired` directly, which meant that an `EXPECT_DISPATCH` on the `expired` method should not fire. However, a separate bug in `EXPECT_DISPATCH` (MESOS-5886) meant that the test expectations were actually being satisfied by a dispatch to `GroupProcess::timedout`, which meant the tests happened to work (!). Fix this by changing `GroupProcess::timedout` to dispatch to `expired` rather than invoking it directly. Diffs - src/zookeeper/group.cpp 20f1928e05123d362147ce4ab0d6a752d95e466d Diff: https://reviews.apache.org/r/60162/diff/1/ Testing --- `make check` Validated that if the fix for MESOS-5886 is applied without this change, `GroupTest.ConnectTimer` fails. With this change, the test passes (both with and without the fix for MESOS-5886 applied). Thanks, Neil Conway
Review Request 60161: Cleaned up zookeeper binding code slightly.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60161/ --- Review request for mesos and Andrei Budnik. Repository: mesos Description --- Cleaned up zookeeper binding code slightly. Diffs - src/zookeeper/detector.cpp 973f5f297208dac2050d38fc5140a78c244666b7 src/zookeeper/group.cpp 20f1928e05123d362147ce4ab0d6a752d95e466d Diff: https://reviews.apache.org/r/60161/diff/1/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 60070: Updated the agent to account for the new resources format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60070/#review178018 --- It would be great to have a comment somewhere (not sure the best place) that describes our overall approach to representation/evolving resource formats. For example: * what do we checkpoint to disk at the agent? * what do we use in-memory at the agent? * what do we send back to the master? * what does the master store in Zk? (One subtle thing is that regardless of what we send back to the master, the `SlaveInfo` in Zk won't be updated, at least for the time being...) src/slave/slave.cpp Line 5926 (original), 5946 (patched) <https://reviews.apache.org/r/60070/#comment251769> This change seems unnecessary, I think? src/slave/state.hpp Lines 45 (patched) <https://reviews.apache.org/r/60070/#comment251770> We usually add a newline here, I think. src/slave/state.cpp Lines 45 (patched) <https://reviews.apache.org/r/60070/#comment251771> Newline. src/slave/state.cpp Line 223 (original), 227 (patched) <https://reviews.apache.org/r/60070/#comment251772> Are we using `const` here but not elsewhere in the RR for a reason? - Neil Conway On June 14, 2017, 9:56 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60070/ > --- > > (Updated June 14, 2017, 9:56 a.m.) > > > Review request for mesos and Neil Conway. > > > Bugs: MESOS-7666 > https://issues.apache.org/jira/browse/MESOS-7666 > > > Repository: mesos > > > Description > --- > > Updated the agent to account for the new resources format. > > > Diffs > - > > src/slave/paths.cpp b2709ad3b111f102ff223182a30c993a0c643230 > src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d > src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6 > src/slave/state.hpp a497ce1f58fb8dc7718ee5bb10bc62dd7479efa5 > src/slave/state.cpp 18b790d2cc4f537cc9b0c3cca59b9cbaac0eda10 > > > Diff: https://reviews.apache.org/r/60070/diff/2/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Review Request 60139: Avoided needless copy of `Owned` in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60139/ --- Review request for mesos and Alexander Rojas. Repository: mesos Description --- Spotted via clang-tidy. Diffs - 3rdparty/libprocess/src/authenticator_manager.hpp 0dc8fd24b411d649bcc62208bde5784cac4ea997 3rdparty/libprocess/src/authenticator_manager.cpp 5cbed53e7085f227d90679e1b56ad803d9b74a47 Diff: https://reviews.apache.org/r/60139/diff/1/ Testing --- `make check` Thanks, Neil Conway
