Move merging of workflow section into webhook #168

kstrenkova · 2024-08-12T18:34:28Z

Currently, the merging of workflow section is done in controllers of related CRs. We want to move the merging logic into webhooks to make the process cleaner.

Depends on: #160

openshift-ci · 2024-08-12T18:34:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-08-12T18:34:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kstrenkova

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kstrenkova]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lpiwowar

@kstrenkova, this is really nice progress! 🎉 I say we schedule a call and discuss details but generally speaking I think we can make this work. I've just tried to test this locally. I had to make some minor tweaks (see comments) for the merging to work on the TempestRun section on my machine.

Anyway, really good job! 👍 Even when you do not manage to finish this task and somebody will be working on it instead of you then you still laid a really good foundation here!

From our last conversation I was under the impression that this won't work. I think this will still require 2 - 3 PR updates and reviews but it is on a good path.

api/v1beta1/tempest_webhook.go

lpiwowar · 2024-09-24T12:57:12Z

controllers/tempest_controller.go

+func getTempestRun(instance *testv1beta1.Tempest, workflowStepNum int) testv1beta1.TempestRunSpec {
+	if workflowStepNum < len(instance.Spec.Workflow) {
+		wtRun := instance.Spec.Workflow[workflowStepNum].TempestRun
+		tRun := changeType(wtRun)
+		return tRun
+	}
+	return instance.Spec.TempestRun
+}
+
+func changeType(wtRun testv1beta1.WorkflowTempestRunSpec) testv1beta1.TempestRunSpec {
+	var tRun testv1beta1.TempestRunSpec
+
+	wtReflected := reflect.ValueOf(wtRun)
+	tReflected := reflect.ValueOf(&tRun).Elem()
+
+	for i := 0; i < wtReflected.NumField(); i++ {
+		tName := tReflected.Type().Field(i).Name
+		tValue := tReflected.Field(i)
+
+		wtValue := wtReflected.FieldByName(tName)
+		if !wtValue.IsNil() {
+			wtValue = wtValue.Elem()
+			tValue.Set(wtValue)
+		}
+	}
+	return tRun
+}


Nice solution! 🥳

I'm wondering whether we can bypass this somehow. I'm thinking about getting rid of the pointers entirely. But this can be a little bit tricky.

But even if we do not make the workflows work without the pointers then I think that calling a slight modification of this function at the beginning of the reconcile loop won't hurt.

Again, we can make this little bit more generic:).

lpiwowar · 2024-09-24T13:30:37Z

controllers/tempest_controller.go

