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,