disable use of AppWrapper managedBy in multikueue

I discovered a bug in how the AppWrapper controller interprets managedBy. Disable use of it until we can fix and make a new release of AppWrapper.
kubernetes-sigs · Jan 30, 2025 · 1cbd704 · 1cbd704
1 parent 868eeb9
commit 1cbd704
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 6 deletions.
diff --git a/pkg/controller/jobs/appwrapper/appwrapper_controller.go b/pkg/controller/jobs/appwrapper/appwrapper_controller.go
@@ -159,7 +159,7 @@ func (aw *AppWrapper) PodsReady() bool {
 
 func (j *AppWrapper) CanDefaultManagedBy() bool {
 	jobSpecManagedBy := j.Spec.ManagedBy
-	return features.Enabled(features.MultiKueue) &&
+	return features.Enabled(features.MultiKueue) && HaveManagedBy &&
 		(jobSpecManagedBy == nil || *jobSpecManagedBy == awv1beta2.AppWrapperControllerName)
 }
 

diff --git a/pkg/controller/jobs/appwrapper/appwrapper_multikueue_adapter.go b/pkg/controller/jobs/appwrapper/appwrapper_multikueue_adapter.go
@@ -41,6 +41,9 @@ type multikueueAdapter struct{}
 
 var _ jobframework.MultiKueueAdapter = (*multikueueAdapter)(nil)
 
+// TODO: manangedBy in AppWrapper v1.0.0 mishandles its finalizers.  Disable until fixed appwrapper is released
+const HaveManagedBy = false
+
 func (b *multikueueAdapter) SyncJob(ctx context.Context, localClient client.Client, remoteClient client.Client, key types.NamespacedName, workloadName, origin string) error {
 	log := ctrl.LoggerFrom(ctx)
 
@@ -102,10 +105,13 @@ func (b *multikueueAdapter) GVK() schema.GroupVersionKind {
 }
 
 func (b *multikueueAdapter) KeepAdmissionCheckPending() bool {
-	return false
+	return !HaveManagedBy
 }
 
 func (b *multikueueAdapter) IsJobManagedByKueue(ctx context.Context, c client.Client, key types.NamespacedName) (bool, string, error) {
+	if !HaveManagedBy {
+		return true, "", nil
+	}
 	aw := awv1beta2.AppWrapper{}
 	err := c.Get(ctx, key, &aw)
 	if err != nil {

diff --git a/pkg/controller/jobs/appwrapper/appwrapper_multikueue_adapter_test.go b/pkg/controller/jobs/appwrapper/appwrapper_multikueue_adapter_test.go
@@ -152,8 +152,10 @@ func TestMultikueueAdapter(t *testing.T) {
 		},
 		"missing appwrapper is not considered managed": {
 			operation: func(ctx context.Context, adapter *multikueueAdapter, managerClient, workerClient client.Client) error {
-				if isManged, _, _ := adapter.IsJobManagedByKueue(ctx, managerClient, types.NamespacedName{Name: "aw1", Namespace: TestNamespace}); isManged {
-					return errors.New("expecting false")
+				if HaveManagedBy {
+					if isManged, _, _ := adapter.IsJobManagedByKueue(ctx, managerClient, types.NamespacedName{Name: "aw1", Namespace: TestNamespace}); isManged {
+						return errors.New("expecting false")
+					}
 				}
 				return nil
 			},
@@ -163,8 +165,10 @@ func TestMultikueueAdapter(t *testing.T) {
 				*baseAppWrapperBuilder.DeepCopy().Obj(),
 			},
 			operation: func(ctx context.Context, adapter *multikueueAdapter, managerClient, workerClient client.Client) error {
-				if isManged, _, _ := adapter.IsJobManagedByKueue(ctx, managerClient, types.NamespacedName{Name: "aw1", Namespace: TestNamespace}); isManged {
-					return errors.New("expecting false")
+				if HaveManagedBy {
+					if isManged, _, _ := adapter.IsJobManagedByKueue(ctx, managerClient, types.NamespacedName{Name: "aw1", Namespace: TestNamespace}); isManged {
+						return errors.New("expecting false")
+					}
 				}
 				return nil
 			},