scale: [NPM] ignore leaked ipset references #1502
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
huntergregory
changed the title
vakalapa
reviewed
Aug 8, 2022
| grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, azureNPMPrefix) | ||
| azureIPSets, haveAzureIPSets, commandError := ioutil.PipeCommandToGrep(listCommand, grepCommand) | ||
| klog.Infof("running this command while resetting ipsets: [%s %s %s | %s %s]", ipsetCommand, ipsetListFlag, ipsetNameFlag, ioutil.Grep, azureNPMRegex) | ||
| azureIPSets, haveAzureIPSets, commandError := ioutil.PipeCommandToGrep(listNamesCommand, grepCommand) |
Contributor
There was a problem hiding this comment.
Can we add an extra check here ? if the total number of IPSets == IPSets with azure-npm- prefix, then wouldn't it be easy to just call in generic ipset flush and destroy ?
i am afraid there might some cases i am missing here, or have some weird scenario in linux which we are not thinking of. Wdyt ?
Contributor
There was a problem hiding this comment.
ipset list --names | grep -v azurenpmregex == 0
ipset flush && ipset destroy
vakalapa
reviewed
Aug 8, 2022
vakalapa
reviewed
Aug 8, 2022
| grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, ioutil.GrepQuietFlag, ioutil.GrepAntiMatchFlag, azureNPMPrefix) | ||
| commandString := fmt.Sprintf(" [%s %s %s | %s %s %s %s]", ipsetCommand, ipsetListFlag, ipsetNameFlag, ioutil.Grep, ioutil.GrepQuietFlag, ioutil.GrepAntiMatchFlag, azureNPMPrefix) | ||
| klog.Infof("running this command while resetting ipsets: [%s]", commandString) | ||
| _, haveNonAzureIPSets, commandError := ioutil.PipeCommandToGrep(listNamesCommand, grepCommand) |
Contributor
There was a problem hiding this comment.
nit: haveNonAzureIPSets -> haveNonAzureNPMIPSets
vakalapa
approved these changes
Aug 9, 2022
matmerr
approved these changes
Aug 9, 2022
Member
matmerr
left a comment
matmerr
left a comment
There was a problem hiding this comment.
marking for posterity that would like to steer away from all the grep/piping/exit code redirection, opt for something more built in
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes at high scale, the Linux ipset utility will leak reference counts for ipsets e.g. there will be no iptables rules or lists referencing an ipset, yet it will have a positive reference count. As a result, some lingering ipsets can't be deleted when NPM boots up.
In NPM "v1", we ignore these in-use-by-kernel errors. Now we do the same in "v2" to prevent a CrashLoopBackOff when this kernel leak happens.