----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72003/#review219270 -----------------------------------------------------------
src/slave/slave.cpp Line 6583 (original), 6583 (patched) <https://reviews.apache.org/r/72003/#comment307435> If we are not validating that `ExecutorInfo` of a task does not have `generated_by_agent` set, maybe we can at least forcibly clear it? either here or in `Slave::runTask()`, and document (in the previous patch) that agent **ignores** this field when received from outside? src/slave/slave.cpp Lines 6593 (patched) <https://reviews.apache.org/r/72003/#comment307434> The logic "executor is generated by agent -> master receives no updates" is not covered by any of the local tests; if we are not introducing the test right now, consider adding TODO (and, probably, a separate ticket). Personally, I'm more or less OK with `upgradeExecutorInfo(...)` not covered: the necessary setup is difficult to create, that code is hacky anyway, and we want to phase it out. src/slave/slave.cpp Lines 10604 (patched) <https://reviews.apache.org/r/72003/#comment307433> My impression is that adding some logging here could help in case this hack misfires. Something like ``` LOG(WARNING) << "Detecting whether executor " << execiutor.id() << " was generated by agent: executor command is " << executor.command().value() << "; current path of " << MESOS_EXECUTOR << " binary is " << executorPath.get(); ``` ? src/slave/slave.cpp Line 10601 (original), 10626 (patched) <https://reviews.apache.org/r/72003/#comment307432> Maybe add/move CHECK_NOTNULL to this line, so that it does not rely on the fact that `slave` is **declared** before `info`? - Andrei Sekretenko On Jan. 15, 2020, 11:25 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72003/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2020, 11:25 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10084 > https://issues.apache.org/jira/browse/MESOS-10084 > > > Repository: mesos > > > Description > ------- > > This patch modifies the agent code so it uses > `ExecutorInfo::generated_by_agent` instead of the previous heuristic to > detect agent-generated executors for command tasks. For that we add > recovery logic for upgrade scenarios to inject a correct value for > recovered executors, and set the appropriate value for newly created > executors. > > > Diffs > ----- > > src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 > src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 > > > Diff: https://reviews.apache.org/r/72003/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
