-
Notifications
You must be signed in to change notification settings - Fork 359
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 NetworkPolicy span calculation #5554
Conversation
/test-all |
7e35bc9
to
3940df1
Compare
@tnqn you marked this as as draft, so you no longer need an immediate review? |
Yes, I'm considering another fix, I will mark it as ready after I finalize the solution. |
138860d
to
cabc71d
Compare
/test-all |
@antoninbas it's ready for review now. |
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.
LGTM
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.
Question - can we add a flag to a NP object in storage to indicate the span is calculated or not (and store the object before span calculation)?
|
||
import "sync" | ||
|
||
// subscriber notifies multiple subscribers about any events that happen to the object they have subscribed. |
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.
object -> objects
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.
done
import "sync" | ||
|
||
// subscriber notifies multiple subscribers about any events that happen to the object they have subscribed. | ||
type subscriber struct { |
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.
Should we call it something like subscriptionService?
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.
I feel it's more common in Go to name objects with an -er suffix, but I understand subscriber
is somewhat confusing. I have renamed it to notifier
, it should make sense to subscribe to a notifier and send a message via a notifier, what do you think?
Change LGTM. I'm assuming by the definition of the issue that this only happens for new internalNPs? For internalNPs update events this won't happen as they are already in store |
I considered adding a flag but think it is more complicated and risky than decoupling notification mechanism from the storage:
|
A NetworkPolicy's span is calculated in internalNetworkPolicyWorker, based on the span of the AppliedToGroups it refers to, while the span of AppliedToGroup is calculated in appliedToGroupWorker which runs in parallel with internalNetworkPolicyWorker. It could happen that the calcuated span is out of date if AppliedToGroups' span is updated after internalNetworkPolicyWorker calculates a NetworkPolicy's span, and the NetworkPolicy wouldn't be enqueued for another sync if it's not committed to the storage yet. On the other hand, if we commit the NetworkPolicy to the storage before calculating the NetworkPolicy's span, it would have to use a stale span first and might need to update the NetworkPolicy twice and generate two update events in one sync. To fix the issue without generating extra events, we introduce a separate subscription mechanism that allows subscribing to update of AppliedToGroup for NetworkPolicy. With the subscription, we can still calculate the NetworkPolicy's span first, then commit it to the storage. If any of the subscribed AppliedToGroups are updated, the NetworkPolicy will be notified and resynced. Signed-off-by: Quan Tian <qtian@vmware.com>
It could also happen if a NP is updated to use a new AppliedToGroup. |
cabc71d
to
f0b30b4
Compare
Your points make sense. @tnqn |
/test-all |
A NetworkPolicy's span is calculated in internalNetworkPolicyWorker, based on the span of the AppliedToGroups it refers to, while the span of AppliedToGroup is calculated in appliedToGroupWorker which runs in parallel with internalNetworkPolicyWorker. It could happen that the calcuated span is out of date if AppliedToGroups' span is updated after internalNetworkPolicyWorker calculates a NetworkPolicy's span, and the NetworkPolicy wouldn't be enqueued for another sync if it's not committed to the storage yet.
On the other hand, if we commit the NetworkPolicy to the storage before calculating the NetworkPolicy's span, it would have to use a stale span first and might need to update the NetworkPolicy twice and generate two update events in one sync.
To fix the issue without generating extra events, we introduce a separate subscription mechanism that allows subscribing to update of AppliedToGroup for NetworkPolicy. With the subscription, we can still calculate the NetworkPolicy's span first, then commit it to the storage. If any of the subscribed AppliedToGroups are updated, the NetworkPolicy will be notified and resynced.
Fixes #5553