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

Use WaitAndGetVNID() instead of GetVNID() when initializing network policy plugin on the node #13666

Merged

Conversation

pravisankar
Copy link

If a new namespace was created after vnid population and before listing namespaces in initNamespaces()[networkpolicy.go],GetVNID() could fail. Similar case after listing policies in initNamespaces().
Using WaitAndGetVNID() may not fix the issue but will alleviate the problem.

@pravisankar
Copy link
Author

@openshift/networking @danwinship PTAL

@pravisankar
Copy link
Author

Similar issue: #13265

@danwinship
Copy link
Contributor

If a new namespace was created after vnid population and before listing namespaces in initNamespaces()[networkpolicy.go],GetVNID() could fail.

If we're going to use WaitAndGetVNID in this case (and maybe #13265 suggests we have to?) then should we just get rid of the non-waiting version? It seems like it's basically never safe...

(Longer term it might be nicer to have a smarter EventQueue that can delay telling us about Pod/Service/NetworkPolicy/EgressNetworkPolicy events until their corresonding NetNamespace exists. Then we don't have to worry about the watcher threads blocking waiting for netids.)

…olicy plugin on the node

If a new namespace was created after vnid population and before listing namespaces in initNamespaces()[networkpolicy.go],
GetVNID() could fail. Similar case after listing policies in initNamespaces().
Using WaitAndGetVNID() may not fix the issue but will alleviate the problem.

GetVNID() is not used expect vnids_node.go and may not be used elsewhere due to the above reasons.
So renamed GetVNID() to getVNID().
@pravisankar pravisankar force-pushed the fix-vnid-fetch-networkpolicy branch from bf073a6 to 0b3e24b Compare April 10, 2017 18:57
@pravisankar
Copy link
Author

@danwinship
Yes, GetVNID() is not used expect vnids_node.go and may not be used elsewhere due to the above reasons. So renamed GetVNID() to getVNID().

Long term we could do either smarter EventQueue or some way to embed VNID annotation on Namespace before persisting the object.

@dcbw
Copy link
Contributor

dcbw commented Apr 10, 2017

LGTM

@danwinship
Copy link
Contributor

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 0b3e24b

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 11, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/313/) (Base Commit: dec90cf) (Image: devenv-rhel7_6134)

@openshift-bot openshift-bot merged commit e01d36c into openshift:master Apr 11, 2017
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.

4 participants