diff --git a/pkg/controllers/nodeclaim/lifecycle/initialization.go b/pkg/controllers/nodeclaim/lifecycle/initialization.go index de22b0188a..4a4405fbf3 100644 --- a/pkg/controllers/nodeclaim/lifecycle/initialization.go +++ b/pkg/controllers/nodeclaim/lifecycle/initialization.go @@ -45,7 +45,9 @@ type Initialization struct { // c) all extended resources have been registered // This method handles both nil nodepools and nodes without extended resources gracefully. func (i *Initialization) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { - if !nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsUnknown() { + if cond := nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized); !cond.IsUnknown() { + // Ensure that we always set the status condition to the latest generation + nodeClaim.StatusConditions().Set(*cond) return reconcile.Result{}, nil } if !nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue() { diff --git a/pkg/controllers/nodeclaim/lifecycle/launch.go b/pkg/controllers/nodeclaim/lifecycle/launch.go index fc5fe0baf3..381b530562 100644 --- a/pkg/controllers/nodeclaim/lifecycle/launch.go +++ b/pkg/controllers/nodeclaim/lifecycle/launch.go @@ -43,7 +43,9 @@ type Launch struct { } func (l *Launch) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { - if !nodeClaim.StatusConditions().Get(v1.ConditionTypeLaunched).IsUnknown() { + if cond := nodeClaim.StatusConditions().Get(v1.ConditionTypeLaunched); !cond.IsUnknown() { + // Ensure that we always set the status condition to the latest generation + nodeClaim.StatusConditions().Set(*cond) return reconcile.Result{}, nil } diff --git a/pkg/controllers/nodeclaim/lifecycle/registration.go b/pkg/controllers/nodeclaim/lifecycle/registration.go index c23cfe7799..0cbcaf156e 100644 --- a/pkg/controllers/nodeclaim/lifecycle/registration.go +++ b/pkg/controllers/nodeclaim/lifecycle/registration.go @@ -41,7 +41,9 @@ type Registration struct { } func (r *Registration) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (reconcile.Result, error) { - if !nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsUnknown() { + if cond := nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered); !cond.IsUnknown() { + // Ensure that we always set the status condition to the latest generation + nodeClaim.StatusConditions().Set(*cond) return reconcile.Result{}, nil } ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("provider-id", nodeClaim.Status.ProviderID)) diff --git a/pkg/controllers/nodeclaim/lifecycle/suite_test.go b/pkg/controllers/nodeclaim/lifecycle/suite_test.go index ffba573a5f..eeca378310 100644 --- a/pkg/controllers/nodeclaim/lifecycle/suite_test.go +++ b/pkg/controllers/nodeclaim/lifecycle/suite_test.go @@ -21,9 +21,13 @@ import ( "testing" "time" + "github.com/awslabs/operatorpkg/status" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/samber/lo" + corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" clock "k8s.io/utils/clock/testing" @@ -52,9 +56,21 @@ func TestAPIs(t *testing.T) { RunSpecs(t, "Lifecycle") } +func removeNodeClaimImmutabilityValidation(crds ...*apiextensionsv1.CustomResourceDefinition) []*apiextensionsv1.CustomResourceDefinition { + for _, crd := range crds { + if crd.Name != "nodeclaims.karpenter.sh" { + continue + } + overrideProperties := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"] + overrideProperties.XValidations = []apiextensionsv1.ValidationRule{} + crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"] = overrideProperties + } + return crds +} + var _ = BeforeSuite(func() { fakeClock = clock.NewFakeClock(time.Now()) - env = test.NewEnvironment(test.WithCRDs(apis.CRDs...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeProviderIDFieldIndexer(ctx))) + env = test.NewEnvironment(test.WithCRDs(removeNodeClaimImmutabilityValidation(apis.CRDs...)...), test.WithCRDs(v1alpha1.CRDs...), test.WithFieldIndexers(test.NodeProviderIDFieldIndexer(ctx))) ctx = options.ToContext(ctx, test.Options()) cloudProvider = fake.NewCloudProvider() @@ -120,4 +136,74 @@ var _ = Describe("Finalizer", func() { Expect(ok).To(BeFalse()) }) }) + It("should update observedGeneration if generation increases after all conditions are marked True", func() { + nodeClaim := test.NodeClaim(v1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + v1.NodePoolLabelKey: nodePool.Name, + }, + }, + Spec: v1.NodeClaimSpec{ + Resources: v1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + corev1.ResourceMemory: resource.MustParse("50Mi"), + corev1.ResourcePods: resource.MustParse("5"), + }, + }, + }, + }) + ExpectApplied(ctx, env.Client, nodePool, nodeClaim) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + + node := test.Node(test.NodeOptions{ + ProviderID: nodeClaim.Status.ProviderID, + Taints: []corev1.Taint{v1.UnregisteredNoExecuteTaint}, + }) + ExpectApplied(ctx, env.Client, node) + + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsUnknown()).To(BeTrue()) + + node = ExpectExists(ctx, env.Client, node) + node.Status.Capacity = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("100Mi"), + corev1.ResourcePods: resource.MustParse("110"), + } + node.Status.Allocatable = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("80Mi"), + corev1.ResourcePods: resource.MustParse("110"), + } + ExpectApplied(ctx, env.Client, node) + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsTrue()).To(BeTrue()) + + // Change a field to increase the generation + nodeClaim.Spec.Taints = append(nodeClaim.Spec.Taints, corev1.Taint{Key: "test", Value: "value", Effect: corev1.TaintEffectNoSchedule}) + ExpectApplied(ctx, env.Client, nodeClaim) + + // Expect that when the object re-reconciles, all of the observedGenerations across all status condition match + ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim) + nodeClaim = ExpectExists(ctx, env.Client, nodeClaim) + + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeLaunched).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeLaunched).ObservedGeneration).To(Equal(nodeClaim.Generation)) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeRegistered).ObservedGeneration).To(Equal(nodeClaim.Generation)) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized).ObservedGeneration).To(Equal(nodeClaim.Generation)) + + Expect(nodeClaim.StatusConditions().Get(status.ConditionReady).IsTrue()).To(BeTrue()) + Expect(nodeClaim.StatusConditions().Get(status.ConditionReady).ObservedGeneration).To(Equal(nodeClaim.Generation)) + }) })