From ca3e55288120011789c24514b1ed11247fa0966c Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Mon, 27 Apr 2026 12:32:48 +0200 Subject: [PATCH 1/3] Add spec.groups field with typed group memberships Introduce a new spec.groups field on the Hypervisor CRD that unifies traits and aggregates as typed group entries using the field-presence union pattern. Each entry populates exactly one type-specific sub-field (trait or aggregate), enforced by a CEL validation rule. Includes library helper functions (HasTrait, GetTraits, HasAggregate, GetAggregates) following the meta.IsStatusConditionTrue pattern, reusable across the shim, operator, and scheduler. Existing spec.customTraits and spec.aggregates fields remain untouched. --- api/v1/hypervisor_types.go | 97 +++++++ api/v1/hypervisor_validation_test.go | 253 ++++++++++++++++++ api/v1/zz_generated.deepcopy.go | 69 +++++ applyconfigurations/api/v1/aggregategroup.go | 47 ++++ applyconfigurations/api/v1/group.go | 32 +++ applyconfigurations/api/v1/hypervisorspec.go | 14 + applyconfigurations/api/v1/traitgroup.go | 23 ++ applyconfigurations/utils.go | 6 + .../crds/kvm.cloud.sap_hypervisors.yaml | 62 +++++ 9 files changed, 603 insertions(+) create mode 100644 applyconfigurations/api/v1/aggregategroup.go create mode 100644 applyconfigurations/api/v1/group.go create mode 100644 applyconfigurations/api/v1/traitgroup.go diff --git a/api/v1/hypervisor_types.go b/api/v1/hypervisor_types.go index a615547..6fb9b8f 100644 --- a/api/v1/hypervisor_types.go +++ b/api/v1/hypervisor_types.go @@ -132,6 +132,25 @@ type HypervisorSpec struct { // Aggregates are used to apply aggregates to the hypervisor. Aggregates []string `json:"aggregates"` + // Groups defines typed group memberships for this hypervisor. + // + // Both traits and aggregates are forms of grouping: traits group + // hypervisors by capability, aggregates group them by administrative + // assignment. Each entry follows the field-presence union pattern + // (as used by PodSpec.volumes in core Kubernetes): exactly one + // type-specific sub-field must be populated per entry. + // + // The Cortex Placement shim and scheduler read group memberships + // directly from this field. + // + // Note: uniqueness of trait names and aggregate UUIDs is not enforced + // via CEL because the required O(n^2) comparison exceeds the + // Kubernetes CEL cost budget. Enforce uniqueness in the consuming + // controller or via a validating webhook if needed. + // + // +kubebuilder:validation:Optional + Groups []Group `json:"groups,omitempty"` + // +kubebuilder:default:={} // AllowedProjects defines which openstack projects are allowed to schedule // instances on this hypervisor. The values of this list should be project @@ -212,6 +231,84 @@ type Aggregate struct { Metadata map[string]string `json:"metadata,omitempty"` } +// TraitGroup represents a capability trait, such as an OpenStack +// Placement trait (e.g. HW_CPU_X86_AVX2, COMPUTE_STATUS_DISABLED). +type TraitGroup struct { + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` +} + +// AggregateGroup represents an administrative grouping, such as an +// OpenStack host aggregate. +type AggregateGroup struct { + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + + // +kubebuilder:validation:MinLength=1 + UUID string `json:"uuid"` + + // +kubebuilder:validation:Optional + Metadata map[string]string `json:"metadata,omitempty"` +} + +// Group is a typed group membership entry for a hypervisor. +// +// This follows the field-presence union pattern (as used by +// PodSpec.volumes in core Kubernetes): each entry populates exactly +// one type-specific sub-field, and the populated field identifies +// the group type. +// +// +kubebuilder:validation:XValidation:rule="(has(self.trait) ? 1 : 0) + (has(self.aggregate) ? 1 : 0) == 1",message="exactly one group type must be set" +type Group struct { + // +kubebuilder:validation:Optional + Trait *TraitGroup `json:"trait,omitempty"` + + // +kubebuilder:validation:Optional + Aggregate *AggregateGroup `json:"aggregate,omitempty"` +} + +// HasTrait reports whether groups contains a trait entry with the given name. +func HasTrait(groups []Group, name string) bool { + for _, g := range groups { + if g.Trait != nil && g.Trait.Name == name { + return true + } + } + return false +} + +// GetTraits returns all TraitGroup entries from groups. +func GetTraits(groups []Group) []TraitGroup { + var out []TraitGroup + for _, g := range groups { + if g.Trait != nil { + out = append(out, *g.Trait) + } + } + return out +} + +// HasAggregate reports whether groups contains an aggregate entry with the given UUID. +func HasAggregate(groups []Group, uuid string) bool { + for _, g := range groups { + if g.Aggregate != nil && g.Aggregate.UUID == uuid { + return true + } + } + return false +} + +// GetAggregates returns all AggregateGroup entries from groups. +func GetAggregates(groups []Group) []AggregateGroup { + var out []AggregateGroup + for _, g := range groups { + if g.Aggregate != nil { + out = append(out, *g.Aggregate) + } + } + return out +} + type HyperVisorUpdateStatus struct { // +kubebuilder:default:=false // Represents a running Operating System update. diff --git a/api/v1/hypervisor_validation_test.go b/api/v1/hypervisor_validation_test.go index 25439bf..32fbe7e 100644 --- a/api/v1/hypervisor_validation_test.go +++ b/api/v1/hypervisor_validation_test.go @@ -18,6 +18,8 @@ limitations under the License. package v1 import ( + "fmt" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -396,3 +398,254 @@ var _ = Describe("MaintenanceReason CEL Validation", func() { }) }) }) + +// TestGroupsCELValidation tests the CEL validation rules for spec.groups: +// 1. Exactly one group type must be set per entry (union rule on Group) +// 2. Field-level validation (minLength) on trait name, aggregate name, and aggregate UUID +var _ = Describe("Groups CEL Validation", func() { + var ( + hypervisor *Hypervisor + hypervisorName types.NamespacedName + counter int + ) + + BeforeEach(func(ctx SpecContext) { + counter++ + hypervisorName = types.NamespacedName{ + Name: fmt.Sprintf("test-groups-hv-%d", counter), + } + }) + + AfterEach(func(ctx SpecContext) { + if hypervisor != nil { + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, hypervisor))).To(Succeed()) + hypervisor = nil + } + }) + + Context("Union rule: exactly one group type per entry", func() { + It("should accept a group with only trait set", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}}, + }, + }, + } + Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + }) + + It("should accept a group with only aggregate set", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "abc-123"}}, + }, + }, + } + Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + }) + + It("should accept mixed trait and aggregate entries", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}}, + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "abc-123"}}, + {Trait: &TraitGroup{Name: "COMPUTE_STATUS_DISABLED"}}, + }, + }, + } + Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + }) + + It("should reject a group with both trait and aggregate set", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + { + Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}, + Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "abc-123"}, + }, + }, + }, + } + err := k8sClient.Create(ctx, hypervisor) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("exactly one group type must be set")) + }) + + It("should reject a group with neither trait nor aggregate set", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {}, + }, + }, + } + err := k8sClient.Create(ctx, hypervisor) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("exactly one group type must be set")) + }) + }) + + Context("Field validation", func() { + It("should reject a trait with empty name", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Trait: &TraitGroup{Name: ""}}, + }, + }, + } + err := k8sClient.Create(ctx, hypervisor) + Expect(err).To(HaveOccurred()) + }) + + It("should reject an aggregate with empty name", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Aggregate: &AggregateGroup{Name: "", UUID: "uuid-1"}}, + }, + }, + } + err := k8sClient.Create(ctx, hypervisor) + Expect(err).To(HaveOccurred()) + }) + + It("should reject an aggregate with empty UUID", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: ""}}, + }, + }, + } + err := k8sClient.Create(ctx, hypervisor) + Expect(err).To(HaveOccurred()) + }) + + It("should accept an aggregate without metadata", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "uuid-1"}}, + }, + }, + } + Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + }) + + It("should accept an aggregate with metadata", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{ + {Aggregate: &AggregateGroup{ + Name: "fast-storage", + UUID: "uuid-1", + Metadata: map[string]string{"ssd": "true"}, + }}, + }, + }, + } + Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + }) + + It("should accept an empty groups list", func(ctx SpecContext) { + hypervisor = &Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, + Spec: HypervisorSpec{ + Groups: []Group{}, + }, + } + Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + }) + }) +}) + +var _ = Describe("Group Helper Functions", func() { + groups := []Group{ + {Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}}, + {Trait: &TraitGroup{Name: "COMPUTE_STATUS_DISABLED"}}, + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "uuid-1", Metadata: map[string]string{"ssd": "true"}}}, + {Aggregate: &AggregateGroup{Name: "slow-storage", UUID: "uuid-2"}}, + } + + Context("HasTrait", func() { + It("should return true for an existing trait", func() { + Expect(HasTrait(groups, "HW_CPU_X86_AVX2")).To(BeTrue()) + }) + + It("should return false for a missing trait", func() { + Expect(HasTrait(groups, "NONEXISTENT")).To(BeFalse()) + }) + + It("should return false for an empty list", func() { + Expect(HasTrait(nil, "HW_CPU_X86_AVX2")).To(BeFalse()) + }) + }) + + Context("GetTraits", func() { + It("should return all trait entries", func() { + traits := GetTraits(groups) + Expect(traits).To(HaveLen(2)) + Expect(traits[0].Name).To(Equal("HW_CPU_X86_AVX2")) + Expect(traits[1].Name).To(Equal("COMPUTE_STATUS_DISABLED")) + }) + + It("should return empty for a list with no traits", func() { + aggs := []Group{{Aggregate: &AggregateGroup{Name: "a", UUID: "u"}}} + Expect(GetTraits(aggs)).To(BeEmpty()) + }) + + It("should return nil for an empty list", func() { + Expect(GetTraits(nil)).To(BeNil()) + }) + }) + + Context("HasAggregate", func() { + It("should return true for an existing aggregate UUID", func() { + Expect(HasAggregate(groups, "uuid-1")).To(BeTrue()) + }) + + It("should return false for a missing aggregate UUID", func() { + Expect(HasAggregate(groups, "uuid-999")).To(BeFalse()) + }) + + It("should return false for an empty list", func() { + Expect(HasAggregate(nil, "uuid-1")).To(BeFalse()) + }) + }) + + Context("GetAggregates", func() { + It("should return all aggregate entries", func() { + aggs := GetAggregates(groups) + Expect(aggs).To(HaveLen(2)) + Expect(aggs[0].Name).To(Equal("fast-storage")) + Expect(aggs[0].UUID).To(Equal("uuid-1")) + Expect(aggs[0].Metadata).To(HaveKeyWithValue("ssd", "true")) + Expect(aggs[1].Name).To(Equal("slow-storage")) + Expect(aggs[1].UUID).To(Equal("uuid-2")) + }) + + It("should return empty for a list with no aggregates", func() { + traits := []Group{{Trait: &TraitGroup{Name: "T"}}} + Expect(GetAggregates(traits)).To(BeEmpty()) + }) + + It("should return nil for an empty list", func() { + Expect(GetAggregates(nil)).To(BeNil()) + }) + }) +}) diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 802d5f3..7e70b04 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -49,6 +49,28 @@ func (in *Aggregate) DeepCopy() *Aggregate { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AggregateGroup) DeepCopyInto(out *AggregateGroup) { + *out = *in + if in.Metadata != nil { + in, out := &in.Metadata, &out.Metadata + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AggregateGroup. +func (in *AggregateGroup) DeepCopy() *AggregateGroup { + if in == nil { + return nil + } + out := new(AggregateGroup) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Capabilities) DeepCopyInto(out *Capabilities) { *out = *in @@ -233,6 +255,31 @@ func (in *EvictionStatus) DeepCopy() *EvictionStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Group) DeepCopyInto(out *Group) { + *out = *in + if in.Trait != nil { + in, out := &in.Trait, &out.Trait + *out = new(TraitGroup) + **out = **in + } + if in.Aggregate != nil { + in, out := &in.Aggregate, &out.Aggregate + *out = new(AggregateGroup) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Group. +func (in *Group) DeepCopy() *Group { + if in == nil { + return nil + } + out := new(Group) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HyperVisorUpdateStatus) DeepCopyInto(out *HyperVisorUpdateStatus) { *out = *in @@ -320,6 +367,13 @@ func (in *HypervisorSpec) DeepCopyInto(out *HypervisorSpec) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.Groups != nil { + in, out := &in.Groups, &out.Groups + *out = make([]Group, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.AllowedProjects != nil { in, out := &in.AllowedProjects, &out.AllowedProjects *out = make([]string, len(*in)) @@ -450,3 +504,18 @@ func (in *OperatingSystemStatus) DeepCopy() *OperatingSystemStatus { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *TraitGroup) DeepCopyInto(out *TraitGroup) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TraitGroup. +func (in *TraitGroup) DeepCopy() *TraitGroup { + if in == nil { + return nil + } + out := new(TraitGroup) + in.DeepCopyInto(out) + return out +} diff --git a/applyconfigurations/api/v1/aggregategroup.go b/applyconfigurations/api/v1/aggregategroup.go new file mode 100644 index 0000000..f2327e9 --- /dev/null +++ b/applyconfigurations/api/v1/aggregategroup.go @@ -0,0 +1,47 @@ +// Code generated by controller-gen. DO NOT EDIT. + +package v1 + +// AggregateGroupApplyConfiguration represents a declarative configuration of the AggregateGroup type for use +// with apply. +type AggregateGroupApplyConfiguration struct { + Name *string `json:"name,omitempty"` + UUID *string `json:"uuid,omitempty"` + Metadata map[string]string `json:"metadata,omitempty"` +} + +// AggregateGroupApplyConfiguration constructs a declarative configuration of the AggregateGroup type for use with +// apply. +func AggregateGroup() *AggregateGroupApplyConfiguration { + return &AggregateGroupApplyConfiguration{} +} + +// WithName sets the Name field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Name field is set to the value of the last call. +func (b *AggregateGroupApplyConfiguration) WithName(value string) *AggregateGroupApplyConfiguration { + b.Name = &value + return b +} + +// WithUUID sets the UUID field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the UUID field is set to the value of the last call. +func (b *AggregateGroupApplyConfiguration) WithUUID(value string) *AggregateGroupApplyConfiguration { + b.UUID = &value + return b +} + +// WithMetadata puts the entries into the Metadata field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, the entries provided by each call will be put on the Metadata field, +// overwriting an existing map entries in Metadata field with the same key. +func (b *AggregateGroupApplyConfiguration) WithMetadata(entries map[string]string) *AggregateGroupApplyConfiguration { + if b.Metadata == nil && len(entries) > 0 { + b.Metadata = make(map[string]string, len(entries)) + } + for k, v := range entries { + b.Metadata[k] = v + } + return b +} diff --git a/applyconfigurations/api/v1/group.go b/applyconfigurations/api/v1/group.go new file mode 100644 index 0000000..550e1f4 --- /dev/null +++ b/applyconfigurations/api/v1/group.go @@ -0,0 +1,32 @@ +// Code generated by controller-gen. DO NOT EDIT. + +package v1 + +// GroupApplyConfiguration represents a declarative configuration of the Group type for use +// with apply. +type GroupApplyConfiguration struct { + Trait *TraitGroupApplyConfiguration `json:"trait,omitempty"` + Aggregate *AggregateGroupApplyConfiguration `json:"aggregate,omitempty"` +} + +// GroupApplyConfiguration constructs a declarative configuration of the Group type for use with +// apply. +func Group() *GroupApplyConfiguration { + return &GroupApplyConfiguration{} +} + +// WithTrait sets the Trait field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Trait field is set to the value of the last call. +func (b *GroupApplyConfiguration) WithTrait(value *TraitGroupApplyConfiguration) *GroupApplyConfiguration { + b.Trait = value + return b +} + +// WithAggregate sets the Aggregate field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Aggregate field is set to the value of the last call. +func (b *GroupApplyConfiguration) WithAggregate(value *AggregateGroupApplyConfiguration) *GroupApplyConfiguration { + b.Aggregate = value + return b +} diff --git a/applyconfigurations/api/v1/hypervisorspec.go b/applyconfigurations/api/v1/hypervisorspec.go index 4d0930b..127136f 100644 --- a/applyconfigurations/api/v1/hypervisorspec.go +++ b/applyconfigurations/api/v1/hypervisorspec.go @@ -16,6 +16,7 @@ type HypervisorSpecApplyConfiguration struct { SkipTests *bool `json:"skipTests,omitempty"` CustomTraits []string `json:"customTraits,omitempty"` Aggregates []string `json:"aggregates,omitempty"` + Groups []GroupApplyConfiguration `json:"groups,omitempty"` AllowedProjects []string `json:"allowedProjects,omitempty"` HighAvailability *bool `json:"highAvailability,omitempty"` CreateCertManagerCertificate *bool `json:"createCertManagerCertificate,omitempty"` @@ -91,6 +92,19 @@ func (b *HypervisorSpecApplyConfiguration) WithAggregates(values ...string) *Hyp return b } +// WithGroups adds the given value to the Groups field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the Groups field. +func (b *HypervisorSpecApplyConfiguration) WithGroups(values ...*GroupApplyConfiguration) *HypervisorSpecApplyConfiguration { + for i := range values { + if values[i] == nil { + panic("nil value passed to WithGroups") + } + b.Groups = append(b.Groups, *values[i]) + } + return b +} + // WithAllowedProjects adds the given value to the AllowedProjects field in the declarative configuration // and returns the receiver, so that objects can be build by chaining "With" function invocations. // If called multiple times, values provided by each call will be appended to the AllowedProjects field. diff --git a/applyconfigurations/api/v1/traitgroup.go b/applyconfigurations/api/v1/traitgroup.go new file mode 100644 index 0000000..7f6817a --- /dev/null +++ b/applyconfigurations/api/v1/traitgroup.go @@ -0,0 +1,23 @@ +// Code generated by controller-gen. DO NOT EDIT. + +package v1 + +// TraitGroupApplyConfiguration represents a declarative configuration of the TraitGroup type for use +// with apply. +type TraitGroupApplyConfiguration struct { + Name *string `json:"name,omitempty"` +} + +// TraitGroupApplyConfiguration constructs a declarative configuration of the TraitGroup type for use with +// apply. +func TraitGroup() *TraitGroupApplyConfiguration { + return &TraitGroupApplyConfiguration{} +} + +// WithName sets the Name field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Name field is set to the value of the last call. +func (b *TraitGroupApplyConfiguration) WithName(value string) *TraitGroupApplyConfiguration { + b.Name = &value + return b +} diff --git a/applyconfigurations/utils.go b/applyconfigurations/utils.go index 2f68c61..5bc0d22 100644 --- a/applyconfigurations/utils.go +++ b/applyconfigurations/utils.go @@ -18,6 +18,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { // Group=kvm.cloud.sap, Version=v1 case v1.SchemeGroupVersion.WithKind("Aggregate"): return &apiv1.AggregateApplyConfiguration{} + case v1.SchemeGroupVersion.WithKind("AggregateGroup"): + return &apiv1.AggregateGroupApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("Capabilities"): return &apiv1.CapabilitiesApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("Cell"): @@ -30,6 +32,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1.EvictionSpecApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("EvictionStatus"): return &apiv1.EvictionStatusApplyConfiguration{} + case v1.SchemeGroupVersion.WithKind("Group"): + return &apiv1.GroupApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("Hypervisor"): return &apiv1.HypervisorApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("HypervisorSpec"): @@ -42,6 +46,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1.InstanceApplyConfiguration{} case v1.SchemeGroupVersion.WithKind("OperatingSystemStatus"): return &apiv1.OperatingSystemStatusApplyConfiguration{} + case v1.SchemeGroupVersion.WithKind("TraitGroup"): + return &apiv1.TraitGroupApplyConfiguration{} } return nil diff --git a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml index fb1e494..d9c7dcf 100644 --- a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml +++ b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml @@ -136,6 +136,68 @@ spec: description: EvacuateOnReboot request an evacuation of all instances before reboot. type: boolean + groups: + description: |- + Groups defines typed group memberships for this hypervisor. + + Both traits and aggregates are forms of grouping: traits group + hypervisors by capability, aggregates group them by administrative + assignment. Each entry follows the field-presence union pattern + (as used by PodSpec.volumes in core Kubernetes): exactly one + type-specific sub-field must be populated per entry. + + The Cortex Placement Shim and scheduler read group memberships + directly from this field. + + Note: uniqueness of trait names and aggregate UUIDs is not enforced + via CEL because the required O(n^2) comparison exceeds the + Kubernetes CEL cost budget. Enforce uniqueness in the consuming + controller or via a validating webhook if needed. + items: + description: |- + Group is a typed group membership entry for a hypervisor. + + This follows the field-presence union pattern (as used by + PodSpec.volumes in core Kubernetes): each entry populates exactly + one type-specific sub-field, and the populated field identifies + the group type. + properties: + aggregate: + description: |- + AggregateGroup represents an administrative grouping, such as an + OpenStack host aggregate. + properties: + metadata: + additionalProperties: + type: string + type: object + name: + minLength: 1 + type: string + uuid: + minLength: 1 + type: string + required: + - name + - uuid + type: object + trait: + description: |- + TraitGroup represents a capability trait, such as an OpenStack + Placement trait (e.g. HW_CPU_X86_AVX2, COMPUTE_STATUS_DISABLED). + properties: + name: + minLength: 1 + type: string + required: + - name + type: object + type: object + x-kubernetes-validations: + - message: exactly one group type must be set + rule: '(has(self.trait) ? 1 : 0) + (has(self.aggregate) ? 1 : + 0) == 1' + type: array highAvailability: default: true description: HighAvailability is used to enable the high availability From 8f84a6e9a2ed361e9e17cfd9e717d18e7b3b7100 Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 29 Apr 2026 08:32:55 +0200 Subject: [PATCH 2/3] Address review feedback for spec.groups validation - Replace MinLength=1 with RFC 4122 UUID pattern validation on AggregateGroup.UUID to reject malformed UUIDs at the CRD level. - Tighten negative validation test assertions to check specific field paths in error messages (trait.name, aggregate.name, aggregate.uuid). - Add round-trip assertion verifying aggregate metadata survives serialization after creation. - Update all test UUIDs to valid RFC 4122 format. --- api/v1/hypervisor_types.go | 2 +- api/v1/hypervisor_validation_test.go | 31 ++++++++++++------- .../crds/kvm.cloud.sap_hypervisors.yaml | 2 +- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/api/v1/hypervisor_types.go b/api/v1/hypervisor_types.go index 6fb9b8f..67792b6 100644 --- a/api/v1/hypervisor_types.go +++ b/api/v1/hypervisor_types.go @@ -244,7 +244,7 @@ type AggregateGroup struct { // +kubebuilder:validation:MinLength=1 Name string `json:"name"` - // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:Pattern=`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$` UUID string `json:"uuid"` // +kubebuilder:validation:Optional diff --git a/api/v1/hypervisor_validation_test.go b/api/v1/hypervisor_validation_test.go index 32fbe7e..64f50db 100644 --- a/api/v1/hypervisor_validation_test.go +++ b/api/v1/hypervisor_validation_test.go @@ -506,6 +506,7 @@ var _ = Describe("Groups CEL Validation", func() { } err := k8sClient.Create(ctx, hypervisor) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.groups[0].trait.name")) }) It("should reject an aggregate with empty name", func(ctx SpecContext) { @@ -513,12 +514,13 @@ var _ = Describe("Groups CEL Validation", func() { ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, Spec: HypervisorSpec{ Groups: []Group{ - {Aggregate: &AggregateGroup{Name: "", UUID: "uuid-1"}}, + {Aggregate: &AggregateGroup{Name: "", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"}}, }, }, } err := k8sClient.Create(ctx, hypervisor) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.groups[0].aggregate.name")) }) It("should reject an aggregate with empty UUID", func(ctx SpecContext) { @@ -532,6 +534,7 @@ var _ = Describe("Groups CEL Validation", func() { } err := k8sClient.Create(ctx, hypervisor) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.groups[0].aggregate.uuid")) }) It("should accept an aggregate without metadata", func(ctx SpecContext) { @@ -539,7 +542,7 @@ var _ = Describe("Groups CEL Validation", func() { ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, Spec: HypervisorSpec{ Groups: []Group{ - {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "uuid-1"}}, + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"}}, }, }, } @@ -553,13 +556,19 @@ var _ = Describe("Groups CEL Validation", func() { Groups: []Group{ {Aggregate: &AggregateGroup{ Name: "fast-storage", - UUID: "uuid-1", + UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", Metadata: map[string]string{"ssd": "true"}, }}, }, }, } Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed()) + + created := &Hypervisor{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(hypervisor), created)).To(Succeed()) + Expect(created.Spec.Groups).To(HaveLen(1)) + Expect(created.Spec.Groups[0].Aggregate).NotTo(BeNil()) + Expect(created.Spec.Groups[0].Aggregate.Metadata).To(HaveKeyWithValue("ssd", "true")) }) It("should accept an empty groups list", func(ctx SpecContext) { @@ -578,8 +587,8 @@ var _ = Describe("Group Helper Functions", func() { groups := []Group{ {Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}}, {Trait: &TraitGroup{Name: "COMPUTE_STATUS_DISABLED"}}, - {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "uuid-1", Metadata: map[string]string{"ssd": "true"}}}, - {Aggregate: &AggregateGroup{Name: "slow-storage", UUID: "uuid-2"}}, + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11", Metadata: map[string]string{"ssd": "true"}}}, + {Aggregate: &AggregateGroup{Name: "slow-storage", UUID: "b1ffbc99-9c0b-4ef8-bb6d-6bb9bd380a22"}}, } Context("HasTrait", func() { @@ -605,7 +614,7 @@ var _ = Describe("Group Helper Functions", func() { }) It("should return empty for a list with no traits", func() { - aggs := []Group{{Aggregate: &AggregateGroup{Name: "a", UUID: "u"}}} + aggs := []Group{{Aggregate: &AggregateGroup{Name: "a", UUID: "d0eebc99-9c0b-4ef8-bb6d-6bb9bd380a33"}}} Expect(GetTraits(aggs)).To(BeEmpty()) }) @@ -616,15 +625,15 @@ var _ = Describe("Group Helper Functions", func() { Context("HasAggregate", func() { It("should return true for an existing aggregate UUID", func() { - Expect(HasAggregate(groups, "uuid-1")).To(BeTrue()) + Expect(HasAggregate(groups, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11")).To(BeTrue()) }) It("should return false for a missing aggregate UUID", func() { - Expect(HasAggregate(groups, "uuid-999")).To(BeFalse()) + Expect(HasAggregate(groups, "c2ffbc99-9c0b-4ef8-bb6d-6bb9bd380a99")).To(BeFalse()) }) It("should return false for an empty list", func() { - Expect(HasAggregate(nil, "uuid-1")).To(BeFalse()) + Expect(HasAggregate(nil, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11")).To(BeFalse()) }) }) @@ -633,10 +642,10 @@ var _ = Describe("Group Helper Functions", func() { aggs := GetAggregates(groups) Expect(aggs).To(HaveLen(2)) Expect(aggs[0].Name).To(Equal("fast-storage")) - Expect(aggs[0].UUID).To(Equal("uuid-1")) + Expect(aggs[0].UUID).To(Equal("a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11")) Expect(aggs[0].Metadata).To(HaveKeyWithValue("ssd", "true")) Expect(aggs[1].Name).To(Equal("slow-storage")) - Expect(aggs[1].UUID).To(Equal("uuid-2")) + Expect(aggs[1].UUID).To(Equal("b1ffbc99-9c0b-4ef8-bb6d-6bb9bd380a22")) }) It("should return empty for a list with no aggregates", func() { diff --git a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml index d9c7dcf..c1c3002 100644 --- a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml +++ b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml @@ -175,7 +175,7 @@ spec: minLength: 1 type: string uuid: - minLength: 1 + pattern: ^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$ type: string required: - name From d35f2bc2c5d4fd634b17e7a8dd77ff3025758fbb Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 29 Apr 2026 08:40:11 +0200 Subject: [PATCH 3/3] Fix test UUIDs missed in prior commit The "union rule" tests still used "abc-123" which doesn't match the new RFC 4122 pattern validation. Replace with valid UUIDs. --- api/v1/hypervisor_validation_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/v1/hypervisor_validation_test.go b/api/v1/hypervisor_validation_test.go index 64f50db..5ff5d8c 100644 --- a/api/v1/hypervisor_validation_test.go +++ b/api/v1/hypervisor_validation_test.go @@ -441,7 +441,7 @@ var _ = Describe("Groups CEL Validation", func() { ObjectMeta: metav1.ObjectMeta{Name: hypervisorName.Name}, Spec: HypervisorSpec{ Groups: []Group{ - {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "abc-123"}}, + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"}}, }, }, } @@ -454,7 +454,7 @@ var _ = Describe("Groups CEL Validation", func() { Spec: HypervisorSpec{ Groups: []Group{ {Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}}, - {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "abc-123"}}, + {Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"}}, {Trait: &TraitGroup{Name: "COMPUTE_STATUS_DISABLED"}}, }, }, @@ -469,7 +469,7 @@ var _ = Describe("Groups CEL Validation", func() { Groups: []Group{ { Trait: &TraitGroup{Name: "HW_CPU_X86_AVX2"}, - Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "abc-123"}, + Aggregate: &AggregateGroup{Name: "fast-storage", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11"}, }, }, },