Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix nri removeMount does not work bug #87

Closed
wants to merge 2 commits into from

Conversation

liangjingtao11
Copy link

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

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 <jingtao.liang@easystack.cn>
@liangjingtao11
Copy link
Author

/retest

@mikebrow
Copy link
Member

@mikebrow
Copy link
Member

mikebrow commented May 29, 2024

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

@klihub
Copy link
Member

klihub commented Jun 7, 2024

@liangjingtao11 It would be nice to have a test case for this.

@mikebrow mikebrow added this to the 1.0 milestone Aug 1, 2024
@mikebrow
Copy link
Member

mikebrow commented Aug 8, 2024

@liangjingtao11 do you have time to finish this PR?

@klihub klihub self-requested a review August 19, 2024 07:58
Comment on lines +326 to +328
for _, m := range del {
r.reply.adjust.Mounts = append(r.reply.adjust.Mounts, m)
}
Copy link
Member

@klihub klihub Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@klihub
Copy link
Member

klihub commented Aug 19, 2024

@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.

@klihub
Copy link
Member

klihub commented Aug 23, 2024

@liangjingtao11 Ping. Do you have time to address the remaining issues ?

@klihub
Copy link
Member

klihub commented Sep 6, 2024

Closing in favor of updated PR #107.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RemoveMount does not work
5 participants