> On Sept. 21, 2018, 10:50 a.m., Benjamin Bannier wrote: > > src/resource_provider/daemon.cpp > > Lines 426-427 (original), 478-481 (patched) > > <https://reviews.apache.org/r/68763/diff/2/?file=2090658#file2090658line481> > > > > Wouldn't this be a situation where we'd like to respond with a > > `Failure`?
Thanks for catching this. In the current logic, `data.removing` is set iff a previous remove returned a success. Since `launch` is called synchronously in `add` or `start`, this should always be false. I'll change this to a check. > On Sept. 21, 2018, 10:50 a.m., Benjamin Bannier wrote: > > src/resource_provider/daemon.cpp > > Lines 431-433 (original), 483-485 (patched) > > <https://reviews.apache.org/r/68763/diff/2/?file=2090658#file2090658line488> > > > > How does this hold? It seems we should e.g., insert a `remove` call > > before continuing with `_launch`. I assume you're talking about an `erase`. No we cannot simply erase it, because the `providers` struct is the source of truth whether we should accept or reject an add, an update, or a remove when requesting the auth token. > On Sept. 21, 2018, 10:50 a.m., Benjamin Bannier wrote: > > src/resource_provider/daemon.cpp > > Lines 505-508 (patched) > > <https://reviews.apache.org/r/68763/diff/2/?file=2090658#file2090658line510> > > > > Wouldn't this be a situation where we'd like to respond with a > > `Failure`? Even if we return a failure here, this failure would only surface as an error log, since the add/update/remove APIs doesn't depend on the launch. Also, `data.remove` is set only if there is a remove call between `launch` and `_launch`, which is allowed, so we simply abort the launch here. > On Sept. 21, 2018, 10:50 a.m., Benjamin Bannier wrote: > > src/resource_provider/daemon.cpp > > Lines 566 (patched) > > <https://reviews.apache.org/r/68763/diff/2/?file=2090658#file2090658line571> > > > > Let's assert here instead of letting the exception propagate. Can you explain more about what assertion to add? You mean making `cleanupContainers` a `void` function and handling errors here? That would mean that the corresponding entry in `providers` needs to be erased here. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68763/#review208846 ----------------------------------------------------------- On Sept. 20, 2018, 11:21 p.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68763/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2018, 11:21 p.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-9228 > https://issues.apache.org/jira/browse/MESOS-9228 > > > Repository: mesos > > > Description > ------- > > When processing `REMOVE_RESOURCE_PROVIDER_CONFIG`, the local resource > provider daemon now performs a best-effort cleanup by killing all > standalone containers prefixed by the 'cid_prefix' of the resource > provider. During the cleanup, no resource provider config with the same > type and name can be added. > > > Diffs > ----- > > src/resource_provider/daemon.cpp 0a76cc6d1d34413674d1af1aa514679a4d2b7b55 > > > Diff: https://reviews.apache.org/r/68763/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
