Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Expose drifted nodeclaim condition status to printer column output #1846

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −20
Open

feat: Expose drifted nodeclaim condition status to printer column output #1846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions kwok/charts/crds/karpenter.sh_nodeclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ spec:
- jsonPath: .status.conditions[?(@.type=="Ready")].status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to tell, but I think that this PR might be in need of a rebase

name: Ready
type: string
- jsonPath: .status.conditions[?(@.type=="Drifted")].status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How common do you think it will be for folks to look this up? I definitely can see it as useful -- what do you think about starting this in the -o wide output with priority=1 and then elevating it if we hear from more folks that they'd like to see it in the non-wide output?

name: Drifted
type: string
- jsonPath: .metadata.creationTimestamp
name: Age
type: date
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/crds/karpenter.sh_nodeclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ spec:
- jsonPath: .status.conditions[?(@.type=="Ready")].status
name: Ready
type: string
- jsonPath: .status.conditions[?(@.type=="Drifted")].status
name: Drifted
type: string
- jsonPath: .metadata.creationTimestamp
name: Age
type: date
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/v1/nodeclaim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ type Provider = runtime.RawExtension
// +kubebuilder:printcolumn:name="Zone",type="string",JSONPath=".metadata.labels.topology\\.kubernetes\\.io/zone",description=""
// +kubebuilder:printcolumn:name="Node",type="string",JSONPath=".status.nodeName",description=""
// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type==\"Ready\")].status",description=""
// +kubebuilder:printcolumn:name="Drifted",type="string",JSONPath=".status.conditions[?(@.type==\"Drifted\")].status",description=""
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description=""
// +kubebuilder:printcolumn:name="ID",type="string",JSONPath=".status.providerID",priority=1,description=""
// +kubebuilder:printcolumn:name="NodePool",type="string",JSONPath=".metadata.labels.karpenter\\.sh/nodepool",priority=1,description=""
Expand Down
11 changes: 6 additions & 5 deletions pkg/controllers/nodeclaim/disruption/drift.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,13 @@ type Drift struct {
}

func (d *Drift) Reconcile(ctx context.Context, nodePool *v1.NodePool, nodeClaim *v1.NodeClaim) (reconcile.Result, error) {
hasDriftedCondition := nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted) != nil
hasDriftedCondition := nodeClaim.StatusConditions().IsTrue(v1.ConditionTypeDrifted)

