Revert PodFailurePolicy in favor of rejecting v2 charts in the contro…

…ller Turns out using PodFailurePolicy leaves a bunch of old pods around instead of just restarting the containers. Rather than pollute the cluster with failed pods when installation is successfully retried, lets just reject v2 charts at the controller level. Add status conditions and an Event to report this, for better visibility. Signed-off-by: Brad Davidson <[email protected]>
k3s-io · Aug 2, 2024 · dc32789 · dc32789
1 parent 4dd3c99
commit dc32789
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 28 deletions.
diff --git a/pkg/apis/helm.cattle.io/v1/types.go b/pkg/apis/helm.cattle.io/v1/types.go
@@ -45,7 +45,8 @@ type HelmChartSpec struct {
 }
 
 type HelmChartStatus struct {
-	JobName string `json:"jobName,omitempty"`
+	JobName    string               `json:"jobName,omitempty"`
+	Conditions []HelmChartCondition `json:"conditions,omitempty"`
 }
 
 // +genclient
@@ -62,3 +63,23 @@ type HelmChartConfigSpec struct {
 	ValuesContent string `json:"valuesContent,omitempty"`
 	FailurePolicy string `json:"failurePolicy,omitempty"`
 }
+
+type HelmChartConditionType string
+
+const (
+	HelmChartJobCreated HelmChartConditionType = "JobCreated"
+	HelmChartFailed     HelmChartConditionType = "Failed"
+)
+
+type HelmChartCondition struct {
+	// Type of job condition.
+	Type HelmChartConditionType `json:"type"`
+	// Status of the condition, one of True, False, Unknown.
+	Status corev1.ConditionStatus `json:"status"`
+	// (brief) reason for the condition's last transition.
+	// +optional
+	Reason string `json:"reason,omitempty"`
+	// Human readable message indicating details about last transition.
+	// +optional
+	Message string `json:"message,omitempty"`
+}
diff --git a/pkg/apis/helm.cattle.io/v1/zz_generated_deepcopy.go b/pkg/apis/helm.cattle.io/v1/zz_generated_deepcopy.go
diff --git a/pkg/controllers/chart/chart.go b/pkg/controllers/chart/chart.go
@@ -204,16 +204,60 @@ func (c *Controller) OnChange(chart *v1.HelmChart, chartStatus v1.HelmChartStatu
 		return nil, chartStatus, nil
 	}
 	if chart.DeletionTimestamp != nil {
-		// this should only be called if the chart is being installed or upgraded
+		// this should only be called if the chart is being deleted
+		return nil, chartStatus, nil
+	}
+
+	switch chart.Spec.HelmVersion {
+	case "", "v3":
+	default:
+		c.recorder.Eventf(chart, corev1.EventTypeWarning, "UnsupportedVersion", "Unsupported Helm version %s: only v3 charts are supported", chart.Spec.HelmVersion)
+		chartStatus.Conditions = []v1.HelmChartCondition{
+			{
+				Type:   v1.HelmChartJobCreated,
+				Status: corev1.ConditionFalse,
+			},
+			{
+				Type:    v1.HelmChartFailed,
+				Status:  corev1.ConditionTrue,
+				Reason:  "Unsupported version",
+				Message: "Only Helm v3 charts are supported",
+			},
+		}
 		return nil, chartStatus, nil
 	}
 
 	job, objs, err := c.getJobAndRelatedResources(chart)
 	if err != nil {
+		chartStatus.Conditions = []v1.HelmChartCondition{
+			{
+				Type:   v1.HelmChartJobCreated,
+				Status: corev1.ConditionFalse,
+			},
+			{
+				Type:    v1.HelmChartFailed,
+				Status:  corev1.ConditionTrue,
+				Reason:  "Job create failed",
+				Message: fmt.Sprintf("Failed to generate Job: %v", err),
+			},
+		}
 		return nil, chartStatus, err
 	}
+
 	// update status
 	chartStatus.JobName = job.Name
