From 4bcfe5f70a40a2cd8aaf7c214f698dff66a88c7f Mon Sep 17 00:00:00 2001 From: Katarina Strenkova Date: Tue, 24 Mar 2026 08:18:59 -0400 Subject: [PATCH] Allow CR updates This PR introduces new test-operator functionality that will allow users to update their CRs and simultaneously restart the the pod. Currently, any change to the CR will not apply in the pod. The only way a user can use updated CR is to remove the pod and create a new one. --- api/v1beta1/ansibletest_webhook.go | 12 ++- api/v1beta1/common_webhook.go | 12 +++ api/v1beta1/horizontest_webhook.go | 12 ++- api/v1beta1/tempest_webhook.go | 12 +-- api/v1beta1/tobiko_webhook.go | 11 ++- internal/ansibletest/pod.go | 3 +- internal/controller/ansibletest_controller.go | 3 +- internal/controller/common.go | 75 +++++++++++++++++++ internal/controller/common_controller.go | 17 ++++- internal/controller/horizontest_controller.go | 3 +- internal/horizontest/pod.go | 3 +- 11 files changed, 141 insertions(+), 22 deletions(-) diff --git a/api/v1beta1/ansibletest_webhook.go b/api/v1beta1/ansibletest_webhook.go index 859f5a50..c20378a8 100644 --- a/api/v1beta1/ansibletest_webhook.go +++ b/api/v1beta1/ansibletest_webhook.go @@ -23,6 +23,8 @@ limitations under the License. package v1beta1 import ( + "errors" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -74,8 +76,14 @@ func (r *AnsibleTest) ValidateCreate() (admission.Warnings, error) { func (r *AnsibleTest) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { ansibletestlog.Info("validate update", "name", r.Name) - // TODO(user): fill in your validation logic upon object update. - return nil, nil + oldAnsibleTest, ok := old.(*AnsibleTest) + if !ok || oldAnsibleTest == nil { + return nil, errors.New("unable to convert existing object") + } + + allWarnings := admission.Warnings{} + allWarnings = CheckSpecUpdated(allWarnings, oldAnsibleTest.Spec, r.Spec, r.Kind) + return allWarnings, nil } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type diff --git a/api/v1beta1/common_webhook.go b/api/v1beta1/common_webhook.go index 29e07c1f..91c25ba1 100644 --- a/api/v1beta1/common_webhook.go +++ b/api/v1beta1/common_webhook.go @@ -4,6 +4,7 @@ import ( "fmt" "reflect" + "github.com/google/go-cmp/cmp" "github.com/openstack-k8s-operators/lib-common/modules/common/util" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" @@ -75,6 +76,9 @@ const ( "set to true. Please, consider setting %[1]s.Spec.SELinuxLevel. This " + "ensures that the copying of the logs to the PV is completed without any " + "complications." + + // WarnSpecUpdated + WarnSpecUpdated = "%s CR updated. The associated pods will be recreated to apply changes." ) const ( @@ -193,3 +197,11 @@ func BuildValidationError(kind, name string, errs field.ErrorList) error { } return nil } + +// CheckSpecUpdated returns warning if spec has changed +func CheckSpecUpdated(allWarn admission.Warnings, oldSpec, newSpec interface{}, kind string) admission.Warnings { + if !cmp.Equal(oldSpec, newSpec) { + allWarn = append(allWarn, fmt.Sprintf(WarnSpecUpdated, kind)) + } + return allWarn +} diff --git a/api/v1beta1/horizontest_webhook.go b/api/v1beta1/horizontest_webhook.go index 61565cf8..391fe744 100644 --- a/api/v1beta1/horizontest_webhook.go +++ b/api/v1beta1/horizontest_webhook.go @@ -23,6 +23,8 @@ limitations under the License. package v1beta1 import ( + "errors" + "k8s.io/apimachinery/pkg/runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -59,8 +61,14 @@ func (r *HorizonTest) ValidateCreate() (admission.Warnings, error) { func (r *HorizonTest) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { horizontestlog.Info("validate update", "name", r.Name) - // TODO(user): fill in your validation logic upon object update. - return nil, nil + oldHorizonTest, ok := old.(*HorizonTest) + if !ok || oldHorizonTest == nil { + return nil, errors.New("unable to convert existing object") + } + + allWarnings := admission.Warnings{} + allWarnings = CheckSpecUpdated(allWarnings, oldHorizonTest.Spec, r.Spec, r.Kind) + return allWarnings, nil } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type diff --git a/api/v1beta1/tempest_webhook.go b/api/v1beta1/tempest_webhook.go index 53f84596..1b924e18 100644 --- a/api/v1beta1/tempest_webhook.go +++ b/api/v1beta1/tempest_webhook.go @@ -26,7 +26,6 @@ import ( "errors" "fmt" - "github.com/google/go-cmp/cmp" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -108,14 +107,9 @@ func (r *Tempest) ValidateUpdate(old runtime.Object) (admission.Warnings, error) return nil, errors.New("unable to convert existing object") } - if !cmp.Equal(oldTempest.Spec, r.Spec) { - warnings := admission.Warnings{} - warnings = append(warnings, "You are updating an already existing instance of a "+ - "Tempest CR! Be aware that changes won't be applied.") - - return warnings, errors.New("updating an existing Tempest CR is not supported") - } - return nil, nil + allWarnings := admission.Warnings{} + allWarnings = CheckSpecUpdated(allWarnings, oldTempest.Spec, r.Spec, r.Kind) + return allWarnings, nil } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type diff --git a/api/v1beta1/tobiko_webhook.go b/api/v1beta1/tobiko_webhook.go index 87a4999d..3ce791f4 100644 --- a/api/v1beta1/tobiko_webhook.go +++ b/api/v1beta1/tobiko_webhook.go @@ -23,6 +23,7 @@ limitations under the License. package v1beta1 import ( + "errors" "fmt" "k8s.io/apimachinery/pkg/runtime" @@ -82,8 +83,14 @@ func (r *Tobiko) ValidateCreate() (admission.Warnings, error) { func (r *Tobiko) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { tobikolog.Info("validate update", "name", r.Name) - // TODO(user): fill in your validation logic upon object update. - return nil, nil + oldTobiko, ok := old.(*Tobiko) + if !ok || oldTobiko == nil { + return nil, errors.New("unable to convert existing object") + } + + allWarnings := admission.Warnings{} + allWarnings = CheckSpecUpdated(allWarnings, oldTobiko.Spec, r.Spec, r.Kind) + return allWarnings, nil } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type diff --git a/internal/ansibletest/pod.go b/internal/ansibletest/pod.go index 2c7fdea3..17637de4 100644 --- a/internal/ansibletest/pod.go +++ b/internal/ansibletest/pod.go @@ -12,6 +12,7 @@ import ( func Pod( instance *testv1beta1.AnsibleTest, labels map[string]string, + annotations map[string]string, podName string, logsPVCName string, mountCerts bool, @@ -20,7 +21,7 @@ func Pod( containerImage string, ) *corev1.Pod { return util.BuildTestPod( - nil, // No annotations + annotations, PodCapabilities, containerImage, instance.Name, diff --git a/internal/controller/ansibletest_controller.go b/internal/controller/ansibletest_controller.go index de436f5e..67e9c80c 100644 --- a/internal/controller/ansibletest_controller.go +++ b/internal/controller/ansibletest_controller.go @@ -96,7 +96,7 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request) func (r *AnsibleTestReconciler) buildAnsibleTestPod( ctx context.Context, instance *testv1beta1.AnsibleTest, - labels, _ map[string]string, + labels, annotations map[string]string, workflowStepIndex int, pvcIndex int, ) (*corev1.Pod, error) { @@ -114,6 +114,7 @@ func (r *AnsibleTestReconciler) buildAnsibleTestPod( return ansibletest.Pod( instance, labels, + annotations, podName, logsPVCName, mountCerts, diff --git a/internal/controller/common.go b/internal/controller/common.go index b6c174bd..b893fb2e 100644 --- a/internal/controller/common.go +++ b/internal/controller/common.go @@ -2,6 +2,7 @@ package controller import ( "context" + "encoding/json" "errors" "fmt" "reflect" @@ -254,6 +255,11 @@ func (r *Reconciler) GetLastPod( maxPodWorkflowStep := 0 for _, pod := range podList.Items { + // Skip pods that are being deleted + if pod.DeletionTimestamp != nil { + continue + } + workflowStep, err := strconv.Atoi(pod.Labels[workflowStepLabel]) if err != nil { return &corev1.Pod{}, err @@ -914,3 +920,72 @@ func MergeSections(main interface{}, workflow interface{}) { } } } + +// CalculateConfigHash calculates a hash of the entire Spec to detect any changes +func CalculateConfigHash(instance client.Object) string { + v := reflect.ValueOf(instance) + spec, err := SafetyCheck(v, "Spec") + if err != nil { + return "" + } + + data, err := json.Marshal(spec.Interface()) + if err != nil { + return "" + } + + hash := sha256.Sum256(data) + return fmt.Sprintf("%x", hash[:8]) +} + +// CheckConfigChange checks if the spec has changed and recreates all pods related to the instance if needed +func (r *Reconciler) CheckConfigChange( + ctx context.Context, + instance client.Object, + newHash string, +) (ctrl.Result, error) { + Log := r.GetLogger(ctx) + + if newHash == "" { + return ctrl.Result{}, nil + } + + labels := map[string]string{instanceNameLabel: instance.GetName()} + podList := &corev1.PodList{} + err := r.Client.List(ctx, podList, + client.InNamespace(instance.GetNamespace()), + client.MatchingLabels(labels)) + if err != nil { + return ctrl.Result{}, err + } + + var currentHash string + for _, pod := range podList.Items { + if pod.DeletionTimestamp != nil { + continue + } + + if hash := pod.Annotations["test.openstack.org/config-hash"]; hash != "" { + currentHash = hash + break + } + } + + if currentHash == "" || currentHash == newHash { + return ctrl.Result{}, nil + } + + for _, pod := range podList.Items { + if pod.DeletionTimestamp != nil { + continue + } + + Log.Info("Configuration changed, deleting pod", "pod", pod.Name) + + if err := r.Client.Delete(ctx, &pod); err != nil && !k8s_errors.IsNotFound(err) { + return ctrl.Result{}, err + } + } + + return ctrl.Result{Requeue: true}, nil +} diff --git a/internal/controller/common_controller.go b/internal/controller/common_controller.go index ea7ebdb8..9099b820 100644 --- a/internal/controller/common_controller.go +++ b/internal/controller/common_controller.go @@ -184,6 +184,13 @@ func CommonReconcile[T TestResource]( return ctrl.Result{}, err } + // Check for config changes and handle pod recreation + configHash := CalculateConfigHash(instance) + ctrlResult, err := r.CheckConfigChange(ctx, instance, configHash) + if err != nil || (ctrlResult != ctrl.Result{}) { + return ctrlResult, err + } + // Apply workflow step overrides to the base spec if config.SupportsWorkflow && workflowStepIndex < workflowLength { spec := config.GetSpec(instance) @@ -271,7 +278,7 @@ func CommonReconcile[T TestResource]( } // Create PersistentVolumeClaim - ctrlResult, err := r.EnsureLogsPVCExists( + ctrlResult, err = r.EnsureLogsPVCExists( ctx, instance, helper, @@ -285,6 +292,9 @@ func CommonReconcile[T TestResource]( return ctrlResult, nil } + serviceAnnotations := make(map[string]string) + serviceAnnotations["test.openstack.org/config-hash"] = configHash + // Generate ConfigMaps containing test configuration if config.NeedsConfigMaps { err = config.GenerateServiceConfigMaps(ctx, helper, serviceLabels, instance, workflowStepIndex) @@ -300,7 +310,6 @@ func CommonReconcile[T TestResource]( conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage) } - var serviceAnnotations map[string]string if config.NeedsNetworkAttachments { annotations, ctrlResult, err := r.EnsureNetworkAttachments( ctx, @@ -313,7 +322,9 @@ func CommonReconcile[T TestResource]( if err != nil || (ctrlResult != ctrl.Result{}) { return ctrlResult, err } - serviceAnnotations = annotations + for k, v := range annotations { + serviceAnnotations[k] = v + } } // Build pod diff --git a/internal/controller/horizontest_controller.go b/internal/controller/horizontest_controller.go index ddef57ce..f78729d1 100644 --- a/internal/controller/horizontest_controller.go +++ b/internal/controller/horizontest_controller.go @@ -96,7 +96,7 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request) func (r *HorizonTestReconciler) buildHorizonTestPod( ctx context.Context, instance *testv1beta1.HorizonTest, - labels, _ map[string]string, + labels, annotations map[string]string, workflowStepIndex int, pvcIndex int, ) (*corev1.Pod, error) { @@ -115,6 +115,7 @@ func (r *HorizonTestReconciler) buildHorizonTestPod( return horizontest.Pod( instance, labels, + annotations, podName, logsPVCName, mountCerts, diff --git a/internal/horizontest/pod.go b/internal/horizontest/pod.go index 62154e9c..b0ca9100 100644 --- a/internal/horizontest/pod.go +++ b/internal/horizontest/pod.go @@ -12,6 +12,7 @@ import ( func Pod( instance *testv1beta1.HorizonTest, labels map[string]string, + annotations map[string]string, podName string, logsPVCName string, mountCerts bool, @@ -20,7 +21,7 @@ func Pod( containerImage string, ) *corev1.Pod { return util.BuildTestPod( - nil, // No annotations + annotations, PodCapabilities, containerImage, instance.Name,