[OSPRH-10386] Update logic for workflows #231

lpiwowar · 2024-10-18T15:51:12Z

There was a race condition in the previous implementation of the
workflows that was caused by the dependency of the workflow feature
on a ConfigMap that served as counter.

This commit simplifies the logic of the Reconcile loop in two ways:

Removes the dependency for an external workflow counter
Introduces NextAction function which decides what next action
should be performed by the Reconcile loop based on the current
state of the OpenShift cluster. The input of this function is the
instance which is currently being processed and a workflowLength.
Using these two arguments it then tells the Reconcile loop which
actions it should perform: Wait, CreateFirstJob, CreateNextJob,
EndTesting, Failure.

This approach should simplify the reconcile logic and move the
test-operator towards a unified Reconcile loop.

openshift-ci · 2024-10-18T15:51:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lpiwowar

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~~ [lpiwowar]

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

softwarefactory-project-zuul · 2024-10-18T20:03:12Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/b71be3f0076a40b68f502cf0246bdd43

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 28m 25s
❌ podified-multinode-edpm-deployment-crc-test-operator TIMED_OUT in 3h 11m 49s

softwarefactory-project-zuul · 2024-10-21T21:15:41Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5ca1eddc2fe14e05b305a4a9c30f1d7f

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 47m 44s
❌ podified-multinode-edpm-deployment-crc-test-operator TIMED_OUT in 3h 13m 05s

softwarefactory-project-zuul · 2024-10-22T16:25:35Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/7b19afcc3de6489aa1848fd9c82f4347

❌ openstack-k8s-operators-content-provider FAILURE in 6m 30s
⚠️ podified-multinode-edpm-deployment-crc-test-operator SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

There was a race condition in the previous implementation of the workflows that was caused by the dependency of the workflow feature on a ConfigMap that served as counter. This commit simplifies the logic of the Reconcile loop in two ways: 1. Removes the dependency for an external workflow counter 2. Introduces NextAction function which decides what next action should be performed by the Reconcile loop based on the current state of the OpenShift cluster. The input of this function is the instance which is currently being processed and a workflowLength. Using these two arguments it then tells the Reconcile loop which actions it should perform: Wait, CreateFirstJob, CreateNextJob, EndTesting, Failure. This approach should simplify the reconcile logic and move the test-operator towards a unified Reconcile loop.

openshift-ci · 2024-11-13T12:40:14Z

@lpiwowar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/precommit-check	`eef85ec`	link	true	`/test precommit-check`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

slawqo · 2024-11-13T13:50:56Z

controllers/horizontest_controller.go

