node-labeller: Remove obsolete functionalities #11146
Conversation
Node labeller was moved into core Kubevirt as part of v0.40 release [0]. As part of this move the labels were renamed to match Kubevirt practice and functionality to remove the old ones was introduced. Now when we don't support upgrade from the old version we can remove this functionality. [0] kubevirt@5eb8807 Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com> Signed-off-by: Ram Lavi <ralavi@redhat.com>
kubevirt-bot
added
release-note
RamLavi
changed the title
RamLavi
force-pushed
the
remove_obsolete_node_label_functions
branch
from
474b4ec
to
73524af
Compare
|
It's strange that the unit test lane failed, as I didn't add/remove any in this PR.. |
| @@ -230,20 +226,7 @@ func (n *NodeLabeller) patchNode(originalNode, node *v1.Node) error { | |||
| }) | |||
| } | |||
|
|
|||
| if !equality.Semantic.DeepEqual(originalNode.Annotations, node.Annotations) { | |||
There was a problem hiding this comment.
@RamLavi Are you sure its completely safe to remove the annotation handling? what about [LabellerSkipNodeAnnotation]
There was a problem hiding this comment.
WRT the LabellerSkipNodeAnnotation annotation, AFAIU this annotation is set by the user in order to opt-out the node-labeling entirely (see here). If that annotation is set then node-labeller will not patch anything, so this part of "patchNode" code won't run. In other words, I'm not removing this annotation's functionality.
As for if I'm completely sure that the annotations are not needed, it says so on the commit message that introduced the node-labeller on release 0.40 (see commit here):
This commit migrates the node labeller from ssp operator into kubevirt.
This feature labels nodes with host supported cpu models and features and hyper-v features.
After new labeller runs it deletes all old labels / annotations and
creates new labels with new namespace.
So IIUC now that we don't have to support the upgrade from this version then we can remove this code handling old annotations/labels.
... But I didn't stop there and further examined the code to double check..
Notice that:
- The original (now archived) node-labeller repo added both labels and annotations (see here).
- When the node-labeller moved to kubevirt (see main branch PR) on v0.40.0 - the behavior changed:
- new labels were only added to the metadata.labels (see here).
- annotations were only used in order to be removed (see here)
- Specifically - only the annotations with the DeprecatedLabellerNamespaceAnnotation prefix were removed. This deprecated annotation is exactly the one we are now no longer supporting.
Do you think I should check any further?
There was a problem hiding this comment.
@RamLavi Thank you for the detailed explanation!
kubevirt-bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xpivarc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
Thanks for asking! Please see answer |
|
/test pull-kubevirt-e2e-arm64 |
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
|
Required labels detected, running phase 2 presubmits: |
What this PR does
Before this PR:
node labeller supported deprecated annotations (since v0.40 release).
After this PR:
node labeller will not support these annotations, as kubevirt does not support upgrade from this release anymore.
Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place:
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note