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

perf: [WIN-NPM] apply ipsets in background #1875

Merged
merged 20 commits into from
Apr 19, 2023

Conversation

huntergregory
Copy link
Contributor

@huntergregory huntergregory commented Mar 28, 2023

Reason for Change:
Makes IPSet SysCalls 96% (23x) faster when there are no NetworkPolicies to process. Further optimized in #1900.

Issue Fixed:
First of three PRs addressing Windows limitations in #1614.

Requirements:

Notes:

Experiment

1.5K Pods, 10 NetworkPolicies

Results

  • Old NPM: 3 hours to steady state
  • This PR: 8 minutes to steady state

@huntergregory huntergregory added npm Related to NPM. windows labels Mar 28, 2023
@huntergregory huntergregory force-pushed the hgregory/03-28-apply-ipsets-in-background branch from 4f2da6a to 038137d Compare March 29, 2023 01:30
@huntergregory
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@huntergregory huntergregory force-pushed the hgregory/03-28-apply-ipsets-in-background branch from ff7062d to a6eaf90 Compare April 5, 2023 23:16
@huntergregory huntergregory marked this pull request as ready for review April 5, 2023 23:18
@huntergregory huntergregory requested a review from a team as a code owner April 5, 2023 23:18
@huntergregory huntergregory requested review from ck319 and removed request for a team April 5, 2023 23:18
Copy link
Contributor

@vakalapa vakalapa left a comment

Choose a reason for hiding this comment

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

Overall looks good, some comments on user experience, also, we will need to thoroughly test these changes

@@ -148,6 +148,9 @@ metadata:
data:
azure-npm.json: |
{
"WindowsNetworkName": "azure",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep this network name for simplicity.

WindowsNetworkName: util.AzureNetworkName,
ResyncPeriodInMinutes: defaultResyncPeriod,
WindowsNetworkName: util.AzureNetworkName,
ApplyDataPlaneMaxBatches: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure these default are real numbers, if config map is not applied properly we should not be erroring out.

@@ -464,3 +564,7 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
}
return nil
}

func (dp *DataPlane) configuredToApplyInBackground() bool {
return util.IsWindowsDP() && dp.ApplyDataPlaneMaxBatches > 0 && dp.ApplyDataPlaneInterval > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of this, we should add a new flag which defines this new behavior.

@huntergregory huntergregory mentioned this pull request Apr 7, 2023
3 tasks
@huntergregory
Copy link
Contributor Author

huntergregory commented Apr 14, 2023

also, we will need to thoroughly test these changes

Conformance and Cyclonus have succeeded for a few rounds (excluding Windows issues). Also finished multiple rounds of testing using these scripts: https://github.com/Azure/azure-container-networking/tree/master/test/scale

@@ -451,3 +547,7 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
}
return nil
}

func (dp *DataPlane) configuredToApplyInBackground() bool {
return util.IsWindowsDP() && dp.ApplyInBackground
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking for isWindowsDP everytime we need to applyDP, can we assign a global DP variable which we can set once in INIT and then only keep checking that value ?

"PlaceAzureChainFirst": true
"PlaceAzureChainFirst": true,
"ApplyIPSetsOnNeed": false,
"ApplyInBackground": true
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting of space.

@vakalapa vakalapa merged commit aa163aa into master Apr 19, 2023
@vakalapa vakalapa deleted the hgregory/03-28-apply-ipsets-in-background branch April 19, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Related to NPM. windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants