Skip to content

Commit

Permalink
added constructor
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmdeal
jmdeal committed Aug 11, 2024
1 parent 97bb0cd commit b67ec92
Show file tree
Hide file tree
Showing 16 changed files with 111 additions and 85 deletions.
11 changes: 10 additions & 1 deletion pkg/apis/v1/duration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ package v1

import (
"encoding/json"
"fmt"
"slices"
"time"

"github.com/samber/lo"
)

const Never = "Never"
Expand All @@ -32,7 +35,13 @@ type NillableDuration struct {

// Raw is used to ensure we remarshal the NillableDuration in the same format it was specified.
// This ensures tools like Flux and ArgoCD don't mistakenly detect drift due to our conversion webhooks.
Raw []byte
Raw []byte `hash:"ignore"`
}

func MustParseNillableDuration(val string) NillableDuration {
nd := NillableDuration{}
lo.Must0(json.Unmarshal([]byte(fmt.Sprintf("%q", val)), &nd))
return nd
}

// UnmarshalJSON implements the json.Unmarshaller interface.
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/v1/nodeclaim_conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ var _ = Describe("Convert V1beta1 to V1 NodeClaim API", func() {

BeforeEach(func() {
v1nodePool = test.NodePool()
v1nodePool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(30 * time.Minute)}
v1nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30m")
v1nodeclaim = &NodeClaim{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
Expand Down Expand Up @@ -296,13 +296,13 @@ var _ = Describe("Convert V1beta1 to V1 NodeClaim API", func() {
})
It("should default the v1beta1 expireAfter to v1 when the nodepool doesn't exist", func() {
Expect(env.Client.Delete(ctx, v1nodePool)).To(Succeed())
v1nodePool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(30 * time.Minute)}
v1nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30m")
Expect(v1nodeclaim.ConvertFrom(ctx, v1beta1nodeclaim)).To(Succeed())
Expect(v1nodeclaim.Spec.ExpireAfter.Duration).To(BeNil())
})
It("should default the v1beta1 expireAfter to v1 when the nodepool label doesn't exist", func() {
delete(v1beta1nodeclaim.Labels, v1beta1.NodePoolLabelKey)
v1nodePool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(30 * time.Minute)}
v1nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30m")
Expect(env.Client.Update(ctx, v1nodePool)).To(Succeed())
Expect(v1nodeclaim.ConvertFrom(ctx, v1beta1nodeclaim)).To(Succeed())
Expect(v1nodeclaim.Spec.ExpireAfter.Duration).To(BeNil())
Expand Down
61 changes: 37 additions & 24 deletions pkg/apis/v1/nodepool_conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,13 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
Context("Disruption", func() {
It("should convert v1 nodepool consolidateAfter to nil with WhenEmptyOrUnderutilized", func() {
v1nodepool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmptyOrUnderutilized
v1nodepool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(time.Second * 2121)}
v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("2121s")
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1beta1nodepool.Spec.Disruption.ConsolidateAfter).To(BeNil())
})
It("should convert v1 nodepool consolidateAfter with WhenEmpty", func() {
v1nodepool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
v1nodepool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(time.Second * 2121)}
v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("2121s")
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
Expect(lo.FromPtr(v1beta1nodepool.Spec.Disruption.ConsolidateAfter.Duration)).To(Equal(lo.FromPtr(v1nodepool.Spec.Disruption.ConsolidateAfter.Duration)))
})
Expand All @@ -225,7 +225,7 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
Expect(string(v1beta1nodepool.Spec.Disruption.ConsolidationPolicy)).To(Equal(string(v1nodepool.Spec.Disruption.ConsolidationPolicy)))
})
It("should convert v1 nodepool ExpireAfter", func() {
v1nodepool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(time.Second * 2121)}
v1nodepool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("2121s")
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1beta1nodepool.Spec.Disruption.ExpireAfter.Duration).To(Equal(v1nodepool.Spec.Template.Spec.ExpireAfter.Duration))
})
Expand Down Expand Up @@ -270,6 +270,24 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
Expect(v1beta1nodepool.Status.Resources[resource]).To(Equal(v1nodepool.Status.Resources[resource]))
}
})
Context("Round Trip", func() {
It("spec.template.spec.expireAfter", func() {
v1nodepool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("10h")
Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
result, err := json.Marshal(v1nodepool.Spec.Template.Spec.ExpireAfter)
Expect(err).To(BeNil())
Expect(string(result)).To(Equal(`"10h"`))
})
It("spec.disruption.consolidateAfter", func() {
v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("10h")
Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
result, err := json.Marshal(v1nodepool.Spec.Disruption.ConsolidateAfter)
Expect(err).To(BeNil())
Expect(string(result)).To(Equal(`"10h"`))
})
})
Context("Round Trip", func() {
DescribeTable(
"NillableDuration",
Expand Down Expand Up @@ -557,26 +575,21 @@ var _ = Describe("Convert V1beta1 to V1 NodePool API", func() {
}
})
Context("Round Trip", func() {
DescribeTable(
"NillableDuration",
func(value string, field func() *v1beta1.NillableDuration) {
str := fmt.Sprintf("%q", value)
duration := v1beta1.NillableDuration{}
Expect(json.Unmarshal([]byte(str), &duration)).Should(Succeed())
*field() = duration
Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
result, err := json.Marshal(*field())
Expect(err).To(BeNil())
Expect(string(result)).To(Equal(str))
},
Entry("spec.template.spec.expireAfter", "720h", func() *v1beta1.NillableDuration { return &v1beta1nodepool.Spec.Disruption.ExpireAfter }),
Entry("spec.disruption.consolidateAfter", "15m", func() *v1beta1.NillableDuration {
if v1beta1nodepool.Spec.Disruption.ConsolidateAfter == nil {
v1beta1nodepool.Spec.Disruption.ConsolidateAfter = &v1beta1.NillableDuration{}
}
return v1beta1nodepool.Spec.Disruption.ConsolidateAfter
}),
)
It("spec.disruption.expireAfter", func() {
v1beta1nodepool.Spec.Disruption.ExpireAfter = v1beta1.MustParseNillableDuration("10h")
Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
result, err := json.Marshal(v1beta1nodepool.Spec.Disruption.ExpireAfter)
Expect(err).To(BeNil())
Expect(string(result)).To(Equal(`"10h"`))
})
It("spec.disruption.consolidateAfter", func() {
v1beta1nodepool.Spec.Disruption.ConsolidateAfter = lo.ToPtr(v1beta1.MustParseNillableDuration("10h"))
Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
result, err := json.Marshal(lo.FromPtr(v1beta1nodepool.Spec.Disruption.ConsolidateAfter))
Expect(err).To(BeNil())
Expect(string(result)).To(Equal(`"10h"`))
})
})
})
20 changes: 10 additions & 10 deletions pkg/apis/v1/nodepool_validation_cel_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,47 +64,47 @@ var _ = Describe("CEL/Validation", func() {
})
Context("Disruption", func() {
It("should fail on negative expireAfter", func() {
nodePool.Spec.Template.Spec.ExpireAfter.Duration = lo.ToPtr(lo.Must(time.ParseDuration("-1s")))
nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("-1s")
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
It("should succeed on a disabled expireAfter", func() {
nodePool.Spec.Template.Spec.ExpireAfter.Duration = nil
nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("Never")
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed on a valid expireAfter", func() {
nodePool.Spec.Template.Spec.ExpireAfter.Duration = lo.ToPtr(lo.Must(time.ParseDuration("30s")))
nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30s")
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should fail on negative consolidateAfter", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("-1s")))}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("-1s")
Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
})
It("should succeed on a disabled consolidateAfter", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: nil}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed on a valid consolidateAfter", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("30s")))}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed when setting consolidateAfter with consolidationPolicy=WhenEmpty", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("30s")))}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed when setting consolidateAfter with consolidationPolicy=WhenUnderutilized", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("30s")))}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmptyOrUnderutilized
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed when setting consolidateAfter to 'Never' with consolidationPolicy=WhenUnderutilized", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: nil}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmptyOrUnderutilized
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
It("should succeed when setting consolidateAfter to 'Never' with consolidationPolicy=WhenEmpty", func() {
nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: nil}
nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
})
Expand Down
11 changes: 10 additions & 1 deletion pkg/apis/v1beta1/duration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ package v1beta1

