Skip to content

Commit

Permalink
fix: reorder drift checks to save on computation (aws#464)
Browse files Browse the repository at this point in the history
* reorder drift

* tests
  • Loading branch information
njtran authored Aug 10, 2023
1 parent ff6de1a commit 04ab9b9
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 6 deletions.
14 changes: 8 additions & 6 deletions pkg/controllers/machine/disruption/drift.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,16 +102,18 @@ func (d *Drift) Reconcile(ctx context.Context, provisioner *v1alpha5.Provisioner
// isDrifted will check if a machine is drifted from the fields in the provisioner.Spec and
// the cloudprovider
func (d *Drift) isDrifted(ctx context.Context, provisioner *v1alpha5.Provisioner, machine *v1alpha5.Machine) (cloudprovider.DriftReason, error) {
// First check for static drift or node requirements have drifted to save on API calls.
if reason := lo.FindOrElse([]cloudprovider.DriftReason{areStaticFieldsDrifted(provisioner, machine), areNodeRequirementsDrifted(provisioner, machine)}, "", func(i cloudprovider.DriftReason) bool {
return i != ""
}); reason != "" {
return reason, nil
}

driftedReason, err := d.cloudProvider.IsMachineDrifted(ctx, machine)
if err != nil {
return "", err
}

reason := lo.FindOrElse([]cloudprovider.DriftReason{driftedReason, areStaticFieldsDrifted(provisioner, machine), areNodeRequirementsDrifted(provisioner, machine)}, "", func(i cloudprovider.DriftReason) bool {
return i != ""
})

return reason, nil
return driftedReason, nil
}

// Eligible fields for static drift are described in the docs
Expand Down
28 changes: 28 additions & 0 deletions pkg/controllers/machine/disruption/drift_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"github.com/aws/karpenter-core/pkg/apis/settings"
"github.com/aws/karpenter-core/pkg/apis/v1alpha5"
"github.com/aws/karpenter-core/pkg/controllers/machine/disruption"
controllerprov "github.com/aws/karpenter-core/pkg/controllers/provisioner"
"github.com/aws/karpenter-core/pkg/test"

Expand Down Expand Up @@ -63,6 +64,33 @@ var _ = Describe("Drift", func() {
machine = ExpectExists(ctx, env.Client, machine)
Expect(machine.StatusConditions().GetCondition(v1alpha5.MachineDrifted).IsTrue()).To(BeTrue())
})
It("should detect static drift before cloud provider drift", func() {
cp.Drifted = "drifted"
provisioner.Annotations = lo.Assign(provisioner.Annotations, map[string]string{
v1alpha5.ProvisionerHashAnnotationKey: "123456789",
})
ExpectApplied(ctx, env.Client, provisioner, machine)
ExpectReconcileSucceeded(ctx, disruptionController, client.ObjectKeyFromObject(machine))

machine = ExpectExists(ctx, env.Client, machine)
Expect(machine.StatusConditions().GetCondition(v1alpha5.MachineDrifted).IsTrue()).To(BeTrue())
Expect(machine.StatusConditions().GetCondition(v1alpha5.MachineDrifted).Reason).To(Equal(string(disruption.ProvisionerStaticallyDrifted)))
})
It("should detect node requirement drift before cloud provider drift", func() {
cp.Drifted = "drifted"
provisioner.Spec.Requirements = []v1.NodeSelectorRequirement{
v1.NodeSelectorRequirement{
Key: v1.LabelInstanceTypeStable,
Operator: v1.NodeSelectorOpDoesNotExist,
},
}
ExpectApplied(ctx, env.Client, provisioner, machine)
ExpectReconcileSucceeded(ctx, disruptionController, client.ObjectKeyFromObject(machine))

machine = ExpectExists(ctx, env.Client, machine)
Expect(machine.StatusConditions().GetCondition(v1alpha5.MachineDrifted).IsTrue()).To(BeTrue())
Expect(machine.StatusConditions().GetCondition(v1alpha5.MachineDrifted).Reason).To(Equal(string(disruption.NodeRequirementDrifted)))
})
It("should not detect drift if the feature flag is disabled", func() {
cp.Drifted = "drifted"
ctx = settings.ToContext(ctx, test.Settings(settings.Settings{DriftEnabled: false}))
Expand Down

0 comments on commit 04ab9b9

Please sign in to comment.