Apply mc-name-suffix annotation to kubeletconfig #3353
Conversation
SrinivasAtmakuri
force-pushed
the
mc-name-suffix-kubeletconfig
branch
from
f9f2e8e
to
9943d45
Compare
SrinivasAtmakuri
force-pushed
the
mc-name-suffix-kubeletconfig
branch
from
9943d45
to
ae87b31
Compare
SrinivasAtmakuri
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
s-amann,
SudoBrendan,
Shivkumar13,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig and
dofinn
as code owners
SrinivasAtmakuri
added
ready-for-review
next-release
|
/azp run E2E - PullRequest |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| // machineconfiguration annotation added on the kubelet CRD - dynamic-node | ||
| mcAnnotationName = "machineconfiguration.openshift.io/mc-name-suffix" | ||
| mcAnnotationValue = "" |
There was a problem hiding this comment.
I would comment why we use "" here just so we remember in a while, it's unintuitive.
There was a problem hiding this comment.
I would comment why we use
""here just so we remember in a while, it's unintuitive.
Or we can make the name descriptive (e.g. lowPriority)?
There was a problem hiding this comment.
It's because the value "" is viewed by MCO as zero. Hm.
| if config.Annotations == nil { | ||
| config.Annotations = defaultConfig.Annotations | ||
| } | ||
| if val, ok := config.Annotations[mcAnnotationName]; !ok || val != mcAnnotationValue { |
There was a problem hiding this comment.
If this is already set, we shouldn't be changing it regardless of value, as it will disconnect the kubeletconfig from the already rendered machineconfig
There was a problem hiding this comment.
This is true, we understand MCO behaviour in this condition.
pkg/operator/controllers/autosizednodes/autosizednodes_controller.go
Outdated
Show resolved
Hide resolved
| config.Annotations = defaultConfig.Annotations | ||
| } | ||
| if val, ok := config.Annotations[mcAnnotationName]; !ok || val != mcAnnotationValue { | ||
| config.Annotations[mcAnnotationName] = mcAnnotationValue |
There was a problem hiding this comment.
What happens if a different kubeletconfig (maybe one provided by the customer) already his this annotation and value?
There was a problem hiding this comment.
Srini did some testing on this and found that if two kubeletconfig have the same value for this annotation it breaks MCO. This is potentially a breaking issue. I think we should rethink this approach because of this
|
The solution is creating stale finalizers in kubeletconfig, after discussions this approach is dropped. |
Which issue this PR addresses:
JIRA: https://issues.redhat.com/browse/ARO-5028
We want to proactively add MCO's
mc-name-suffixannotation set to empty string "" to the kubeletconfig dynamic-node. This will make the resulting rendered machineconfig 99-worker-generated-kubelet have no suffix, resulting in the lowest priority of kubeletconfigs.Fixes
What this PR does / why we need it:
Test plan for issue:
Is there any documentation that needs to be updated for this PR?