api: support NodeSelector for NodeGroup

As a preparation step for supporting TAS on HCP (hosted control planes) known as Hypershift, this PR introduces a new API for selecting nodes on which the RTE will be running, exposing the nodes' topologies.

On OCP:
 The way that cluster nodes are grouped is by using Machine Config Pools (MCPs); On the operator side,
 umaresourcesoperator CR defines an MCP selector that, deep down, uses the node selector on the corresponding MCP and sets it under the RTE daemonset's node selector.

On HCP:
 The MCP term does not exist on hypershift, but instead, the hypershift platform has a node-pool, which essentially groups the nodes based on a specific node-pool label that is added under the nodes' labels.

This PR proposes to enable additional options for selecting
node groups to run the RTE pods that will work on both OCP and HCP platforms. The new API does not change how MCP selector works, so it is backward compatible, as it only adds a NodeSelector option under the NodeGroup which will be reflected in the numaresourcesoperator spec. Values for
this field are valid node labels.

Restrictions and limitations:
- Only one selector should be specified per node group; more than one will not be accepted and will cause a degraded state.
- No extra validations are applied whether nodes of one node group that specifies MCP Selector correlate with nodes from another that specifies NodeSelector. The user takes responsibility for providing nonconflicting nodes per selector group.
- Like was for MCP selectors, there is no validation on the selected nodes whether or not they have the correct machine configuration needed for TAS to be operational.

Signed-off-by: Shereen Haj <[email protected]>

api/numaresourcesoperator/v1/helper/nodegroup/nodegroup.go

