Merge pull request #2159 from rabi/ttl-after-finished

Collect dataplane AnsibleEE jobs only when deployment has finished

Author: openshift-merge-bot[bot]

api/bases/dataplane.openstack.org_openstackdataplanedeployments.yaml

@@ -105,7 +105,11 @@ spec:
               preserveJobs:
                 default: true
                 description: |-
-                  PreserveJobs - do not delete jobs after they finished e.g. to check logs
+                  PreserveJobs - do not delete jobs after they finished e.g. to check logs.
+                  When false the jobs are deleted after a delay once the deployment is
+                  finished - deployed, or failed with the backoff limit exceeded - and not
+                  when an individual job finishes. While a deployment is in flight its
+                  jobs are what tells the operator which services already ran.
                   PreserveJobs default: true
                 enum:
                 - true

api/dataplane/v1beta1/openstackdataplanedeployment_types.go

@@ -39,7 +39,11 @@ type OpenStackDataPlaneDeploymentSpec struct {
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:com.tectonic.ui:number"}
 	BackoffLimit *int32 `json:"backoffLimit,omitempty"`
 
-	// PreserveJobs - do not delete jobs after they finished e.g. to check logs
+	// PreserveJobs - do not delete jobs after they finished e.g. to check logs.
+	// When false the jobs are deleted after a delay once the deployment is
+	// finished - deployed, or failed with the backoff limit exceeded - and not
+	// when an individual job finishes. While a deployment is in flight its
+	// jobs are what tells the operator which services already ran.
 	// PreserveJobs default: true
 	// +kubebuilder:validation:Enum:=true;false
 	// +kubebuilder:default:=true
@@ -189,6 +193,42 @@ func (instance OpenStackDataPlaneDeployment) IsReady() bool {
 	return instance.Status.Conditions.IsTrue(condition.ReadyCondition)
 }
 
+// IsFinished reports whether the deployment reached a state it will not advance
+// from. The failure verdict is only readable before InitConditions resets the
+// conditions.
+func (instance *OpenStackDataPlaneDeployment) IsFinished() bool {
+	return instance.Status.Deployed || instance.isTerminalFailure()
+}
+
+// FinishedAt reports when the deployment reached that state, or nil while it can
+// still progress. LastTransitionTime only moves when the condition state
+// changes, so the value is stable across reconciles.
+func (instance *OpenStackDataPlaneDeployment) FinishedAt() *metav1.Time {
+	if instance.Status.Conditions == nil {
+		return nil
+	}
+	deploymentCondition := instance.Status.Conditions.Get(condition.DeploymentReadyCondition)
+	if deploymentCondition == nil || deploymentCondition.LastTransitionTime.IsZero() {
+		return nil
+	}
+	if !instance.Status.Deployed && !instance.isTerminalFailure() {
+		return nil
+	}
+
+	finishedAt := deploymentCondition.LastTransitionTime
+	return &finishedAt
+}
+
+func (instance *OpenStackDataPlaneDeployment) isTerminalFailure() bool {
+	if instance.Status.Conditions == nil {
+		return false
+	}
+	deploymentCondition := instance.Status.Conditions.Get(condition.DeploymentReadyCondition)
+	return deploymentCondition != nil &&
+		deploymentCondition.Severity == condition.SeverityError &&
+		deploymentCondition.Reason == condition.JobReasonBackoffLimitExceeded
+}
+
 // InitConditions - Initializes Status Conditons
 func (instance *OpenStackDataPlaneDeployment) InitConditions() {
 	instance.Status.Conditions = condition.Conditions{}

bindata/crds/crds.yaml

@@ -20732,7 +20732,11 @@ spec:
               preserveJobs:
                 default: true
                 description: |-
-                  PreserveJobs - do not delete jobs after they finished e.g. to check logs
+                  PreserveJobs - do not delete jobs after they finished e.g. to check logs.
+                  When false the jobs are deleted after a delay once the deployment is
+                  finished - deployed, or failed with the backoff limit exceeded - and not
+                  when an individual job finishes. While a deployment is in flight its
+                  jobs are what tells the operator which services already ran.
                   PreserveJobs default: true
                 enum:
                 - true

config/crd/bases/dataplane.openstack.org_openstackdataplanedeployments.yaml

@@ -105,7 +105,11 @@ spec:
               preserveJobs:
                 default: true
                 description: |-
-                  PreserveJobs - do not delete jobs after they finished e.g. to check logs
+                  PreserveJobs - do not delete jobs after they finished e.g. to check logs.
+                  When false the jobs are deleted after a delay once the deployment is
+                  finished - deployed, or failed with the backoff limit exceeded - and not
+                  when an individual job finishes. While a deployment is in flight its
+                  jobs are what tells the operator which services already ran.
                   PreserveJobs default: true
                 enum:
                 - true

docs/assemblies/dataplane_resources.adoc

@@ -93,7 +93,7 @@ OpenStackDataPlaneDeploymentSpec defines the desired state of OpenStackDataPlane
 | false
 
 | preserveJobs
-| PreserveJobs - do not delete jobs after they finished e.g. to check logs PreserveJobs default: true
+| PreserveJobs - do not delete jobs after they finished e.g. to check logs. When false the jobs are deleted after a delay once the deployment is finished - deployed, or failed with the backoff limit exceeded - and not when an individual job finishes. While a deployment is in flight its jobs are what tells the operator which services already ran. PreserveJobs default: true
 | bool
 | false
 

internal/controller/dataplane/openstackdataplanedeployment_controller.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"math"
 	"slices"
 	"time"
 
@@ -29,6 +30,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
@@ -107,12 +109,30 @@ func (r *OpenStackDataPlaneDeploymentReconciler) Reconcile(ctx context.Context,
 		Log,
 	)
 
+	// Whether Job cleanup may be scheduled: a verdict freshly computed by this
+	// reconcile is only trustworthy once the status patch below persists it.
+	finishedOnEntry := instance.IsFinished()
+	statusPersisted := false
+
+	// Registered ahead of the status patch defer so it runs after the verdict persists.
+	defer func() {
+		if err := r.reconcileJobTTL(ctx, helper, instance, finishedOnEntry || statusPersisted); err != nil {
+			Log.Error(err, "Failed to reconcile AnsibleEE job cleanup", "deployment", instance.Name)
+			if _err == nil {
+				_err = err
+			}
+		}
+	}()
+
 	// If the deploy is already done, return immediately.
 	if instance.Status.Deployed {
 		Log.Info("Already deployed", "instance.Status.Deployed", instance.Status.Deployed)
 		return ctrl.Result{}, nil
 	}
 
+	// If the deployment is in a terminal state, do not reconcile even if
+	// nodesets or other watched resources change. The deployment should not
+	// be retried.
 	if terminalFailure(instance) {
 		Log.Info("Deployment is terminal, skipping reconciliation",
 			"deployment", instance.Name)
@@ -162,6 +182,7 @@ func (r *OpenStackDataPlaneDeploymentReconciler) Reconcile(ctx context.Context,
 			_err = err
 			return
 		}
+		statusPersisted = true
 	}()
 
 	// Ensure NodeSets
@@ -547,6 +568,98 @@ func (r *OpenStackDataPlaneDeploymentReconciler) deployGlobalServices(
 	return nil
 }
 
+// Mirrors the delay lib-common applies to finished Jobs.
+const jobCleanupDelaySeconds int32 = 10 * 60
+
+// reconcileJobTTL defers Job cleanup until the deployment is finished: while it
+// runs the Jobs are the only record of which services already ran. scheduleCleanup
+// is false while the terminal verdict exists only in memory.
+func (r *OpenStackDataPlaneDeploymentReconciler) reconcileJobTTL(
+	ctx context.Context,
+	helper *helper.Helper,
+	instance *dataplanev1.OpenStackDataPlaneDeployment,
+	scheduleCleanup bool,
+) error {
+	if instance.Spec.PreserveJobs {
+		return nil
+	}
+
+	jobList := &batchv1.JobList{}
+	err := helper.GetClient().List(
+		ctx,
+		jobList,
+		client.InNamespace(instance.Namespace),
+		client.MatchingLabels{dataplaneutil.AnsibleEEDeploymentLabel: instance.Name},
+	)
+	if err != nil {
+		return err
+	}
+
+	finished := scheduleCleanup && instance.IsFinished()
+	finishedAt := instance.FinishedAt()
+
+	for i := range jobList.Items {
+		job := &jobList.Items[i]
+		if finished {
+			// Ageing starts at the Job's own completion time, so one schedule per Job
+			// is enough and the value must not be re-derived afterwards.
+			if job.Spec.TTLSecondsAfterFinished != nil {
+				continue
+			}
+			patch := client.MergeFrom(job.DeepCopy())
+			ttl := jobCleanupDelay(job, finishedAt)
+			job.Spec.TTLSecondsAfterFinished = &ttl
+
+			err := helper.GetClient().Patch(ctx, job, patch)
+			if err != nil && !k8s_errors.IsNotFound(err) {
+				return err
+			}
+			continue
+		}
+
+		// An older operator may have left a deadline on a running deployment.
+		if job.Spec.TTLSecondsAfterFinished == nil {
+			continue
+		}
+		patch := client.MergeFrom(job.DeepCopy())
+		job.Spec.TTLSecondsAfterFinished = nil
+
+		err := helper.GetClient().Patch(ctx, job, patch)
+		if err != nil && !k8s_errors.IsNotFound(err) {
+			return err
+		}
+	}
+	return nil
+}
+
+// jobCleanupDelay returns the TTLSecondsAfterFinished that makes the Job expire
+// jobCleanupDelaySeconds after the deployment finished. Ageing starts at the
+// Job's own completion time, so a Job that finished while the deployment still
+// ran gets that elapsed time added back - a flat delay would delete it at once.
+func jobCleanupDelay(job *batchv1.Job, finishedAt *metav1.Time) int32 {
+	finishTime := jobFinishTime(job)
+	if finishedAt == nil || finishTime == nil || !finishedAt.After(finishTime.Time) {
+		return jobCleanupDelaySeconds
+	}
+
+	delay := int64(jobCleanupDelaySeconds) + int64(math.Floor(finishedAt.Time.Sub(finishTime.Time).Seconds()))
+	return int32(min(delay, math.MaxInt32))
+}
+
+// jobFinishTime mirrors the TTL-after-finished controller, which measures from
+// the Complete/Failed transition rather than CompletionTime.
+func jobFinishTime(job *batchv1.Job) *metav1.Time {
+	for _, jobCondition := range job.Status.Conditions {
+		isFinished := (jobCondition.Type == batchv1.JobComplete || jobCondition.Type == batchv1.JobFailed) &&
+			jobCondition.Status == corev1.ConditionTrue
+		if isFinished && !jobCondition.LastTransitionTime.IsZero() {
+			finishTime := jobCondition.LastTransitionTime
+			return &finishTime
+		}
+	}
+	return job.Status.CompletionTime
+}
+
 // GetService retrieves a service for the OpenStackDataPlaneDeployment
 func (r *OpenStackDataPlaneDeploymentReconciler) GetService(
 	ctx context.Context,

internal/dataplane/util/ansible_execution.go

@@ -247,6 +247,13 @@ func ParseAnsibleExecutionSummaryFromPod(
 	return nil, nil
 }
 
+// Labels the AnsibleEE Jobs carry, to build the Job spec and find them again.
+const (
+	AnsibleEEServiceLabel    = "openstackdataplaneservice"
+	AnsibleEEDeploymentLabel = "openstackdataplanedeployment"
+	AnsibleEENodeSetLabel    = "openstackdataplanenodeset"
+)
+
 // GetAnsibleExecutionNameAndLabels Name and Labels of AnsibleEE
 func GetAnsibleExecutionNameAndLabels(service *dataplanev1.OpenStackDataPlaneService,
 	deploymentName string,
@@ -267,7 +274,7 @@ func GetAnsibleExecutionNameAndLabels(service *dataplanev1.OpenStackDataPlaneSer
 	}
 
 	labels := GetGlobalAnsibleExecutionLabels(service.Name, deploymentName)
-	labels["openstackdataplanenodeset"] = nodeSetName
+	labels[AnsibleEENodeSetLabel] = nodeSetName
 	return executionName, labels
 }
 
@@ -276,8 +283,8 @@ func GetAnsibleExecutionNameAndLabels(service *dataplanev1.OpenStackDataPlaneSer
 // selector must not carry the nodeset label.
 func GetGlobalAnsibleExecutionLabels(serviceName string, deploymentName string) map[string]string {
 	return map[string]string{
-		"openstackdataplaneservice":    serviceName,
-		"openstackdataplanedeployment": deploymentName,
+		AnsibleEEServiceLabel:    serviceName,
+		AnsibleEEDeploymentLabel: deploymentName,
 	}
 }
 
@@ -306,7 +313,9 @@ func (a *EEJob) BuildAeeJobSpec(
 	}
 
 	a.BackoffLimit = deployment.Spec.BackoffLimit
-	a.PreserveJobs = deployment.Spec.PreserveJobs
+	// Cleanup must not start while the deployment still runs: lib-common never
+	// recreates a Job whose hash is unchanged.
+	a.PreserveJobs = deployment.Spec.PreserveJobs || !deployment.IsFinished()
 	a.FormatAEECmdLineArguments(aeeSpec)
 	a.FormatAEEExtraVars(aeeSpec, service, deployment, nodeSet)
 	a.DetermineAeeImage(aeeSpec)