Use RestartPolicy: Never for the jobs

With RestartPolicy: OnFailure new pods are not created[1], but the containers in esiting pod are restarted. We were using RestartPolicy: Never with openstack-ansibleee-operator. This was changed when we switched to drop it's usage. [1] https://kubernetes.io/docs/concepts/workloads/controllers/job/#handling-pod-and-container-failures Signed-off-by: rabi <[email protected]>
openstack-k8s-operators · Sep 18, 2024 · 5021b09 · 5021b09
1 parent 749d164
commit 5021b09
Show file tree

Hide file tree

Showing 16 changed files with 49 additions and 59 deletions.
diff --git a/pkg/dataplane/deployment.go b/pkg/dataplane/deployment.go
@@ -197,6 +197,7 @@ func (d *Deployer) ConditionalDeploy(
 
 	}
 
+	var ansibleCondition *batchv1.JobCondition
 	if nsConditions.IsFalse(readyCondition) {
 		var ansibleEE *batchv1.Job
 		_, labelSelector := dataplaneutil.GetAnsibleExecutionNameAndLabels(&foundService, d.Deployment.Name, d.NodeSet.Name)
@@ -221,19 +222,14 @@ func (d *Deployer) ConditionalDeploy(
 			nsConditions.Set(condition.TrueCondition(
 				readyCondition,
 				readyMessage))
-		}
-
-		if ansibleEE.Status.Active > 0 {
+		} else if ansibleEE.Status.Active > 0 {
 			log.Info(fmt.Sprintf("AnsibleEE job is not yet completed: Execution: %s, Active pods: %d", ansibleEE.Name, ansibleEE.Status.Active))
 			nsConditions.Set(condition.FalseCondition(
 				readyCondition,
 				condition.RequestedReason,
 				condition.SeverityInfo,
 				readyWaitingMessage))
-		}
-
-		var ansibleCondition *batchv1.JobCondition
-		if ansibleEE.Status.Failed > 0 {
+		} else if ansibleEE.Status.Failed > 0 {
 			errorMsg := fmt.Sprintf("execution.name %s execution.namespace %s failed pods: %d", ansibleEE.Name, ansibleEE.Namespace, ansibleEE.Status.Failed)
 			for _, condition := range ansibleEE.Status.Conditions {
 				if condition.Type == batchv1.JobFailed {

diff --git a/pkg/dataplane/util/ansible_execution.go b/pkg/dataplane/util/ansible_execution.go
@@ -178,8 +178,6 @@ func (a *EEJob) BuildAeeJobSpec(
 	service *dataplanev1.OpenStackDataPlaneService,
 	nodeSet client.Object,
 ) {
-	const jobRestartPolicy string = "OnFailure"
-
 	if aeeSpec.DNSConfig != nil {
 		a.DNSConfig = aeeSpec.DNSConfig
 	}
@@ -195,7 +193,6 @@ func (a *EEJob) BuildAeeJobSpec(
 	}
 
 	a.BackoffLimit = deployment.Spec.BackoffLimit
-	a.RestartPolicy = jobRestartPolicy
 	a.FormatAEECmdLineArguments(aeeSpec)
 	a.FormatAEEExtraVars(aeeSpec, service, deployment, nodeSet)
 	a.DetermineAeeImage(aeeSpec)

diff --git a/pkg/dataplane/util/ansibleee.go b/pkg/dataplane/util/ansibleee.go
@@ -30,9 +30,6 @@ type EEJob struct {
 	Namespace string `json:"namespace,omitempty"`
 	// EnvConfigMapName is the name of the k8s config map that contains the ansible env variables
 	EnvConfigMapName string `json:"envConfigMapName,omitempty"`
-	// RestartPolicy is the policy applied to the Job on whether it needs to restart the Pod. It can be "OnFailure" or "Never".
-	// RestartPolicy default: Never
-	RestartPolicy string `json:"restartPolicy,omitempty"`
 	// CmdLine is the command line passed to ansible-runner
 	CmdLine string `json:"cmdLine,omitempty"`
 	// ServiceAccountName allows to specify what ServiceAccountName do we want the ansible execution run with. Without specifying,
@@ -103,9 +100,9 @@ func (a *EEJob) JobForOpenStackAnsibleEE(h *helper.Helper) (*batchv1.Job, error)
 	}
 
 	podSpec := corev1.PodSpec{
-		RestartPolicy: corev1.RestartPolicy(a.RestartPolicy),
+		RestartPolicy: corev1.RestartPolicyNever,
 		Containers: []corev1.Container{{
-			ImagePullPolicy: "Always",
+			ImagePullPolicy: corev1.PullAlways,
 			Image:           a.Image,
 			Name:            a.Name,
 			Args:            args,

diff --git a/tests/kuttl/tests/dataplane-deploy-global-service-test/01-assert.yaml b/tests/kuttl/tests/dataplane-deploy-global-service-test/01-assert.yaml
@@ -153,7 +153,7 @@ spec:
         - mountPath: /runner/inventory/inventory-0
           name: inventory-0
           subPath: inventory-0
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -258,7 +258,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -365,7 +365,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -473,7 +473,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -581,7 +581,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -689,7 +689,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -797,7 +797,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -899,7 +899,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -1017,7 +1017,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -1147,7 +1147,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -1256,7 +1256,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -1365,7 +1365,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -1474,7 +1474,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global
@@ -1592,7 +1592,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-global

diff --git a/tests/kuttl/tests/dataplane-deploy-global-service-test/02-assert.yaml b/tests/kuttl/tests/dataplane-deploy-global-service-test/02-assert.yaml
@@ -149,7 +149,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-beta-nodeset
@@ -255,7 +255,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-beta-nodeset

diff --git a/tests/kuttl/tests/dataplane-deploy-multiple-secrets/02-assert.yaml b/tests/kuttl/tests/dataplane-deploy-multiple-secrets/02-assert.yaml
@@ -189,7 +189,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: openstack-edpm-tls
@@ -313,7 +313,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: openstack-edpm-tls

diff --git a/tests/kuttl/tests/dataplane-deploy-no-nodes-test/01-assert.yaml b/tests/kuttl/tests/dataplane-deploy-no-nodes-test/01-assert.yaml
@@ -144,7 +144,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -252,7 +252,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -361,7 +361,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -470,7 +470,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -579,7 +579,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -688,7 +688,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -791,7 +791,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -910,7 +910,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -1041,7 +1041,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -1151,7 +1151,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -1261,7 +1261,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -1371,7 +1371,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes
@@ -1490,7 +1490,7 @@ spec:
         - mountPath: /runner/inventory/hosts
           name: inventory
           subPath: inventory
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes

diff --git a/tests/kuttl/tests/dataplane-deploy-no-nodes-test/02-assert.yaml b/tests/kuttl/tests/dataplane-deploy-no-nodes-test/02-assert.yaml
@@ -87,7 +87,7 @@ spec:
           name: inventory
           subPath: inventory
       dnsPolicy: ClusterFirst
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes

diff --git a/tests/kuttl/tests/dataplane-deploy-no-nodes-test/04-assert.yaml b/tests/kuttl/tests/dataplane-deploy-no-nodes-test/04-assert.yaml
@@ -148,7 +148,7 @@ spec:
           name: inventory
           subPath: inventory
       dnsPolicy: ClusterFirst
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-no-nodes

diff --git a/tests/kuttl/tests/dataplane-deploy-no-nodes-test/06-assert.yaml b/tests/kuttl/tests/dataplane-deploy-no-nodes-test/06-assert.yaml
@@ -147,7 +147,7 @@ spec:
           name: inventory
           subPath: inventory
       dnsPolicy: ClusterFirst
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-beta-nodeset
@@ -256,7 +256,7 @@ spec:
           name: inventory
           subPath: inventory
       dnsPolicy: ClusterFirst
-      restartPolicy: OnFailure
+      restartPolicy: Never
       schedulerName: default-scheduler
       securityContext: {}
       serviceAccount: edpm-compute-beta-nodeset