fix(controller): preserve shared Skyhook cordons

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>

Author: fallintoplace

operator/internal/wrapper/node.go

@@ -467,10 +467,14 @@ func (node *skyhookNode) HasSkyhookAnnotations() bool {
 
 // Cordon marks the node unschedulable and records the cordon in annotations for this Skyhook.
 func (node *skyhookNode) Cordon() {
-	_, ok := node.Annotations[fmt.Sprintf("%s/cordon_%s", v1alpha1.METADATA_PREFIX, node.skyhookName)]
+	if node.Annotations == nil {
+		node.Annotations = make(map[string]string)
+	}
+
+	_, ok := node.Annotations[cordonAnnotationKey(node.skyhookName)]
 	if !node.Spec.Unschedulable || !ok {
 		node.Spec.Unschedulable = true
-		node.Annotations[fmt.Sprintf("%s/cordon_%s", v1alpha1.METADATA_PREFIX, node.skyhookName)] = "true"
+		node.Annotations[cordonAnnotationKey(node.skyhookName)] = "true"
 		node.updated = true
 	}
 }
@@ -479,14 +483,32 @@ func (node *skyhookNode) Cordon() {
 func (node *skyhookNode) Uncordon() {
 
 	// if we hold a cordon remove it, also we dont want to remove a cordon if we dont have one...
-	_, ok := node.Annotations[fmt.Sprintf("%s/cordon_%s", v1alpha1.METADATA_PREFIX, node.skyhookName)]
+	_, ok := node.Annotations[cordonAnnotationKey(node.skyhookName)]
 	if ok {
-		node.Spec.Unschedulable = false
-		delete(node.Annotations, fmt.Sprintf("%s/cordon_%s", v1alpha1.METADATA_PREFIX, node.skyhookName))
+		delete(node.Annotations, cordonAnnotationKey(node.skyhookName))
+		// Multiple Skyhooks can cordon the same node; only mark it schedulable
+		// after every Skyhook-owned cordon has been released.
+		if !hasSkyhookCordon(node.Annotations) {
+			node.Spec.Unschedulable = false
+		}
 		node.updated = true
 	}
 }
 
+func cordonAnnotationKey(skyhookName string) string {
+	return fmt.Sprintf("%s/cordon_%s", v1alpha1.METADATA_PREFIX, skyhookName)
+}
+
+func hasSkyhookCordon(annotations map[string]string) bool {
+	prefix := fmt.Sprintf("%s/cordon_", v1alpha1.METADATA_PREFIX)
+	for key := range annotations {
+		if strings.HasPrefix(key, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 // Reset clears Skyhook-related state and annotations so the node can be reconfigured from scratch.
 func (node *skyhookNode) Reset() {
 
@@ -495,7 +517,7 @@ func (node *skyhookNode) Reset() {
 	node.skyhook.Status.Status = v1alpha1.StatusUnknown
 	node.skyhook.Updated = true
 
-	delete(node.Annotations, fmt.Sprintf("%s/cordon_%s", v1alpha1.METADATA_PREFIX, node.skyhookName))
+	delete(node.Annotations, cordonAnnotationKey(node.skyhookName))
 	delete(node.Annotations, fmt.Sprintf("%s/nodeState_%s", v1alpha1.METADATA_PREFIX, node.skyhookName))
 	delete(node.Annotations, fmt.Sprintf("%s/status_%s", v1alpha1.METADATA_PREFIX, node.skyhookName))
 

operator/internal/wrapper/node_test.go

@@ -178,6 +178,96 @@ var _ = Describe("SkyhookNode", func() {
 		})
 	})
 
+	Context("Cordon", func() {
+		It("should initialize annotations if the node has none", func() {
+			myCordonKey := cordonAnnotationKey("my-skyhook")
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-node",
+				},
+			}
+
+			sn, err := NewSkyhookNodeOnly(node, "my-skyhook")
+			Expect(err).ToNot(HaveOccurred())
+
+			sn.Cordon()
+
+			Expect(node.Spec.Unschedulable).To(BeTrue())
+			Expect(node.Annotations).To(HaveKeyWithValue(myCordonKey, "true"))
+			Expect(sn.Changed()).To(BeTrue())
+		})
+	})
+
+	Context("Uncordon", func() {
+		myCordonKey := cordonAnnotationKey("my-skyhook")
+		otherCordonKey := cordonAnnotationKey("other-skyhook")
+
+		It("should make the node schedulable if this Skyhook owns the only cordon", func() {
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-node",
+					Annotations: map[string]string{
+						myCordonKey: "true",
+					},
+				},
+				Spec: corev1.NodeSpec{Unschedulable: true},
+			}
+
+			sn, err := NewSkyhookNodeOnly(node, "my-skyhook")
+			Expect(err).ToNot(HaveOccurred())
+
+			sn.Uncordon()
+
+			Expect(node.Spec.Unschedulable).To(BeFalse())
+			Expect(node.Annotations).ToNot(HaveKey(myCordonKey))
+			Expect(sn.Changed()).To(BeTrue())
+		})
+
+		It("should keep the node unschedulable if another Skyhook still owns a cordon", func() {
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-node",
+					Annotations: map[string]string{
+						myCordonKey:    "true",
+						otherCordonKey: "true",
+					},
+				},
+				Spec: corev1.NodeSpec{Unschedulable: true},
+			}
+
+			sn, err := NewSkyhookNodeOnly(node, "my-skyhook")
+			Expect(err).ToNot(HaveOccurred())
+
+			sn.Uncordon()
+
+			Expect(node.Spec.Unschedulable).To(BeTrue())
+			Expect(node.Annotations).ToNot(HaveKey(myCordonKey))
+			Expect(node.Annotations).To(HaveKeyWithValue(otherCordonKey, "true"))
+			Expect(sn.Changed()).To(BeTrue())
+		})
+
+		It("should not change the node if this Skyhook does not own a cordon", func() {
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-node",
+					Annotations: map[string]string{
+						otherCordonKey: "true",
+					},
+				},
+				Spec: corev1.NodeSpec{Unschedulable: true},
+			}
+
+			sn, err := NewSkyhookNodeOnly(node, "my-skyhook")
+			Expect(err).ToNot(HaveOccurred())
+
+			sn.Uncordon()
+
+			Expect(node.Spec.Unschedulable).To(BeTrue())
+			Expect(node.Annotations).To(HaveKeyWithValue(otherCordonKey, "true"))
+			Expect(sn.Changed()).To(BeFalse())
+		})
+	})
+
 	Context("CleanupSCRMetadata", func() {
 		It("should remove matching keys and preserve others", func() {
 			node := &corev1.Node{