Merge pull request #168 from kstrenkova/merge-workflow-section-in-webhook

Refactor merging of workflow section

Author: kstrenkova

.zuul.yaml

@@ -86,6 +86,10 @@
           tempestRun:
             includeList: |
               tempest.api.compute.admin.test_flavors.FlavorsAdminTestJSON.test_create_flavor_using_string_ram
+            concurrency: 8
+          tempestconfRun:
+            collectTiming: true
+
       cifmw_test_operator_tempest_extra_images:
         - URL: https://download.cirros-cloud.net/0.6.2/cirros-0.6.2-x86_64-disk.img
           name: cirros-0.6.2-test-operator

api/v1beta1/ansibletest_types.go

@@ -99,8 +99,8 @@ type AnsibleTestSpec struct {
 }
 
 type AnsibleTestWorkflowSpec struct {
-	WorkflowCommonParameters `json:",inline"`
-	CommonOpenstackConfig    `json:",inline"`
+	WorkflowCommonOptions `json:",inline"`
+	CommonOpenstackConfig `json:",inline"`
 
 	// +operator-sdk:csv:customresourcedefinitions:type=spec
 	// +kubebuilder:validation:Required

api/v1beta1/common.go

@@ -133,7 +133,7 @@ type CommonTestStatus struct {
 	NetworkAttachments map[string][]string `json:"networkAttachments,omitempty"`
 }
 
-type WorkflowCommonParameters struct {
+type WorkflowCommonOptions struct {
 	// +operator-sdk:csv:customresourcedefinitions:type=spec
 	// +kubebuilder:validation:optional
 	// +optional

api/v1beta1/tempest_types_workflow.go

@@ -227,8 +227,8 @@ type WorkflowTempestconfRunSpec struct {
 // TempestSpec - configuration of execution of tempest. For specific configuration
 // of tempest see TempestRunSpec and for discover-tempest-config see TempestconfRunSpec.
 type WorkflowTempestSpec struct {
-	WorkflowCommonParameters              `json:",inline"`
-	CommonOpenstackConfig      `json:",inline"`
+	WorkflowCommonOptions `json:",inline"`
+	CommonOpenstackConfig `json:",inline"`
 
 	// The desired amount of resources that should be assigned to each test pod
 	// spawned using the Tempest CR. https://pkg.go.dev/k8s.io/api/core/v1#ResourceRequirements

api/v1beta1/tobiko_types.go

@@ -119,7 +119,7 @@ type TobikoSpec struct {
 }
 
 type TobikoWorkflowSpec struct {
-	WorkflowCommonParameters `json:",inline"`
+	WorkflowCommonOptions `json:",inline"`
 
 	// +kubebuilder:default:={limits: {cpu: "8000m", memory: "8Gi"}, requests: {cpu: "4000m", memory: "4Gi"}}
 	// The desired amount of resources that should be assigned to each test pod

api/v1beta1/zz_generated.deepcopy.go

@@ -22,15 +22,12 @@ import (
 	"fmt"
 	"strconv"
 
-	"reflect"
-
 	"github.com/go-logr/logr"
 	"github.com/openstack-k8s-operators/lib-common/modules/common"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/env"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/helper"
 	testv1beta1 "github.com/openstack-k8s-operators/test-operator/api/v1beta1"
-	v1beta1 "github.com/openstack-k8s-operators/test-operator/api/v1beta1"
 	"github.com/openstack-k8s-operators/test-operator/pkg/ansibletest"
 	corev1 "k8s.io/api/core/v1"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
@@ -129,6 +126,9 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	workflowLength := len(instance.Spec.Workflow)
 	nextAction, nextWorkflowStep, err := r.NextAction(ctx, instance, workflowLength)
+	if nextWorkflowStep < workflowLength {
+		MergeSections(&instance.Spec, instance.Spec.Workflow[nextWorkflowStep])
+	}
 
 	switch nextAction {
 	case Failure:
@@ -206,32 +206,14 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	// Create a new pod
 	mountCerts := r.CheckSecretExists(ctx, instance, "combined-ca-bundle")
 	podName := r.GetPodName(instance, nextWorkflowStep)
-	envVars, workflowOverrideParams := r.PrepareAnsibleEnv(instance, nextWorkflowStep)
+	envVars, workflowOverrideParams := r.PrepareAnsibleEnv(instance)
 	logsPVCName := r.GetPVCLogsName(instance, 0)
 	containerImage, err := r.GetContainerImage(ctx, workflowOverrideParams["ContainerImage"], instance)
-	privileged := r.OverwriteAnsibleWithWorkflow(instance.Spec, "Privileged", "pbool", nextWorkflowStep).(bool)
+	privileged := instance.Spec.Privileged
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	if nextWorkflowStep < len(instance.Spec.Workflow) {
-		if instance.Spec.Workflow[nextWorkflowStep].NodeSelector != nil {
-			instance.Spec.NodeSelector = *instance.Spec.Workflow[nextWorkflowStep].NodeSelector
-		}
-
-		if instance.Spec.Workflow[nextWorkflowStep].Tolerations != nil {
-			instance.Spec.Tolerations = *instance.Spec.Workflow[nextWorkflowStep].Tolerations
-		}
-
-		if instance.Spec.Workflow[nextWorkflowStep].SELinuxLevel != nil {
-			instance.Spec.SELinuxLevel = *instance.Spec.Workflow[nextWorkflowStep].SELinuxLevel
-		}
-
-		if instance.Spec.Workflow[nextWorkflowStep].Resources != nil {
-			instance.Spec.Resources = *instance.Spec.Workflow[nextWorkflowStep].Resources
-		}
-	}
-
 	podDef := ansibletest.Pod(
 		instance,
 		serviceLabels,
@@ -284,82 +266,32 @@ func (r *AnsibleTestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-func (r *Reconciler) OverwriteAnsibleWithWorkflow(
-	instance v1beta1.AnsibleTestSpec,
-	sectionName string,
-	workflowValueType string,
-	workflowStepNum int,
-) interface{} {
-	if len(instance.Workflow)-1 < workflowStepNum {
-		reflected := reflect.ValueOf(instance)
-		fieldValue := reflected.FieldByName(sectionName)
-		return fieldValue.Interface()
-	}
-
-	reflected := reflect.ValueOf(instance)
-	SpecValue := reflected.FieldByName(sectionName).Interface()
-
-	reflected = reflect.ValueOf(instance.Workflow[workflowStepNum])
-	WorkflowValue := reflected.FieldByName(sectionName).Interface()
-
-	if workflowValueType == "pbool" {
-		if val, ok := WorkflowValue.(*bool); ok && val != nil {
-			return *(WorkflowValue.(*bool))
-		}
-		return SpecValue.(bool)
-	} else if workflowValueType == "puint8" {
-		if val, ok := WorkflowValue.(*uint8); ok && val != nil {
-			return *(WorkflowValue.(*uint8))
-		}
-		return SpecValue
-	} else if workflowValueType == "string" {
-		if val, ok := WorkflowValue.(string); ok && val != "" {
-			return WorkflowValue
-		}
-		return SpecValue
-	}
-
-	return nil
-}
-
 // This function prepares env variables for a single workflow step.
 func (r *AnsibleTestReconciler) PrepareAnsibleEnv(
 	instance *testv1beta1.AnsibleTest,
-	step int,
 ) (map[string]env.Setter, map[string]string) {
 	// Prepare env vars
 	envVars := make(map[string]env.Setter)
 	workflowOverrideParams := make(map[string]string)
 
 	// volumes workflow override
-	workflowOverrideParams["WorkloadSSHKeySecretName"] = r.OverwriteAnsibleWithWorkflow(instance.Spec, "WorkloadSSHKeySecretName", "string", step).(string)
-	workflowOverrideParams["ComputesSSHKeySecretName"] = r.OverwriteAnsibleWithWorkflow(instance.Spec, "ComputesSSHKeySecretName", "string", step).(string)
-	workflowOverrideParams["ContainerImage"] = r.OverwriteAnsibleWithWorkflow(instance.Spec, "ContainerImage", "string", step).(string)
+	workflowOverrideParams["WorkloadSSHKeySecretName"] = instance.Spec.WorkloadSSHKeySecretName
+	workflowOverrideParams["ComputesSSHKeySecretName"] = instance.Spec.ComputesSSHKeySecretName
+	workflowOverrideParams["ContainerImage"] = instance.Spec.ContainerImage
 
 	// bool
-	debug := r.OverwriteAnsibleWithWorkflow(instance.Spec, "Debug", "pbool", step).(bool)
+	debug := instance.Spec.Debug
 	if debug {
 		envVars["POD_DEBUG"] = env.SetValue("true")
 	}
 
 	// strings
-	extraVars := r.OverwriteAnsibleWithWorkflow(instance.Spec, "AnsibleExtraVars", "string", step).(string)
-	envVars["POD_ANSIBLE_EXTRA_VARS"] = env.SetValue(extraVars)
-
-	extraVarsFile := r.OverwriteAnsibleWithWorkflow(instance.Spec, "AnsibleVarFiles", "string", step).(string)
-	envVars["POD_ANSIBLE_FILE_EXTRA_VARS"] = env.SetValue(extraVarsFile)
-
-	inventory := r.OverwriteAnsibleWithWorkflow(instance.Spec, "AnsibleInventory", "string", step).(string)
-	envVars["POD_ANSIBLE_INVENTORY"] = env.SetValue(inventory)
-
-	gitRepo := r.OverwriteAnsibleWithWorkflow(instance.Spec, "AnsibleGitRepo", "string", step).(string)
-	envVars["POD_ANSIBLE_GIT_REPO"] = env.SetValue(gitRepo)
-
-	playbookPath := r.OverwriteAnsibleWithWorkflow(instance.Spec, "AnsiblePlaybookPath", "string", step).(string)
-	envVars["POD_ANSIBLE_PLAYBOOK"] = env.SetValue(playbookPath)
-
-	ansibleCollections := r.OverwriteAnsibleWithWorkflow(instance.Spec, "AnsibleCollections", "string", step).(string)
-	envVars["POD_INSTALL_COLLECTIONS"] = env.SetValue(ansibleCollections)
+	envVars["POD_ANSIBLE_EXTRA_VARS"] = env.SetValue(instance.Spec.AnsibleExtraVars)
+	envVars["POD_ANSIBLE_FILE_EXTRA_VARS"] = env.SetValue(instance.Spec.AnsibleVarFiles)
+	envVars["POD_ANSIBLE_INVENTORY"] = env.SetValue(instance.Spec.AnsibleInventory)
+	envVars["POD_ANSIBLE_GIT_REPO"] = env.SetValue(instance.Spec.AnsibleGitRepo)
+	envVars["POD_ANSIBLE_PLAYBOOK"] = env.SetValue(instance.Spec.AnsiblePlaybookPath)
+	envVars["POD_INSTALL_COLLECTIONS"] = env.SetValue(instance.Spec.AnsibleCollections)
 
 	return envVars, workflowOverrideParams
 }

controllers/ansibletest_controller.go

@@ -611,44 +611,6 @@ func (r *Reconciler) setConfigOverwrite(customData map[string]string, configOver
 	}
 }
 
-func (r *Reconciler) OverwriteValueWithWorkflow(
-	instance v1beta1.TobikoSpec,
-	sectionName string,
-	workflowValueType string,
-	workflowStepNum int,
-) interface{} {
-	if len(instance.Workflow)-1 < workflowStepNum {
-		reflected := reflect.ValueOf(instance)
-		fieldValue := reflected.FieldByName(sectionName)
-		return fieldValue.Interface()
-	}
-
-	reflected := reflect.ValueOf(instance)
-	tobikoSpecValue := reflected.FieldByName(sectionName).Interface()
-
-	reflected = reflect.ValueOf(instance.Workflow[workflowStepNum])
-	tobikoWorkflowValue := reflected.FieldByName(sectionName).Interface()
-
-	if workflowValueType == "pbool" {
-		if val, ok := tobikoWorkflowValue.(*bool); ok && val != nil {
-			return *(tobikoWorkflowValue.(*bool))
-		}
-		return tobikoSpecValue.(bool)
-	} else if workflowValueType == "puint8" {
-		if val, ok := tobikoWorkflowValue.(*uint8); ok && val != nil {
-			return *(tobikoWorkflowValue.(*uint8))
-		}
-		return tobikoSpecValue
-	} else if workflowValueType == "string" {
-		if val, ok := tobikoWorkflowValue.(string); ok && val != "" {
-			return tobikoWorkflowValue
-		}
-		return tobikoSpecValue
-	}
-
-	return nil
-}
-
 func GetCommonRbacRules(privileged bool) []rbacv1.PolicyRule {
 	rbacPolicyRule := rbacv1.PolicyRule{
 		APIGroups:     []string{"security.openshift.io"},
@@ -734,3 +696,46 @@ func EnsureCloudsConfigMapExists(
 
 	return ctrl.Result{}, nil
 }
+
+func IsEmpty(value interface{}) bool {
+	if v, ok := value.(reflect.Value); ok {
+		switch v.Kind() {
+		case reflect.String, reflect.Map:
+			return v.Len() == 0
+		case reflect.Ptr, reflect.Interface, reflect.Slice:
+			return v.IsNil()
+		}
+	}
+	return false
+}
+
+// MergeSections iterates through all CR parameters and overrides them
+// with non-empty values from the workflow section of the current step.
+func MergeSections(main interface{}, workflow interface{}) {
+	mReflect := reflect.ValueOf(main).Elem()
+	wReflect := reflect.ValueOf(workflow)
+
+	for i := 0; i < mReflect.NumField(); i++ {
+		name := mReflect.Type().Field(i).Name
+		mValue := mReflect.Field(i)
+		wValue := wReflect.FieldByName(name)
+
+		if mValue.Kind() == reflect.Struct && wValue.Kind() != reflect.Ptr {
+			switch name {
+			case "CommonOptions":
+				wValue := wReflect.FieldByName("WorkflowCommonOptions")
+				MergeSections(mValue.Addr().Interface(), wValue.Interface())
+			case "TempestRun", "TempestconfRun":
+				MergeSections(mValue.Addr().Interface(), wValue.Interface())
+			}
+			continue
+		}
+
+		if wValue.IsValid() && !IsEmpty(wValue) {
+			if wValue.Kind() == reflect.Ptr && mValue.Kind() != reflect.Ptr {
+				wValue = wValue.Elem()
+			}
+			mValue.Set(wValue)
+		}
+	}
+}