import (
"encoding/json"
"fmt"
"slices"
"time"

"github.com/samber/lo"
)

const Never = "Never"
Expand All @@ -32,7 +35,13 @@ type NillableDuration struct {

// Raw is used to ensure we remarshal the NillableDuration in the same format it was specified.
// This ensures tools like Flux and ArgoCD don't mistakenly detect drift due to our conversion webhooks.
Raw []byte
Raw []byte `hash:"ignore"`
}

func MustParseNillableDuration(val string) NillableDuration {
nd := NillableDuration{}
lo.Must0(json.Unmarshal([]byte(fmt.Sprintf("%q", val)), &nd))
return nd
}

// UnmarshalJSON implements the json.Unmarshaller interface.
Expand Down
24 changes: 12 additions & 12 deletions pkg/controllers/disruption/consolidation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ var _ = Describe("Consolidation", func() {
Budgets: []v1.Budget{{
Nodes: "100%",
}},
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(0 * time.Second)},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
},
},
})
Expand Down Expand Up @@ -103,7 +103,7 @@ var _ = Describe("Consolidation", func() {
Context("Events", func() {
It("should not fire an event for ConsolidationDisabled when the NodePool has consolidation set to WhenEmptyOrUnderutilized", func() {
nodePool.Spec.Disruption.ConsolidationPolicy = v1.ConsolidationPolicyWhenEmptyOrUnderutilized
nodePool.Spec.Disruption.ConsolidateAfter = v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))}
nodePool.Spec.Disruption.ConsolidateAfter = v1.MustParseNillableDuration("0s")
ExpectApplied(ctx, env.Client, node, nodeClaim, nodePool)

ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, []*corev1.Node{node}, []*v1.NodeClaim{nodeClaim})
Expand All @@ -118,7 +118,7 @@ var _ = Describe("Consolidation", func() {
It("should fire an event for ConsolidationDisabled when the NodePool has consolidation set to WhenEmpty", func() {
pod := test.Pod()
nodePool.Spec.Disruption.ConsolidationPolicy = v1.ConsolidationPolicyWhenEmpty
nodePool.Spec.Disruption.ConsolidateAfter = v1.NillableDuration{Duration: lo.ToPtr(time.Minute)}
nodePool.Spec.Disruption.ConsolidateAfter = v1.MustParseNillableDuration("1m")
ExpectApplied(ctx, env.Client, pod, node, nodeClaim, nodePool)
ExpectManualBinding(ctx, env.Client, pod, node)

Expand All @@ -128,7 +128,7 @@ var _ = Describe("Consolidation", func() {
})
It("should fire an event for ConsolidationDisabled when the NodePool has consolidateAfter set to 'Never'", func() {
pod := test.Pod()
nodePool.Spec.Disruption.ConsolidateAfter = v1.NillableDuration{}
nodePool.Spec.Disruption.ConsolidateAfter = v1.MustParseNillableDuration("Never")
ExpectApplied(ctx, env.Client, pod, node, nodeClaim, nodePool)
ExpectManualBinding(ctx, env.Client, pod, node)

Expand Down Expand Up @@ -379,7 +379,7 @@ var _ = Describe("Consolidation", func() {
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidationPolicy: v1.ConsolidationPolicyWhenEmptyOrUnderutilized,
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
Budgets: []v1.Budget{{
// 1/2 of 3 nodes == 1.5 nodes. This should round up to 2.
Nodes: "50%",
Expand Down Expand Up @@ -446,7 +446,7 @@ var _ = Describe("Consolidation", func() {
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidationPolicy: v1.ConsolidationPolicyWhenEmptyOrUnderutilized,
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
Budgets: []v1.Budget{{
Nodes: "100%",
}},
Expand Down Expand Up @@ -512,7 +512,7 @@ var _ = Describe("Consolidation", func() {
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidationPolicy: v1.ConsolidationPolicyWhenEmptyOrUnderutilized,
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
Budgets: []v1.Budget{{
Nodes: "0%",
}},
Expand Down Expand Up @@ -597,7 +597,7 @@ var _ = Describe("Consolidation", func() {
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidationPolicy: v1.ConsolidationPolicyWhenEmptyOrUnderutilized,
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
Budgets: []v1.Budget{{
Nodes: "0%",
}},
Expand Down Expand Up @@ -684,7 +684,7 @@ var _ = Describe("Consolidation", func() {
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidationPolicy: v1.ConsolidationPolicyWhenEmptyOrUnderutilized,
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
Budgets: []v1.Budget{{
Nodes: "0%",
}},
Expand Down Expand Up @@ -771,7 +771,7 @@ var _ = Describe("Consolidation", func() {
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidationPolicy: v1.ConsolidationPolicyWhenEmptyOrUnderutilized,
ConsolidateAfter: v1.NillableDuration{Duration: lo.ToPtr(time.Duration(0))},
ConsolidateAfter: v1.MustParseNillableDuration("0s"),
Budgets: []v1.Budget{{
Nodes: "0%",
}},
Expand Down Expand Up @@ -3947,7 +3947,7 @@ var _ = Describe("Consolidation", func() {
var nodes []*corev1.Node

BeforeEach(func() {
nodePool.Spec.Template.Spec.ExpireAfter = v1.NillableDuration{Duration: lo.ToPtr(3 * time.Second)}
nodePool.Spec.Template.Spec.ExpireAfter = v1.MustParseNillableDuration("3s")
nodeClaims, nodes = test.NodeClaimsAndNodes(2, v1.NodeClaim{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
Expand All @@ -3958,7 +3958,7 @@ var _ = Describe("Consolidation", func() {
},
},
Spec: v1.NodeClaimSpec{
ExpireAfter: v1.NillableDuration{Duration: lo.ToPtr(3 * time.Second)},
ExpireAfter: v1.MustParseNillableDuration("3s"),
},
Status: v1.NodeClaimStatus{
Allocatable: map[corev1.ResourceName]resource.Quantity{
Expand Down
8 changes: 4 additions & 4 deletions pkg/controllers/disruption/drift_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ var _ = Describe("Drift", func() {
nodePool = test.NodePool(v1.NodePool{
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidateAfter: v1.NillableDuration{Duration: nil},
ConsolidateAfter: v1.MustParseNillableDuration("Never"),
// Disrupt away!
Budgets: []v1.Budget{{
Nodes: "100%",
Expand Down Expand Up @@ -316,15 +316,15 @@ var _ = Describe("Drift", func() {
nps := test.NodePools(10, v1.NodePool{
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidateAfter: v1.NillableDuration{Duration: nil},
ConsolidateAfter: v1.MustParseNillableDuration("Never"),
Budgets: []v1.Budget{{
// 1/2 of 3 nodes == 1.5 nodes. This should round up to 2.
Nodes: "50%",
}},
},
Template: v1.NodeClaimTemplate{
Spec: v1.NodeClaimTemplateSpec{
ExpireAfter: v1.NillableDuration{Duration: nil},
ExpireAfter: v1.MustParseNillableDuration("Never"),
},
},
},
Expand Down Expand Up @@ -383,7 +383,7 @@ var _ = Describe("Drift", func() {
nps := test.NodePools(10, v1.NodePool{
Spec: v1.NodePoolSpec{
Disruption: v1.Disruption{
ConsolidateAfter: v1.NillableDuration{Duration: nil},
ConsolidateAfter: v1.MustParseNillableDuration("Never"),
Budgets: []v1.Budget{{
Nodes: "100%",
}},
Expand Down
Loading

0 comments on commit b67ec92

Please sign in to comment.