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

Release IPAM resources for exited containers #164

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

cathy-zhou
Copy link
Contributor

SRIOV plugin needs to delegate the DEL action to the IPAM plugin to
release IPAM resources even for terminated containers without valid
network namespace path.

Signed-off-by: Yun Zhou yunz@nvidia.com

SRIOV plugin needs to delegate the DEL action to the IPAM plugin to
release IPAM resources even for terminated containers without valid
network namespace path.

Signed-off-by: Yun Zhou <yunz@nvidia.com>
Copy link

@moshe010 moshe010 left a comment

Choose a reason for hiding this comment

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

LGTM

@girishmg
Copy link

girishmg commented Jan 7, 2021

+1

@ahalimx86
Copy link
Collaborator

I am just curious how this change will behave when cmdDel is invoked repeatedly with same IPAM and no network namespace?

if netConf.IPAM.Type != "" {
err = ipam.ExecDel(netConf.IPAM.Type, args.StdinData)
if err != nil {
return err
}
}

// https://github.com/kubernetes/kubernetes/pull/35240
if args.Netns == "" {

Choose a reason for hiding this comment

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

At this time the interface would just be moved into default net namespace (in case of net device) due to container exit, but we still need to rename the interface back to original name.

Copy link

Choose a reason for hiding this comment

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

That issue, if exists, is then orthogonal to what this PR is trying to fix, right? If rename of an interface is required, then it should be done in a separate PR.

@cathy-zhou
Copy link
Contributor Author

@ahalim-intel I think ipam must be able to handle it, if not, it is a bug need to be fixed. But passing this cmdDel to ipam is necessary to avoid ip leakage.

@martinkennelly martinkennelly merged commit adcf914 into k8snetworkplumbingwg:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants