-
Notifications
You must be signed in to change notification settings - Fork 370
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
[WIP] Skip processing ADD events of init Pods and Namespaces #636
Conversation
Thanks for your PR. The following commands are available:
These commands can only be run by members of the vmware-tanzu organization. |
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Changes look good to me. Do you think you could add some of the tests we talked about a while back by email?
@@ -1208,11 +1238,11 @@ func (n *NetworkPolicyController) syncInternalNetworkPolicy(key string) error { | |||
// Lock the internal NetworkPolicy store as we may have a case where in the | |||
// same internal NetworkPolicy is being updated in the NetworkPolicy UPDATE | |||
// handler. | |||
n.internalNetworkPolicyMutex.Lock() | |||
n.internalNetworkPolicyMutex.RLock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be an orthogonal change, do you think you can put it in a separate PR that we can merge right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually we do update the store at L1271.. should this be R?
|
||
// initPodSet caches UIDs of the Pods that exist before starting computation. | ||
// We don't need to handle their ADD events as all AddressGroups and AppliedToGroups | ||
// will be computed once at the beginning and they have counted these Pods in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of "all AddressGroups and AppliedToGroups will be computed once at the beginning and they have counted these Pods in.", do you think it would be more accurate to say "all AddressGroups and AppliedToGroups will be computed when the ADD events for NetworkPolicies are processed and they will account for these Pods"?
npController.podListerSynced = alwaysReady | ||
npController.namespaceListerSynced = alwaysReady | ||
npController.networkPolicyListerSynced = alwaysReady | ||
//npController.podListerSynced = alwaysReady |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking: do we need to remove this because otherwise the List
operations may not return the entire set of Pods / Namespaces?
do you know why we used alwaysReady
in the first place, I can't remember?
Can one of the admins verify this patch? |
#1937 took care of this via |
No description provided.