+	return instance.Spec.TempestRun
+}
+
+func changeType(wtRun testv1beta1.WorkflowTempestRunSpec) testv1beta1.TempestRunSpec {


I think we should aim at making this function more generic by replacing the types with interface{}. The caller of the function will be responsible of casting the result to a correct type.

Suggested change

func changeType(wtRun testv1beta1.WorkflowTempestRunSpec) testv1beta1.TempestRunSpec {

func changeType(instance interface{}) instance interface{} {

controllers/tempest_controller.go

Currently, the merging of workflow section is done in controllers of related CRs. We want to move the merging logic into webhooks to make the process cleaner.

kstrenkova · 2024-10-22T12:48:22Z

I have added a new version of this patch that can also work with structures by calling the function for adding values from TempestRun section recursively. There is maybe needed a bit more generalization to get the functions working for all resources, but for Tempest this version should work.

lpiwowar

I think the PR is moving in the right direction. Nice work! 👍

I've added couple of comments @kstrenkova. Let me know what you think:).

lpiwowar · 2024-10-23T12:32:08Z

api/v1beta1/common_webhook.go

+                tRunValue := src.Field(i)
+                wtRunValue := dest.FieldByName(tRunName)
+
+                if wtRunValue.IsZero() && !tRunValue.IsZero() {


The use of IsZero() function is not 100 % accurate here because IsZero() returns true when variable e.g. concurrency is set to 0 [1].

What do you think @kstrenkova about this? Would it be a problem?

Suggested change

if wtRunValue.IsZero() && !tRunValue.IsZero() {

if wtRunValue.IsNil() {

You can also use wtRunValue.IsZero() it is an equivalent to wtRunValue.IsNil() when wtRunValue is a pointer.

[1] https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/reflect/value.go;l=1572

Nice catch, I think the need for !tRunValue.IsNil is still there though 🤔 The problem was that if the value in tempestRun was nil, it would create an error.

Yes, you are right:

The Tempest "tempest-tests" is invalid: * spec.workflow[0].tempestRun.extraImagesType: Invalid value: "null": spec.workflow[0].tempestRun.extraImagesType in body must be of type array: "null" * spec.workflow[0].tempestRun.externalPlugin: Invalid value: "null": spec.workflow[0].tempestRun.externalPlugin in body must be of type array: "null" * spec.workflow[0].tempestRun.extraRPMs: Invalid value: "null": spec.workflow[0].tempestRun.extraRPMs in body must be of type array: "null"

Notice that the error only impacts arrays. We are trying to assign null to what should be a pointer to an array. Is there a way how to fix this? What about checking whether the array is empty in the tRun section and if it is then do not perform the assignment?

lpiwowar · 2024-10-23T12:23:02Z

api/v1beta1/common_webhook.go

@@ -28,3 +33,42 @@ const (
 		"ensures that the copying of the logs to the PV is completed without any " +
 		"complications."
 )
+
+// merge non-workflow section into workflow
+func mergeSectionIntoWorkflow(instance interface{}, workflowStepNum int) {


I would modify the interface of this function like this:

Suggested change

func mergeSectionIntoWorkflow(instance interface{}, workflowStepNum int) {

func mergeSectionIntoWorkflow(instance interface{}, instanceWorkflowSection {}interface) {

It would be a responsibility of the webhook to pass the correct values. Wdyt?:)

I looked into it and I don't understand why this would be needed 😄 Could you please explain your thought behind it a bit more?

Ok, so the idea for this function is to be generic because it is inside of the commonw_webhook.go . It should work for Tempest, Tobiko, HorizonTest and AnsibleTest CR. Yet you have a code which is specific to Tempest CR inside the function [1].

The idea is that inside the webhook you would call this function like this:

if len(spec.Workflow) > 0 { for _, workflowStepSpec := range spec.Workflow { mergeSectionIntoWorkflow(spec, workflowStepSpec) } }

This would allow you to avoid the need to cast the instance variable to TempestSpec before you can grab the workflow section.

[1] here

api/v1beta1/common_webhook.go

lpiwowar · 2024-10-23T12:33:48Z

controllers/common.go

+}
+
+func changeType(instance interface{}, workflowStepNum int) interface{} {
+	// TODO other types; else if typedInstance, ok := instance.(*v1beta1.HorizonTest);


Yes, I agree:)

lpiwowar · 2024-10-23T12:33:54Z

controllers/common.go

@@ -550,3 +550,38 @@ func GetCommonRbacRules(privileged bool) []rbacv1.PolicyRule {

 	return []rbacv1.PolicyRule{rbacPolicyRule}
 }
+
+// TODO make general for all resources


lpiwowar · 2024-10-23T12:37:55Z

api/v1beta1/common_webhook.go

+        tRun := spec.TempestRun
+        wtRun := &spec.Workflow[workflowStepNum].TempestRun


We can start the copying of the non-workflow values directly from Spec.. I know it was not done like so up until now for Tempest CR. The long term goal is to unify the behavior of the CRs so it would be nice to keep it consistent with the rest of them (like tobiko, ansibletest, ...).

Suggested change

tRun := spec.TempestRun

wtRun := &spec.Workflow[workflowStepNum].TempestRun

tRun := spec

wtRun := &spec.Workflow[workflowStepNum]

It might require some changes here:) [1].

Also, we should add similar behavior for the TempestconfRunSpec.

[1] here
[2] here

openshift-ci bot added the do-not-merge/work-in-progress label Aug 12, 2024

openshift-ci bot added the approved label Aug 12, 2024

lpiwowar added the do-not-merge/hold label Aug 15, 2024

lpiwowar assigned kstrenkova Sep 19, 2024

lpiwowar self-requested a review September 19, 2024 14:57

lpiwowar mentioned this pull request Sep 24, 2024

Embed common structs #200

Merged

lpiwowar reviewed Sep 24, 2024

View reviewed changes

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from e880a74 to af17dde Compare September 30, 2024 19:24

Move merging of workflow section into webhook

6b1c12c

Currently, the merging of workflow section is done in controllers of related CRs. We want to move the merging logic into webhooks to make the process cleaner.

kstrenkova force-pushed the merge-workflow-section-in-webhook branch from af17dde to 6b1c12c Compare October 22, 2024 12:43

lpiwowar requested changes Oct 23, 2024

View reviewed changes

openshift-ci bot assigned lpiwowar Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move merging of workflow section into webhook #168

Move merging of workflow section into webhook #168

kstrenkova commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

lpiwowar left a comment

lpiwowar Sep 24, 2024

lpiwowar Sep 24, 2024

kstrenkova commented Oct 22, 2024

lpiwowar left a comment

lpiwowar Oct 23, 2024

kstrenkova Oct 31, 2024

lpiwowar Nov 1, 2024

lpiwowar Oct 23, 2024

kstrenkova Oct 31, 2024

lpiwowar Oct 31, 2024

lpiwowar Oct 23, 2024

lpiwowar Oct 23, 2024

lpiwowar Oct 23, 2024

	func changeType(wtRun testv1beta1.WorkflowTempestRunSpec) testv1beta1.TempestRunSpec {
	func changeType(instance interface{}) instance interface{} {

	if wtRunValue.IsZero() && !tRunValue.IsZero() {
	if wtRunValue.IsNil() {

	func mergeSectionIntoWorkflow(instance interface{}, workflowStepNum int) {
	func mergeSectionIntoWorkflow(instance interface{}, instanceWorkflowSection {}interface) {

		tRun := spec.TempestRun
		wtRun := &spec.Workflow[workflowStepNum].TempestRun

Move merging of workflow section into webhook #168

Are you sure you want to change the base?

Move merging of workflow section into webhook #168

Conversation

kstrenkova commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

lpiwowar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstrenkova commented Oct 22, 2024

lpiwowar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment