Skip to content

Commit b44bdb5

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #8427 from fabriziopandini/fix-path-nodes
🐛 fix node label propagation
2 parents 828c7ea + c21b898 commit b44bdb5

File tree

5 files changed

+291
-90
lines changed

5 files changed

+291
-90
lines changed

‎api/v1beta1/common_types.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ const (
7474
// OwnerKindAnnotation is the annotation set on nodes identifying the owner kind.
7575
OwnerKindAnnotation = "cluster.x-k8s.io/owner-kind"
7676

77+
// LabelsFromMachineAnnotation is the annotation set on nodes to track the labels originated from machines.
78+
LabelsFromMachineAnnotation = "cluster.x-k8s.io/labels-from-machine"
79+
7780
// OwnerNameAnnotation is the annotation set on nodes identifying the owner name.
7881
OwnerNameAnnotation = "cluster.x-k8s.io/owner-name"
7982

‎docs/book/src/reference/labels_and_annotations.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
| unsafe.topology.cluster.x-k8s.io/disable-update-class-name-check | It can be used to disable the webhook check on update that disallows a pre-existing Cluster to be populated with Topology information and Class. |
2626
| cluster.x-k8s.io/cluster-name | It is set on nodes identifying the name of the cluster the node belongs to. |
2727
| cluster.x-k8s.io/cluster-namespace | It is set on nodes identifying the namespace of the cluster the node belongs to. |
28+
| cluster.x-k8s.io/labels-from-machine | It is set on nodes to track the labels originated from machines. |
2829
| cluster.x-k8s.io/machine | It is set on nodes identifying the machine the node belongs to. |
2930
| cluster.x-k8s.io/owner-kind | It is set on nodes identifying the owner kind. |
3031
| cluster.x-k8s.io/owner-name | It is set on nodes identifying the owner name. |

‎internal/controllers/machine/machine_controller.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,6 @@ import (
6060
const (
6161
// controllerName defines the controller used when creating clients.
6262
controllerName = "machine-controller"
63-
64-
// machineManagerName is the manager name used for Server-Side-Apply (SSA) operations
65-
// in the Machine controller.
66-
machineManagerName = "capi-machine"
6763
)
6864

6965
var (

‎internal/controllers/machine/machine_controller_noderef.go

Lines changed: 52 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,17 @@ import (
2424
"github.com/pkg/errors"
2525
corev1 "k8s.io/api/core/v1"
2626
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27-
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
28-
"k8s.io/apimachinery/pkg/types"
2927
"k8s.io/klog/v2"
3028
ctrl "sigs.k8s.io/controller-runtime"
3129
"sigs.k8s.io/controller-runtime/pkg/client"
3230

3331
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3432
"sigs.k8s.io/cluster-api/api/v1beta1/index"
3533
"sigs.k8s.io/cluster-api/controllers/noderefutil"
36-
"sigs.k8s.io/cluster-api/internal/util/ssa"
3734
"sigs.k8s.io/cluster-api/internal/util/taints"
3835
"sigs.k8s.io/cluster-api/util"
3936
"sigs.k8s.io/cluster-api/util/annotations"
4037
"sigs.k8s.io/cluster-api/util/conditions"
41-
"sigs.k8s.io/cluster-api/util/patch"
4238
)
4339

4440
var (
@@ -105,40 +101,26 @@ func (r *Reconciler) reconcileNode(ctx context.Context, cluster *clusterv1.Clust
105101
// Set the NodeSystemInfo.
106102
machine.Status.NodeInfo = &node.Status.NodeInfo
107103

108-
// Reconcile node annotations.
109-
patchHelper, err := patch.NewHelper(node, remoteClient)
110-
if err != nil {
111-
return ctrl.Result{}, err
112-
}
113-
desired := map[string]string{
104+
// Compute all the annotations that CAPI is setting on nodes;
105+
// CAPI only enforces some annotations and never changes or removes them.
106+
nodeAnnotations := map[string]string{
114107
clusterv1.ClusterNameAnnotation: machine.Spec.ClusterName,
115108
clusterv1.ClusterNamespaceAnnotation: machine.GetNamespace(),
116109
clusterv1.MachineAnnotation: machine.Name,
117110
}
118111
if owner := metav1.GetControllerOfNoCopy(machine); owner != nil {
119-
desired[clusterv1.OwnerKindAnnotation] = owner.Kind
120-
desired[clusterv1.OwnerNameAnnotation] = owner.Name
121-
}
122-
if annotations.AddAnnotations(node, desired) {
123-
if err := patchHelper.Patch(ctx, node); err != nil {
124-
log.V(2).Info("Failed patch node to set annotations", "err", err, "node name", node.Name)
125-
return ctrl.Result{}, err
126-
}
112+
nodeAnnotations[clusterv1.OwnerKindAnnotation] = owner.Kind
113+
nodeAnnotations[clusterv1.OwnerNameAnnotation] = owner.Name
127114
}
128115

129-
updatedNode := unstructuredNode(node.Name, node.UID, getManagedLabels(machine.Labels))
130-
err = ssa.Patch(ctx, remoteClient, machineManagerName, updatedNode, ssa.WithCachingProxy{Cache: r.ssaCache, Original: node})
131-
if err != nil {
132-
return ctrl.Result{}, errors.Wrap(err, "failed to apply labels to Node")
133-
}
134-
// Update `node` with the new version of the object.
135-
if err := r.Client.Scheme().Convert(updatedNode, node, nil); err != nil {
136-
return ctrl.Result{}, errors.Wrapf(err, "failed to convert node to structured object %s", klog.KObj(node))
137-
}
116+
// Compute labels to be propagated from Machines to nodes.
117+
// NOTE: CAPI should manage only a subset of node labels, everything else should be preserved.
118+
// NOTE: Once we reconcile node labels for the first time, the NodeUninitializedTaint is removed from the node.
119+
nodeLabels := getManagedLabels(machine.Labels)
138120

139121
// Reconcile node taints
140-
if err := r.reconcileNodeTaints(ctx, remoteClient, node); err != nil {
141-
return ctrl.Result{}, errors.Wrapf(err, "failed to reconcile taints on Node %s", klog.KObj(node))
122+
if err := r.patchNode(ctx, remoteClient, node, nodeLabels, nodeAnnotations); err != nil {
123+
return ctrl.Result{}, errors.Wrapf(err, "failed to reconcile Node %s", klog.KObj(node))
142124
}
143125

144126
// Do the remaining node health checks, then set the node health to true if all checks pass.
@@ -156,17 +138,6 @@ func (r *Reconciler) reconcileNode(ctx context.Context, cluster *clusterv1.Clust
156138
return ctrl.Result{}, nil
157139
}
158140

159-
// unstructuredNode returns a raw unstructured from Node input.
160-
func unstructuredNode(name string, uid types.UID, labels map[string]string) *unstructured.Unstructured {
161-
obj := &unstructured.Unstructured{}
162-
obj.SetAPIVersion("v1")
163-
obj.SetKind("Node")
164-
obj.SetName(name)
165-
obj.SetUID(uid)
166-
obj.SetLabels(labels)
167-
return obj
168-
}
169-
170141
// getManagedLabels gets a map[string]string and returns another map[string]string
171142
// filtering out labels not managed by CAPI.
172143
func getManagedLabels(labels map[string]string) map[string]string {
@@ -269,16 +240,48 @@ func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID *n
269240
return &nodeList.Items[0], nil
270241
}
271242

272-
func (r *Reconciler) reconcileNodeTaints(ctx context.Context, remoteClient client.Client, node *corev1.Node) error {
273-
patchHelper, err := patch.NewHelper(node, remoteClient)
274-
if err != nil {
275-
return errors.Wrapf(err, "failed to create patch helper for Node %s", klog.KObj(node))
243+
// PatchNode is required to workaround an issue on Node.Status.Address which is incorrectly annotated as patchStrategy=merge
244+
// and this causes SSA patch to fail in case there are two addresses with the same key https://github.com/kubernetes-sigs/cluster-api/issues/8417
245+
func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client, node *corev1.Node, newLabels, newAnnotations map[string]string) error {
246+
newNode := node.DeepCopy()
247+
248+
// Adds the annotations CAPI sets on the node.
249+
hasAnnotationChanges := annotations.AddAnnotations(newNode, newAnnotations)
250+
251+
// Adds the labels from the Machine.
252+
// NOTE: in order to handle deletion we are tracking the labels set from the Machine in an annotation.
253+
// At the next reconcile we are going to use this for deleting labels previously set by the Machine, but
254+
// not present anymore. Labels not set from machines should be always preserved.
255+
if newNode.Labels == nil {
256+
newNode.Labels = make(map[string]string)
257+
}
258+
hasLabelChanges := false
259+
labelsFromPreviousReconcile := strings.Split(newNode.Annotations[clusterv1.LabelsFromMachineAnnotation], ",")
260+
if len(labelsFromPreviousReconcile) == 1 && labelsFromPreviousReconcile[0] == "" {
261+
labelsFromPreviousReconcile = []string{}
262+
}
263+
labelsFromCurrentReconcile := []string{}
264+
for k, v := range newLabels {
265+
if cur, ok := newNode.Labels[k]; !ok || cur != v {
266+
newNode.Labels[k] = v
267+
hasLabelChanges = true
268+
}
269+
labelsFromCurrentReconcile = append(labelsFromCurrentReconcile, k)
276270
}
277-
// Drop the NodeUninitializedTaint taint on the node.
278-
if taints.RemoveNodeTaint(node, clusterv1.NodeUninitializedTaint) {
279-
if err := patchHelper.Patch(ctx, node); err != nil {
280-
return errors.Wrapf(err, "failed to patch Node %s to modify taints", klog.KObj(node))
271+
for _, k := range labelsFromPreviousReconcile {
272+
if _, ok := newLabels[k]; !ok {
273+
delete(newNode.Labels, k)
274+
hasLabelChanges = true
281275
}
282276
}
283-
return nil
277+
annotations.AddAnnotations(newNode, map[string]string{clusterv1.LabelsFromMachineAnnotation: strings.Join(labelsFromCurrentReconcile, ",")})
278+
279+
// Drop the NodeUninitializedTaint taint on the node given that we are reconciling labels.
280+
hasTaintChanges := taints.RemoveNodeTaint(newNode, clusterv1.NodeUninitializedTaint)
281+
282+
if !hasAnnotationChanges && !hasLabelChanges && !hasTaintChanges {
283+
return nil
284+
}
285+
286+
return remoteClient.Patch(ctx, newNode, client.StrategicMergeFrom(node))
284287
}

0 commit comments

Comments
 (0)