> On March 20, 2017, 11:43 a.m., Benjamin Bannier wrote: > > We already discussed this offline, and I still (maybe even stronger) feel > > that cases of duplicate env vars with differing values should be errors, > > dispite the fact that in the current implementation Mesos makes sure that > > isolators' `prepare` are called in deterministic order (there is no > > guarantee on the interface level). We should strongly consider tightening > > the behavior here if we do not have to support the current behavior for > > e.g., backwards compatibility reasons. > > > > Env vars potentially mix user input from `CommandInfo` with output from > > isolators configured by an operator. Conflicting values cause settings by > > either the user or an isolator to be silently dropped. The first case would > > provide a bad UX, while the second one could be a configuration bug or > > worse from incompatible isolators. Diagnosing either of these through > > non-fatal log messages seems to do to little. As a user it would not be > > enough to just manually make sure none of these non-fatal errors are > > reported in `stdout` (not even `stderr`) when developing the task since an > > administrator might later on introduce introduce isolators making use of > > already taken user variables which would now be be broken by the task's > > setup. As an operator I wouldn't even be able to see these messages a user > > might not care about since they are reported in the task's output, not > > e.g., the agent log. > > > > It we'd just rejected tasks with conflicting env settings outright, the > > situation would improve for both users and operators. If we'd go that route > > I believe it would make sense to add duplicate detection to > > `validateEnvironment` and directly invoke it here.
I feel very tempted to agree with you here. However, given that the impact is rather harsh, I would like to leave that up to the shepherd in spe. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57763/#review169410 ----------------------------------------------------------- On March 20, 2017, 2:51 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57763/ > ----------------------------------------------------------- > > (Updated March 20, 2017, 2:51 a.m.) > > > Review request for mesos, Adam B, Benjamin Bannier, Jie Yu, and Joseph Wu. > > > Bugs: MESOS-7264 > https://issues.apache.org/jira/browse/MESOS-7264 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/slave/containerizer/mesos/launch.cpp > 8658525b00e78bed9227d6d400eccccae2cf25dd > > > Diff: https://reviews.apache.org/r/57763/diff/1/ > > > Testing > ------- > > make check & functional testing... > > ``` > ./src/mesos-execute --name="test" --env='{"key1":"value1"}' --command='sleep > 1000' --master=127.0.0.1:5050 > ``` > > Within the contents of `stdout` before applying this: > ``` > Overwriting environment variable 'key1', original: 'value1', new: 'value1' > ``` > > After applying this there is no mention of the actually duplicate but equally > valued variable anymore. Note, this is before applying > https://reviews.apache.org/r/57762. > > > Thanks, > > Till Toenshoff > >
