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

Allows cmdDel to finish if netns doesn't exist #269

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

dougbtv
Copy link
Member

@dougbtv dougbtv commented Feb 21, 2019

More detail in the referenced issue. Generally the gist is that while a pod is in a failed state, the netns that Multus receives doesn't exist and the cmdDel function returns prematurely. With the premature return, we don't call the delegates during the delete -- in the case that the delegated CNI plugin needs to perform cleanup functions, this can leave it in a dirty state.

This fix lets the cmdDel continue.

Fixes #267

@dougbtv dougbtv self-assigned this Feb 21, 2019
@dougbtv dougbtv added high priority release-v3 PRs to make it in release branch labels Feb 21, 2019
@coveralls
Copy link

Pull Request Test Coverage Report for Build 662

  • 3 of 6 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 52.862%

Changes Missing Coverage Covered Lines Changed/Added Lines %
multus/multus.go 3 6 50.0%
Totals Coverage Status
Change from base Build 660: -0.005%
Covered Lines: 591
Relevant Lines: 1118

💛 - Coveralls

@@ -402,11 +402,15 @@ func cmdDel(args *skel.CmdArgs, exec invoke.Exec, kubeClient k8s.KubeClient) err
// https://github.com/kubernetes/kubernetes/issues/43014#issuecomment-287164444
_, ok := err.(ns.NSPathNotExistErr)
if ok {
return nil
logging.Debugf("cmdDel: WARNING netns may not exist, netns: %s, err: %s", netns, err)
Copy link
Member

Choose a reason for hiding this comment

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

if there is no namespace, it should delegate the issue to the plugins ? Example Bridge plugin: https://github.com/containernetworking/plugins/blob/master/plugins/main/bridge/bridge.go#L508
Macvlan plugin: https://github.com/containernetworking/plugins/blob/master/plugins/main/macvlan/macvlan.go#L251

So the conclusion is the delegate plugins should take care of the issues, Multus should delegate it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, consider the case without Multus that the plugin is being called directly -- the plugin would have to handle this case just the same.

And in the case where the plugin has cleanup to do that's irrespective of the namespace being there or not, it can still perform that cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

That is cool.. Agreed

Copy link
Member

@rkamudhan rkamudhan left a comment

Choose a reason for hiding this comment

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

LGTM :) Ready to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority release-v3 PRs to make it in release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete deletion when pod is failing
3 participants