diff --git a/pkg/validation/validation.go b/pkg/validation/validation.go index b62e3ea59..0e8eca1e5 100644 --- a/pkg/validation/validation.go +++ b/pkg/validation/validation.go @@ -20,11 +20,6 @@ import ( "errors" "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "github.com/k8stopologyawareschedwg/deployer/pkg/deployer/platform" - - nropv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1" nodegroupv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1/helper/nodegroup" "github.com/openshift-kni/numaresources-operator/internal/api/annotations" ) @@ -48,139 +43,6 @@ func MachineConfigPoolDuplicates(trees []nodegroupv1.Tree) error { return duplicateErrors } -// NodeGroups validates the node groups for nil values and duplicates. -func NodeGroups(nodeGroups []nropv1.NodeGroup, platf platform.Platform) error { - if platf == platform.HyperShift { - if err := nodeGroupForHypershift(nodeGroups); err != nil { - return err - } - } - - if err := nodeGroupPools(nodeGroups); err != nil { - return err - } - - if err := nodeGroupsDuplicatesByMCPSelector(nodeGroups); err != nil { - return err - } - - if err := nodeGroupsValidPoolName(nodeGroups); err != nil { - return err - } - - if err := nodeGroupsDuplicatesByPoolName(nodeGroups); err != nil { - return err - } - - if err := nodeGroupMachineConfigPoolSelector(nodeGroups); err != nil { - return err - } - - return nil -} - -func nodeGroupForHypershift(nodeGroups []nropv1.NodeGroup) error { - for idx, nodeGroup := range nodeGroups { - if nodeGroup.MachineConfigPoolSelector != nil { - return fmt.Errorf("node group %d specifies MachineConfigPoolSelector on Hypershift platform; Should specify PoolName only", idx) - } - if nodeGroup.PoolName == nil { - return fmt.Errorf("node group %d must specify PoolName on Hypershift platform", idx) - } - } - return nil -} - -func nodeGroupPools(nodeGroups []nropv1.NodeGroup) error { - for idx, nodeGroup := range nodeGroups { - if nodeGroup.MachineConfigPoolSelector == nil && nodeGroup.PoolName == nil { - return fmt.Errorf("node group %d missing any pool specifier", idx) - } - if nodeGroup.MachineConfigPoolSelector != nil && nodeGroup.PoolName != nil { - return fmt.Errorf("node group %d must have only a single specifier set: either PoolName or MachineConfigPoolSelector", idx) - } - } - - return nil -} - -func nodeGroupsValidPoolName(nodeGroups []nropv1.NodeGroup) error { - var err error - for idx, nodeGroup := range nodeGroups { - if nodeGroup.PoolName == nil { - continue - } - if *nodeGroup.PoolName != "" { - continue - } - err = errors.Join(err, fmt.Errorf("pool name for pool #%d cannot be empty", idx)) - } - return err -} - -func nodeGroupsDuplicatesByMCPSelector(nodeGroups []nropv1.NodeGroup) error { - duplicates := map[string]int{} - for _, nodeGroup := range nodeGroups { - if nodeGroup.MachineConfigPoolSelector == nil { - continue - } - - key := nodeGroup.MachineConfigPoolSelector.String() - if _, ok := duplicates[key]; !ok { - duplicates[key] = 0 - } - duplicates[key] += 1 - } - - var duplicateErrors error - for selector, count := range duplicates { - if count > 1 { - duplicateErrors = errors.Join(duplicateErrors, fmt.Errorf("the node group with the machineConfigPoolSelector %q has duplicates", selector)) - } - } - - return duplicateErrors -} - -func nodeGroupsDuplicatesByPoolName(nodeGroups []nropv1.NodeGroup) error { - duplicates := map[string]int{} - for _, nodeGroup := range nodeGroups { - if nodeGroup.PoolName == nil { - continue - } - - key := *nodeGroup.PoolName - if _, ok := duplicates[key]; !ok { - duplicates[key] = 0 - } - duplicates[key] += 1 - } - - var duplicateErrors error - for name, count := range duplicates { - if count > 1 { - duplicateErrors = errors.Join(duplicateErrors, fmt.Errorf("the pool name %q has duplicates", name)) - } - } - - return duplicateErrors -} - -func nodeGroupMachineConfigPoolSelector(nodeGroups []nropv1.NodeGroup) error { - var selectorsErrors error - for _, nodeGroup := range nodeGroups { - if nodeGroup.MachineConfigPoolSelector == nil { - continue - } - - if _, err := metav1.LabelSelectorAsSelector(nodeGroup.MachineConfigPoolSelector); err != nil { - selectorsErrors = errors.Join(selectorsErrors, err) - } - } - - return selectorsErrors -} - func MultipleMCPsPerTree(annot map[string]string, trees []nodegroupv1.Tree) error { multiMCPsPerTree := annotations.IsMultiplePoolsPerTreeEnabled(annot) if multiMCPsPerTree { diff --git a/pkg/validation/validation_test.go b/pkg/validation/validation_test.go index d699aa388..8a1c9babc 100644 --- a/pkg/validation/validation_test.go +++ b/pkg/validation/validation_test.go @@ -20,12 +20,8 @@ import ( "strings" "testing" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "github.com/k8stopologyawareschedwg/deployer/pkg/deployer/platform" machineconfigv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" - nropv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1" nodegroupv1 "github.com/openshift-kni/numaresources-operator/api/numaresourcesoperator/v1/helper/nodegroup" "github.com/openshift-kni/numaresources-operator/internal/api/annotations" testobjs "github.com/openshift-kni/numaresources-operator/internal/objects" @@ -84,190 +80,6 @@ func TestMachineConfigPoolDuplicates(t *testing.T) { } } -func TestNodeGroupsSanity(t *testing.T) { - type testCase struct { - name string - nodeGroups []nropv1.NodeGroup - expectedError bool - expectedErrorMessage string - platf platform.Platform - } - - emptyString := "" - poolName := "poolname-test" - config := nropv1.DefaultNodeGroupConfig() - - testCases := []testCase{ - { - name: "both source pools are nil", - nodeGroups: []nropv1.NodeGroup{ - { - MachineConfigPoolSelector: nil, - PoolName: nil, - }, - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test": "test", - }, - }, - }, - }, - expectedError: true, - expectedErrorMessage: "missing any pool specifier", - platf: platform.OpenShift, - }, - { - name: "both source pools are set", - nodeGroups: []nropv1.NodeGroup{ - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test": "test", - }, - }, - PoolName: &poolName, - }, - }, - expectedError: true, - expectedErrorMessage: "must have only a single specifier set", - platf: platform.OpenShift, - }, - { - name: "with duplicates - mcp selector", - nodeGroups: []nropv1.NodeGroup{ - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test": "test", - }, - }, - }, - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test": "test", - }, - }, - }, - }, - expectedError: true, - expectedErrorMessage: "has duplicates", - platf: platform.OpenShift, - }, - { - name: "with duplicates - pool name", - nodeGroups: []nropv1.NodeGroup{ - { - PoolName: &poolName, - }, - { - PoolName: &poolName, - }, - }, - expectedError: true, - expectedErrorMessage: "has duplicates", - platf: platform.OpenShift, - }, - { - name: "bad MCP selector", - nodeGroups: []nropv1.NodeGroup{ - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "test", - Operator: "bad-operator", - Values: []string{"test"}, - }, - }, - }, - }, - }, - expectedError: true, - expectedErrorMessage: "not a valid label selector operator", - platf: platform.OpenShift, - }, - { - name: "correct values", - nodeGroups: []nropv1.NodeGroup{ - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test": "test", - }, - }, - }, - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test1": "test1", - }, - }, - }, - { - PoolName: &poolName, - }, - }, - platf: platform.OpenShift, - }, - { - name: "MCP selector set on Hypershift platform", - nodeGroups: []nropv1.NodeGroup{ - { - MachineConfigPoolSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "test": "test", - }, - }, - PoolName: &poolName, - }, - }, - expectedError: true, - expectedErrorMessage: "MachineConfigPoolSelector on Hypershift platform", - platf: platform.HyperShift, - }, - { - name: "empty PoolName on Hypershift platform", - nodeGroups: []nropv1.NodeGroup{ - { - Config: &config, - }, - }, - expectedError: true, - expectedErrorMessage: "must specify PoolName on Hypershift platform", - platf: platform.HyperShift, - }, - { - name: "empty pool name", - nodeGroups: []nropv1.NodeGroup{ - { - PoolName: &emptyString, - }, - }, - expectedError: true, - expectedErrorMessage: "cannot be empty", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - err := NodeGroups(tc.nodeGroups, tc.platf) - if err == nil && tc.expectedError { - t.Errorf("expected error, succeeded") - } - if err != nil && !tc.expectedError { - t.Errorf("expected success, failed") - } - if tc.expectedErrorMessage != "" { - if !strings.Contains(err.Error(), tc.expectedErrorMessage) { - t.Errorf("unexpected error: %v (expected %q)", err, tc.expectedErrorMessage) - } - } - }) - } -} - func TestMultipleMCPsPerTree(t *testing.T) { testCases := []struct { name string