Skip to content

OCPEDGE-1309: feat: add arbiter node check #1882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Dec 13, 2024
Merged

OCPEDGE-1309: feat: add arbiter node check #1882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions pkg/operator/staticpod/controller/guard/guard_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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")
Expand Down Expand Up @@ -101,6 +104,7 @@ func NewGuardController(
pdbLister: kubeInformersForTargetNamespace.Policy().V1().PodDisruptionBudgets().Lister(),
installerPodImageFn: getInstallerPodImageFromEnv,
createConditionalFunc: createConditionalFunc,
extraNodeSelector: extraNodeSelector,
}

return factory.New().
Expand Down Expand Up @@ -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
Expand Down
143 changes: 141 additions & 2 deletions pkg/operator/staticpod/controller/guard/guard_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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()

Expand All @@ -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 {
Expand Down
15 changes: 15 additions & 0 deletions pkg/operator/staticpod/controller/node/node_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type NodeController struct {
controllerInstanceName string
operatorClient v1helpers.StaticPodOperatorClient
nodeLister corelisterv1.NodeLister
extraNodeSelector labels.Selector
}

// NewNodeController creates a new node controller.
Expand All @@ -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(),
Expand Down Expand Up @@ -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{}
Expand Down
Loading