Merge pull request #464 from zeeke/validate-disable-drain

Validate .Spec.DisableDrain

pkg/webhook/client.go

@@ -11,7 +11,7 @@ import (
 	snclientset "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/client/clientset/versioned"
 )
 
-var snclient *snclientset.Clientset
+var snclient snclientset.Interface
 var kubeclient *kubernetes.Clientset
 
 func SetupInClusterClient() error {

pkg/webhook/validate.go

@@ -17,6 +17,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+
 	sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
 )
 
@@ -35,17 +37,60 @@ func validateSriovOperatorConfig(cr *sriovnetworkv1.SriovOperatorConfig, operati
 	glog.V(2).Infof("validateSriovOperatorConfig: %v", cr)
 	var warnings []string
 
-	if cr.GetName() == constants.DefaultConfigName {
-		if operation == v1.Delete {
-			return false, warnings, fmt.Errorf("default SriovOperatorConfig shouldn't be deleted")
+	if cr.GetName() != constants.DefaultConfigName {
+		return false, warnings, fmt.Errorf("only default SriovOperatorConfig is used")
+	}
+
+	if operation == v1.Delete {
+		return false, warnings, fmt.Errorf("default SriovOperatorConfig shouldn't be deleted")
+	}
+
+	if cr.Spec.DisableDrain {
+		warnings = append(warnings, "Node draining is disabled for applying SriovNetworkNodePolicy, it may result in workload interruption.")
+	}
+
+	err := validateSriovOperatorConfigDisableDrain(cr)
+	if err != nil {
+		return false, warnings, err
+	}
+
+	return true, warnings, nil
+}
+
+// validateSriovOperatorConfigDisableDrain checks if the user is setting `.Spec.DisableDrain` from false to true while
+// operator is updating one or more nodes. Disabling the drain at this stage would prevent the operator to uncordon a node at
+// the end of the update operation, keeping nodes un-schedulable until manual intervention.
+func validateSriovOperatorConfigDisableDrain(cr *sriovnetworkv1.SriovOperatorConfig) error {
+	if !cr.Spec.DisableDrain {
+		return nil
+	}
+
+	previousConfig, err := snclient.SriovnetworkV1().SriovOperatorConfigs(cr.Namespace).Get(context.Background(), cr.Name, metav1.GetOptions{})
+	if err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil
 		}
+		return fmt.Errorf("can't validate SriovOperatorConfig[%s] DisableDrain against its previous value: %q", cr.Name, err)
+	}
+
+	if previousConfig.Spec.DisableDrain == cr.Spec.DisableDrain {
+		// DisableDrain didn't change
+		return nil
+	}
+
+	// DisableDrain has been changed `false -> true`, check if any node is updating
+	nodeStates, err := snclient.SriovnetworkV1().SriovNetworkNodeStates(namespace).List(context.Background(), metav1.ListOptions{})
+	if err != nil {
+		return fmt.Errorf("can't validate SriovOperatorConfig[%s] DisableDrain transition to true: %q", cr.Name, err)
+	}
 
-		if cr.Spec.DisableDrain {
-			warnings = append(warnings, "Node draining is disabled for applying SriovNetworkNodePolicy, it may result in workload interruption.")
+	for _, nodeState := range nodeStates.Items {
+		if nodeState.Status.SyncStatus == "InProgress" {
+			return fmt.Errorf("can't set Spec.DisableDrain = true while node[%s] is updating", nodeState.Name)
 		}
-		return true, warnings, nil
 	}
-	return false, warnings, fmt.Errorf("only default SriovOperatorConfig is used")
+
+	return nil
 }
 
 func validateSriovNetworkNodePolicy(cr *sriovnetworkv1.SriovNetworkNodePolicy, operation v1.Operation) (bool, []string, error) {

pkg/webhook/validate_test.go

@@ -1,6 +1,7 @@
 package webhook
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"testing"
@@ -14,6 +15,8 @@ import (
 
 	. "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
 	constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
+
+	fakesnclientset "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/client/clientset/versioned/fake"
 )
 
 func TestMain(m *testing.M) {
@@ -129,11 +132,8 @@ func NewNode() *corev1.Node {
 	return &corev1.Node{Spec: corev1.NodeSpec{ProviderID: "openstack"}}
 }
 
-func TestValidateSriovOperatorConfigWithDefaultOperatorConfig(t *testing.T) {
-	var err error
-	var ok bool
-	var w []string
-	config := &SriovOperatorConfig{
+func newDefaultOperatorConfig() *SriovOperatorConfig {
+	return &SriovOperatorConfig{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "default",
 		},
@@ -145,16 +145,23 @@ func TestValidateSriovOperatorConfigWithDefaultOperatorConfig(t *testing.T) {
 			LogLevel:                 2,
 		},
 	}
+}
+
+func TestValidateSriovOperatorConfigWithDefaultOperatorConfig(t *testing.T) {
 	g := NewGomegaWithT(t)
-	ok, _, err = validateSriovOperatorConfig(config, "DELETE")
+
+	config := newDefaultOperatorConfig()
+	snclient = fakesnclientset.NewSimpleClientset()
+
+	ok, _, err := validateSriovOperatorConfig(config, "DELETE")
 	g.Expect(err).To(HaveOccurred())
 	g.Expect(ok).To(Equal(false))
 
 	ok, _, err = validateSriovOperatorConfig(config, "UPDATE")
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(ok).To(Equal(true))
 
-	ok, w, err = validateSriovOperatorConfig(config, "UPDATE")
+	ok, w, err := validateSriovOperatorConfig(config, "UPDATE")
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(ok).To(Equal(true))
 	g.Expect(w[0]).To(ContainSubstring("Node draining is disabled"))
@@ -164,6 +171,39 @@ func TestValidateSriovOperatorConfigWithDefaultOperatorConfig(t *testing.T) {
 	g.Expect(ok).To(Equal(true))
 }
 
+func TestValidateSriovOperatorConfigDisableDrain(t *testing.T) {
+	g := NewGomegaWithT(t)
+
+	config := newDefaultOperatorConfig()
+	config.Spec.DisableDrain = false
+
+	nodeState := &SriovNetworkNodeState{
+		ObjectMeta: metav1.ObjectMeta{Name: "worker-1", Namespace: namespace},
+		Status: SriovNetworkNodeStateStatus{
+			SyncStatus: "InProgress",
+		},
+	}
+
+	snclient = fakesnclientset.NewSimpleClientset(
+		config,
+		nodeState,
+	)
+
+	config.Spec.DisableDrain = true
+	ok, _, err := validateSriovOperatorConfig(config, "UPDATE")
+	g.Expect(err).To(MatchError("can't set Spec.DisableDrain = true while node[worker-1] is updating"))
+	g.Expect(ok).To(Equal(false))
+
+	// Simulate node update finished
+	nodeState.Status.SyncStatus = "Succeeded"
+	snclient.SriovnetworkV1().SriovNetworkNodeStates(namespace).
+		Update(context.Background(), nodeState, metav1.UpdateOptions{})
+
+	ok, _, err = validateSriovOperatorConfig(config, "UPDATE")
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(ok).To(Equal(true))
+}
+
 func TestValidateSriovNetworkNodePolicyWithDefaultPolicy(t *testing.T) {
 	var err error
 	var ok bool