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.

Author: kstrenkova

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

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 pod 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
+}

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

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

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

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,

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,

internal/controller/common.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"reflect"
@@ -31,6 +32,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 const (
@@ -253,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
@@ -489,8 +496,8 @@ func (r *Reconciler) EnsureLogsPVCExists(
 }
 
 // GetLogger returns the logger instance
-func (r *Reconciler) GetLogger() logr.Logger {
-	return r.Log
+func (r *Reconciler) GetLogger(ctx context.Context) logr.Logger {
+	return log.FromContext(ctx)
 }
 
 // GetScheme returns the runtime scheme
@@ -555,7 +562,7 @@ func (r *Reconciler) AcquireLock(
 
 // ReleaseLock releases the lock for the given instance
 func (r *Reconciler) ReleaseLock(ctx context.Context, instance client.Object) (bool, error) {
-	Log := r.GetLogger()
+	Log := r.GetLogger(ctx)
 
 	cm, err := r.GetLockInfo(ctx, instance)
 	if err != nil && k8s_errors.IsNotFound(err) {
@@ -913,3 +920,78 @@ 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
+		}
+
+		currentHash = pod.Annotations["test.openstack.org/config-hash"]
+		if currentHash != "" && currentHash != newHash {
+			break
+		}
+		currentHash = ""
+	}
+
+	if currentHash == "" || currentHash == newHash {
+		return ctrl.Result{}, nil
+	}
+
+	Log.Info("Configuration changed, deleting all related pods for recreation",
+		"instance", instance.GetName(),
+		"currentHash", currentHash,
+		"newHash", newHash)
+
+	for _, pod := range podList.Items {
+		if pod.DeletionTimestamp != nil {
+			continue
+		}
+
+		Log.Info("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
+}

internal/controller/common_controller.go

@@ -180,6 +180,16 @@ func CommonReconcile[T TestResource](
 	}
 
 	nextAction, workflowStepIndex, err := r.NextAction(ctx, instance, workflowLength)
+	if nextAction == Failure {
+		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 {
@@ -200,9 +210,6 @@ func CommonReconcile[T TestResource](
 	}
 
 	switch nextAction {
-	case Failure:
-		return ctrl.Result{}, err
-
 	case Wait:
 		Log.Info(InfoWaitingOnPod)
 		return ctrl.Result{RequeueAfter: RequeueAfterValue}, nil
@@ -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

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,