diff --git a/pkg/operator/staticpod/controller/guard/guard_controller.go b/pkg/operator/staticpod/controller/guard/guard_controller.go index e62a362ba7..34cc973c46 100644 --- a/pkg/operator/staticpod/controller/guard/guard_controller.go +++ b/pkg/operator/staticpod/controller/guard/guard_controller.go @@ -53,6 +53,8 @@ type GuardController struct { // installerPodImageFn returns the image name for the installer pod installerPodImageFn func() string createConditionalFunc func() (bool, bool, error) + + extraNodeSelector labels.Selector } func NewGuardController( @@ -70,6 +72,7 @@ func NewGuardController( pdbGetter policyclientv1.PodDisruptionBudgetsGetter, eventRecorder events.Recorder, createConditionalFunc func() (bool, bool, error), + extraNodeSelector labels.Selector, ) (factory.Controller, error) { if operandPodLabelSelector == nil { return nil, fmt.Errorf("GuardController: missing required operandPodLabelSelector") @@ -101,6 +104,7 @@ func NewGuardController( pdbLister: kubeInformersForTargetNamespace.Policy().V1().PodDisruptionBudgets().Lister(), installerPodImageFn: getInstallerPodImageFromEnv, createConditionalFunc: createConditionalFunc, + extraNodeSelector: extraNodeSelector, } return factory.New(). @@ -233,6 +237,17 @@ func (c *GuardController) sync(ctx context.Context, syncCtx factory.SyncContext) return err } + // Due to a design choice on ORing keys in label selectors, we run this query again to allow for additional + // selectors as well as selectors that want to OR with master nodes. + // see: https://github.com/kubernetes/kubernetes/issues/90549#issuecomment-620625847 + if c.extraNodeSelector != nil { + extraNodes, err := c.nodeLister.List(c.extraNodeSelector) + if err != nil { + return err + } + nodes = append(nodes, extraNodes...) + } + pods, err := c.podLister.Pods(c.targetNamespace).List(c.operandPodLabelSelector) if err != nil { return err diff --git a/pkg/operator/staticpod/controller/guard/guard_controller_test.go b/pkg/operator/staticpod/controller/guard/guard_controller_test.go index 19146dc32c..a8bf43e882 100644 --- a/pkg/operator/staticpod/controller/guard/guard_controller_test.go +++ b/pkg/operator/staticpod/controller/guard/guard_controller_test.go @@ -3,15 +3,17 @@ package guard import ( "context" "fmt" - clocktesting "k8s.io/utils/clock/testing" "testing" "time" + clocktesting "k8s.io/utils/clock/testing" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" @@ -218,6 +220,13 @@ func fakeMasterNode(name string) *corev1.Node { return n } +func fakeArbiterNode(name string) *corev1.Node { + n := fakeMasterNode(name) + delete(n.Labels, "node-role.kubernetes.io/master") + n.Labels["node-role.kubernetes.io/arbiter"] = "" + return n +} + type FakeSyncContext struct { recorder events.Recorder } @@ -253,6 +262,7 @@ func TestRenderGuardPod(t *testing.T) { guardExists bool guardPod *corev1.Pod createConditionalFunc func() (bool, bool, error) + withArbiter bool }{ { name: "Operand pod missing", @@ -320,6 +330,31 @@ func TestRenderGuardPod(t *testing.T) { node: fakeMasterNode("master1"), guardExists: true, }, + { + name: "Operand guard pod created on arbiter", + infraObject: &configv1.Infrastructure{ + ObjectMeta: v1.ObjectMeta{ + Name: "cluster", + }, + }, + createConditionalFunc: func() (bool, bool, error) { return true, true, nil }, + operandPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operand1", + Namespace: "test", + Labels: map[string]string{"app": "operand"}, + }, + Spec: corev1.PodSpec{ + NodeName: "arbiter", + }, + Status: corev1.PodStatus{ + PodIP: "1.1.1.1", + }, + }, + node: fakeArbiterNode("arbiter"), + guardExists: true, + withArbiter: true, + }, { name: "Master node not schedulable", infraObject: &configv1.Infrastructure{ @@ -385,6 +420,47 @@ func TestRenderGuardPod(t *testing.T) { }, node: fakeMasterNode("master1"), }, + { + name: "Operand guard pod deleted on arbiter", + infraObject: &configv1.Infrastructure{ + ObjectMeta: v1.ObjectMeta{ + Name: "cluster", + }, + Status: configv1.InfrastructureStatus{ + ControlPlaneTopology: configv1.TopologyMode("HighlyAvailableArbiter"), + }, + }, + operandPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operand1", + Namespace: "test", + Labels: map[string]string{"app": "operand"}, + }, + Spec: corev1.PodSpec{ + NodeName: "arbiter", + }, + Status: corev1.PodStatus{ + PodIP: "1.1.1.1", + }, + }, + guardPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: getGuardPodName("operand", "arbiter"), + Namespace: "test", + Labels: map[string]string{"app": "guard"}, + }, + Spec: corev1.PodSpec{ + Hostname: "guard-arbiter", + NodeName: "arbiter", + }, + Status: corev1.PodStatus{ + PodIP: "1.1.1.1", + }, + }, + //createConditionalFunc: func() (bool, bool, error) { return true, true, nil }, + node: fakeArbiterNode("arbiter"), + withArbiter: true, + }, { name: "Guard pod is not pending nor running", infraObject: &configv1.Infrastructure{ @@ -440,6 +516,60 @@ func TestRenderGuardPod(t *testing.T) { node: fakeMasterNode("master1"), guardExists: true, }, + { + name: "Guard pod is not pending nor running on arbiter", + infraObject: &configv1.Infrastructure{ + ObjectMeta: v1.ObjectMeta{ + Name: "cluster", + }, + }, + operandPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operand1", + Namespace: "test", + Labels: map[string]string{"app": "operand"}, + }, + Spec: corev1.PodSpec{ + NodeName: "arbiter", + }, + Status: corev1.PodStatus{ + PodIP: "1.1.1.1", + }, + }, + guardPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: getGuardPodName("operand", "arbiter"), + Namespace: "test", + Labels: map[string]string{"app": "guard"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Image: "", + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ + Host: "1.1.1.1", + Port: intstr.FromInt(99999), + Path: "", + }, + }, + }, + }, + }, + Hostname: getGuardPodHostname("test", "arbiter"), + NodeName: "arbiter", + }, + Status: corev1.PodStatus{ + PodIP: "1.1.1.1", + Phase: corev1.PodSucceeded, + }, + }, + createConditionalFunc: func() (bool, bool, error) { return true, true, nil }, + node: fakeArbiterNode("arbiter"), + guardExists: true, + withArbiter: true, + }, { name: "Conditional return precheckSucceeded is false", infraObject: &configv1.Infrastructure{ @@ -515,6 +645,15 @@ func TestRenderGuardPod(t *testing.T) { createConditionalFunc: createConditionalFunc, } + if test.withArbiter { + arbiterNodeRequirement, err := labels.NewRequirement("node-role.kubernetes.io/arbiter", selection.Equals, []string{""}) + if err != nil { + panic(err) + } + selector := labels.NewSelector().Add(*arbiterNodeRequirement) + ctrl.extraNodeSelector = selector + } + ctx, cancel := context.WithCancel(context.TODO()) defer cancel() @@ -528,7 +667,7 @@ func TestRenderGuardPod(t *testing.T) { } } else { if test.guardExists { - p, err := kubeClient.CoreV1().Pods("test").Get(ctx, getGuardPodName("operand", "master1"), metav1.GetOptions{}) + p, err := kubeClient.CoreV1().Pods("test").Get(ctx, getGuardPodName("operand", test.node.Name), metav1.GetOptions{}) if err != nil { t.Errorf("%s: unexpected error: %v", test.name, err) } else { diff --git a/pkg/operator/staticpod/controller/node/node_controller.go b/pkg/operator/staticpod/controller/node/node_controller.go index ef2c9c50f7..11c3d778c0 100644 --- a/pkg/operator/staticpod/controller/node/node_controller.go +++ b/pkg/operator/staticpod/controller/node/node_controller.go @@ -25,6 +25,7 @@ type NodeController struct { controllerInstanceName string operatorClient v1helpers.StaticPodOperatorClient nodeLister corelisterv1.NodeLister + extraNodeSelector labels.Selector } // NewNodeController creates a new node controller. @@ -33,12 +34,15 @@ func NewNodeController( operatorClient v1helpers.StaticPodOperatorClient, kubeInformersClusterScoped informers.SharedInformerFactory, eventRecorder events.Recorder, + extraNodeSelector labels.Selector, ) factory.Controller { c := &NodeController{ controllerInstanceName: factory.ControllerInstanceName(instanceName, "Node"), operatorClient: operatorClient, nodeLister: kubeInformersClusterScoped.Core().V1().Nodes().Lister(), + extraNodeSelector: extraNodeSelector, } + return factory.New(). WithInformers( operatorClient.Informer(), @@ -67,6 +71,17 @@ func (c *NodeController) sync(ctx context.Context, syncCtx factory.SyncContext) return err } + // Due to a design choice on ORing keys in label selectors, we run this query again to allow for additional + // selectors as well as selectors that want to OR with master nodes. + // see: https://github.com/kubernetes/kubernetes/issues/90549#issuecomment-620625847 + if c.extraNodeSelector != nil { + extraNodes, err := c.nodeLister.List(c.extraNodeSelector) + if err != nil { + return err + } + nodes = append(nodes, extraNodes...) + } + jsonPatch := jsonpatch.New() var removedNodeStatusesCounter int newTargetNodeStates := []*applyoperatorv1.NodeStatusApplyConfiguration{} diff --git a/pkg/operator/staticpod/controller/node/node_controller_test.go b/pkg/operator/staticpod/controller/node/node_controller_test.go index c3946a5f00..b42f3921b0 100644 --- a/pkg/operator/staticpod/controller/node/node_controller_test.go +++ b/pkg/operator/staticpod/controller/node/node_controller_test.go @@ -3,15 +3,17 @@ package node import ( "context" "fmt" - clocktesting "k8s.io/utils/clock/testing" "testing" "time" + clocktesting "k8s.io/utils/clock/testing" + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/selection" "k8s.io/client-go/kubernetes/fake" clientgotesting "k8s.io/client-go/testing" @@ -21,6 +23,7 @@ import ( "github.com/openshift/library-go/pkg/operator/condition" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/v1helpers" + "k8s.io/apimachinery/pkg/labels" ) func fakeMasterNode(name string) *corev1.Node { @@ -33,6 +36,13 @@ func fakeMasterNode(name string) *corev1.Node { return n } +func fakeArbiterNode(name string) *corev1.Node { + n := fakeMasterNode(name) + delete(n.Labels, "node-role.kubernetes.io/master") + n.Labels["node-role.kubernetes.io/arbiter"] = "" + return n +} + func makeNodeNotReady(node *corev1.Node) *corev1.Node { return addNodeReadyCondition(node, corev1.ConditionFalse) } @@ -92,6 +102,7 @@ func TestNodeControllerDegradedConditionType(t *testing.T) { verifyNodeStatus func([]operatorv1.OperatorCondition) error verifyJSONPatch func(*jsonpatch.PatchSet) error verifyKubeClientActions func(actions []clientgotesting.Action) error + withArbiter bool }{ { name: "scenario 1: one unhealthy master node is reported", @@ -106,6 +117,20 @@ func TestNodeControllerDegradedConditionType(t *testing.T) { return validateNodeControllerDegradedCondition(conditions, expectedCondition) }, }, + { + name: "scenario 1 with arbiter: one unhealthy arbiter node is reported", + withArbiter: true, + masterNodes: []runtime.Object{makeNodeNotReady(fakeArbiterNode("arbiter")), makeNodeReady(fakeMasterNode("test-node-2"))}, + verifyNodeStatus: func(conditions []operatorv1.OperatorCondition) error { + var expectedCondition operatorv1.OperatorCondition + expectedCondition.Type = condition.NodeControllerDegradedConditionType + expectedCondition.Reason = "MasterNodesReady" + expectedCondition.Status = operatorv1.ConditionTrue + expectedCondition.Message = `The master nodes not ready: node "arbiter" not ready since 2018-01-12 22:51:48.324359102 +0000 UTC because TestReason (test message)` + + return validateNodeControllerDegradedCondition(conditions, expectedCondition) + }, + }, { name: "scenario 2: all master nodes are healthy", @@ -121,6 +146,20 @@ func TestNodeControllerDegradedConditionType(t *testing.T) { }, }, + { + name: "scenario 2 with arbiter: all master nodes are healthy", + masterNodes: []runtime.Object{makeNodeReady(fakeMasterNode("test-node-1")), makeNodeReady(fakeMasterNode("test-node-2")), makeNodeReady(fakeArbiterNode("test-arbiter-node-0"))}, + verifyNodeStatus: func(conditions []operatorv1.OperatorCondition) error { + var expectedCondition operatorv1.OperatorCondition + expectedCondition.Type = condition.NodeControllerDegradedConditionType + expectedCondition.Reason = "MasterNodesReady" + expectedCondition.Status = operatorv1.ConditionFalse + expectedCondition.Message = "All master nodes are ready" + + return validateNodeControllerDegradedCondition(conditions, expectedCondition) + }, + }, + { name: "scenario 3: multiple master nodes are unhealthy", masterNodes: []runtime.Object{makeNodeNotReady(fakeMasterNode("test-node-1")), makeNodeReady(fakeMasterNode("test-node-2")), makeNodeNotReady(fakeMasterNode("test-node-3"))}, @@ -479,6 +518,15 @@ func TestNodeControllerDegradedConditionType(t *testing.T) { operatorClient: fakeStaticPodOperatorClient, nodeLister: fakeLister, } + + if scenario.withArbiter { + arbiterNodeRequirement, err := labels.NewRequirement("node-role.kubernetes.io/arbiter", selection.Equals, []string{""}) + if err != nil { + panic(err) + } + selector := labels.NewSelector().Add(*arbiterNodeRequirement) + c.extraNodeSelector = selector + } if err := c.sync(context.TODO(), factory.NewSyncContext("NodeController", eventRecorder)); err != nil { t.Fatal(err) } @@ -506,6 +554,7 @@ func TestNodeControllerDegradedConditionType(t *testing.T) { func TestNewNodeController(t *testing.T) { tests := []struct { name string + withArbiter bool startNodes []runtime.Object startNodeStatus []operatorv1.NodeStatus evaluateNodeStatus func([]operatorv1.NodeStatus) error @@ -544,6 +593,31 @@ func TestNewNodeController(t *testing.T) { return nil }, }, + { + name: "multi-node with arbiter", + startNodes: []runtime.Object{fakeMasterNode("test-node-1"), fakeMasterNode("test-node-2"), fakeArbiterNode("test-arbiter-node-1")}, + startNodeStatus: []operatorv1.NodeStatus{ + { + NodeName: "test-node-1", + }, + }, + withArbiter: true, + evaluateNodeStatus: func(s []operatorv1.NodeStatus) error { + if len(s) != 3 { + return fmt.Errorf("expected 3 node status, got %d", len(s)) + } + if s[0].NodeName != "test-node-1" { + return fmt.Errorf("expected first node to be test-node-1, got %q", s[0].NodeName) + } + if s[1].NodeName != "test-node-2" { + return fmt.Errorf("expected second node to be test-node-2, got %q", s[1].NodeName) + } + if s[2].NodeName != "test-arbiter-node-1" { + return fmt.Errorf("expected second node to be test-arbiter-node-1, got %q", s[1].NodeName) + } + return nil + }, + }, { name: "single-node-removed", startNodes: []runtime.Object{}, @@ -574,6 +648,25 @@ func TestNewNodeController(t *testing.T) { return nil }, }, + { + name: "no-op with arbiter", + startNodes: []runtime.Object{fakeMasterNode("test-node-1"), fakeArbiterNode("test-arbiter-node-1")}, + withArbiter: true, + startNodeStatus: []operatorv1.NodeStatus{ + { + NodeName: "test-node-1", + }, + { + NodeName: "test-arbiter-node-1", + }, + }, + evaluateNodeStatus: func(s []operatorv1.NodeStatus) error { + if len(s) != 2 { + return fmt.Errorf("expected one node status, got %d", len(s)) + } + return nil + }, + }, } for _, test := range tests { @@ -602,6 +695,16 @@ func TestNewNodeController(t *testing.T) { operatorClient: fakeStaticPodOperatorClient, nodeLister: fakeLister, } + + if test.withArbiter { + arbiterNodeRequirement, err := labels.NewRequirement("node-role.kubernetes.io/arbiter", selection.Equals, []string{""}) + if err != nil { + panic(err) + } + selector := labels.NewSelector().Add(*arbiterNodeRequirement) + c.extraNodeSelector = selector + } + // override the lister so we don't have to run the informer to list nodes c.nodeLister = fakeLister if err := c.sync(context.TODO(), factory.NewSyncContext("NodeController", eventRecorder)); err != nil { diff --git a/pkg/operator/staticpod/controllers.go b/pkg/operator/staticpod/controllers.go index 72202ab990..8d321ad3a7 100644 --- a/pkg/operator/staticpod/controllers.go +++ b/pkg/operator/staticpod/controllers.go @@ -2,9 +2,10 @@ package staticpod import ( "fmt" - "k8s.io/utils/clock" "time" + "k8s.io/utils/clock" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/client-go/config/informers/externalversions" "github.com/openshift/library-go/pkg/controller/manager" @@ -79,6 +80,8 @@ type staticPodOperatorControllerBuilder struct { guardCreateConditionalFunc func() (bool, bool, error) revisionControllerPrecondition revisioncontroller.PreconditionFunc + + extraNodeSelector labels.Selector } func NewBuilder( @@ -108,6 +111,9 @@ type Builder interface { WithMinReadyDuration(minReadyDuration time.Duration) Builder WithStartupMonitor(enabledStartupMonitor func() (bool, error)) Builder + // WithExtraNodeSelector Informs controllers to handle extra nodes as well as master nodes. + WithExtraNodeSelector(extraNodeSelector labels.Selector) Builder + // WithCustomInstaller allows mutating the installer pod definition just before // the installer pod is created for a revision. WithCustomInstaller(command []string, installerPodMutationFunc installer.InstallerPodMutationFunc) Builder @@ -124,6 +130,11 @@ type Builder interface { ToControllers() (manager.ControllerManager, error) } +func (b *staticPodOperatorControllerBuilder) WithExtraNodeSelector(extraNodeSelector labels.Selector) Builder { + b.extraNodeSelector = extraNodeSelector + return b +} + func (b *staticPodOperatorControllerBuilder) WithEvents(eventRecorder events.Recorder) Builder { b.eventRecorder = eventRecorder return b @@ -351,6 +362,7 @@ func (b *staticPodOperatorControllerBuilder) ToControllers() (manager.Controller b.staticPodOperatorClient, clusterInformers, eventRecorder, + b.extraNodeSelector, ), 1) // this cleverly sets the same condition that used to be set because of the way that the names are constructed @@ -385,6 +397,7 @@ func (b *staticPodOperatorControllerBuilder) ToControllers() (manager.Controller pdbClient, eventRecorder, b.guardCreateConditionalFunc, + b.extraNodeSelector, ); err == nil { manager.WithController(guardController, 1) } else {