@@ -45,33 +45,33 @@ func (ttr Tree) Clone() Tree {
 }
 
 func FindTrees(mcps *mcov1.MachineConfigPoolList, nodeGroups []nropv1.NodeGroup) ([]Tree, error) {
+	// node groups are validated by the controller before getting to this phase, so for sure all node groups will be valid at this point.
+	// a valid node group has either NodeSelector OR MachineConfigPoolSelector, not both
 	var result []Tree
 	for idx := range nodeGroups {
 		nodeGroup := &nodeGroups[idx] // shortcut
-
-		if nodeGroup.MachineConfigPoolSelector == nil {
-			continue
-		}
-		selector, err := metav1.LabelSelectorAsSelector(nodeGroup.MachineConfigPoolSelector)
-		if err != nil {
-			klog.Errorf("bad node group machine config pool selector %q", nodeGroup.MachineConfigPoolSelector.String())
-			continue
-		}
-
 		treeMCPs := []*mcov1.MachineConfigPool{}
-		for i := range mcps.Items {
-			mcp := &mcps.Items[i] // shortcut
-			mcpLabels := labels.Set(mcp.Labels)
-			if !selector.Matches(mcpLabels) {
+
+		if nodeGroup.MachineConfigPoolSelector != nil {
+			selector, err := metav1.LabelSelectorAsSelector(nodeGroup.MachineConfigPoolSelector)
+			if err != nil {
+				klog.Errorf("bad node group machine config pool selector %q", nodeGroup.MachineConfigPoolSelector.String())
 				continue
 			}
-			treeMCPs = append(treeMCPs, mcp)
-		}
 
-		if len(treeMCPs) == 0 {
-			return nil, fmt.Errorf("failed to find MachineConfigPool for the node group with the selector %q", nodeGroup.MachineConfigPoolSelector.String())
-		}
+			for i := range mcps.Items {
+				mcp := &mcps.Items[i] // shortcut
+				mcpLabels := labels.Set(mcp.Labels)
+				if !selector.Matches(mcpLabels) {
+					continue
+				}
+				treeMCPs = append(treeMCPs, mcp)
+			}
 
+			if len(treeMCPs) == 0 {
+				return nil, fmt.Errorf("failed to find MachineConfigPool for the node group with the selector %q", nodeGroup.MachineConfigPoolSelector.String())
+			}
+		}
 		result = append(result, Tree{
 			NodeGroup:          nodeGroup,
 			MachineConfigPools: treeMCPs,

api/numaresourcesoperator/v1/helper/nodegroup/nodegroup_test.go

@@ -87,6 +87,19 @@ func TestFindTrees(t *testing.T) {
 			name: "no-node-groups",
 			mcps: &mcpList,
 		},
+		{
+			name: "ng1-mcp not found",
+			mcps: &mcpList,
+			ngs: []nropv1.NodeGroup{
+				{
+					MachineConfigPoolSelector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"mcp-label": "notfound",
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "ng1-mcp1",
 			mcps: &mcpList,
@@ -185,11 +198,42 @@ func TestFindTrees(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "ng1- nodeSelector and MCPSelector one per nodegroup",
+			mcps: &mcpList,
+			ngs: []nropv1.NodeGroup{
+				{
+					NodeSelector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"ns-label": "ns1",
+						},
+					},
+				},
+				{
+					MachineConfigPoolSelector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"mcp-label-2a": "test2a",
+						},
+					},
+				},
+			},
+			expected: []Tree{
+				{
+					MachineConfigPools: []*mcov1.MachineConfigPool{
+						{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "mcp2",
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := FindTrees(tt.mcps, tt.ngs)
-			if err != nil {
+			if err != nil && len(tt.expected) != 0 {
 				t.Errorf("unexpected error: %v", err)
 			}
 			gotNames := mcpNamesFromTrees(got)
@@ -200,7 +244,7 @@ func TestFindTrees(t *testing.T) {
 
 			// backward compat
 			gotMcps, err := findListByNodeGroups(tt.mcps, tt.ngs)
-			if err != nil {
+			if err != nil && len(tt.expected) != 0 {
 				t.Errorf("unexpected error checking backward compat: %v", err)
 			}
 			compatNames := mcpNamesFromList(gotMcps)
@@ -209,6 +253,72 @@ func TestFindTrees(t *testing.T) {
 			}
 		})
 	}
+
+	ngName := "ng-test"
+	nodeGroupNameGenerationTestcases := []struct {
+		description    string
+		ng             nropv1.NodeGroup
+		expectedNGName string
+	}{
+		{
+			description: "node group with node selector and custom name",
+			ng: nropv1.NodeGroup{
+				NodeSelector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"ns-label": "ns1",
+					},
+				},
+				Name: &ngName,
+			},
+			expectedNGName: ngName,
+		},
+		{
+			description: "node group with MCP selector and custom name",
+			ng: nropv1.NodeGroup{
+				MachineConfigPoolSelector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"mcp-label-2a": "test2a",
+					},
+				},
+				Name: &ngName,
+			},
+			expectedNGName: ngName,
+		},
+		{
+			description: "node group with MCP selector and unset name",
+			ng: nropv1.NodeGroup{
+				MachineConfigPoolSelector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"mcp-label-2a": "test2a",
+					},
+				},
+			},
+		},
+		{
+			description: "node group with node selector and unset name",
+			ng: nropv1.NodeGroup{
+				NodeSelector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"ns-label": "ns1",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range nodeGroupNameGenerationTestcases {
+		t.Run(tt.description, func(t *testing.T) {
+			got, err := FindTrees(&mcpList, []nropv1.NodeGroup{tt.ng})
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+			if tt.expectedNGName != "" {
+				if tt.expectedNGName != *got[0].NodeGroup.Name {
+					t.Errorf("mismatch generated node group names: got=%v expected=%v", got[0].NodeGroup.Name, tt.expectedNGName)
+				}
+			}
+		})
+	}
 }
 
 func TestFindMachineConfigPools(t *testing.T) {

api/numaresourcesoperator/v1/numaresourcesoperator_types.go

@@ -118,7 +118,14 @@ type NodeGroup struct {
 	// If not provided, is determined by the system.
 	// +optional
 	Name *string `json:"name,omitempty"`
-	// MachineConfigPoolSelector defines label selector for the machine config pool
+	// NodeSelector allows to set directly the labels which nodes belonging to this nodegroup must match.
+	// This offer greater flexibility than using a MachineConfigPoolSelector, You can use either NodeSelector
+	// or machineConfigPoolSelector, not both at the same time.
+	// +optional
+	NodeSelector *metav1.LabelSelector `json:"nodeSelector,omitempty"`
+	// MachineConfigPoolSelector defines label selector for the Machine Config Pool. If used, a NodeGroup will
+	// match the same nodes this Machine Config Pool is matching.
+	// You can use either NodeSelector or machineConfigPoolSelector, not both at the same time.
 	// +optional
 	MachineConfigPoolSelector *metav1.LabelSelector `json:"machineConfigPoolSelector,omitempty"`
 	// Config defines the RTE behavior for this NodeGroup
@@ -152,9 +159,11 @@ type NodeGroupStatus struct {
 // NUMAResourcesOperatorStatus defines the observed state of NUMAResourcesOperator
 type NUMAResourcesOperatorStatus struct {
 	// DaemonSets of the configured RTEs, one per node group
+	// This field is not populated on HyperShift. Use NodeGroups instead.
 	//+operator-sdk:csv:customresourcedefinitions:type=status,displayName="RTE DaemonSets"
 	DaemonSets []NamespacedName `json:"daemonsets,omitempty"`
 	// MachineConfigPools resolved from configured node groups
+	// This field is not populated on HyperShift. Use NodeGroups instead.
 	//+operator-sdk:csv:customresourcedefinitions:type=status,displayName="RTE MCPs from node groups"
 	MachineConfigPools []MachineConfigPool `json:"machineconfigpools,omitempty"`
 	// NodeGroups report the observed status of the configured NodeGroups, matching by their name

bundle/manifests/nodetopology.openshift.io_numaresourcesoperators.yaml

@@ -138,8 +138,10 @@ spec:
                           type: array
                       type: object
                     machineConfigPoolSelector:
-                      description: MachineConfigPoolSelector defines label selector
-                        for the machine config pool
+                      description: |-
+                        MachineConfigPoolSelector defines label selector for the Machine Config Pool. If used, a NodeGroup will
+                        match the same nodes this Machine Config Pool is matching.
+                        You can use either NodeSelector or machineConfigPoolSelector, not both at the same time.
                       properties:
                         matchExpressions:
                           description: matchExpressions is a list of label selector
@@ -187,6 +189,53 @@ spec:
                         Name is the name used to identify this node group. Has to be unique among node groups.
                         If not provided, is determined by the system.
                       type: string
+                    nodeSelector:
+                      description: |-
+                        NodeSelector allows to set directly the labels which nodes belonging to this nodegroup must match.
+                        This offer greater flexibility than using a MachineConfigPoolSelector, You can use either NodeSelector
+                        or machineConfigPoolSelector, not both at the same time.
+                      properties:
+                        matchExpressions:
+                          description: matchExpressions is a list of label selector
+                            requirements. The requirements are ANDed.
+                          items:
+                            description: |-
+                              A label selector requirement is a selector that contains values, a key, and an operator that
+                              relates the key and values.
+                            properties:
+                              key:
+                                description: key is the label key that the selector
+                                  applies to.
+                                type: string
+                              operator:
+                                description: |-
+                                  operator represents a key's relationship to a set of values.
+                                  Valid operators are In, NotIn, Exists and DoesNotExist.
+                                type: string
+                              values:
+                                description: |-
+                                  values is an array of string values. If the operator is In or NotIn,
+                                  the values array must be non-empty. If the operator is Exists or DoesNotExist,
+                                  the values array must be empty. This array is replaced during a strategic
+                                  merge patch.
+                                items:
+                                  type: string
+                                type: array
+                            required:
+                            - key
+                            - operator
+                            type: object
+                          type: array
+                        matchLabels:
+                          additionalProperties:
+                            type: string
+                          description: |-
+                            matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels
+                            map is equivalent to an element of matchExpressions, whose key field is "key", the
+                            operator is "In", and the values array contains only "value". The requirements are ANDed.
+                          type: object
+                      type: object
+                      x-kubernetes-map-type: atomic
                   type: object
                 type: array
               podExcludes:
@@ -267,7 +316,9 @@ spec:
                   type: object
                 type: array
               daemonsets:
-                description: DaemonSets of the configured RTEs, one per node group
+                description: |-
+                  DaemonSets of the configured RTEs, one per node group
+                  This field is not populated on HyperShift. Use NodeGroups instead.
                 items:
                   description: |-
                     NamespacedName comprises a resource name, with a mandatory namespace,
@@ -280,7 +331,9 @@ spec:
                   type: object
                 type: array
               machineconfigpools:
-                description: MachineConfigPools resolved from configured node groups
+                description: |-
+                  MachineConfigPools resolved from configured node groups
+                  This field is not populated on HyperShift. Use NodeGroups instead.
                 items:
                   description: MachineConfigPool defines the observed state of each
                     MachineConfigPool selected by node groups

bundle/manifests/numaresources-operator.clusterserviceversion.yaml

@@ -62,7 +62,7 @@ metadata:
         }
       ]
     capabilities: Basic Install
-    createdAt: "2024-09-24T09:03:48Z"
+    createdAt: "2024-09-24T10:03:52Z"
     olm.skipRange: '>=4.17.0 <4.18.0'
     operators.operatorframework.io/builder: operator-sdk-v1.36.1
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
@@ -135,10 +135,14 @@ spec:
           Operator
         displayName: Condition reported
         path: conditions
-      - description: DaemonSets of the configured RTEs, one per node group
+      - description: |-
+          DaemonSets of the configured RTEs, one per node group
+          This field is not populated on HyperShift. Use NodeGroups instead.
         displayName: RTE DaemonSets
         path: daemonsets
-      - description: MachineConfigPools resolved from configured node groups
+      - description: |-
+          MachineConfigPools resolved from configured node groups
+          This field is not populated on HyperShift. Use NodeGroups instead.
         displayName: RTE MCPs from node groups
         path: machineconfigpools
       - description: Conditions represents the latest available observations of MachineConfigPool