fix nri removeMount does not work bug #87
Conversation
nri needs to pass the del mount info to containerd, so that containerd can handle the mount records that need to be deleted correctly Signed-off-by: jingtao.liang <[email protected]>
|
/retest |
|
https://github.com/containerd/nri/blob/main/pkg/api/adjustment.go#L20-L32 for consideration... |
|
Additional consideration.. the removeMount test, linked below, only does the removeMount when the adjustment is set to "overwrite"= true.. If we are to support removal of a mount that is not an overwrite we should probably have another test for it.. https://github.com/containerd/nri/blob/main/pkg/adaptation/adaptation_suite_test.go#L453-L456 |
fix nri removeMount does not work bug
| @@ -322,6 +322,11 @@ func (r *result) adjustMounts(mounts []*Mount, plugin string) error { | |||
| r.reply.adjust.Mounts = append(r.reply.adjust.Mounts, m) | |||
| } | |||
|
|
|||
| // need add del mount, containerd needs to process this del mounts | |||
There was a problem hiding this comment.
nitpick: I would change 'containerd' in the above comment to 'runtime'. Maybe rephrase to something like ' add deleted mounts, the runtime needs them to properly remove deleted mounts'.
|
@liangjingtao11 It would be nice to have a test case for this. |
|
@liangjingtao11 do you have time to finish this PR? |
| for _, m := range del { | ||
| r.reply.adjust.Mounts = append(r.reply.adjust.Mounts, m) | ||
| } |
There was a problem hiding this comment.
@liangjingtao11 I think doing it this way breaks cases where a single plugin does a RemoveMount() followed by an AddMount(), both using the same destination. This is the idiomatic way for a plugin to change a mount in the case where that mount might have been injected (or updated) by another plugin earlier in the chain. But I think with your changes in place, that will end up with a final adjustment which causes the runtime to remove such a mount (by destination) from the container.
|
@liangjingtao11 This fix is not sufficient in it's current form as it breaks replacing a mount with another one to the same destination. I think we'd need a bit of additional code to prevent that from happening. Maybe something like this, where 8f509e7 is an updated version of your commit, and 7ea0a47 adds a new test case for mount removal. |
|
@liangjingtao11 Ping. Do you have time to address the remaining issues ? |
|
Closing in favor of updated PR #107. |
nri needs to pass the del mount info to containerd, so that containerd can handle the mount records that need to be deleted correctly
fix: #80