From 818f4d3c8d1b27154491f5f3c0e9fc1d4e11b5c6 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Tue, 4 Jun 2024 11:27:32 +0200 Subject: [PATCH] Fix size overlapping not checked symmetrically (#532) --- .../internal/grpc/boot-service_test.go | 6 +- cmd/metal-api/internal/metal/size.go | 30 +- cmd/metal-api/internal/metal/size_test.go | 261 +++++++++++++----- .../internal/service/size-service.go | 4 +- cmd/metal-api/internal/testdata/testdata.go | 5 + 5 files changed, 233 insertions(+), 73 deletions(-) diff --git a/cmd/metal-api/internal/grpc/boot-service_test.go b/cmd/metal-api/internal/grpc/boot-service_test.go index ba33f0685..cd9107dcd 100644 --- a/cmd/metal-api/internal/grpc/boot-service_test.go +++ b/cmd/metal-api/internal/grpc/boot-service_test.go @@ -119,7 +119,11 @@ func TestBootService_Register(t *testing.T) { Uuid: tt.uuid, Hardware: &v1.MachineHardware{ Memory: uint64(tt.memory), - + Disks: []*v1.MachineBlockDevice{ + { + Size: 1000000000000, + }, + }, Cpus: []*v1.MachineCPU{ { Model: "Intel Xeon Silver", diff --git a/cmd/metal-api/internal/metal/size.go b/cmd/metal-api/internal/metal/size.go index 0913889c0..698ad5222 100644 --- a/cmd/metal-api/internal/metal/size.go +++ b/cmd/metal-api/internal/metal/size.go @@ -61,6 +61,10 @@ func countDisk(disk BlockDevice) (model string, count uint64) { return disk.Name, disk.Size } +func countMemory(size uint64) (model string, count uint64) { + return "", size +} + // Sizes is a list of sizes. type Sizes []Size @@ -114,9 +118,6 @@ func (c *Constraint) matches(hw MachineHardware) bool { // With this we ensure that hardware matches exhaustive against the constraints. func (hw *MachineHardware) matches(constraints []Constraint, constraintType ConstraintType) bool { filtered := lo.Filter(constraints, func(c Constraint, _ int) bool { return c.Type == constraintType }) - if len(filtered) == 0 { - return true - } switch constraintType { case StorageConstraint: @@ -126,10 +127,9 @@ func (hw *MachineHardware) matches(constraints []Constraint, constraintType Cons case CoreConstraint: return exhaustiveMatch(filtered, hw.MetalCPUs, countCPU) case MemoryConstraint: - // Noop because we do not have different Memory types - return true + return exhaustiveMatch(filtered, []uint64{hw.Memory}, countMemory) default: - return true + return false } } @@ -168,15 +168,17 @@ nextsize: } func (s *Size) overlaps(so *Size) bool { - if len(lo.FromPtr(so).Constraints) == 0 { + if len(lo.FromPtr(so).Constraints) == 0 || len(lo.FromPtr(s).Constraints) == 0 { return false } + srcTypes := lo.GroupBy(s.Constraints, func(item Constraint) ConstraintType { return item.Type }) destTypes := lo.GroupBy(so.Constraints, func(item Constraint) ConstraintType { return item.Type }) + for t, srcConstraints := range srcTypes { constraints, ok := destTypes[t] if !ok { @@ -191,6 +193,20 @@ func (s *Size) overlaps(so *Size) bool { } } + for t, destConstraints := range destTypes { + constraints, ok := srcTypes[t] + if !ok { + return false + } + for _, sc := range destConstraints { + for _, c := range constraints { + if !c.overlaps(sc) { + return false + } + } + } + } + return true } diff --git a/cmd/metal-api/internal/metal/size_test.go b/cmd/metal-api/internal/metal/size_test.go index de4d3fcf1..97c4d4cd1 100644 --- a/cmd/metal-api/internal/metal/size_test.go +++ b/cmd/metal-api/internal/metal/size_test.go @@ -270,6 +270,11 @@ var ( Min: 1, Max: 1, }, + { + Type: MemoryConstraint, + Min: 2048, + Max: 2048, + }, }, } amdCPUSize = Size{ @@ -283,6 +288,11 @@ var ( Min: 1, Max: 1, }, + { + Type: MemoryConstraint, + Min: 2048, + Max: 2048, + }, }, } // Sizes @@ -636,6 +646,7 @@ func TestSizes_FromHardware(t *testing.T) { Threads: 1, }, }, + Memory: 2048, }, }, want: &intelCPUSize, @@ -656,6 +667,7 @@ func TestSizes_FromHardware(t *testing.T) { Threads: 1, }, }, + Memory: 2048, }, }, want: &amdCPUSize, @@ -688,6 +700,68 @@ func TestSizes_FromHardware(t *testing.T) { want: &miniLabSize, wantErr: false, }, + { + name: "memory exhaustive check", + sz: Sizes{ + { + Base: Base{ + ID: "without core constraint", + }, + Constraints: []Constraint{ + { + Type: MemoryConstraint, + Min: 2147483648, + Max: 2147483648, + }, + }, + }, + { + Base: Base{ + ID: "with core constraint", + }, + Constraints: []Constraint{ + { + Type: MemoryConstraint, + Min: 2147483648, + Max: 2147483648, + }, + { + Type: CoreConstraint, + Min: 1, + Max: 1, + }, + }, + }, + }, + args: args{ + hardware: MachineHardware{ + MetalCPUs: []MetalCPU{ + { + Cores: 1, + }, + }, + Memory: 2147483648, + }, + }, + want: &Size{ + Base: Base{ + ID: "with core constraint", + }, + Constraints: []Constraint{ + { + Type: MemoryConstraint, + Min: 2147483648, + Max: 2147483648, + }, + { + Type: CoreConstraint, + Min: 1, + Max: 1, + }, + }, + }, + wantErr: false, + }, } for i := range tests { @@ -907,50 +981,8 @@ func TestSizes_Overlaps(t *testing.T) { }, }, sizes: Sizes{ - Size{ - Base: Base{ - ID: "micro", - }, - Constraints: []Constraint{ - { - Type: CoreConstraint, - Min: 1, - Max: 1, - }, - { - Type: MemoryConstraint, - Min: 1024, - Max: 1024, - }, - { - Type: StorageConstraint, - Min: 0, - Max: 1024, - }, - }, - }, - Size{ - Base: Base{ - ID: "tiny", - }, - Constraints: []Constraint{ - { - Type: CoreConstraint, - Min: 1, - Max: 1, - }, - { - Type: MemoryConstraint, - Min: 1025, - Max: 1077838336, - }, - { - Type: StorageConstraint, - Min: 1024, - Max: 2048, - }, - }, - }, + microSize, + tinySize, Size{ Base: Base{ ID: "large", @@ -974,7 +1006,7 @@ func TestSizes_Overlaps(t *testing.T) { }, }, }, - want: µSize, + want: nil, }, { @@ -1536,7 +1568,6 @@ func TestConstraint_overlaps(t *testing.T) { }, want: true, }, - { name: "partial overlap, in range", this: Constraint{ @@ -1582,10 +1613,10 @@ func TestConstraint_overlaps(t *testing.T) { func TestSize_overlaps(t *testing.T) { tests := []struct { - name string - this *Size - other *Size - want bool + name string + this *Size + other *Size + wantOverlap bool }{ { name: "no overlap, different types", @@ -1599,9 +1630,8 @@ func TestSize_overlaps(t *testing.T) { {Type: CoreConstraint}, }, }, - want: false, + wantOverlap: false, }, - { name: "no overlap, different identifiers", this: &Size{ @@ -1614,9 +1644,8 @@ func TestSize_overlaps(t *testing.T) { {Type: MemoryConstraint, Identifier: "b"}, }, }, - want: false, + wantOverlap: false, }, - { name: "no overlap, different range", this: &Size{ @@ -1629,9 +1658,8 @@ func TestSize_overlaps(t *testing.T) { {Type: MemoryConstraint, Identifier: "a", Min: 5, Max: 8}, }, }, - want: false, + wantOverlap: false, }, - { name: "no overlap, different gpus", this: &Size{ @@ -1645,9 +1673,8 @@ func TestSize_overlaps(t *testing.T) { {Type: GPUConstraint, Identifier: "b", Min: 2, Max: 2}, }, }, - want: false, + wantOverlap: false, }, - { name: "overlapping size", this: &Size{ @@ -1688,7 +1715,7 @@ func TestSize_overlaps(t *testing.T) { }, }, }, - want: true, + wantOverlap: true, }, { name: "non overlapping size", @@ -1732,7 +1759,7 @@ func TestSize_overlaps(t *testing.T) { }, }, }, - want: false, + wantOverlap: false, }, { name: "overlap, all the same", @@ -1750,13 +1777,121 @@ func TestSize_overlaps(t *testing.T) { {Type: MemoryConstraint, Identifier: "a", Min: 5, Max: 8}, }, }, - want: true, + wantOverlap: true, + }, + { + name: "independent of order #1", + this: &Size{ + Base: Base{ + ID: "g1-medium-x86", + }, + Constraints: []Constraint{ + { + Type: CoreConstraint, + Min: 32, + Max: 32, + }, + { + Type: MemoryConstraint, + Min: 257698037760, + Max: 300647710720, + }, + { + Type: StorageConstraint, + Min: 1500000000000, + Max: 2000000000000, + }, + { + Type: GPUConstraint, + Min: 1, + Max: 1, + Identifier: "AD102GL*", + }, + }, + }, + other: &Size{ + Base: Base{ + ID: "c2-xlarge-x86", + }, + Constraints: []Constraint{ + { + Type: CoreConstraint, + Min: 32, + Max: 32, + }, + { + Type: MemoryConstraint, + Min: 220000000000, + Max: 280000000000, + }, + { + Type: StorageConstraint, + Min: 500000000000, + Max: 4000000000000, + }, + }, + }, + wantOverlap: false, + }, + { + name: "independent of order #2", + this: &Size{ + Base: Base{ + ID: "c2-xlarge-x86", + }, + Constraints: []Constraint{ + { + Type: CoreConstraint, + Min: 32, + Max: 32, + }, + { + Type: MemoryConstraint, + Min: 220000000000, + Max: 280000000000, + }, + { + Type: StorageConstraint, + Min: 500000000000, + Max: 4000000000000, + }, + }, + }, + other: &Size{ + Base: Base{ + ID: "g1-medium-x86", + }, + Constraints: []Constraint{ + { + Type: CoreConstraint, + Min: 32, + Max: 32, + }, + { + Type: MemoryConstraint, + Min: 257698037760, + Max: 300647710720, + }, + { + Type: StorageConstraint, + Min: 1500000000000, + Max: 2000000000000, + }, + { + Type: GPUConstraint, + Min: 1, + Max: 1, + Identifier: "AD102GL*", + }, + }, + }, + wantOverlap: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := tt.this.overlaps(tt.other); !reflect.DeepEqual(got, tt.want) { - t.Errorf("Size.Overlaps() = %v, want %v", got, tt.want) + if got := tt.this.overlaps(tt.other); !reflect.DeepEqual(got, tt.wantOverlap) { + t.Errorf("Size.Overlaps() = %v, want %v", got, tt.wantOverlap) } }) } diff --git a/cmd/metal-api/internal/service/size-service.go b/cmd/metal-api/internal/service/size-service.go index 417b49ba8..d80e540c8 100644 --- a/cmd/metal-api/internal/service/size-service.go +++ b/cmd/metal-api/internal/service/size-service.go @@ -306,7 +306,7 @@ func (r *sizeResource) createSize(request *restful.Request, response *restful.Re } if so := s.Overlaps(&ss); so != nil { - r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("size overlaps with %q", so.GetID()))) + r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("size %q overlaps with %q", s.GetID(), so.GetID()))) return } @@ -414,7 +414,7 @@ func (r *sizeResource) updateSize(request *restful.Request, response *restful.Re } if so := newSize.Overlaps(&ss); so != nil { - r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("size overlaps with %q", so.GetID()))) + r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("size %q overlaps with %q", newSize.GetID(), so.GetID()))) return } diff --git a/cmd/metal-api/internal/testdata/testdata.go b/cmd/metal-api/internal/testdata/testdata.go index 97336f7ad..273f5bfb0 100644 --- a/cmd/metal-api/internal/testdata/testdata.go +++ b/cmd/metal-api/internal/testdata/testdata.go @@ -184,6 +184,11 @@ var ( Min: 100, Max: 100, }, + { + Type: metal.StorageConstraint, + Min: 1000000000000, + Max: 1000000000000, + }, }, Reservations: metal.Reservations{ {