-
Notifications
You must be signed in to change notification settings - Fork 233
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
perf: [WIN-NPM] add all cached NetworkPolicies to a Pod at once #1893
Conversation
f2620f1
to
15857d9
Compare
15857d9
to
9ba6ecc
Compare
npm/config/config.go
Outdated
defaultResyncPeriod = 15 | ||
defaultApplyMaxBatches = 100 | ||
defaultApplyInterval = 500 | ||
defaultMaxBatchedACLsPerPod = 100 |
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.
Lets make this smaller, if there is an issue with applying ACLs due to some weird limit of data type they are using underneath, we can end up in a limbo of not being able to apply a bunch of network policies at once.
Smaller means more network policies are guaranteed to get applied. Wdyt ?
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.
discussed offline to make this 30
} | ||
|
||
klog.Infof("[PolicyManager] finished applying all rules to endpoint for batch %d out of %d. endpoint ID: %s", i+1, len(ruleBatches), epToModifyID) | ||
for policyKey := range policyKeys { |
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.
What if you applied a portion of policies and you error out ? you need to persist the state of policies you already applied on that endpoint so we dont end up with duplicate policies?
"ListeningPort": 10091, | ||
"ListeningAddress": "0.0.0.0", | ||
"ApplyMaxBatches": 100, | ||
"ResyncPeriodInMinutes": 15, |
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.
Add windows ds to this yaml.
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.
discussed offline that there may be script dependencies on either yaml
} | ||
} | ||
|
||
return nil |
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.
return list of policyKeys you applied here
|
||
if err := dp.policyMgr.AddAllPolicies(toAddPolicies, endpoint.id, endpoint.ip); err != nil { |
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.
make sure to update netpolrtef even if this errors out. because we could have applied a portion of the batch of policies
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Reason for Change:
Reduces time spent on ACL SysCalls by 99% at mid/high scale.
Specifically, when all of a Pod's NetworkPolicies have been cached. For example, for any Pod CRUD after NPM has reached steady state. Further optimized in #1900.
Issue Fixed:
Second of three PRs addressing Windows limitations in #1614.
Requirements:
Notes:
Experiment
500 NetworkPolicies applied to a Pod.
Results
MaxBatchedACLsPerPod=33
): 1 min 10 seconds for all Pods (even adding ACLs to the 15th Pod).