+	chartStatus.Conditions = []v1.HelmChartCondition{
+		{
+			Type:    v1.HelmChartJobCreated,
+			Status:  corev1.ConditionTrue,
+			Reason:  "Job created",
+			Message: fmt.Sprintf("Applying HelmChart using Job %s/%s", job.Namespace, job.Name),
+		},
+		{
+			Type:   v1.HelmChartFailed,
+			Status: corev1.ConditionFalse,
+		},
+	}
 
 	// emit an event to indicate that this Helm chart is being applied
 	c.recorder.Eventf(chart, corev1.EventTypeNormal, "ApplyJob", "Applying HelmChart using Job %s/%s", job.Namespace, job.Name)
@@ -409,18 +453,6 @@ func job(chart *v1.HelmChart, apiServerPort string) (*batch.Job, *corev1.Secret,
 			},
 		},
 		Spec: batch.JobSpec{
-			PodFailurePolicy: &batch.PodFailurePolicy{
-				Rules: []batch.PodFailurePolicyRule{
-					{
-						Action: batch.PodFailurePolicyActionFailJob,
-						OnExitCodes: &batch.PodFailurePolicyOnExitCodesRequirement{
-							ContainerName: pointer.String("helm"),
-							Operator:      batch.PodFailurePolicyOnExitCodesOpIn,
-							Values:        []int32{64},
-						},
-					},
-				},
-			},
 			Template: corev1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{},
@@ -429,7 +461,7 @@ func job(chart *v1.HelmChart, apiServerPort string) (*batch.Job, *corev1.Secret,
 					},
 				},
 				Spec: corev1.PodSpec{
-					RestartPolicy: corev1.RestartPolicyNever,
+					RestartPolicy: corev1.RestartPolicyOnFailure,
 					Containers: []corev1.Container{
 						{
 							Name:            "helm",

diff --git a/test/framework/framework.go b/test/framework/framework.go
@@ -237,10 +237,10 @@ func (f *Framework) GetChartContent(url string) (string, error) {
 	return string(b.Bytes()), nil
 }
 
-// GetJobCondition returns true if there is a condition on the job matching the selected type and status
-func (f *Framework) GetJobCondition(job *batchv1.Job, condition batchv1.JobConditionType, status corev1.ConditionStatus) bool {
-	for _, v := range job.Status.Conditions {
-		if v.Type == condition && v.Status == status {
+// GetHelmChartCondition returns true if there is a condition on the chart matching the selected type, status, and reason
+func (f *Framework) GetHelmChartCondition(chart *v1.HelmChart, condition v1.HelmChartConditionType, status corev1.ConditionStatus, reason string) bool {
+	for _, v := range chart.Status.Conditions {
+		if v.Type == condition && v.Status == status && v.Reason == reason {
 			return true
 		}
 	}

diff --git a/test/suite/helm_test.go b/test/suite/helm_test.go
@@ -356,7 +356,6 @@ var _ = Describe("Helm Tests", Ordered, func() {
 		var (
 			err   error
 			chart *v1.HelmChart
-			job   *batchv1.Job
 		)
 		BeforeEach(func() {
 			chart = framework.NewHelmChart("traefik-example-v2",
@@ -377,18 +376,14 @@ var _ = Describe("Helm Tests", Ordered, func() {
 			chart, err = framework.CreateHelmChart(chart, framework.Namespace)
 			Expect(err).ToNot(HaveOccurred())
 		})
-		It("Job should have failed condition", func() {
+		It("Chart should have failed condition", func() {
 			Eventually(func() error {
 				chart, err = framework.GetHelmChart(chart.Name, chart.Namespace)
 				if err != nil {
 					return err
 				}
-				job, err = framework.GetJob(chart)
-				if err != nil {
-					return err
-				}
-				if !framework.GetJobCondition(job, batchv1.JobFailed, corev1.ConditionTrue) {
-					return fmt.Errorf("expected condition %v=%v not found", batchv1.JobFailed, corev1.ConditionTrue)
+				if !framework.GetHelmChartCondition(chart, v1.HelmChartFailed, corev1.ConditionTrue, "Unsupported version") {
+					return fmt.Errorf("expected condition %v=%v not found", v1.HelmChartFailed, corev1.ConditionTrue)
 				}
 				return nil
 			}, 120*time.Second).ShouldNot(HaveOccurred())