----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44258/#review121770 -----------------------------------------------------------
src/master/http.cpp (lines 1999 - 2000) <https://reviews.apache.org/r/44258/#comment183589> We originally did not set the mode here because this loop updates the "unavailability" regardless of the "mode". If you'd like, you can add a note here. i.e. ``` // NOTE: We do not update the `MachineInfo::MODE` here because the machine may be in any mode. ``` --- If we really want to prevent the double-inverse-offer, you'd need to add something like this at inside the existing `if`: ``` if (master->machines[id].info.mode() == MachineInfo::UP) { continue; } ``` Although at this point, we might as well consider folding the body of this function into `Master::updateUnavailability`. src/master/http.cpp (lines 2014 - 2016) <https://reviews.apache.org/r/44258/#comment183590> Nice catch. This loop is in fact not representative of the comment above. However, to perform the update correctly (including the issue above), you'd need to account for any existing machines that are DRAINING or DOWN. ``` if (master->machines.contains(id) && master->machines[id].info.mode() != MachineInfo::UP) { continue; } ``` --- Nit: Newline after the `if`. src/tests/master_maintenance_tests.cpp (lines 427 - 429) <https://reviews.apache.org/r/44258/#comment183591> Can you also change this to: ``` EXPECT_FALSE(event.get().offers().offers(0).has_unavailability()); ``` src/tests/master_maintenance_tests.cpp (lines 469 - 478) <https://reviews.apache.org/r/44258/#comment183594> Can you also change this: ``` EXPECT_TRUE(event.get().offers().offers(0).has_unavailability()); EXPECT_EQ( unavailability.start().nanoseconds(), event.get().offers().offers(0).unavailability().start().nanoseconds()); EXPECT_EQ( unavailability.duration().nanoseconds(), event.get().offers().offers(0).unavailability().duration().nanoseconds()); ``` - Joseph Wu On March 1, 2016, 11:45 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44258/ > ----------------------------------------------------------- > > (Updated March 1, 2016, 11:45 p.m.) > > > Review request for mesos, Anand Mazumdar, Joris Van Remoortere, and Joseph Wu. > > > Bugs: MESOS-4831 > https://issues.apache.org/jira/browse/MESOS-4831 > > > Repository: mesos > > > Description > ------- > > There is a bug when setting host maintain with http endpoint: > https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1987-L2021 > The logic is as this: > 1) Get all host list from maintain window and put it to updated hashmap. > 2) If the machine in was in updated was also in master->machines, call master > updateUnavailability to trigger recoverResources, updateUnavailability etc in > allocator > 3) Otherwise, clear the unavailabity time window for the machine. > 4) Update each new machines in updated to call master updateUnavailability > > But the logic in step 4) is getting all machines from the schedule windows > but not the machines that is new to the cluster, this caused master get two > updateUnavailability calls for a machine in the updated hashmap. > > The fix is filter machines in updated hashmap when handling new machines. > > > Diffs > ----- > > src/master/http.cpp 5e9e28e904ba0045ee27eb828f47231632a91d74 > src/tests/master_maintenance_tests.cpp > 3faa8136cf57276295553910319480028f433e4c > > Diff: https://reviews.apache.org/r/44258/diff/ > > > Testing > ------- > > make > make check > ./bin/mesos-tests.sh --gtest_filter="MasterMaintenanceTest.*" --verbose > > > Thanks, > > Guangya Liu > >
