🐛Bump addon-framework to v1.3.1 and surface JobFailed status feedback (#1667)

* Bump addon-framework to v1.3.1 and surface JobFailed status feedback

addon-framework v1.3.1 makes addon pre-delete hooks retry when their
Pod/Job terminally fails (e.g. pod evicted under MemoryPressure/
DiskPressure, node lost, or Job backoffLimit exhausted). Previously an
evicted hook pod stayed Failed forever, the pre-delete hook finalizer
was never removed, and the ManagedClusterAddOn (and its owning
ManagedCluster) hung on deletion. See ACM-37228.

Add a JobFailed WellKnownStatus feedback rule to the work-agent so the
Job 'Failed' condition is reported back on the ManifestWork. The v1.3.1
hook completion logic consumes this to detect a terminally failed hook
Job and recreate it.

Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>

* restore pinned kind node image for e2e jobs

Signed-off-by: Mike Ng <ming@redhat.com>

---------

Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
Signed-off-by: Mike Ng <ming@redhat.com>
Co-authored-by: Mike Ng <ming@redhat.com>

.github/workflows/e2e.yml

@@ -16,6 +16,7 @@ on:
 
 env:
   USE_EXISTING_CLUSTER: false # set to true to use an existing kind cluster for debugging with act
+  KUBERNETES_VERSION: 'v1.29.2'
 
 permissions:
   contents: read
@@ -37,7 +38,7 @@ jobs:
           go-version-file: go.mod
       - name: Setup kind
         if: ${{ env.USE_EXISTING_CLUSTER != 'true' }}
-        run: kind create cluster --wait 300s
+        run: kind create cluster --image kindest/node:${{ env.KUBERNETES_VERSION }} --wait 300s
       - name: Set KUBECONFIG
         run: |
           mkdir -p /home/runner/.kube
@@ -64,7 +65,7 @@ jobs:
           go-version-file: go.mod
       - name: Setup kind
         if: ${{ env.USE_EXISTING_CLUSTER != 'true' }}
-        run: kind create cluster --wait 300s
+        run: kind create cluster --image kindest/node:${{ env.KUBERNETES_VERSION }} --wait 300s
       - name: Set KUBECONFIG
         run: |
           mkdir -p /home/runner/.kube
@@ -91,7 +92,7 @@ jobs:
           go-version-file: go.mod
       - name: Setup kind
         if: ${{ env.USE_EXISTING_CLUSTER != 'true' }}
-        run: kind create cluster --wait 300s
+        run: kind create cluster --image kindest/node:${{ env.KUBERNETES_VERSION }} --wait 300s
       - name: Set KUBECONFIG
         run: |
           mkdir -p /home/runner/.kube
@@ -118,7 +119,7 @@ jobs:
           go-version-file: go.mod
       - name: Setup kind
         if: ${{ env.USE_EXISTING_CLUSTER != 'true' }}
-        run: kind create cluster --wait 300s
+        run: kind create cluster --image kindest/node:${{ env.KUBERNETES_VERSION }} --wait 300s
       - name: Set KUBECONFIG
         run: |
           mkdir -p /home/runner/.kube

go.mod

@@ -41,7 +41,7 @@ require (
 	k8s.io/kube-aggregator v0.35.4
 	k8s.io/kubectl v0.35.4
 	k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2
-	open-cluster-management.io/addon-framework v1.3.0
+	open-cluster-management.io/addon-framework v1.3.1
 	open-cluster-management.io/api v1.3.1-0.20260824155636-3ff9ad2f868d
 	open-cluster-management.io/sdk-go v1.3.1-0.20260713072928-c812bc0c3b06
 	sigs.k8s.io/about-api v0.0.0-20250131010323-518069c31c03

go.sum

@@ -610,8 +610,8 @@ k8s.io/streaming v0.36.3 h1:9rAaqBk0C0Pc7+/fqGekj07NV+/Xrew58p647A0JT8w=
 k8s.io/streaming v0.36.3/go.mod h1:z6fV3D+NVkoeqRMtWwlUZK6U17SY/LqNzOxWL6GyR/s=
 k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2 h1:AZYQSJemyQB5eRxqcPky+/7EdBj0xi3g0ZcxxJ7vbWU=
 k8s.io/utils v0.0.0-20260210185600-b8788abfbbc2/go.mod h1:xDxuJ0whA3d0I4mf/C4ppKHxXynQ+fxnkmQH0vTHnuk=
-open-cluster-management.io/addon-framework v1.3.0 h1:rqW8Dl2Gcbac+8C4mQhOtbbmXb+JbIp3jPwP6AA5DBw=
-open-cluster-management.io/addon-framework v1.3.0/go.mod h1:UP3lXnEKduMF+6CTPJKzsIahrO6Hd55MSPhDYo1OwaM=
+open-cluster-management.io/addon-framework v1.3.1 h1:z5lf/Rx19UrHSYmxXg3q7o6yyFqEUbgf6fFhxbR+qq4=
+open-cluster-management.io/addon-framework v1.3.1/go.mod h1:UP3lXnEKduMF+6CTPJKzsIahrO6Hd55MSPhDYo1OwaM=
 open-cluster-management.io/api v1.3.1-0.20260824155636-3ff9ad2f868d h1:ggO+UQBCE/hNxjTVdw4CUu2YWN/llXRJ7v+lHBrOXeo=
 open-cluster-management.io/api v1.3.1-0.20260824155636-3ff9ad2f868d/go.mod h1:/qLKuMbMS1+MpirTjaw4BoS3INKG8jV3s5J7iju48tg=
 open-cluster-management.io/sdk-go v1.3.1-0.20260713072928-c812bc0c3b06 h1:gpk3LkPj0zMI8lt0A619JQ5KzB5RjxQob5eiF7zeZ28=

pkg/work/spoke/statusfeedback/reader_test.go

@@ -103,6 +103,24 @@ const (
 		}
 	}
 	`
+	failedJobJson = `
+	{
+		"apiVersion": "batch/v1",
+		"kind": "Job",
+		"metadata": {
+			"name": "test"
+		},
+		"status": {
+			"conditions": [
+				{
+					"status": "True",
+					"type": "Failed"
+				}
+			],
+			"failed": 6
+		}
+	}
+	`
 	podJson = `
 	{
 		"apiVersion": "v1",
@@ -269,6 +287,21 @@ func TestStatusReader(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:        "Failed Job values",
+			object:      unstrctureObject(failedJobJson),
+			rule:        workapiv1.FeedbackRule{Type: workapiv1.WellKnownStatusType},
+			expectError: false,
+			expectedValue: []workapiv1.FeedbackValue{
+				{
+					Name: "JobFailed",
+					Value: workapiv1.FieldValue{
+						Type:   workapiv1.String,
+						String: pointer.String("True"),
+					},
+				},
+			},
+		},
 		{
 			name:        "Pod values",
 			object:      unstrctureObject(podJson),

pkg/work/spoke/statusfeedback/rules/rule.go

@@ -49,6 +49,10 @@ var jobRule = []workapiv1.JsonPath{
 		Name: "JobComplete",
 		Path: `.status.conditions[?(@.type=="Complete")].status`,
 	},
+	{
+		Name: "JobFailed",
+		Path: `.status.conditions[?(@.type=="Failed")].status`,
+	},
 	{
 		Name: "JobSucceeded",
 		Path: `.status.succeeded`,

vendor/modules.txt

@@ -1935,7 +1935,7 @@ k8s.io/utils/path
 k8s.io/utils/pointer
 k8s.io/utils/ptr
 k8s.io/utils/trace
-# open-cluster-management.io/addon-framework v1.3.0
+# open-cluster-management.io/addon-framework v1.3.1
 ## explicit; go 1.25.0
 open-cluster-management.io/addon-framework/pkg/addonfactory
 open-cluster-management.io/addon-framework/pkg/addonmanager

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/agentdeploy/controller.go

@@ -285,6 +285,7 @@ func (c *addonDeployController) sync(ctx context.Context, syncCtx factory.SyncCo
 				addonapiv1beta1.ManagedClusterAddOnManifestApplied,
 			),
 			applyWork:  c.applyWork,
+			deleteWork: c.workApplier.Delete,
 			agentAddon: agentAddon},
 		&hostedHookSyncer{
 			buildWorks: c.buildHookManifestWorkFunc(

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/agentdeploy/default_hook_sync.go

@@ -18,6 +18,7 @@ type defaultHookSyncer struct {
 	buildWorks buildDeployHookFunc
 	applyWork  func(ctx context.Context, appliedType string,
 		work *workapiv1.ManifestWork, addon *addonapiv1beta1.ManagedClusterAddOn) (*workapiv1.ManifestWork, error)
+	deleteWork func(ctx context.Context, workNamespace, workName string) error
 	agentAddon agent.AgentAddon
 }
 
@@ -64,6 +65,28 @@ func (s *defaultHookSyncer) sync(ctx context.Context,
 		return addon, nil
 	}
 
+	// The hook has not completed. If the hook resource has reached a terminal
+	// failed state (e.g. the pod was evicted due to node pressure, its node
+	// became unreachable, or the job exhausted its backoffLimit), the work-agent
+	// will not recreate it on its own: the resource still exists with an
+	// unchanged spec, so a plain re-apply is a no-op. Delete the hook
+	// manifestWork so it is rebuilt and re-applied on a subsequent reconcile,
+	// which recreates a fresh hook pod/job. This retries indefinitely until the
+	// hook eventually succeeds, so the pre-delete hook finalizer is never left
+	// dangling because of a transient eviction.
+	if hookWorkIsFailed(hookWork) && hookWork.DeletionTimestamp.IsZero() {
+		if err = s.deleteWork(ctx, hookWork.Namespace, hookWork.Name); err != nil {
+			return addon, err
+		}
+		meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
+			Type:    addonapiv1beta1.ManagedClusterAddOnHookManifestCompleted,
+			Status:  metav1.ConditionFalse,
+			Reason:  "HookManifestFailedRetrying",
+			Message: fmt.Sprintf("hook manifestWork %v failed and is being recreated to retry.", hookWork.Name),
+		})
+		return addon, nil
+	}
+
 	meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
 		Type:    addonapiv1beta1.ManagedClusterAddOnHookManifestCompleted,
 		Status:  metav1.ConditionFalse,

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/agentdeploy/hosted_hook_sync.go

@@ -112,14 +112,32 @@ func (s *hostedHookSyncer) sync(ctx context.Context,
 	}
 
 	// TODO: will surface more message here
-	if hookWorkIsCompleted(hookWork) {
+	switch {
+	case hookWorkIsCompleted(hookWork):
 		meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
 			Type:    addonapiv1beta1.ManagedClusterAddOnHookManifestCompleted,
 			Status:  metav1.ConditionTrue,
 			Reason:  "HookManifestIsCompleted",
 			Message: fmt.Sprintf("hook manifestWork %v is completed.", hookWork.Name),
 		})
-	} else {
+	case hookWorkIsFailed(hookWork) && hookWork.DeletionTimestamp.IsZero():
+		// The hook resource has reached a terminal failed state (e.g. the pod was
+		// evicted due to node pressure, its node became unreachable, or the job
+		// exhausted its backoffLimit). The work-agent will not recreate it on its
+		// own because the resource still exists with an unchanged spec. Delete the
+		// hook manifestWork so it is rebuilt and re-applied on a subsequent
+		// reconcile, recreating a fresh hook pod/job and retrying indefinitely
+		// until it succeeds.
+		if err = s.deleteWork(ctx, hookWork.Namespace, hookWork.Name); err != nil {
+			return addon, err
+		}
+		meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
+			Type:    addonapiv1beta1.ManagedClusterAddOnHookManifestCompleted,
+			Status:  metav1.ConditionFalse,
+			Reason:  "HookManifestFailedRetrying",
+			Message: fmt.Sprintf("hook manifestWork %v failed and is being recreated to retry.", hookWork.Name),
+		})
+	default:
 		meta.SetStatusCondition(&addon.Status.Conditions, metav1.Condition{
 			Type:    addonapiv1beta1.ManagedClusterAddOnHookManifestCompleted,
 			Status:  metav1.ConditionFalse,

vendor/open-cluster-management.io/addon-framework/pkg/addonmanager/controllers/agentdeploy/utils.go

@@ -11,6 +11,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/klog/v2"
+
+	corev1 "k8s.io/api/core/v1"
 	addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
 	addonapiv1beta1 "open-cluster-management.io/api/addon/v1beta1"
 	clusterv1 "open-cluster-management.io/api/cluster/v1"
@@ -22,6 +24,12 @@ import (
 	"open-cluster-management.io/addon-framework/pkg/utils"
 )
 
+// Work API resource names for the pre-delete hook resource kinds we support.
+const (
+	hookResourceJobs = "jobs"
+	hookResourcePods = "pods"
+)
+
 func addonHasFinalizer(addon *addonapiv1beta1.ManagedClusterAddOn, finalizer string) bool {
 	for _, f := range addon.Finalizers {
 		if f == finalizer {
@@ -116,9 +124,9 @@ func (b *addonWorksBuilder) isPreDeleteHookObject(obj runtime.Object) (bool, *wo
 	gvk := obj.GetObjectKind().GroupVersionKind()
 	switch gvk.Kind {
 	case "Job":
-		resource = "jobs"
+		resource = hookResourceJobs
 	case "Pod":
-		resource = "pods"
+		resource = hookResourcePods
 	default:
 		return false, nil
 	}
@@ -387,19 +395,20 @@ func FindManifestValue(
 	identifier workapiv1.ResourceIdentifier,
 	valueName string) workapiv1.FieldValue {
 	for _, manifest := range resourceStatus.Manifests {
-		values := manifest.StatusFeedbacks.Values
-		if len(values) == 0 {
-			return workapiv1.FieldValue{}
-		}
+		// Match the resource first. A manifest that does not match the requested
+		// identifier - including one that has no feedback values reported yet -
+		// must not short-circuit the search, otherwise a later matching resource
+		// (e.g. a pod that reports PodPhase=Failed) would be missed.
 		resourceMeta := manifest.ResourceMeta
-		if identifier.Group == resourceMeta.Group &&
-			identifier.Resource == resourceMeta.Resource &&
-			identifier.Name == resourceMeta.Name &&
-			identifier.Namespace == resourceMeta.Namespace {
-			for _, v := range values {
-				if v.Name == valueName {
-					return v.Value
-				}
+		if identifier.Group != resourceMeta.Group ||
+			identifier.Resource != resourceMeta.Resource ||
+			identifier.Name != resourceMeta.Name ||
+			identifier.Namespace != resourceMeta.Namespace {
+			continue
+		}
+		for _, v := range manifest.StatusFeedbacks.Values {
+			if v.Name == valueName {
+				return v.Value
 			}
 		}
 	}
@@ -425,7 +434,7 @@ func hookWorkIsCompleted(hookWork *workapiv1.ManifestWork) bool {
 	}
 	for _, manifestConfig := range hookWork.Spec.ManifestConfigs {
 		switch manifestConfig.ResourceIdentifier.Resource {
-		case "jobs":
+		case hookResourceJobs:
 			value := FindManifestValue(hookWork.Status.ResourceStatus, manifestConfig.ResourceIdentifier, "JobComplete")
 			if value.Type == "" {
 				return false
@@ -433,11 +442,11 @@ func hookWorkIsCompleted(hookWork *workapiv1.ManifestWork) bool {
 			if value.String == nil {
 				return false
 			}
-			if *value.String != "True" {
+			if *value.String != string(metav1.ConditionTrue) {
 				return false
 			}
 
-		case "pods":
+		case hookResourcePods:
 			value := FindManifestValue(hookWork.Status.ResourceStatus, manifestConfig.ResourceIdentifier, "PodPhase")
 			if value.Type == "" {
 				return false
@@ -445,7 +454,7 @@ func hookWorkIsCompleted(hookWork *workapiv1.ManifestWork) bool {
 			if value.String == nil {
 				return false
 			}
-			if *value.String != "Succeeded" {
+			if *value.String != string(corev1.PodSucceeded) {
 				return false
 			}
 		default:
@@ -456,6 +465,63 @@ func hookWorkIsCompleted(hookWork *workapiv1.ManifestWork) bool {
 	return true
 }
 
+// hookWorkIsFailed checks whether any hook resource has reached a terminal
+// failed state and therefore should be recreated/retried.
+//
+// We deliberately treat *any* terminal, non-succeeded state as a failure that
+// warrants a retry, because a hook that never runs to completion will otherwise
+// block removal of the pre-delete hook finalizer forever (and, in turn, block
+// deletion of the ManagedClusterAddOn and its owning ManagedCluster). This
+// covers, among others:
+//   - pods evicted due to node pressure (MemoryPressure/DiskPressure), which end
+//     up in phase "Failed" with reason "Evicted";
+//   - pods whose status can no longer be determined, phase "Unknown" (e.g. the
+//     node hosting the pod became unreachable);
+//   - jobs that have exhausted their backoffLimit and report a "Failed"
+//     condition.
+//
+// A hook is only considered failed once the work-agent has reported a definitive
+// terminal state; in-progress states (Pending/Running, or no status reported
+// yet) are not treated as failures.
+func hookWorkIsFailed(hookWork *workapiv1.ManifestWork) bool {
+	if hookWork == nil {
+		return false
+	}
+	if !meta.IsStatusConditionTrue(hookWork.Status.Conditions, workapiv1.WorkAvailable) {
+		return false
+	}
+	if len(hookWork.Spec.ManifestConfigs) == 0 {
+		return false
+	}
+
+	for _, manifestConfig := range hookWork.Spec.ManifestConfigs {
+		switch manifestConfig.ResourceIdentifier.Resource {
+		case hookResourceJobs:
+			// A job reports a "Failed" condition once it has exhausted its
+			// backoffLimit (or hit an activeDeadlineSeconds/podFailurePolicy).
+			value := FindManifestValue(hookWork.Status.ResourceStatus, manifestConfig.ResourceIdentifier, "JobFailed")
+			if value.Type == workapiv1.String && value.String != nil && *value.String == string(metav1.ConditionTrue) {
+				return true
+			}
+		case hookResourcePods:
+			// A pod is terminally failed when its phase is "Failed" (this
+			// includes evicted pods) or "Unknown" (the pod's status can no
+			// longer be determined, e.g. its node is unreachable).
+			value := FindManifestValue(hookWork.Status.ResourceStatus, manifestConfig.ResourceIdentifier, "PodPhase")
+			if value.Type == workapiv1.String && value.String != nil &&
+				(*value.String == string(corev1.PodFailed) || *value.String == string(corev1.PodUnknown)) {
+				return true
+			}
+		default:
+			// Unsupported hook resource kinds cannot be evaluated for failure;
+			// leave them to hookWorkIsCompleted's handling.
+			continue
+		}
+	}
+
+	return false
+}
+
 func newAddonWorkObjectMeta(namePrefix, addonName, addonNamespace, workNamespace string,
 	owner *metav1.OwnerReference) workbuilder.GenerateManifestWorkObjectMeta {
 	return func(index int) metav1.ObjectMeta {