Remove NotManagedByMCM when not applicable (#866)

* Remove NotManagedByMCM when not applicable

* enhanced unit test in case #AnnotateNodesUnmanagedByMCM

* log both node and machine name when removing NotManagedByMCM annotation

* review comment: leverage updateNodeWithAnnotations for removal

* fixed import reordering to original in cli options

Author: elankath

pkg/util/provider/app/options/options.go

@@ -70,7 +70,7 @@ func NewMCServer() *MCServer {
 				MaxEvictRetries:                          drain.DefaultMaxEvictRetries,
 				PvDetachTimeout:                          metav1.Duration{Duration: 2 * time.Minute},
 				PvReattachTimeout:                        metav1.Duration{Duration: 90 * time.Second},
-				MachineSafetyOrphanVMsPeriod:             metav1.Duration{Duration: 30 * time.Minute},
+				MachineSafetyOrphanVMsPeriod:             metav1.Duration{Duration: 15 * time.Minute},
 				MachineSafetyAPIServerStatusCheckPeriod:  metav1.Duration{Duration: 1 * time.Minute},
 				MachineSafetyAPIServerStatusCheckTimeout: metav1.Duration{Duration: 30 * time.Second},
 			},

pkg/util/provider/machinecontroller/machine_safety.go

@@ -170,7 +170,7 @@ func (c *controller) AnnotateNodesUnmanagedByMCM(ctx context.Context) (machineut
 		return machineutils.LongRetry, err
 	}
 	for _, node := range nodes {
-		_, err := c.getMachineFromNode(node.Name)
+		machine, err := c.getMachineFromNode(node.Name)
 		if err != nil {
 			if err == errMultipleMachineMatch {
 				klog.Errorf("Couldn't fetch machine, Error: %s", err)
@@ -193,11 +193,23 @@ func (c *controller) AnnotateNodesUnmanagedByMCM(ctx context.Context) (machineut
 					machineutils.NotManagedByMCM: "1",
 				}
 
+				klog.V(3).Infof("Adding NotManagedByMCM annotation to Node %q", node.Name)
 				// err is returned only when node update fails
-				if err := c.updateNodeWithAnnotation(ctx, nodeCopy, annotations); err != nil {
+				if err := c.updateNodeWithAnnotations(ctx, nodeCopy, annotations); err != nil {
 					return machineutils.MediumRetry, err
 				}
 			}
+		} else {
+			_, hasAnnot := node.Annotations[machineutils.NotManagedByMCM]
+			if !hasAnnot {
+				continue
+			}
+			klog.V(3).Infof("Removing NotManagedByMCM annotation from Node %q associated with Machine %q", node.Name, machine.Name)
+			nodeCopy := node.DeepCopy()
+			delete(nodeCopy.Annotations, machineutils.NotManagedByMCM)
+			if err := c.updateNodeWithAnnotations(ctx, nodeCopy, nil); err != nil {
+				return machineutils.MediumRetry, err
+			}
 		}
 	}
 

pkg/util/provider/machinecontroller/machine_safety_test.go

@@ -418,7 +418,8 @@ var _ = Describe("safety_logic", func() {
 	Describe("#AnnotateNodesUnmanagedByMCM", func() {
 
 		type setup struct {
-			node *corev1.Node
+			node                     *corev1.Node
+			associateNodeWithMachine bool
 		}
 		type action struct {
 		}
@@ -441,6 +442,25 @@ var _ = Describe("safety_logic", func() {
 				targetCoreObjects := []runtime.Object{}
 				controlMachineObjects := []runtime.Object{}
 
+				if data.setup.associateNodeWithMachine {
+					//optional machine object for test-node-1 ie data.setup.node
+					testMachineObject := &v1alpha1.Machine{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "testmachine_0",
+							Namespace: testNamespace,
+							Labels: map[string]string{
+								v1alpha1.NodeLabelKey: data.setup.node.Name,
+							},
+						},
+						Status: v1alpha1.MachineStatus{
+							CurrentStatus: v1alpha1.CurrentStatus{
+								Phase: v1alpha1.MachineRunning,
+							},
+						},
+					}
+					controlMachineObjects = append(controlMachineObjects, testMachineObject)
+				}
+
 				//machine object for test-node-1
 				testMachineObject := &v1alpha1.Machine{
 					ObjectMeta: metav1.ObjectMeta{
@@ -726,6 +746,66 @@ var _ = Describe("safety_logic", func() {
 					err:   nil,
 				},
 			}),
+			Entry("Node incorrectly assigned NotManagedByMCM annotation", &data{
+				setup: setup{
+					node: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-0",
+							Annotations: map[string]string{
+								"anno1":                      "value1",
+								machineutils.NotManagedByMCM: "1",
+							},
+						},
+					},
+					associateNodeWithMachine: true,
+				},
+				action: action{},
+				expect: expect{
+					node0: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-0",
+							// node0 no longer has NotManagedByMCM since it has a backing machine object.
+							Annotations: map[string]string{
+								"anno1": "value1",
+							},
+						},
+					},
+					node1: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-1",
+							Annotations: map[string]string{
+								"anno1": "value1",
+							},
+						},
+					},
+					node2: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-2",
+							Annotations: map[string]string{
+								"anno1": "value1",
+							},
+						},
+					},
+					retry: machineutils.LongRetry,
+					err:   nil,
+				},
+			}),
 		)
 	})
 })

pkg/util/provider/machinecontroller/machine_safety_util.go

@@ -22,7 +22,7 @@ import (
 	"k8s.io/klog/v2"
 )
 
-func (c *controller) updateNodeWithAnnotation(ctx context.Context, node *v1.Node, annotations map[string]string) error {
+func (c *controller) updateNodeWithAnnotations(ctx context.Context, node *v1.Node, annotations map[string]string) error {
 
 	// Initialize node annotations if empty
 	if node.Annotations == nil {
@@ -37,10 +37,9 @@ func (c *controller) updateNodeWithAnnotation(ctx context.Context, node *v1.Node
 	_, err := c.targetCoreClient.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{})
 
 	if err != nil {
-		klog.Errorf("Couldn't patch the node %q , Error: %s", node.Name, err)
+		klog.Errorf("Failed to update annotations for Node %q due to error: %s", node.Name, err)
 		return err
 	}
-	klog.V(2).Infof("Annotated node %q was annotated with NotManagedByMCM successfully", node.Name)
 
 	return nil
 }

pkg/util/provider/machinecontroller/machine_safety_util_test.go

@@ -17,13 +17,14 @@ package controller
 import (
 	"context"
 
-	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 )
 
 var _ = Describe("machine_safety_util", func() {
@@ -56,7 +57,7 @@ var _ = Describe("machine_safety_util", func() {
 				testNode := data.action.node
 				expectedNode := data.expect.node
 
-				err := c.updateNodeWithAnnotation(context.TODO(), testNode, data.action.annotations)
+				err := c.updateNodeWithAnnotations(context.TODO(), testNode, data.action.annotations)
 
 				waitForCacheSync(stop, c)