cilium: minor lb follow-ups #13821
cilium: minor lb follow-ups #13821
Conversation
Normalize status output and dump actual passed values instead of strings.Title(strings.ToLower()) hackery. Also remove these for cases where it's not needed. Signed-off-by: Daniel Borkmann <[email protected]>
We've long moved them out of beta in the main doc. Signed-off-by: Daniel Borkmann <[email protected]>
borkmann
added
area/daemon
borkmann
force-pushed
the
pr/cilium-lb-follow-ups
branch
from
b638aa6 to
989a34a
Compare
|
test-me-please |
borkmann
force-pushed
the
pr/cilium-lb-follow-ups
branch
from
989a34a to
95f20c9
Compare
|
test-me-please |
|
retest-gke |
|
(gke flakes unrelated & random tests) |
There was a problem hiding this comment.
LGTM.
The rearranging of help text was a bit of a surprise in the middle and I think we do have the option to take one step further on the arg renaming on the Helm side, but assuming we agree with the overall changes the code looks fine. More specific comments below.
daemon/cmd/cmdhelp_template.go
Outdated
| @@ -1,98 +0,0 @@ | |||
| // Copyright 2020 Authors of Cilium | |||
There was a problem hiding this comment.
Regarding the help changes:
There are various issues, e.g. flags are forgotten to be added to sections given this needs to be explicitly specified and then we end up with a random chunk of help
flags at the end of the --help dump.
If the random chunk at the end is the problem, I think we can write some simple trickery to ensure the help commands are categorized as part of the build process, so if they're not categorized then we tell the contributor to go back and categorize the flag.
Personally I don't really refer to the help text at all so I can't really speak for whether the sectioning is useful or not, but I was a little surprised to see a CLI change like this in an LB followups PR. Has there been a community discussion on this topic?
The change itself seems to accurately reflect the intent though, so code-wise 👍
There was a problem hiding this comment.
I've moved the commit out and will open a separate PR for discussion.
| @@ -2494,6 +2500,38 @@ func (c *DaemonConfig) populateDevices() { | |||
| } | |||
| } | |||
|
|
|||
| func (c *DaemonConfig) populateLoadBalancerSettings() { | |||
There was a problem hiding this comment.
I think BindEnvWithLegacyEnvFallback() achieves the same logic but in a simpler way (albeit with silent preference for the newer option)?
At a glance, the logic below looks right.
| @@ -479,7 +479,7 @@ data: | |||
| node-port-mode: {{ .Values.nodePort.mode | quote }} | |||
| {{- end }} | |||
| {{- if hasKey .Values.nodePort "algorithm" }} | |||
| node-port-algorithm: {{ .Values.nodePort.algorithm | quote }} | |||
| bpf-lb-algorithm: {{ .Values.nodePort.algorithm | quote }} | |||
| {{- end }} | |||
There was a problem hiding this comment.
(lines above LGTM) 👍
We're changing a bunch of other Helm options this release, so if we were to also rename the other acceleration/mode options in Helm then this would be a good time to do so.
There was a problem hiding this comment.
Thanks, do you have some pointers to commits from our side & how we retain backwards compat?
There was a problem hiding this comment.
The current approach of the Helm changes replaces every option with an equivalent, differently-named option which the user must specify (using the helm options table in the upgrade guide: https://docs.cilium.io/en/latest/operations/upgrade/#helm-options)
There was a problem hiding this comment.
Nice, I'll rename the LB ones then, but in new PR.
This adds more generic --bpf-lb-{mode,algorithm,acceleration} which
replace the --node-port-{mode,algorithm,acceleration} options since the
latter have nothing really to do with NodePort itself but are rather
more generic for service load-balancing. The old ones are hidden from
the --help output but can still be used in order to not break existing
setups as alias.
Change the Helm LB algorithm setting from node-port-algorithm over to
bpf-lb-algorithm. This is not emitted by default, and is only newly
introduced in 1.9. I'm not alias'ing anything more at this point wrt
.Values.nodePort* as there are multiple options also from prior releases
that could get a rework for alias'ing here.
Signed-off-by: Daniel Borkmann <[email protected]>
borkmann
force-pushed
the
pr/cilium-lb-follow-ups
branch
from
95f20c9 to
a170369
Compare
|
(CI from current run green) |
This option was marked deprecated and scheduled for removal in 1.9. Similarly as in 1e1ea64 ("daemon, option: remove deprecated ipv4-cluster-cidr-mask-size option") document this option as removed. Signed-off-by: Daniel Borkmann <[email protected]>
borkmann
force-pushed
the
pr/cilium-lb-follow-ups
branch
from
a170369 to
0ed231f
Compare
See commit msgs. Addresses items marked as release-blocker from #13138 .