From 06083a90bae5ffa9b647f165c8023b24ccd6326a Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Thu, 17 Sep 2026 15:19:31 +0200 Subject: [PATCH] Add SSA conditions helpers in internal/utils Introduce ConditionFromStatus and SetApplyConfigurationStatusCondition as the foundation for migrating controllers to server-side apply of status subresources. ConditionFromStatus converts a metav1.Condition into a ConditionApplyConfiguration while preserving all fields, including LastTransitionTime and ObservedGeneration. SetApplyConfigurationStatusCondition mirrors the relevant semantics of meta.SetStatusCondition on apply-configuration slices: it appends or updates conditions by Type, refreshes LastTransitionTime only when Status changes, and guards against nil or empty condition types. Together they enable per-condition seeding so each controller claims field ownership only over the conditions it manages, without disturbing conditions owned by other field managers on Apply. Signed-off-by: Fabian Wiesel --- applyconfigurations/api/v1/cell.go | 2 + .../api/v1/domaincapabilities.go | 3 + applyconfigurations/api/v1/eviction.go | 2 +- applyconfigurations/api/v1/hypervisor.go | 2 +- applyconfigurations/api/v1/hypervisorspec.go | 2 + .../api/v1/hypervisorstatus.go | 2 + .../crds/kvm.cloud.sap_evictions.yaml | 2 +- .../crds/kvm.cloud.sap_hypervisors.yaml | 2 +- internal/utils/conditions.go | 102 ++++++ internal/utils/conditions_test.go | 294 ++++++++++++++++++ 10 files changed, 409 insertions(+), 4 deletions(-) create mode 100644 internal/utils/conditions.go create mode 100644 internal/utils/conditions_test.go diff --git a/applyconfigurations/api/v1/cell.go b/applyconfigurations/api/v1/cell.go index 531a7eea..1fcf0643 100644 --- a/applyconfigurations/api/v1/cell.go +++ b/applyconfigurations/api/v1/cell.go @@ -21,6 +21,7 @@ type CellApplyConfiguration struct { // Note that this capacity does not include the applied overcommit ratios, // and represents the actual capacity of the cell. Use the effective capacity // field to get the capacity considering the applied overcommit ratios. + // Capacity map[apiv1.ResourceName]resource.Quantity `json:"capacity,omitempty"` // Auto-discovered capacity of this cell, considering the // applied overcommit ratios. @@ -31,6 +32,7 @@ type CellApplyConfiguration struct { // // If the overcommit ratio results in a fractional effective capacity, the // effective capacity is expected to be rounded down. + // EffectiveCapacity map[apiv1.ResourceName]resource.Quantity `json:"effectiveCapacity,omitempty"` } diff --git a/applyconfigurations/api/v1/domaincapabilities.go b/applyconfigurations/api/v1/domaincapabilities.go index aec3d0c9..b70dc41d 100644 --- a/applyconfigurations/api/v1/domaincapabilities.go +++ b/applyconfigurations/api/v1/domaincapabilities.go @@ -24,6 +24,7 @@ type DomainCapabilitiesApplyConfiguration struct { // // // The corresponding entries in this list would be "video" and "video/nvidia". + // SupportedDevices []string `json:"supportedDevices,omitempty"` // Supported cpu modes for domains. // @@ -36,6 +37,7 @@ type DomainCapabilitiesApplyConfiguration struct { // // The corresponding entries in this list would be "host-passthrough" and // "host-passthrough/migratable". + // SupportedCpuModes []string `json:"supportedCpuModes,omitempty"` // Supported features for domains, such as "sev" or "sgx". // @@ -47,6 +49,7 @@ type DomainCapabilitiesApplyConfiguration struct { // // // Would correspond to the entries "sev" and "sgx" in this list. + // SupportedFeatures []string `json:"supportedFeatures,omitempty"` } diff --git a/applyconfigurations/api/v1/eviction.go b/applyconfigurations/api/v1/eviction.go index b1b90c30..36caab49 100644 --- a/applyconfigurations/api/v1/eviction.go +++ b/applyconfigurations/api/v1/eviction.go @@ -16,7 +16,7 @@ import ( // // Eviction is the Schema for the evictions API type EvictionApplyConfiguration struct { - metav1.TypeMetaApplyConfiguration `json:",inline"` + metav1.TypeMetaApplyConfiguration `json:""` *metav1.ObjectMetaApplyConfiguration `json:"metadata,omitempty"` Spec *EvictionSpecApplyConfiguration `json:"spec,omitempty"` Status *EvictionStatusApplyConfiguration `json:"status,omitempty"` diff --git a/applyconfigurations/api/v1/hypervisor.go b/applyconfigurations/api/v1/hypervisor.go index f0eecc72..7481882b 100644 --- a/applyconfigurations/api/v1/hypervisor.go +++ b/applyconfigurations/api/v1/hypervisor.go @@ -16,7 +16,7 @@ import ( // // Hypervisor is the Schema for the hypervisors API type HypervisorApplyConfiguration struct { - metav1.TypeMetaApplyConfiguration `json:",inline"` + metav1.TypeMetaApplyConfiguration `json:""` *metav1.ObjectMetaApplyConfiguration `json:"metadata,omitempty"` Spec *HypervisorSpecApplyConfiguration `json:"spec,omitempty"` Status *HypervisorStatusApplyConfiguration `json:"status,omitempty"` diff --git a/applyconfigurations/api/v1/hypervisorspec.go b/applyconfigurations/api/v1/hypervisorspec.go index ab539e17..a584d186 100644 --- a/applyconfigurations/api/v1/hypervisorspec.go +++ b/applyconfigurations/api/v1/hypervisorspec.go @@ -40,6 +40,7 @@ type HypervisorSpecApplyConfiguration struct { // 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. + // Groups []GroupApplyConfiguration `json:"groups,omitempty"` // AllowedProjects defines which openstack projects are allowed to schedule // instances on this hypervisor. The values of this list should be project @@ -67,6 +68,7 @@ type HypervisorSpecApplyConfiguration struct { // the effective capacity is expected to be rounded down. This allows // gradually adjusting the hypervisor capacity. // + // // It is validated that all overcommit ratios are greater than or equal to // 1.0, if specified. For this we don't need extra validating webhooks. // See: https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/#crd-transition-rules diff --git a/applyconfigurations/api/v1/hypervisorstatus.go b/applyconfigurations/api/v1/hypervisorstatus.go index 93e4feae..5585e3de 100644 --- a/applyconfigurations/api/v1/hypervisorstatus.go +++ b/applyconfigurations/api/v1/hypervisorstatus.go @@ -36,6 +36,7 @@ type HypervisorStatusApplyConfiguration struct { // and represents the actual capacity of the hypervisor. Use the // effective capacity field to get the capacity considering the applied // overcommit ratios. + // Capacity map[apiv1.ResourceName]resource.Quantity `json:"capacity,omitempty"` // Auto-discovered capacity of the hypervisor, considering the // applied overcommit ratios. @@ -46,6 +47,7 @@ type HypervisorStatusApplyConfiguration struct { // // If the overcommit ratio results in a fractional effective capacity, the // effective capacity is expected to be rounded down. + // EffectiveCapacity map[apiv1.ResourceName]resource.Quantity `json:"effectiveCapacity,omitempty"` // Auto-discovered cells on this hypervisor. Cells []CellApplyConfiguration `json:"cells,omitempty"` diff --git a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_evictions.yaml b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_evictions.yaml index 81a39885..582c7628 100644 --- a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_evictions.yaml +++ b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_evictions.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.21.0 + controller-gen.kubebuilder.io/version: v0.22.0 name: evictions.kvm.cloud.sap spec: group: kvm.cloud.sap 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 e01399ab..8f418e33 100644 --- a/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml +++ b/charts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.21.0 + controller-gen.kubebuilder.io/version: v0.22.0 name: hypervisors.kvm.cloud.sap spec: group: kvm.cloud.sap diff --git a/internal/utils/conditions.go b/internal/utils/conditions.go new file mode 100644 index 00000000..3bd3037d --- /dev/null +++ b/internal/utils/conditions.go @@ -0,0 +1,102 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" +) + +// ConditionFromStatus converts a single metav1.Condition into a +// *k8sacmetav1.ConditionApplyConfiguration, preserving all fields verbatim +// (including LastTransitionTime and ObservedGeneration). +// +// Controllers must seed their apply-config payload with the conditions they +// own before mutating them. With +listType=map / +listMapKey=type on the CRD, +// SSA merges conditions by the "type" key: conditions owned by other field +// managers are left untouched, and conditions previously written by this +// field manager that are absent from the payload are pruned. Therefore each +// controller should include all conditions it has previously set — and only +// those — in every Apply call. +func ConditionFromStatus(c metav1.Condition) *k8sacmetav1.ConditionApplyConfiguration { + return &k8sacmetav1.ConditionApplyConfiguration{ + Type: &c.Type, + Status: &c.Status, + Reason: &c.Reason, + Message: &c.Message, + LastTransitionTime: &c.LastTransitionTime, + ObservedGeneration: &c.ObservedGeneration, + } +} + +// SetApplyConfigurationStatusCondition sets the corresponding condition in +// conditions to newCondition. It mirrors the behaviour of +// k8s.io/apimachinery/pkg/api/meta.SetStatusCondition: +// +// 1. If a condition of the specified type does not exist, newCondition is +// appended. LastTransitionTime is set to now if not provided. +// 2. If a condition of the specified type already exists, all fields are +// updated. LastTransitionTime is updated to now (or the provided value) +// only when Status changes; otherwise the existing LastTransitionTime is +// preserved. +func SetApplyConfigurationStatusCondition(conditions *[]k8sacmetav1.ConditionApplyConfiguration, newCondition k8sacmetav1.ConditionApplyConfiguration) { + if conditions == nil { + return + } + if newCondition.Type == nil || *newCondition.Type == "" { + return + } + + // Find existing entry by type + var existing *k8sacmetav1.ConditionApplyConfiguration + for i := range *conditions { + if (*conditions)[i].Type != nil && *(*conditions)[i].Type == *newCondition.Type { + existing = &(*conditions)[i] + break + } + } + + if existing == nil { + // New condition: set LastTransitionTime if not provided + if newCondition.LastTransitionTime == nil || newCondition.LastTransitionTime.IsZero() { + now := metav1.Now() + newCondition.LastTransitionTime = &now + } + *conditions = append(*conditions, newCondition) + return + } + + // Existing condition: update fields, handle LastTransitionTime + statusChanged := (existing.Status == nil) != (newCondition.Status == nil) || + (existing.Status != nil && *existing.Status != *newCondition.Status) + + if statusChanged { + existing.Status = newCondition.Status + if newCondition.LastTransitionTime != nil && !newCondition.LastTransitionTime.IsZero() { + existing.LastTransitionTime = newCondition.LastTransitionTime + } else { + now := metav1.Now() + existing.LastTransitionTime = &now + } + } + // When status is unchanged, LastTransitionTime is intentionally preserved. + + existing.Reason = newCondition.Reason + existing.Message = newCondition.Message + existing.ObservedGeneration = newCondition.ObservedGeneration +} diff --git a/internal/utils/conditions_test.go b/internal/utils/conditions_test.go new file mode 100644 index 00000000..a855d3f3 --- /dev/null +++ b/internal/utils/conditions_test.go @@ -0,0 +1,294 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" +) + +func TestUtils(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Utils Suite") +} + +var _ = Describe("ConditionFromStatus", func() { + It("copies all fields verbatim", func() { + ts := metav1.NewTime(time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)) + gen := int64(42) + c := metav1.Condition{ + Type: "Ready", + Status: metav1.ConditionTrue, + Reason: "AllGood", + Message: "everything is fine", + LastTransitionTime: ts, + ObservedGeneration: gen, + } + got := ConditionFromStatus(c) + Expect(*got.Type).To(Equal("Ready")) + Expect(*got.Status).To(Equal(metav1.ConditionTrue)) + Expect(*got.Reason).To(Equal("AllGood")) + Expect(*got.Message).To(Equal("everything is fine")) + Expect(*got.LastTransitionTime).To(Equal(ts)) + Expect(*got.ObservedGeneration).To(Equal(gen)) + }) + + It("includes ObservedGeneration even when zero", func() { + c := metav1.Condition{Type: "T", Status: metav1.ConditionFalse, Reason: "R"} + got := ConditionFromStatus(c) + Expect(got.ObservedGeneration).NotTo(BeNil()) + Expect(*got.ObservedGeneration).To(Equal(int64(0))) + }) +}) + +var _ = Describe("SetApplyConfigurationStatusCondition", func() { + ptr := func(s string) *string { return &s } + condStatus := func(s metav1.ConditionStatus) *metav1.ConditionStatus { return &s } + + Describe("nil / empty-type guards", func() { + It("does not panic on nil slice pointer", func() { + SetApplyConfigurationStatusCondition(nil, + *k8sacmetav1.Condition().WithType("T").WithStatus(metav1.ConditionTrue).WithReason("R")) + }) + + It("does not modify the slice when Type is nil", func() { + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + k8sacmetav1.ConditionApplyConfiguration{}) + Expect(conditions).To(BeEmpty()) + }) + + It("does not modify the slice when Type is empty string", func() { + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition().WithType("")) + Expect(conditions).To(BeEmpty()) + }) + }) + + Describe("appending a new condition", func() { + It("appends when the type is not yet present", func() { + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK")) + Expect(conditions).To(HaveLen(1)) + Expect(*conditions[0].Type).To(Equal("Ready")) + }) + + It("sets LastTransitionTime to now when not provided", func() { + before := time.Now() + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("T"). + WithStatus(metav1.ConditionTrue). + WithReason("R")) + Expect(conditions[0].LastTransitionTime).NotTo(BeNil()) + Expect(conditions[0].LastTransitionTime.Time).To(BeTemporally(">=", before)) + }) + + It("sets LastTransitionTime to now when a zero time is provided", func() { + before := time.Now() + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("T"). + WithStatus(metav1.ConditionTrue). + WithLastTransitionTime(metav1.Time{})) + Expect(conditions[0].LastTransitionTime).NotTo(BeNil()) + Expect(conditions[0].LastTransitionTime.Time).To(BeTemporally(">=", before)) + }) + + It("preserves a caller-supplied LastTransitionTime", func() { + ts := metav1.NewTime(time.Date(2020, 6, 1, 0, 0, 0, 0, time.UTC)) + conditions := []k8sacmetav1.ConditionApplyConfiguration{} + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("T"). + WithStatus(metav1.ConditionTrue). + WithReason("R"). + WithLastTransitionTime(ts)) + Expect(*conditions[0].LastTransitionTime).To(Equal(ts)) + }) + }) + + Describe("updating an existing condition", func() { + var ( + oldTS metav1.Time + conditions []k8sacmetav1.ConditionApplyConfiguration + ) + + BeforeEach(func() { + oldTS = metav1.NewTime(time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)) + conditions = []k8sacmetav1.ConditionApplyConfiguration{ + { + Type: ptr("Ready"), + Status: condStatus(metav1.ConditionFalse), + Reason: ptr("NotReady"), + Message: ptr("waiting"), + LastTransitionTime: &oldTS, + }, + } + }) + + It("updates Status and refreshes LastTransitionTime when Status changes", func() { + before := time.Now() + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK"). + WithMessage("done")) + Expect(*conditions[0].Status).To(Equal(metav1.ConditionTrue)) + Expect(conditions[0].LastTransitionTime.Time).To(BeTemporally(">=", before)) + }) + + It("preserves LastTransitionTime when Status is unchanged", func() { + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("StillNotReady"). + WithMessage("still waiting")) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("preserves the condition when nothing changed", func() { + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting")) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("updates Reason independently of Status", func() { + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("DifferentReason"). + WithMessage("waiting")) + Expect(*conditions[0].Reason).To(Equal("DifferentReason")) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("uses a caller-supplied LastTransitionTime when Status changes", func() { + newTS := metav1.NewTime(time.Date(2025, 6, 1, 0, 0, 0, 0, time.UTC)) + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK"). + WithLastTransitionTime(newTS)) + Expect(*conditions[0].LastTransitionTime).To(Equal(newTS)) + }) + + It("sets LastTransitionTime to now when a zero time is provided on Status change", func() { + before := time.Now() + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithLastTransitionTime(metav1.Time{})) + Expect(conditions[0].LastTransitionTime.Time).To(BeTemporally(">=", before)) + }) + + It("does not affect other conditions in the slice", func() { + otherTS := metav1.NewTime(time.Date(2019, 1, 1, 0, 0, 0, 0, time.UTC)) + conditions = append(conditions, k8sacmetav1.ConditionApplyConfiguration{ + Type: ptr("Other"), + Status: condStatus(metav1.ConditionTrue), + LastTransitionTime: &otherTS, + }) + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionTrue). + WithReason("OK")) + Expect(*conditions[1].LastTransitionTime).To(Equal(otherTS)) + }) + + It("updates ObservedGeneration", func() { + gen := int64(5) + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting"). + WithObservedGeneration(gen)) + Expect(*conditions[0].ObservedGeneration).To(Equal(gen)) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("updates an existing nil Status", func() { + conditions[0].Status = nil + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting")) + Expect(*conditions[0].Status).To(Equal(metav1.ConditionFalse)) + }) + + It("preserves LastTransitionTime when both Status values are nil", func() { + conditions[0].Status = nil + SetApplyConfigurationStatusCondition(&conditions, + k8sacmetav1.ConditionApplyConfiguration{ + Type: ptr("Ready"), + Reason: ptr("NotReady"), + Message: ptr("waiting"), + // Status intentionally nil + }) + Expect(*conditions[0].LastTransitionTime).To(Equal(oldTS)) + }) + + It("updates an existing nil Reason", func() { + conditions[0].Reason = nil + SetApplyConfigurationStatusCondition(&conditions, + *k8sacmetav1.Condition(). + WithType("Ready"). + WithStatus(metav1.ConditionFalse). + WithReason("NotReady"). + WithMessage("waiting")) + Expect(*conditions[0].Reason).To(Equal("NotReady")) + }) + + It("clears an existing Message when the new Message is nil", func() { + SetApplyConfigurationStatusCondition(&conditions, + k8sacmetav1.ConditionApplyConfiguration{ + Type: ptr("Ready"), + Status: condStatus(metav1.ConditionFalse), + Reason: ptr("NotReady"), + // Message intentionally nil + }) + Expect(conditions[0].Message).To(BeNil()) + }) + }) +})