Cleanup of now unused func with little refactoring. (#530)

metal-stack · Jun 3, 2024 · dfbc4a0 · dfbc4a0
1 parent 9a03d83
commit dfbc4a0
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 52 deletions.
diff --git a/cmd/metal-api/internal/metal/machine.go b/cmd/metal-api/internal/metal/machine.go
@@ -535,19 +535,6 @@ func exhaustiveMatch[V comparable](cs []Constraint, vs []V, countFn func(v V) (m
 	return len(unmatched) == 0
 }
 
-func (hw *MachineHardware) GPUModels() map[string]uint64 {
-	models := make(map[string]uint64)
-	for _, gpu := range hw.MetalGPUs {
-		_, ok := models[gpu.Model]
-		if !ok {
-			models[gpu.Model] = 1
-		} else {
-			models[gpu.Model]++
-		}
-	}
-	return models
-}
-
 // ReadableSpec returns a human readable string for the hardware.
 func (hw *MachineHardware) ReadableSpec() string {
 	diskCapacity, _ := capacityOf("*", hw.Disks, countDisk)

diff --git a/cmd/metal-api/internal/metal/size.go b/cmd/metal-api/internal/metal/size.go
@@ -1,6 +1,7 @@
 package metal
 
 import (
+	"errors"
 	"fmt"
 	"path/filepath"
 	"slices"
@@ -142,28 +143,28 @@ func (sz Sizes) FromHardware(hardware MachineHardware) (*Size, error) {
 nextsize:
 	for _, s := range sz {
 		for _, c := range s.Constraints {
-			match := c.matches(hardware)
-			if !match {
+			if !c.matches(hardware) {
 				continue nextsize
 			}
 		}
 
 		for _, ct := range allConstraintTypes {
-			match := hardware.matches(s.Constraints, ct)
-			if !match {
+			if !hardware.matches(s.Constraints, ct) {
 				continue nextsize
 			}
 		}
+
 		matchedSizes = append(matchedSizes, s)
 	}
 
-	if len(matchedSizes) == 0 {
+	switch len(matchedSizes) {
+	case 0:
 		return nil, NotFound("no size found for hardware (%s)", hardware.ReadableSpec())
-	}
-	if len(matchedSizes) > 1 {
+	case 1:
+		return &matchedSizes[0], nil
+	default:
 		return nil, fmt.Errorf("%d sizes found for hardware (%s)", len(matchedSizes), hardware.ReadableSpec())
 	}
-	return &matchedSizes[0], nil
 }
 
 func (s *Size) overlaps(so *Size) bool {
@@ -210,44 +211,64 @@ func (c *Constraint) overlaps(other Constraint) bool {
 	if c.Max < other.Min {
 		return false
 	}
+
 	return true
 }
 
-// Validate a size, returns error if a invalid size is passed
-func (s *Size) Validate(partitions PartitionMap, projects map[string]*mdmv1.Project) error {
-	constraintTypes := map[ConstraintType]uint{}
-	for _, c := range s.Constraints {
-		if c.Max < c.Min {
-			return fmt.Errorf("size:%q type:%q max:%d is smaller than min:%d", s.ID, c.Type, c.Max, c.Min)
-		}
+func (c *Constraint) validate() error {
+	if c.Max < c.Min {
+		return fmt.Errorf("max is smaller than min")
+	}
 
-		// CPU and Memory Constraints are not allowed more than once
-		constraintTypes[c.Type]++
-		count := constraintTypes[c.Type]
-		if c.Type == CoreConstraint || c.Type == MemoryConstraint {
-			if count > 1 {
-				return fmt.Errorf("size:%q type:%q min:%d max:%d has duplicate constraint type", s.ID, c.Type, c.Min, c.Max)
-			}
-		}
+	if _, err := filepath.Match(c.Identifier, ""); err != nil {
+		return fmt.Errorf("identifier is malformed: %w", err)
+	}
 
-		// Ensure GPU Constraints always have identifier specified
-		if c.Type == GPUConstraint && c.Identifier == "" {
-			return fmt.Errorf("size:%q type:%q min:%d max:%d is a gpu size but has no identifier specified", s.ID, c.Type, c.Min, c.Max)
+	switch t := c.Type; t {
+	case GPUConstraint:
+		if c.Identifier == "" {
+			return fmt.Errorf("for gpu constraints an identifier is required")
 		}
-
-		// Ensure Memory Constraints do not have a identifier specified
-		if c.Type == MemoryConstraint && c.Identifier != "" {
-			return fmt.Errorf("size:%q type:%q min:%d max:%d is a memory size but has a identifier specified", s.ID, c.Type, c.Min, c.Max)
+	case MemoryConstraint:
+		if c.Identifier != "" {
+			return fmt.Errorf("for memory constraints an identifier is not allowed")
 		}
+	case CoreConstraint, StorageConstraint:
+	}
+
+	return nil
+}
+
+// Validate a size, returns error if a invalid size is passed
+func (s *Size) Validate(partitions PartitionMap, projects map[string]*mdmv1.Project) error {
+	var (
+		errs       []error
+		typeCounts = map[ConstraintType]uint{}
+	)
 
-		if _, err := filepath.Match(c.Identifier, ""); err != nil {
-			return fmt.Errorf("size:%q type:%q min:%d max:%d identifier:%q identifier is malformed:%w", s.ID, c.Type, c.Min, c.Max, c.Identifier, err)
+	for i, c := range s.Constraints {
+		typeCounts[c.Type]++
+
+		err := c.validate()
+		if err != nil {
+			errs = append(errs, fmt.Errorf("constraint at index %d is invalid: %w", i, err))
 		}
 
+		switch t := c.Type; t {
+		case GPUConstraint, StorageConstraint:
+		case MemoryConstraint, CoreConstraint:
+			if typeCounts[t] > 1 {
+				errs = append(errs, fmt.Errorf("constraint at index %d is invalid: type duplicates are not allowed for type %q", i, t))
+			}
+		}
 	}
 
 	if err := s.Reservations.Validate(partitions, projects); err != nil {
-		return fmt.Errorf("invalid size reservation: %w", err)
+		errs = append(errs, fmt.Errorf("size reservations are invalid: %w", err))
+	}
+
+	if len(errs) > 0 {
+		return fmt.Errorf("size %q is invalid: %w", s.ID, errors.Join(errs...))
 	}
 
 	return nil

diff --git a/cmd/metal-api/internal/metal/size_test.go b/cmd/metal-api/internal/metal/size_test.go
@@ -1043,7 +1043,7 @@ func TestSize_Validate(t *testing.T) {
 					},
 				},
 			},
-			wantErrMessage: pointer.Pointer("size:\"broken-cpu-size\" type:\"cores\" max:2 is smaller than min:8"),
+			wantErrMessage: pointer.Pointer("size \"broken-cpu-size\" is invalid: constraint at index 0 is invalid: max is smaller than min"),
 		},
 		{
 			name: "memory min and max wrong",
@@ -1059,7 +1059,7 @@ func TestSize_Validate(t *testing.T) {
 					},
 				},
 			},
-			wantErrMessage: pointer.Pointer("size:\"broken-memory-size\" type:\"memory\" max:2 is smaller than min:8"),
+			wantErrMessage: pointer.Pointer("size \"broken-memory-size\" is invalid: constraint at index 0 is invalid: max is smaller than min"),
 		},
 		{
 			name: "storage is working",
@@ -1121,7 +1121,7 @@ func TestSize_Validate(t *testing.T) {
 					},
 				},
 			},
-			wantErrMessage: pointer.Pointer("size:\"cpu-size\" type:\"cores\" min:2 max:2 has duplicate constraint type"),
+			wantErrMessage: pointer.Pointer("size \"cpu-size\" is invalid: constraint at index 1 is invalid: type duplicates are not allowed for type \"cores\""),
 		},
 		{
 			name: "gpu size without identifier",
@@ -1137,7 +1137,7 @@ func TestSize_Validate(t *testing.T) {
 					},
 				},
 			},
-			wantErrMessage: pointer.Pointer("size:\"invalid-gpu-size\" type:\"gpu\" min:2 max:8 is a gpu size but has no identifier specified"),
+			wantErrMessage: pointer.Pointer("size \"invalid-gpu-size\" is invalid: constraint at index 0 is invalid: for gpu constraints an identifier is required"),
 		},
 		{
 			name: "storage with invalid identifier",
@@ -1154,7 +1154,7 @@ func TestSize_Validate(t *testing.T) {
 					},
 				},
 			},
-			wantErrMessage: pointer.Pointer("size:\"invalid-storage-size\" type:\"storage\" min:2 max:8 identifier:\"][\" identifier is malformed:syntax error in pattern"),
+			wantErrMessage: pointer.Pointer("size \"invalid-storage-size\" is invalid: constraint at index 0 is invalid: identifier is malformed: syntax error in pattern"),
 		},
 		{
 			name: "memory with identifier",
@@ -1171,7 +1171,7 @@ func TestSize_Validate(t *testing.T) {
 					},
 				},
 			},
-			wantErrMessage: pointer.Pointer("size:\"invalid-memory-size\" type:\"memory\" min:2 max:8 is a memory size but has a identifier specified"),
+			wantErrMessage: pointer.Pointer("size \"invalid-memory-size\" is invalid: constraint at index 0 is invalid: for memory constraints an identifier is not allowed"),
 		},
 	}
 	for _, tt := range tests {