-
Notifications
You must be signed in to change notification settings - Fork 433
Add init container to apply Antrea-specific sysctl configuration #7651
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
base: main
Are you sure you want to change the base?
Add init container to apply Antrea-specific sysctl configuration #7651
Conversation
7297d7b to
b3c26f0
Compare
123ed07 to
2326ffd
Compare
6377498 to
e89dd82
Compare
build/charts/antrea/values.yaml
Outdated
| # -- Resource requests and limits for the antrea-sysctl-init init container. | ||
| resources: | ||
| requests: | ||
| cpu: "100m" |
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.
This is too much for placing a file in the folder. Maybe 20m is enough if we'd like to define it. Or just remove this from the manifest.
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 think removing this is just okay.
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.
Maybe 100m is harmless. It seems that 100m is a convention.
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.
Even it's harmless, it would make more sense to give it a reasonable value.
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.
50m seems to be a good choice for future using.
e89dd82 to
adaadf7
Compare
cmd/antrea-sysctl-init/main.go
Outdated
|
|
||
| func run(opts *options) error { | ||
| filePath := path.Join(opts.sysctlDir, opts.overwriteFile) | ||
| sysctlConfig := buildAntreaSysctlConfig(opts.hostGatewayName) |
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.
It seems systemd-sysctl needs to be restarted? https://github.com/cilium/cilium/pull/20072/files#diff-1cadee1ea10bb25d793baf555b85040a00ff0bc7f049a2542f0c2590ab4e7f0fR132-R136
Can you double check?
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.
In some OS releases, restart is not required. For Ubuntu, we need a test.
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.
The cases are:
- On some OS distributions, adding a new file under /etc/sysctl.d is sufficient to ensure that sysctl settings are automatically re-applied to existing network interfaces when an interface is added or updated. In these environments, the newly added interface also receives theenforced settings.
- On Ubuntu, sysctl configuration files under /etc/sysctl.d are not automatically re-applied when network interfaces are added or updated. New interfaces inherit values from net.ipv4.conf.default, while existing interfaces retain their current settings unless sysctl is explicitly reloaded.
- Executing the command
sysctl --systemon all tested OS distributions explicitly applies sysctl configuration files under /etc/sysctl.d to all existing network interfaces, regardless of distribution-specific behavior.
build/charts/antrea/values.yaml
Outdated
| # The filename is chosen to sort last in lexicographic order within the sysctl | ||
| # configuration directory, so that the settings in this file are applied after other | ||
| # distribution- or administrator- provided sysctl configuration files. | ||
| overwriteFile: "99-zzzz-antrea.conf" |
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 am not seeing there is any need to let user to define this. Better to keep the name as a constant.
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.
We'd better keep the parameter. A corner case is that there is an existing file named 99-zzzzzzzz-xxxx.conf. If that really happens, at least we have a chance to modify our file name so that it can be last in lexicographical order.
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.
We are not modifying sysctl settings for interfaces that are not managed by Antrea, correct? If so, it should be fine that this configuration file is not applied last.
If there is another sysctl configuration overriding Antrea’s settings with 99-zzzzzzz.conf, that is presumably intentional. In that case, rather than continually increasing the filename priority by appending more “z”s, it would make more sense to first understand the purpose of the overriding configuration.
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.
We are not modifying sysctl settings for interfaces that are not managed by Antrea, correct? If so, it should be fine that this configuration file is not applied last.
Will update the comment to state that a relatively high filename prefix is used to ensure the file will be applied after most default distribution- or administrator-provided sysctl configuration files.
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.
The comment already explains it. My point is that it seems unnecessary to make it configurable.
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.
Removed the parameter.
build/charts/antrea/values.yaml
Outdated
| # -- Resource requests and limits for the antrea-sysctl-init init container. | ||
| resources: | ||
| requests: | ||
| cpu: "100m" |
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.
Even it's harmless, it would make more sense to give it a reasonable value.
build/charts/antrea/values.yaml
Outdated
| # The filename is chosen to sort last in lexicographic order within the sysctl | ||
| # configuration directory, so that the settings in this file are applied after other | ||
| # distribution- or administrator- provided sysctl configuration files. | ||
| overwriteFile: "99-zzzz-antrea.conf" |
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.
We are not modifying sysctl settings for interfaces that are not managed by Antrea, correct? If so, it should be fine that this configuration file is not applied last.
If there is another sysctl configuration overriding Antrea’s settings with 99-zzzzzzz.conf, that is presumably intentional. In that case, rather than continually increasing the filename priority by appending more “z”s, it would make more sense to first understand the purpose of the overriding configuration.
857ea44 to
61ce01c
Compare
0ff3bd6 to
a64f469
Compare
| kubeletRootDir: "/var/lib/kubelet" | ||
| antreaSysctlInit: | ||
| # -- Enable the sysctl override. When enabled, the init container will do the process. | ||
| enable: true |
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.
Is it enabled by default intentionally? I thought it's only needed on some platforms?
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.
It's indeed only needed on some OS releases, and is redundant for others. I thought it is generally harmless to enable it by default. An alternative is to keep it disabled by default and clearly document when and why it should be enabled. Any ideas where should we document that?
a64f469 to
33a45ce
Compare
33a45ce to
256e051
Compare
|
/test-all |
|
/scale-agent |
256e051 to
d1c4207
Compare
|
/test-all |
Introduce a new antrea-sysctl-init command to run as an init container before Antrea components start. The command creates a dedicated sysctl.d configuration file containing Antrea-specific sysctl settings required by features that rely on policy routing (e.g. Egress). The sysctl configuration applies only to the interfaces managed by Antrea. A relatively high filename prefix is used so that the Antrea- specific sysctl configuration is applied after most default distribution- or administrator-provided sysctl configuration files, while still allowing explicit higher-priority overrides when desired. This logic is intentionally implemented as an init container rather than as part of antrea-agent, to avoid modifying node-wide sysctl state during normal agent operation. Signed-off-by: Hongliang Liu <[email protected]>
Signed-off-by: Hongliang Liu <[email protected]>
d1c4207 to
7933864
Compare
|
/test-all |
|
/test-all |
|
/test-e2e |
1 similar comment
|
/test-e2e |
Introduce a new antrea-sysctl-init command to run as an init container
before Antrea components start. The command creates a dedicated sysctl.d
configuration file containing Antrea-specific sysctl settings required
by features that rely on policy routing (e.g. Egress).
The sysctl configuration applies only to the interfaces managed by
Antrea. A relatively high filename prefix is used so that the Antrea-
specific sysctl configuration is applied after most default distribution-
or administrator-provided sysctl configuration files, while still allowing
explicit higher-priority overrides when desired.
This logic is intentionally implemented as an init container rather than
as part of antrea-agent, to avoid modifying node-wide sysctl state during
normal agent operation.