// Prefer to set the Drifted condition to false instead of clearing for the printer column output.
// From here there are three scenarios to handle:
// 1. If NodeClaim is not launched, remove the drift status condition
// 1. If NodeClaim is not launched, set the drift status condition false
if !nodeClaim.StatusConditions().Get(v1.ConditionTypeLaunched).IsTrue() {
_ = nodeClaim.StatusConditions().Clear(v1.ConditionTypeDrifted)
_ = nodeClaim.StatusConditions().SetFalse(v1.ConditionTypeDrifted, v1.ConditionTypeDrifted, "")
if hasDriftedCondition {
log.FromContext(ctx).V(1).Info("removing drift status condition, isn't launched")
}
Expand All @@ -59,10 +60,10 @@ func (d *Drift) Reconcile(ctx context.Context, nodePool *v1.NodePool, nodeClaim
if err != nil {
return reconcile.Result{}, cloudprovider.IgnoreNodeClaimNotFoundError(fmt.Errorf("getting drift, %w", err))
}
// 2. Otherwise, if the NodeClaim isn't drifted, but has the status condition, remove it.
// 2. Otherwise, if the NodeClaim isn't drifted, but has the status condition, unset it
if driftedReason == "" {
_ = nodeClaim.StatusConditions().SetFalse(v1.ConditionTypeDrifted, v1.ConditionTypeDrifted, "")
if hasDriftedCondition {
_ = nodeClaim.StatusConditions().Clear(v1.ConditionTypeDrifted)
log.FromContext(ctx).V(1).Info("removing drifted status condition, not drifted")
}
return reconcile.Result{RequeueAfter: 5 * time.Minute}, nil
Expand Down
38 changes: 24 additions & 14 deletions pkg/controllers/nodeclaim/disruption/drift_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ var _ = Describe("Drift", func() {
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)

nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should remove the status condition from the nodeClaim when the nodeClaim launch condition is false", func() {
cp.Drifted = "drifted"
Expand All @@ -179,15 +179,15 @@ var _ = Describe("Drift", func() {
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)

nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should not detect drift if the nodePool does not exist", func() {
cp.Drifted = "drifted"
ExpectApplied(ctx, env.Client, nodeClaim)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)

nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should remove the status condition from the nodeClaim if the nodeClaim is no longer drifted", func() {
cp.Drifted = ""
Expand All @@ -197,7 +197,17 @@ var _ = Describe("Drift", func() {
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)

nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should set the drifted condition to false if unset after reconcile", func() {
cp.Drifted = ""
_ = nodeClaim.StatusConditions().Clear(v1.ConditionTypeDrifted)
ExpectApplied(ctx, env.Client, nodePool, nodeClaim)

ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)

nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
Context("NodeRequirement Drift", func() {
DescribeTable("",
Expand All @@ -210,7 +220,7 @@ var _ = Describe("Drift", func() {
ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())

nodePool.Spec.Template.Spec.Requirements = newNodePoolReq
ExpectApplied(ctx, env.Client, nodePool)
Expand All @@ -219,7 +229,7 @@ var _ = Describe("Drift", func() {
if drifted {
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeTrue())
} else {
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
}
},
Entry(
Expand Down Expand Up @@ -385,8 +395,8 @@ var _ = Describe("Drift", func() {
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaimTwo)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
nodeClaimTwo = ExpectExists(ctx, env.Client, nodeClaimTwo)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaimTwo.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
Expect(nodeClaimTwo.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())

// Removed Windows OS
nodePool.Spec.Template.Spec.Requirements = []v1.NodeSelectorRequirementWithMinValues{
Expand All @@ -397,7 +407,7 @@ var _ = Describe("Drift", func() {

ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())

ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaimTwo)
nodeClaimTwo = ExpectExists(ctx, env.Client, nodeClaimTwo)
Expand Down Expand Up @@ -459,7 +469,7 @@ var _ = Describe("Drift", func() {
ExpectObjectReconciled(ctx, env.Client, nodePoolController, nodePool)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())

nodePool = ExpectExists(ctx, env.Client, nodePool)
Expect(mergo.Merge(nodePool, changes, mergo.WithOverride)).To(Succeed())
Expand All @@ -483,7 +493,7 @@ var _ = Describe("Drift", func() {
ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should not return drifted if karpenter.sh/nodepool-hash annotation is not present on the NodeClaim", func() {
nodeClaim.ObjectMeta.Annotations = map[string]string{
Expand All @@ -492,7 +502,7 @@ var _ = Describe("Drift", func() {
ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should not return drifted if the NodeClaim's karpenter.sh/nodepool-hash-version annotation does not match the NodePool's", func() {
nodePool.ObjectMeta.Annotations = map[string]string{
Expand All @@ -506,7 +516,7 @@ var _ = Describe("Drift", func() {
ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
It("should not return drifted if karpenter.sh/nodepool-hash-version annotation is not present on the NodeClaim", func() {
nodeClaim.ObjectMeta.Annotations = map[string]string{
Expand All @@ -515,7 +525,7 @@ var _ = Describe("Drift", func() {
ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
})
})
})
2 changes: 1 addition & 1 deletion pkg/controllers/nodeclaim/disruption/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ var _ = Describe("Disruption", func() {
ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)

nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted)).To(BeNil())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeDrifted).IsTrue()).To(BeFalse())
Expect(nodeClaim.StatusConditions().Get(v1.ConditionTypeConsolidatable)).To(BeNil())
})
})
Loading