diff --git a/pkg/controller/node/node_controller.go b/pkg/controller/node/node_controller.go index 2bd832be22..f8194b0ddb 100644 --- a/pkg/controller/node/node_controller.go +++ b/pkg/controller/node/node_controller.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "maps" "reflect" "sort" "time" @@ -1529,6 +1530,105 @@ func (ctrl *Controller) filterControlPlaneCandidateNodes(pool *mcfgv1.MachineCon return newCandidates, capacity, nil } +// filterCustomPoolBootedNodes adjusts the candidate list if a node that directly booted +// into a custom pool is found and updates the node with the appropriate label. +func (ctrl *Controller) filterCustomPoolBootedNodes(candidates []*corev1.Node) []*corev1.Node { + var newCandidates []*corev1.Node + for _, node := range candidates { + isCustomBootNode, poolName := ctrl.isCustomPoolBootedNode(node) + if isCustomBootNode { + if err := ctrl.applyCustomPoolLabels(node, poolName); err != nil { + // best effort, log on failure, keep in candidate list + klog.Errorf("Failed to apply custom pool labels to node %s: %v", node.Name, err) + } else { + // On a successful update of the custom pool label, remove it from the candidate list + klog.Infof("Node %s was booted on custom pool %s; dropping from candidate list", node.Name, poolName) + continue + } + } + newCandidates = append(newCandidates, node) + } + return newCandidates +} + +// isCustomPoolBootedNode checks if a node directly booted into a custom pool +// by checking if it has the FirstPivotMachineConfigAnnotation and if that +// MachineConfig belongs to a custom pool (not master/worker). +// Returns a boolean and associated custom pool name. +func (ctrl *Controller) isCustomPoolBootedNode(node *corev1.Node) (bool, string) { + + // Check if custom label has already been automatically applied, nothing to do in that case + _, customPoolApplied := node.Annotations[daemonconsts.CustomPoolLabelsAppliedAnnotationKey] + if customPoolApplied { + return false, "" + } + + // Get first pivot machineConfig, nothing to do if it doesn't exist + mcName, isFirstBoot := node.Annotations[daemonconsts.FirstPivotMachineConfigAnnotationKey] + if !isFirstBoot { + return false, "" + } + + // Get the MachineConfig to check its owner references + mc, err := ctrl.mcLister.Get(mcName) + if err != nil { + klog.V(4).Infof("Failed to get MachineConfig %s: %v", mcName, err) + return false, "" + } + + // Check if the MachineConfig has an owner reference to a MachineConfigPool + ownerRefs := mc.GetOwnerReferences() + if len(ownerRefs) == 0 { + klog.V(4).Infof("MachineConfig %s has no owner references", mcName) + return false, "" + } + + // Get the pool name from the first owner reference + poolName := ownerRefs[0].Name + + // Return true only if this is NOT a standard master or worker pool, along with poolName + return poolName != ctrlcommon.MachineConfigPoolMaster && poolName != ctrlcommon.MachineConfigPoolWorker, poolName +} + +// applyCustomPoolLabels applies the node selector labels from a custom MachineConfigPool +// to the node if the rendered MachineConfig belongs to a pool other than master/worker. +func (ctrl *Controller) applyCustomPoolLabels(node *corev1.Node, poolName string) error { + + // Get the MachineConfigPool + pool, err := ctrl.mcpLister.Get(poolName) + if err != nil { + return fmt.Errorf("failed to get MachineConfigPool %s: %w", poolName, err) + } + + // Extract labels from the pool's node selector + if pool.Spec.NodeSelector == nil || pool.Spec.NodeSelector.MatchLabels == nil { + klog.V(4).Infof("MachineConfigPool %s has no node selector labels", poolName) + return nil + } + + labelsToApply := pool.Spec.NodeSelector.MatchLabels + if len(labelsToApply) == 0 { + return nil + } + + klog.Infof("Node %s was booted into custom pool %s; applying node selector labels: %v", poolName, node.Name, labelsToApply) + + // Apply the labels to the node and add annotation indicating custom pool labels were applied + _, err = internal.UpdateNodeRetry(ctrl.kubeClient.CoreV1().Nodes(), ctrl.nodeLister, node.Name, func(node *corev1.Node) { + // Apply the custom pool labels + maps.Copy(node.Labels, labelsToApply) + + // Add annotation to signal that custom pool labels were automatically applied + node.Annotations[daemonconsts.CustomPoolLabelsAppliedAnnotationKey] = "" + }) + if err != nil { + return fmt.Errorf("failed to apply custom pool labels to node %s: %w", node.Name, err) + } + + klog.Infof("Successfully applied custom pool labels to node %s", node.Name) + return nil +} + // SetDesiredStateFromPool in old mco explains how this works. Somehow you need to NOT FAIL if the mosb doesn't exist. So // we still need to base this whole things on pools but isLayeredPool == does mosb exist // updateCandidateMachines sets the desiredConfig annotation the candidate machines @@ -1542,6 +1642,11 @@ func (ctrl *Controller) updateCandidateMachines(layered bool, mosc *mcfgv1.Machi // In practice right now these counts will be 1 but let's stay general to support 5 etcd nodes in the future ctrl.logPool(pool, "filtered to %d candidate nodes for update, capacity: %d", len(candidates), capacity) } + // Filter out any nodes that have booted into a custom pool from candidate list + candidates = ctrl.filterCustomPoolBootedNodes(candidates) + if len(candidates) == 0 { + return nil + } if capacity < uint(len(candidates)) { // when list is longer than maxUnavailable, rollout nodes in zone order, zones without zone label // are done last from oldest to youngest. this reduces likelihood of randomly picking nodes diff --git a/pkg/controller/node/node_controller_test.go b/pkg/controller/node/node_controller_test.go index fae5d35f70..bd7f217a50 100644 --- a/pkg/controller/node/node_controller_test.go +++ b/pkg/controller/node/node_controller_test.go @@ -1743,6 +1743,491 @@ func TestIsConfigAndOrBuildPresent(t *testing.T) { } } +func TestIsCustomPoolBootedNode(t *testing.T) { + t.Parallel() + + // Create test MachineConfigs with different owner references + infraMC := &mcfgv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rendered-infra-pool-abc123", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: mcfgv1.SchemeGroupVersion.String(), + Kind: "MachineConfigPool", + Name: "infra", + }, + }, + }, + } + + masterMC := &mcfgv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rendered-master-abc123", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: mcfgv1.SchemeGroupVersion.String(), + Kind: "MachineConfigPool", + Name: "master", + }, + }, + }, + } + + workerMC := &mcfgv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rendered-worker-abc123", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: mcfgv1.SchemeGroupVersion.String(), + Kind: "MachineConfigPool", + Name: "worker", + }, + }, + }, + } + + tests := []struct { + name string + node *corev1.Node + machineConfig *mcfgv1.MachineConfig + expectIsCustomPool bool + expectedPoolName string + }{ + { + name: "Node with custom pool boot annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + }, + }, + machineConfig: infraMC, + expectIsCustomPool: true, + expectedPoolName: "infra", + }, + { + name: "Node already has custom pool labels applied", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CustomPoolLabelsAppliedAnnotationKey: "", + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + }, + }, + machineConfig: infraMC, + expectIsCustomPool: false, + expectedPoolName: "", + }, + { + name: "Node without FirstPivotMachineConfig annotation", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: map[string]string{ + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + }, + }, + expectIsCustomPool: false, + expectedPoolName: "", + }, + { + name: "Node booted into master pool", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: masterMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: masterMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: masterMC.Name, + }, + }, + }, + machineConfig: masterMC, + expectIsCustomPool: false, + expectedPoolName: "master", + }, + { + name: "Node booted into worker pool", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: workerMC.Name, + }, + }, + }, + machineConfig: workerMC, + expectIsCustomPool: false, + expectedPoolName: "worker", + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + f := newFixture(t) + if test.machineConfig != nil { + f.objects = append(f.objects, test.machineConfig) + } + + c := f.newController() + + isCustomPool, poolName := c.isCustomPoolBootedNode(test.node) + + assert.Equal(t, test.expectIsCustomPool, isCustomPool, "isCustomPool mismatch") + assert.Equal(t, test.expectedPoolName, poolName, "poolName mismatch") + }) + } +} + +func TestFilterCustomPoolBootedNodes(t *testing.T) { + t.Parallel() + + // Create test MachineConfigs + infraMC := &mcfgv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rendered-infra-abc123", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: mcfgv1.SchemeGroupVersion.String(), + Kind: "MachineConfigPool", + Name: "infra", + }, + }, + }, + } + + workerMC := &mcfgv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rendered-worker-abc123", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: mcfgv1.SchemeGroupVersion.String(), + Kind: "MachineConfigPool", + Name: "worker", + }, + }, + }, + } + + tests := []struct { + name string + candidates []*corev1.Node + machineConfigs []*mcfgv1.MachineConfig + pools []*mcfgv1.MachineConfigPool + expectedCandidates []string + expectedNodeLabels map[string]map[string]string // nodeName -> labels + }{ + { + name: "Node booted into custom pool should be filtered out from candidates and labeled", + candidates: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "infra-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + }, + machineConfigs: []*mcfgv1.MachineConfig{infraMC}, + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPoolBuilder("infra"). + WithNodeSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{ + "node-role.kubernetes.io/infra": "", + }, + }). + MachineConfigPool(), + }, + expectedCandidates: []string{}, // Node should be filtered out + expectedNodeLabels: map[string]map[string]string{ + "infra-node": { + "node-role.kubernetes.io/worker": "", + "node-role.kubernetes.io/infra": "", + }, + }, + }, + { + name: "Node booted into worker pool stays in candidates", + candidates: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "worker-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: workerMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + }, + machineConfigs: []*mcfgv1.MachineConfig{workerMC}, + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPoolBuilder("worker"). + WithNodeSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }). + MachineConfigPool(), + }, + expectedCandidates: []string{"worker-node"}, // Node should remain + expectedNodeLabels: map[string]map[string]string{ + "worker-node": { + "node-role.kubernetes.io/worker": "", + }, + }, + }, + { + name: "Node with custom pool labels already applied stays in candidates", + candidates: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "already-labeled-infra-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CustomPoolLabelsAppliedAnnotationKey: "", + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + "node-role.kubernetes.io/infra": "", + }, + }, + }, + }, + machineConfigs: []*mcfgv1.MachineConfig{infraMC}, + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPoolBuilder("custom"). + WithNodeSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{ + "node-role.kubernetes.io/infra": "", + }, + }). + MachineConfigPool(), + }, + expectedCandidates: []string{"already-labeled-infra-node"}, // Node should remain + expectedNodeLabels: map[string]map[string]string{ + "already-labeled-infra-node": { + "node-role.kubernetes.io/worker": "", + "node-role.kubernetes.io/infra": "", + }, + }, + }, + { + name: "Node that booted into a custom pool should stay in candidate list if custom label was removed", + candidates: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "removed-labeled-infra-node", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CustomPoolLabelsAppliedAnnotationKey: "", + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + }, + machineConfigs: []*mcfgv1.MachineConfig{infraMC}, + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPoolBuilder("custom"). + WithNodeSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{ + "node-role.kubernetes.io/infra": "", + }, + }). + MachineConfigPool(), + }, + expectedCandidates: []string{"removed-labeled-infra-node"}, // Node should remain + expectedNodeLabels: map[string]map[string]string{ + "removed-labeled-infra-node": { + "node-role.kubernetes.io/worker": "", + }, + }, + }, + { + name: "Mix of infra and worker pool nodes", + candidates: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "infra-node-1", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "worker-node-1", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: workerMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "infra-node-2", + Annotations: map[string]string{ + daemonconsts.FirstPivotMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.CurrentMachineConfigAnnotationKey: infraMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: infraMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + }, + machineConfigs: []*mcfgv1.MachineConfig{infraMC, workerMC}, + pools: []*mcfgv1.MachineConfigPool{ + helpers.NewMachineConfigPoolBuilder("infra"). + WithNodeSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{ + "node-role.kubernetes.io/infra": "", + }, + }). + MachineConfigPool(), + helpers.NewMachineConfigPoolBuilder("worker"). + WithNodeSelector(helpers.WorkerSelector). + MachineConfigPool(), + }, + expectedCandidates: []string{"worker-node-1"}, // Only worker node remains + expectedNodeLabels: map[string]map[string]string{ + "infra-node-1": { + "node-role.kubernetes.io/worker": "", + "node-role.kubernetes.io/infra": "", + }, + "worker-node-1": { + "node-role.kubernetes.io/worker": "", + }, + "infra-node-2": { + "node-role.kubernetes.io/worker": "", + "node-role.kubernetes.io/infra": "", + }, + }, + }, + { + name: "Node without FirstPivot annotation stays in candidates(legacy scenario)", + candidates: []*corev1.Node{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "normal-node", + Annotations: map[string]string{ + daemonconsts.CurrentMachineConfigAnnotationKey: workerMC.Name, + daemonconsts.DesiredMachineConfigAnnotationKey: workerMC.Name, + }, + Labels: map[string]string{ + "node-role.kubernetes.io/worker": "", + }, + }, + }, + }, + machineConfigs: []*mcfgv1.MachineConfig{workerMC}, + pools: []*mcfgv1.MachineConfigPool{helpers.NewMachineConfigPoolBuilder("worker"). + WithNodeSelector(helpers.WorkerSelector). + MachineConfigPool()}, + expectedCandidates: []string{"normal-node"}, + expectedNodeLabels: map[string]map[string]string{ + "normal-node": { + "node-role.kubernetes.io/worker": "", + }, + }, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + f := newFixture(t) + + for _, mc := range test.machineConfigs { + f.objects = append(f.objects, mc) + } + + for _, pool := range test.pools { + f.mcpLister = append(f.mcpLister, pool) + f.objects = append(f.objects, pool) + } + + for _, node := range test.candidates { + f.nodeLister = append(f.nodeLister, node) + f.kubeobjects = append(f.kubeobjects, node) + } + + c := f.newController() + + remainingCandidates := c.filterCustomPoolBootedNodes(test.candidates) + + var remainingNames []string + for _, node := range remainingCandidates { + remainingNames = append(remainingNames, node.Name) + } + + // Compare lengths and contents instead of exact slice equality + // to handle nil slice vs empty slice differences + if len(test.expectedCandidates) == 0 { + assert.Empty(t, remainingNames, "Expected no remaining candidates") + } else { + assert.Equal(t, test.expectedCandidates, remainingNames, + "Remaining candidate nodes mismatch") + } + + // Verify node labels after the filter function + if test.expectedNodeLabels != nil { + for nodeName, expectedLabels := range test.expectedNodeLabels { + node, err := f.kubeclient.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) + require.NoError(t, err, "Failed to get node %s", nodeName) + + for labelKey, labelValue := range expectedLabels { + assert.Contains(t, node.Labels, labelKey, "Node %s should have label %s", nodeName, labelKey) + assert.Equal(t, labelValue, node.Labels[labelKey], "Node %s label %s has wrong value", nodeName, labelKey) + } + } + } + }) + } +} + // adds annotation to the node func addNodeAnnotations(node *corev1.Node, annotations map[string]string) { if node.Annotations == nil { diff --git a/pkg/daemon/constants/constants.go b/pkg/daemon/constants/constants.go index f1418921b7..fbfe1b5e01 100644 --- a/pkg/daemon/constants/constants.go +++ b/pkg/daemon/constants/constants.go @@ -17,6 +17,10 @@ const ( CurrentMachineConfigAnnotationKey = "machineconfiguration.openshift.io/currentConfig" // DesiredMachineConfigAnnotationKey is used to specify the desired MachineConfig for a machine DesiredMachineConfigAnnotationKey = "machineconfiguration.openshift.io/desiredConfig" + // FirstPivotMachineConfigAnnotationKey is used to specify the MachineConfig the node pivoted to after firstboot. + FirstPivotMachineConfigAnnotationKey = "machineconfiguration.openshift.io/firstPivotConfig" + // CustomPoolLabelsAppliedAnnotationKey is set by the node controller to indicate custom pool labels were automatically applied + CustomPoolLabelsAppliedAnnotationKey = "machineconfiguration.openshift.io/customPoolLabelsApplied" // MachineConfigDaemonStateAnnotationKey is used to fetch the state of the daemon on the machine. MachineConfigDaemonStateAnnotationKey = "machineconfiguration.openshift.io/state" // DesiredDrainerAnnotationKey is set by the MCD to indicate drain/uncordon requests diff --git a/pkg/server/server.go b/pkg/server/server.go index 67dcc923dc..4c45e4f76d 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -220,6 +220,7 @@ func getNodeAnnotation(conf, image string, mc *mcfgv1.MachineConfig) (string, er nodeAnnotations := map[string]string{ daemonconsts.CurrentMachineConfigAnnotationKey: conf, daemonconsts.DesiredMachineConfigAnnotationKey: conf, + daemonconsts.FirstPivotMachineConfigAnnotationKey: conf, daemonconsts.MachineConfigDaemonStateAnnotationKey: daemonconsts.MachineConfigDaemonStateDone, } diff --git a/test/extended-priv/machineset.go b/test/extended-priv/machineset.go index 3fc4c781fa..55629add53 100644 --- a/test/extended-priv/machineset.go +++ b/test/extended-priv/machineset.go @@ -2,7 +2,9 @@ package extended import ( "fmt" + "time" + o "github.com/onsi/gomega" exutil "github.com/openshift/machine-config-operator/test/extended-priv/util" logger "github.com/openshift/machine-config-operator/test/extended-priv/util/logext" "github.com/tidwall/gjson" @@ -44,6 +46,57 @@ func (msl *MachineSetList) GetAll() ([]*MachineSet, error) { return allMachineSets, nil } +// GetMachines returns a slice with the machines created for this MachineSet +func (ms MachineSet) GetMachines() ([]*Machine, error) { + ml := NewMachineList(ms.oc, ms.GetNamespace()) + ml.ByLabel("machine.openshift.io/cluster-api-machineset=" + ms.GetName()) + ml.SortByTimestamp() + return ml.GetAll() +} + +// WaitForRunningMachines waits for the specified number of running machines to be created from this MachineSet +// Returns the running machines on success +func (ms MachineSet) WaitForRunningMachines(expectedReplicas int, timeout, pollInterval time.Duration) []Machine { + var runningMachines []Machine + + o.Eventually(func() bool { + // Get all machines from the machineset + machines, err := ms.GetMachines() + if err != nil { + logger.Infof("Failed to get machines for machineset %s: %v", ms.GetName(), err) + return false + } + + if len(machines) == 0 { + logger.Infof("No machines found yet for machineset %s", ms.GetName()) + return false + } + + // Check how many machines are running + runningMachines = []Machine{} + for _, machine := range machines { + isRunning, err := machine.IsRunning() + if err != nil { + logger.Infof("Failed to get phase for machine %s: %v", machine.GetName(), err) + continue + } + if isRunning { + runningMachines = append(runningMachines, *machine) + } + } + + if len(runningMachines) >= expectedReplicas { + logger.Infof("MachineSet %s has %d/%d running machines", ms.GetName(), len(runningMachines), expectedReplicas) + return true + } + + logger.Infof("MachineSet %s has %d/%d running machines, waiting...", ms.GetName(), len(runningMachines), expectedReplicas) + return false + }, timeout, pollInterval).Should(o.BeTrue(), "Timed out waiting for %d running machines from MachineSet %s", expectedReplicas, ms.GetName()) + + return runningMachines +} + // convertUserDataToNewVersion converts the provided userData ignition config into the provided version format // //nolint:unparam // newIgnitionVersion is always "2.2.0", but kept for flexibility diff --git a/test/extended-priv/mco_controlplanemachineset.go b/test/extended-priv/mco_controlplanemachineset.go index b522fbc69b..fcd6072f3d 100644 --- a/test/extended-priv/mco_controlplanemachineset.go +++ b/test/extended-priv/mco_controlplanemachineset.go @@ -24,7 +24,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/disruptive // Skip if single node SkipIfSNO(oc.AsAdmin()) // Skip if no machineset - skipTestIfWorkersCannotBeScaled(oc.AsAdmin()) + SkipTestIfWorkersCannotBeScaled(oc.AsAdmin()) // ControlPlaneMachineSet Bootimages Update functionality is only available in GCP, AWS, and Azure (Tech Preview) skipTestIfSupportedPlatformNotMatched(oc, GCPPlatform, AWSPlatform, AzurePlatform) // Skip if ManagedBootImagesCPMS feature gate is not enabled diff --git a/test/extended-priv/util.go b/test/extended-priv/util.go index e2a6318bd3..d5698a01ac 100644 --- a/test/extended-priv/util.go +++ b/test/extended-priv/util.go @@ -559,8 +559,8 @@ func WorkersCanBeScaled(oc *exutil.CLI) (bool, error) { return true, nil } -// skipTestIfWorkersCannotBeScaled skips the current test if the worker pool cannot be scaled via machineset -func skipTestIfWorkersCannotBeScaled(oc *exutil.CLI) { +// SkipTestIfWorkersCannotBeScaled skips the current test if the worker pool cannot be scaled via machineset +func SkipTestIfWorkersCannotBeScaled(oc *exutil.CLI) { canBeScaled, err := WorkersCanBeScaled(oc) o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred(), "Error deciding if worker nodes can be scaled using machinesets") diff --git a/test/extended/custom_pool_booting.go b/test/extended/custom_pool_booting.go new file mode 100644 index 0000000000..248bf96a0a --- /dev/null +++ b/test/extended/custom_pool_booting.go @@ -0,0 +1,245 @@ +package extended + +import ( + "context" + "fmt" + "time" + + g "github.com/onsi/ginkgo/v2" + o "github.com/onsi/gomega" + machinev1beta1 "github.com/openshift/api/machine/v1beta1" + machineclient "github.com/openshift/client-go/machine/clientset/versioned" + machineconfigclient "github.com/openshift/client-go/machineconfiguration/clientset/versioned" + daemonconsts "github.com/openshift/machine-config-operator/pkg/daemon/constants" + extpriv "github.com/openshift/machine-config-operator/test/extended-priv" + exutil "github.com/openshift/machine-config-operator/test/extended-priv/util" + logger "github.com/openshift/machine-config-operator/test/extended-priv/util/logext" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/disruptive][Serial][Disruptive]", func() { + defer g.GinkgoRecover() + + var ( + oc = exutil.NewCLI("custom-pool-booting", exutil.KubeConfigPath()).AsAdmin() + customMCPName = "infra" + clonedMachineSet *machinev1beta1.MachineSet + newNodeName string + machineClient *machineclient.Clientset + machineConfigClient *machineconfigclient.Clientset + err error + ) + + g.BeforeEach(func() { + // Skip if worker scale-ups aren't possible + extpriv.SkipTestIfWorkersCannotBeScaled(oc) + + // Skip on single-node topologies + skipOnSingleNodeTopology(oc) + + machineClient, err = machineclient.NewForConfig(oc.KubeFramework().ClientConfig()) + o.Expect(err).NotTo(o.HaveOccurred()) + + machineConfigClient, err = machineconfigclient.NewForConfig(oc.KubeFramework().ClientConfig()) + o.Expect(err).NotTo(o.HaveOccurred()) + }) + + g.AfterEach(func(ctx context.Context) { + exutil.By("Performing cleanup") + logger.Infof("Cleaning up custom MCP %s", customMCPName) + // Cleanup: Delete custom MCP if it exists + deleteMCPErr := extpriv.DeleteCustomMCP(oc, customMCPName) + if deleteMCPErr != nil { + logger.Infof("Failed to delete MCP %s: %v", customMCPName, deleteMCPErr) + } else { + logger.Infof("Successfully deleted MCP %s", customMCPName) + } + + // Cleanup: Scale down and delete the cloned machineset if it exists + clonedMachineSet, err := machineClient.MachineV1beta1().MachineSets(MAPINamespace).Get(ctx, clonedMachineSet.Name, metav1.GetOptions{}) + if err != nil { + logger.Infof("Failed to get machineset: %v", err) + return + } + + logger.Infof("Scaling down machineset %s to 0", clonedMachineSet.Name) + clonedMachineSet.Spec.Replicas = new(int32) + *clonedMachineSet.Spec.Replicas = 0 + _, err = machineClient.MachineV1beta1().MachineSets(MAPINamespace).Update(ctx, clonedMachineSet, metav1.UpdateOptions{}) + if err != nil { + logger.Infof("Failed to scale down machineset: %v", err) + } + + // Wait for the machine to be deleted + o.Eventually(func() bool { + machines, err := machineClient.MachineV1beta1().Machines(MAPINamespace).List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("machine.openshift.io/cluster-api-machineset=%s", clonedMachineSet.Name), + }) + if err != nil { + logger.Infof("Failed to list machines: %v", err) + return false + } + return len(machines.Items) == 0 + }, 5*time.Minute, 10*time.Second).Should(o.BeTrue(), "Timed out waiting for machine to be deleted") + + // Delete the machineset + logger.Infof("Deleting machineset %s", clonedMachineSet.Name) + err = machineClient.MachineV1beta1().MachineSets(MAPINamespace).Delete(ctx, clonedMachineSet.Name, metav1.DeleteOptions{}) + if err != nil { + logger.Infof("Failed to delete machineset: %v", err) + } + }) + + // 1. Create a custom MCP named infra + // 2. Clone an existing machineset, edit the user-data-secret field to point to infra-user-data-managed + // 3. Scale up a new node from this machineset + // 4. Verify labels and annotations and ensure custom pool count after that. + // 5. Verify that the node moves back to worker pool if the label is removed after this point. + // 6. Clean up test resources + g.It("Node booted into custom pool should have appropriate labels and not moved back to worker pool [apigroup:machineconfiguration.openshift.io]", func(ctx context.Context) { + exutil.By("Create custom `infra` MCP and add the test node to it") + // Create MCP + _, err := extpriv.CreateCustomMCP(oc.AsAdmin(), customMCPName, 0) + o.Expect(err).NotTo(o.HaveOccurred(), "Error creating a new custom pool `%s`: %s", customMCPName, err) + + exutil.By("Cloning an existing machineset, edit the user-data-secret field to point to infra-user-data-managed") + + // Get a random machineset to clone + originalMachineSet := getRandomMachineSet(machineClient) + + // Create a clone without modifying user-data-secret yet + clonedMachineSet = originalMachineSet.DeepCopy() + clonedMachineSet.Name = fmt.Sprintf("%s-custom-pool-test", customMCPName) + clonedMachineSet.ResourceVersion = "" + clonedMachineSet.UID = "" + clonedMachineSet.Spec.Replicas = new(int32) + *clonedMachineSet.Spec.Replicas = 0 // Start with 0 replicas + clonedMachineSet.Spec.Selector.MatchLabels["machine.openshift.io/cluster-api-machineset"] = clonedMachineSet.Name + clonedMachineSet.Spec.Template.Labels["machine.openshift.io/cluster-api-machineset"] = clonedMachineSet.Name + + // Create the cloned machineset + clonedMachineSet, err = machineClient.MachineV1beta1().MachineSets(MAPINamespace).Create(ctx, clonedMachineSet, metav1.CreateOptions{}) + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to create cloned machineset") + logger.Infof("Successfully created cloned machineset %s", clonedMachineSet.Name) + + // Use JSON patch to modify the user-data-secret field to point to infra-user-data-managed + userDataSecretName := fmt.Sprintf("%s-user-data-managed", customMCPName) + jsonPatch := fmt.Sprintf(`[{"op": "replace", "path": "/spec/template/spec/providerSpec/value/userDataSecret/name", "value": "%s"}]`, userDataSecretName) + err = oc.AsAdmin().Run("patch").Args(MAPIMachinesetQualifiedName, clonedMachineSet.Name, "-p", jsonPatch, "-n", MAPINamespace, "--type=json").Execute() + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to patch user-data-secret in machineset") + logger.Infof("Successfully patched machineset %s to use user-data-secret %s", clonedMachineSet.Name, userDataSecretName) + + exutil.By("Scale up a new node from cloned machineset") + // Scale up the machineset to 1 replica + logger.Infof("Scaling up machineset %s to 1 replica", clonedMachineSet.Name) + err = oc.AsAdmin().Run("scale").Args(MAPIMachinesetQualifiedName, clonedMachineSet.Name, "--replicas=1", "-n", MAPINamespace).Execute() + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to scale up machineset") + logger.Infof("Successfully scaled up machineset %s", clonedMachineSet.Name) + + logger.Infof("Waiting for new machine to be running from machineset %s", clonedMachineSet.Name) + machineSetHelper := extpriv.NewMachineSet(oc.AsAdmin(), MAPINamespace, clonedMachineSet.Name) + runningMachines := machineSetHelper.WaitForRunningMachines(1, 15*time.Minute, 30*time.Second) + o.Expect(runningMachines).To(o.HaveLen(1), "Expected 1 running machine from machineset %s", clonedMachineSet.Name) + + // Grab node name from the new machine object + machine := runningMachines[0] + newNodeName = machine.GetNodeOrFail().GetName() + logger.Infof("Machine %s is running with node %s", machine.GetName(), newNodeName) + + exutil.By("Verifying node labels and annotations") + // Wait for the node to have the FirstPivot annotation + o.Eventually(func() bool { + node, err := oc.AsAdmin().KubeClient().CoreV1().Nodes().Get(ctx, newNodeName, metav1.GetOptions{}) + if err != nil { + return false + } + + // Check if FirstPivot annotation exists + if _, exists := node.Annotations[daemonconsts.FirstPivotMachineConfigAnnotationKey]; exists { + return true + } + + logger.Infof("Node %s does not have FirstPivot annotation yet", newNodeName) + return false + }, 5*time.Minute, 10*time.Second).Should(o.BeTrue(), "Timed out waiting for FirstPivot annotation") + + // Verify the node has the custom pool label + o.Eventually(func() bool { + node, err := oc.AsAdmin().KubeClient().CoreV1().Nodes().Get(ctx, newNodeName, metav1.GetOptions{}) + if err != nil { + return false + } + + // Check for custom pool label + expectedLabel := fmt.Sprintf("node-role.kubernetes.io/%s", customMCPName) + if _, exists := node.Labels[expectedLabel]; !exists { + logger.Infof("Node %s does not have custom pool label %s yet", newNodeName, expectedLabel) + return false + } + + // Verify CustomPoolLabelsApplied annotation + if _, exists := node.Annotations[daemonconsts.CustomPoolLabelsAppliedAnnotationKey]; !exists { + logger.Infof("Node %s does not have CustomPoolLabelsApplied annotation yet", newNodeName) + return false + } + + logger.Infof("Node %s has custom pool label %s and CustomPoolLabelsApplied annotation", newNodeName, expectedLabel) + return true + }, 5*time.Minute, 10*time.Second).Should(o.BeTrue(), "Node should have custom pool labels and annotation") + + // Verify the custom MCP has 1 machine + err = extpriv.NewMachineConfigPool(oc, customMCPName).WaitForMachineCount(1, 2*time.Minute) + o.Expect(err).NotTo(o.HaveOccurred(), "Custom MCP should have 1 machine") + + exutil.By("Removing custom pool label and verifying node leaves the custom pool") + + // Get the worker pool count before removing the label + workerMCP, err := machineConfigClient.MachineconfigurationV1().MachineConfigPools().Get(ctx, "worker", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get worker MCP") + initialWorkerCount := workerMCP.Status.MachineCount + logger.Infof("Worker pool machine count before label removal: %d", initialWorkerCount) + + expectedLabel := fmt.Sprintf("node-role.kubernetes.io/%s", customMCPName) + logger.Infof("Removing label %s from node %s", expectedLabel, newNodeName) + + unlabelErr := oc.AsAdmin().Run("label").Args(fmt.Sprintf("node/%s", newNodeName), fmt.Sprintf("%s-", expectedLabel)).Execute() + o.Expect(unlabelErr).NotTo(o.HaveOccurred(), "Failed to remove custom pool label from node") + + // Wait a bit and verify the node does NOT get the label automatically re-applied + time.Sleep(30 * time.Second) + + node, err := oc.AsAdmin().KubeClient().CoreV1().Nodes().Get(ctx, newNodeName, metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get node") + + // The label should NOT be present (node should not be moved back automatically) + _, labelExists := node.Labels[expectedLabel] + o.Expect(labelExists).To(o.BeFalse(), "Custom pool label should not be automatically re-applied after removal") + + // Verify that the node is in the worker pool now and custom pool count is 0 + logger.Infof("Verifying that node %s was removed from the custom pool", newNodeName) + + // Verify the custom MCP count is 0 + customMCPHelper := extpriv.NewMachineConfigPool(oc, customMCPName) + err = customMCPHelper.WaitForMachineCount(0, 5*time.Minute) + o.Expect(err).NotTo(o.HaveOccurred(), "Custom MCP should have 0 machines after label removal") + + // Verify that the custom MCP is ready after the node left + logger.Infof("Verifying that custom MCP %s is ready after node left", customMCPName) + err = customMCPHelper.WaitForUpdatedStatus() + o.Expect(err).NotTo(o.HaveOccurred(), "Custom MCP should be ready after node left the pool") + + // Verify that the worker pool went up by 1 after move + logger.Infof("Verifying that worker pool machine count increased by 1") + workerMCPHelper := extpriv.NewMachineConfigPool(oc, "worker") + err = workerMCPHelper.WaitForMachineCount(int(initialWorkerCount)+1, 5*time.Minute) + o.Expect(err).NotTo(o.HaveOccurred(), "Worker pool should have increased by 1 machine after node moved from custom pool") + + // Verify that the worker pool is ready after accepting the node + logger.Infof("Verifying that worker pool is ready after accepting the node") + err = workerMCPHelper.WaitForUpdatedStatus() + o.Expect(err).NotTo(o.HaveOccurred(), "Worker pool should be ready after accepting the node from custom pool") + + logger.Infof("Verified that node %s is now in the worker pool with custom pool count at 0", newNodeName) + }) +})