Finally decided to prevent tests from flaking 🤷

docs/README.md

@@ -424,5 +424,5 @@ its target, and indicates whether or not those conditions are met.</p>
 <hr/>
 <p><em>
 Generated with <code>gen-crd-api-reference-docs</code>
-on git commit <code>aa1e430</code>.
+on git commit <code>350646c</code>.
 </em></p>

pkg/controllers/allocation/suite_test.go

@@ -47,11 +47,10 @@ var env = test.NewEnvironment(func(e *test.Environment) {
 	cloudProvider := &fake.CloudProvider{}
 	registry.RegisterOrDie(cloudProvider)
 	controller = NewController(
-		e.Manager.GetClient(),
-		corev1.NewForConfigOrDie(e.Manager.GetConfig()),
+		e.Client,
+		corev1.NewForConfigOrDie(e.Config),
 		cloudProvider,
 	)
-	e.Manager.RegisterControllers(controller)
 })
 
 var _ = BeforeSuite(func() {
@@ -86,43 +85,38 @@ var _ = Describe("Allocation", func() {
 		Context("Zones", func() {
 			It("should default to a cluster zone", func() {
 				// Setup
-				pod := AttemptProvisioning(env.Client, provisioner, test.PendingPod())
+				pods := ExpectProvisioningSucceeded(env.Client, controller, provisioner, test.PendingPod())
 				// Assertions
-				node := ExpectNodeExists(env.Client, pod.Spec.NodeName)
+				node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
 				Expect(node.Spec.ProviderID).To(ContainSubstring("test-zone-1"))
 			})
 			It("should default to a provisioner's zone", func() {
 				// Setup
 				provisioner.Spec.Zones = []string{"test-zone-2"}
-				pod := AttemptProvisioning(env.Client, provisioner, test.PendingPod())
+				pods := ExpectProvisioningSucceeded(env.Client, controller, provisioner, test.PendingPod())
 				// Assertions
-				node := ExpectNodeExists(env.Client, pod.Spec.NodeName)
+				node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
 				Expect(node.Spec.ProviderID).To(ContainSubstring("test-zone-2"))
 			})
 			It("should allow a pod to override the zone", func() {
 				// Setup
 				provisioner.Spec.Zones = []string{"test-zone-1"}
-				pod := AttemptProvisioning(env.Client, provisioner,
+				pods := ExpectProvisioningSucceeded(env.Client, controller, provisioner,
 					test.PendingPod(test.PodOptions{NodeSelector: map[string]string{v1alpha2.ZoneLabelKey: "test-zone-2"}}),
 				)
 				// Assertions
-				node := ExpectNodeExists(env.Client, pod.Spec.NodeName)
+				node := ExpectNodeExists(env.Client, pods[0].Spec.NodeName)
 				Expect(node.Spec.ProviderID).To(ContainSubstring("test-zone-2"))
 			})
 		})
 		It("should provision nodes for unconstrained pods", func() {
-			pods := []*v1.Pod{test.PendingPod(), test.PendingPod()}
-			for _, pod := range pods {
-				ExpectCreatedWithStatus(env.Client, pod)
-			}
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
-
+			pods := ExpectProvisioningSucceeded(env.Client, controller, provisioner,
+				test.PendingPod(), test.PendingPod(),
+			)
 			nodes := &v1.NodeList{}
 			Expect(env.Client.List(ctx, nodes)).To(Succeed())
 			Expect(len(nodes.Items)).To(Equal(1))
-			for _, object := range pods {
-				pod := ExpectPodExists(env.Client, object.GetName(), object.GetNamespace())
+			for _, pod := range pods {
 				Expect(pod.Spec.NodeName).To(Equal(nodes.Items[0].Name))
 			}
 		})
@@ -162,8 +156,7 @@ var _ = Describe("Allocation", func() {
 			ExpectCreatedWithStatus(env.Client, schedulable...)
 			ExpectCreatedWithStatus(env.Client, coschedulable...)
 			ExpectCreatedWithStatus(env.Client, unschedulable...)
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
+			ExpectReconcileSucceeded(controller, provisioner)
 
 			nodes := &v1.NodeList{}
 			Expect(env.Client.List(ctx, nodes)).To(Succeed())
@@ -210,8 +203,7 @@ var _ = Describe("Allocation", func() {
 			}
 			ExpectCreatedWithStatus(env.Client, schedulable...)
 			ExpectCreatedWithStatus(env.Client, unschedulable...)
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
+			ExpectReconcileSucceeded(controller, provisioner)
 
 			nodes := &v1.NodeList{}
 			Expect(env.Client.List(ctx, nodes)).To(Succeed())
@@ -226,7 +218,7 @@ var _ = Describe("Allocation", func() {
 			}
 		})
 		It("should provision nodes for accelerators", func() {
-			schedulable := []client.Object{
+			pods := ExpectProvisioningSucceeded(env.Client, controller, provisioner,
 				test.PendingPod(test.PodOptions{
 					ResourceRequirements: v1.ResourceRequirements{Limits: v1.ResourceList{resources.NvidiaGPU: resource.MustParse("1")}},
 				}),
@@ -236,15 +228,11 @@ var _ = Describe("Allocation", func() {
 				test.PendingPod(test.PodOptions{
 					ResourceRequirements: v1.ResourceRequirements{Limits: v1.ResourceList{resources.AWSNeuron: resource.MustParse("1")}},
 				}),
-			}
-			ExpectCreatedWithStatus(env.Client, schedulable...)
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
-
+			)
 			nodes := &v1.NodeList{}
 			Expect(env.Client.List(ctx, nodes)).To(Succeed())
 			Expect(len(nodes.Items)).To(Equal(3))
-			for _, pod := range schedulable {
+			for _, pod := range pods {
 				scheduled := ExpectPodExists(env.Client, pod.GetName(), pod.GetNamespace())
 				ExpectNodeExists(env.Client, scheduled.Spec.NodeName)
 			}
@@ -276,8 +264,7 @@ var _ = Describe("Allocation", func() {
 			}
 			ExpectCreatedWithStatus(env.Client, daemonsets...)
 			ExpectCreatedWithStatus(env.Client, schedulable...)
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
+			ExpectReconcileSucceeded(controller, provisioner)
 
 			nodes := &v1.NodeList{}
 			Expect(env.Client.List(ctx, nodes)).To(Succeed())

pkg/controllers/controller.go

@@ -18,11 +18,9 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/awslabs/karpenter/pkg/utils/conditions"
 	"go.uber.org/zap"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	"knative.dev/pkg/apis"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -51,15 +49,6 @@ func (c *GenericController) Reconcile(ctx context.Context, req reconcile.Request
 	if err != nil {
 		zap.S().Errorf("Controller failed to reconcile kind %s, %s", resource.GetObjectKind().GroupVersionKind().Kind, err.Error())
 	}
-	// 4. Set status based on results of reconcile
-	if conditionsAccessor, ok := resource.(apis.ConditionsAccessor); ok {
-		m := apis.NewLivingConditionSet(conditions.Active).Manage(conditionsAccessor)
-		if err != nil {
-			m.MarkFalse(conditions.Active, err.Error(), "")
-		} else {
-			m.MarkTrue(conditions.Active)
-		}
-	}
 	// 5. Update Status using a merge patch
 	// If the controller is reconciling nodes, don't patch
 	if _, ok := resource.(*v1.Node); !ok {

pkg/controllers/manager.go

@@ -19,12 +19,8 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/awslabs/karpenter/pkg/apis"
-
 	"golang.org/x/time/rate"
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/util/workqueue"
 	controllerruntime "sigs.k8s.io/controller-runtime"
@@ -34,22 +30,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 )
 
-var (
-	scheme = runtime.NewScheme()
-)
-
-func init() {
-	_ = clientgoscheme.AddToScheme(scheme)
-	_ = apis.AddToScheme(scheme)
-}
-
 type GenericControllerManager struct {
 	manager.Manager
 }
 
 // NewManagerOrDie instantiates a controller manager or panics
 func NewManagerOrDie(config *rest.Config, options controllerruntime.Options) Manager {
-	options.Scheme = scheme
 	manager, err := controllerruntime.NewManager(config, options)
 	if err != nil {
 		panic(fmt.Sprintf("Failed to create controller manager, %s", err.Error()))

pkg/controllers/reallocation/suite_test.go

@@ -46,11 +46,10 @@ var env = test.NewEnvironment(func(e *test.Environment) {
 	cloudProvider := &fake.CloudProvider{}
 	registry.RegisterOrDie(cloudProvider)
 	controller = NewController(
-		e.Manager.GetClient(),
-		corev1.NewForConfigOrDie(e.Manager.GetConfig()),
+		e.Client,
+		corev1.NewForConfigOrDie(e.Config),
 		cloudProvider,
 	)
-	e.Manager.RegisterControllers(controller)
 })
 
 var _ = BeforeSuite(func() {
@@ -79,7 +78,7 @@ var _ = Describe("Reallocation", func() {
 	})
 
 	AfterEach(func() {
-		ExpectCleanedUp(env.Manager.GetClient())
+		ExpectCleanedUp(env.Client)
 	})
 
 	Context("Reconciliation", func() {
@@ -91,9 +90,7 @@ var _ = Describe("Reallocation", func() {
 				},
 			})
 			ExpectCreatedWithStatus(env.Client, node)
-
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
+			ExpectReconcileSucceeded(controller, provisioner)
 
 			updatedNode := &v1.Node{}
 			Expect(env.Client.Get(ctx, client.ObjectKey{Name: node.Name}, updatedNode)).To(Succeed())
@@ -111,18 +108,14 @@ var _ = Describe("Reallocation", func() {
 					v1alpha2.ProvisionerTTLKey: time.Now().Add(time.Duration(100) * time.Second).Format(time.RFC3339),
 				},
 			})
-			pod := test.Pod(test.PodOptions{
+			ExpectCreatedWithStatus(env.Client, node)
+			ExpectCreatedWithStatus(env.Client, test.Pod(test.PodOptions{
 				Name:       strings.ToLower(randomdata.SillyName()),
 				Namespace:  provisioner.Namespace,
 				NodeName:   node.Name,
 				Conditions: []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionTrue}},
-			})
-
-			ExpectCreatedWithStatus(env.Client, node)
-			ExpectCreatedWithStatus(env.Client, pod)
-
-			ExpectCreated(env.Client, provisioner)
-			ExpectEventuallyReconciled(env.Client, provisioner)
+			}))
+			ExpectReconcileSucceeded(controller, provisioner)
 
 			updatedNode := &v1.Node{}
 			Expect(env.Client.Get(ctx, client.ObjectKey{Name: node.Name}, updatedNode)).To(Succeed())

pkg/controllers/termination/suite_test.go

@@ -41,11 +41,10 @@ var env = test.NewEnvironment(func(e *test.Environment) {
 	cloudProvider := &fake.CloudProvider{}
 	registry.RegisterOrDie(cloudProvider)
 	controller = NewController(
-		e.Manager.GetClient(),
-		corev1.NewForConfigOrDie(e.Manager.GetConfig()),
+		e.Client,
+		corev1.NewForConfigOrDie(e.Config),
 		cloudProvider,
 	)
-	e.Manager.RegisterControllers(controller)
 })
 
 var _ = BeforeSuite(func() {
@@ -64,7 +63,7 @@ var _ = Describe("Termination", func() {
 	})
 
 	AfterEach(func() {
-		ExpectCleanedUp(env.Manager.GetClient())
+		ExpectCleanedUp(env.Client)
 	})
 
 	Context("Reconciliation", func() {
@@ -76,8 +75,10 @@ var _ = Describe("Termination", func() {
 					v1alpha2.ProvisionerNamespaceLabelKey: "default",
 				},
 			})
-			ExpectCreatedWithStatus(env.Client, node)
+			ExpectCreated(env.Client, node)
 			Expect(env.Client.Delete(ctx, node)).To(Succeed())
+			node = ExpectNodeExists(env.Client, node.Name)
+			ExpectReconcileSucceeded(controller, node)
 			ExpectNotFound(env.Client, node)
 		})
 		It("should not evict pods that tolerate unschedulable taint", func() {
@@ -88,15 +89,16 @@ var _ = Describe("Termination", func() {
 					v1alpha2.ProvisionerNamespaceLabelKey: "default",
 				},
 			})
-			pod := test.Pod(test.PodOptions{
+			ExpectCreated(env.Client, node)
+			ExpectCreated(env.Client, test.Pod(test.PodOptions{
 				NodeName:    node.Name,
 				Tolerations: []v1.Toleration{{Key: v1.TaintNodeUnschedulable, Operator: v1.TolerationOpExists, Effect: v1.TaintEffectNoSchedule}},
-			})
-			ExpectCreatedWithStatus(env.Client, node)
-			ExpectCreatedWithStatus(env.Client, pod)
+			}))
 
-			pods := &v1.PodList{}
 			Expect(env.Client.Delete(ctx, node)).To(Succeed())
+			node = ExpectNodeExists(env.Client, node.Name)
+			ExpectReconcileSucceeded(controller, node)
+			pods := &v1.PodList{}
 			Expect(env.Client.List(ctx, pods, client.MatchingFields{"spec.nodeName": node.Name})).To(Succeed())
 			Expect(pods.Items).To(HaveLen(1))
 			ExpectNotFound(env.Client, node)

pkg/test/environment.go

@@ -17,17 +17,27 @@ import (
 	"fmt"
 	"sync"
 
-	"github.com/awslabs/karpenter/pkg/controllers"
+	"github.com/awslabs/karpenter/pkg/apis"
 	"github.com/awslabs/karpenter/pkg/utils/log"
 	"github.com/awslabs/karpenter/pkg/utils/project"
-	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	controllerruntimezap "sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
+var (
+	scheme = runtime.NewScheme()
+)
+
+func init() {
+	_ = clientgoscheme.AddToScheme(scheme)
+	_ = apis.AddToScheme(scheme)
+}
+
 /*
 Environment is for e2e local testing. It stands up an API Server, ETCD,
 and a controller-runtime manager. It's possible to run multiple environments
@@ -45,8 +55,7 @@ AfterSuite(func() { env.Stop() })
 */
 type Environment struct {
 	envtest.Environment
-	Manager controllers.Manager
-	Client  client.Client
+	Client client.Client
 
 	options []EnvironmentOption
 	ctx     context.Context
@@ -75,36 +84,20 @@ func NewEnvironment(options ...EnvironmentOption) *Environment {
 
 func (e *Environment) Start() (err error) {
 	// Environment
-	if _, err := e.Environment.Start(); err != nil {
+	if _, err = e.Environment.Start(); err != nil {
 		return fmt.Errorf("starting environment, %w", err)
 	}
 
-	// Manager
-	e.Manager = controllers.NewManagerOrDie(e.Config, controllerruntime.Options{
-		MetricsBindAddress: "0", // Skip the metrics server to avoid port conflicts for parallel testing
-	})
-
 	// Client
-	kubeClient, err := client.New(e.Manager.GetConfig(), client.Options{
-		Scheme: e.Manager.GetScheme(),
-		Mapper: e.Manager.GetRESTMapper(),
-	})
+	e.Client, err = client.New(e.Config, client.Options{Scheme: scheme})
 	if err != nil {
 		return err
 	}
-	e.Client = kubeClient
 
 	// options
 	for _, option := range e.options {
 		option(e)
 	}
-
-	// Start manager
-	go func() {
-		if err := e.Manager.Start(e.ctx); err != nil {
-			zap.S().Panic(err)
-		}
-	}()
 	return nil
 }