Skip to content

Check for duplicated sysctl keys#730

Merged
dcbw merged 1 commit intocontainernetworking:mainfrom
mmirecki:tuning_duplicate_check
May 4, 2022
Merged

Check for duplicated sysctl keys#730
dcbw merged 1 commit intocontainernetworking:mainfrom
mmirecki:tuning_duplicate_check

Conversation

@mmirecki
Copy link
Copy Markdown
Contributor

The tuning-cni config currently allows to specif duplicated sysctls, also with conflicting values. Duplicated sysctl entries are usually not used intentionally, but are rather the result of an error in configuration.
This PR adds a check for duplicated sysctl's.

@squeed
Copy link
Copy Markdown
Member

squeed commented Apr 20, 2022

@mmirecki looks good! Can you fix DCO and go fmt? Then I can merge when CI goes green.

@mmirecki mmirecki force-pushed the tuning_duplicate_check branch from c09a7df to 72db601 Compare April 21, 2022 08:08
Comment thread plugins/meta/tuning/tuning.go Outdated
@mmirecki mmirecki force-pushed the tuning_duplicate_check branch from 72db601 to 7accdeb Compare April 21, 2022 13:02
@squeed
Copy link
Copy Markdown
Member

squeed commented Apr 26, 2022

lgtm, will merge tomorrow

Comment thread plugins/meta/tuning/tuning.go Outdated
@mmirecki mmirecki force-pushed the tuning_duplicate_check branch from 7accdeb to 50ca338 Compare April 27, 2022 14:10
@dcbw
Copy link
Copy Markdown
Member

dcbw commented Apr 27, 2022

LGTM after the dupe check gets moved to parseConf() instead of just in cmdAdd().

@squeed
Copy link
Copy Markdown
Member

squeed commented Apr 27, 2022

@mmirecki @dcbw observes we should do this when parsing the config for CHECK too, right?

@mmirecki mmirecki force-pushed the tuning_duplicate_check branch from 50ca338 to 7c452c7 Compare April 27, 2022 20:13
@dcbw
Copy link
Copy Markdown
Member

dcbw commented May 4, 2022

/lgtm

@dcbw dcbw merged commit 9c59728 into containernetworking:main May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants