Merge pull request #428 from kstrenkova/cr-update-restarts-pods

Allow CR updates

Author: openshift-merge-bot[bot]

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

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"
@@ -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
+}

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

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,