-		if lockReleased, err := r.ReleaseLock(ctx, instance); !lockReleased {
-			return ctrl.Result{}, err
+	workflowLength := 0
+	nextAction, nextWorkflowStep, err := r.NextAction(ctx, instance, workflowLength)


workflowLenght will always be 0 here, is that expected?

evallesp · 2024-11-13T16:22:16Z

In general really good patch IMHO.
It's good to have a single pattern across the different controllers, and I see the point (Apart from fixing a race condition) as a good work of refactoring.

Some extra things:

Comments in the switch clauses at ansibletest_controller are also applicable to other controllers.
It seems there's room for some refactoring, as the code between controllers' logic of switch clauses is quite similar.

I'd like to do a second round when my comments are addressed, but it's IMHO in good shape and near to being merged. Thanks!

evallesp · 2024-11-13T12:00:20Z

controllers/ansibletest_controller.go

+	case EndTesting:
+		if lockReleased, _ := r.ReleaseLock(ctx, instance); !lockReleased {
+			Log.Info(fmt.Sprintf(InfoCanNotAcquireLock, testOperatorLockName))
+			return ctrl.Result{RequeueAfter: RequeueAfterValue}, nil


(blocking) Question: If we get this point it's because there was an error releasing the lock and/or there's no lock, right?
Are we loosing here some context by returning nil as error info?

evallesp · 2024-11-13T12:04:42Z

controllers/ansibletest_controller.go

-	runningJobName := r.GetJobName(instance, externalWorkflowCounter-1)
-	err = r.Client.Get(ctx, client.ObjectKey{Namespace: instance.GetNamespace(), Name: runningJobName}, runningAnsibleJob)
-	if err != nil && !k8s_errors.IsNotFound(err) {
+	switch nextAction {


praise: Good refactor and enhancement, much more clear this way!

evallesp · 2024-11-13T12:06:34Z

controllers/ansibletest_controller.go

+		Log.Info(InfoWaitingOnJob)
+		return ctrl.Result{RequeueAfter: RequeueAfterValue}, nil
+
+	case EndTesting:


(non-blocking) suggestion: I'd maintain the comment section about releasing the lock to let other instances spawn a job. I see the point of detilling that EndTesting might not be related to any error, and it's just the job was completed.

evallesp · 2024-11-13T12:07:56Z

controllers/common.go

+)
+
+const (
+	// RequeueAfterValue tells how much time should we wait before calling Reconcile


Praise: I like the new placement for the const.

evallesp · 2024-11-13T12:33:57Z

controllers/ansibletest_controller.go

+		lockAcquired, _ := r.AcquireLock(ctx, instance, helper, false)
+		if !lockAcquired {
+			Log.Info(fmt.Sprintf(InfoCanNotAcquireLock, testOperatorLockName))
+			return ctrl.Result{RequeueAfter: RequeueAfterValue}, nil


(blocking) suggestion: I'd maintain here the error information from Acquirelock.

evallesp · 2024-11-13T14:01:41Z

controllers/common.go

+
+// GetLastJob returns job associated with an instance which has the highest value
+// stored in the workflowStep label
+func (r *Reconciler) GetLastJob(


praise: this type of function easily gets a bit hard to review. I like this one.

evallesp · 2024-11-13T14:05:45Z

controllers/common.go

@@ -276,7 +415,7 @@ func (r *Reconciler) EnsureLogsPVCExists(
 		},
 	}

-	timeDuration, _ := time.ParseDuration("2m")
+	timeDuration, err := time.ParseDuration("2m")


(blocking) question: I'm unsure about this new error. I'd say we would need to check that before pvc.NewPvc(testOperatorPvcDef, timeDuration)

It seems that we're not going to avoid any runtimeError by catching the error if we hold on after line ctrlResult, err := testOperatorPvc.CreateOrPatch(ctx, helper) to do something.

evallesp · 2024-11-13T14:19:51Z

controllers/common.go

@@ -368,6 +508,10 @@ func (r *Reconciler) AcquireLock(
 		return err == nil, err
 	}

+	if cm.Data[testOperatorLockOnwerField] == instanceGUID {


praise: IIUC, we're adding a new behavior of returning True if there's a lock and the lock owner is the same as the one executing AcquireLock.
So basically we're adding idempotency to the method.

I guess that here was the error right?

evallesp · 2024-11-13T16:02:19Z

controllers/tempest_controller.go

-		"instanceName":     instance.Name,
-		"operator":         "test-operator",
+		workflowStepLabel:  strconv.Itoa(nextWorkflowStep),
+		instanceNameLabel:  instance.Name,


(blocking) question: Is not necessary "operator" here?

evallesp · 2024-11-13T16:12:27Z

controllers/tobiko_controller.go

+		lockAcquired, _ := r.AcquireLock(ctx, instance, helper, instance.Spec.Parallel)
+		if !lockAcquired {
+			Log.Info(fmt.Sprintf(InfoCanNotAcquireLock, testOperatorLockName))
+			return ctrl.Result{}, nil


(blocking) question: Would be good to return RequeueAfterValue here? If not, could you share some info? Thanks!

lpiwowar marked this pull request as draft October 18, 2024 15:51

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

openshift-ci bot requested review from abays and viroel October 18, 2024 15:51

openshift-ci bot added the approved label Oct 18, 2024

openshift-merge-robot added the needs-rebase label Oct 21, 2024

lpiwowar force-pushed the bugfix/OSPRH-10386 branch from 5c9e5fb to 7f6cddb Compare October 22, 2024 16:04

openshift-merge-robot removed the needs-rebase label Oct 22, 2024

lpiwowar force-pushed the bugfix/OSPRH-10386 branch from 49f0827 to d03ca9c Compare November 1, 2024 14:47

lpiwowar changed the title ~~Bugfix - incorrect workflow order~~ Update logic for workflows Nov 1, 2024

lpiwowar changed the title ~~Update logic for workflows~~ [OSPRH-10386] Update logic for workflows Nov 1, 2024

lpiwowar force-pushed the bugfix/OSPRH-10386 branch 2 times, most recently from 379e406 to 70f9c29 Compare November 1, 2024 15:03

lpiwowar removed request for abays and viroel November 1, 2024 15:06

lpiwowar force-pushed the bugfix/OSPRH-10386 branch from 70f9c29 to eef85ec Compare November 1, 2024 15:17

lpiwowar marked this pull request as ready for review November 13, 2024 12:18

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 13, 2024

openshift-ci bot requested review from kopecmartin and olliewalsh November 13, 2024 12:18

lpiwowar requested review from evallesp and removed request for kopecmartin and olliewalsh November 13, 2024 12:20

slawqo reviewed Nov 13, 2024

View reviewed changes

evallesp reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OSPRH-10386] Update logic for workflows #231

[OSPRH-10386] Update logic for workflows #231

lpiwowar commented Oct 18, 2024 •

edited

Loading

openshift-ci bot commented Oct 18, 2024

softwarefactory-project-zuul bot commented Oct 18, 2024

softwarefactory-project-zuul bot commented Oct 21, 2024

softwarefactory-project-zuul bot commented Oct 22, 2024

openshift-ci bot commented Nov 13, 2024 •

edited

Loading

slawqo Nov 13, 2024

evallesp commented Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

evallesp Nov 13, 2024

[OSPRH-10386] Update logic for workflows #231

Are you sure you want to change the base?

[OSPRH-10386] Update logic for workflows #231

Conversation

lpiwowar commented Oct 18, 2024 • edited Loading

openshift-ci bot commented Oct 18, 2024

softwarefactory-project-zuul bot commented Oct 18, 2024

softwarefactory-project-zuul bot commented Oct 21, 2024

softwarefactory-project-zuul bot commented Oct 22, 2024

openshift-ci bot commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

evallesp commented Nov 13, 2024

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

lpiwowar commented Oct 18, 2024 •

edited

Loading

openshift-ci bot commented Nov 13, 2024 •

edited

Loading