----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72734/#review221485 ----------------------------------------------------------- src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp Lines 383-387 (patched) After looking at the code, it seems that a failure here would surface as a TASK_FAILED status update to the scheduler due to a failed container termination, is that your understanding as well? I'm wondering if we need to add some extra handling for the failed unpublish case, either here or in the Mesos containerizer somehow. What action needs to be taken in the case where an unpublish call to the CSI plugin fails for some transient reason? src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp Lines 389-391 (patched) I'm wondering if there's an issue that if we return a failure here, the state on disk (the `volumes` file for this container) and the state in memory (the state stored in `infos`) will be inconsistent. If the agent were to restart, the CSI volume isolator could clean up that file if it refers to an unknown container. In that case, we will have left a volume published on this node without a record of it, but maybe that's OK? - Greg Mann On Aug. 5, 2020, 9:20 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72734/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2020, 9:20 a.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10154 > https://issues.apache.org/jira/browse/MESOS-10154 > > > Repository: mesos > > > Description > ------- > > Implemented the `cleanup` method of `volume/csi` isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72734/diff/1/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >