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

Add unit test for scalable node group controller #157

Merged
merged 10 commits into from
Dec 1, 2020
Merged

Add unit test for scalable node group controller #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ddd4dda
Add unit test for scalable node group controller
prateekgogia Nov 18, 2020
ac6d21d
Add some more tests
prateekgogia Nov 18, 2020
cc6a9bd
Add error checks for SetReplicas call
prateekgogia Nov 18, 2020
66d5344
Fix Unit tests
prateekgogia Nov 18, 2020
2b23031
rename fakeError
prateekgogia Nov 18, 2020
b277a7e
update tests
prateekgogia Nov 23, 2020
099f647
rename a few things
prateekgogia Nov 23, 2020
32bfbaa
rename fake error object
prateekgogia Nov 24, 2020
6c25eb0
Fixed pointers comparison
prateekgogia Nov 30, 2020
22609c1
Simplify tests a little
prateekgogia Dec 1, 2020
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
32 changes: 32 additions & 0 deletions pkg/cloudprovider/fake/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package fake

// retryableErr implements controllers.RetryableError & controllers.CodedError
type retryableErr struct {
error
retryable bool
}

func (e *retryableErr) IsRetryable() bool {
return e.retryable
}
func (e *retryableErr) ErrorCode() string {
return e.Error()
}

func RetryableError(err error) *retryableErr {
return &retryableErr{error: err, retryable: true}
}
21 changes: 16 additions & 5 deletions pkg/cloudprovider/fake/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,27 @@ import (

"github.com/awslabs/karpenter/pkg/apis/autoscaling/v1alpha1"
"github.com/awslabs/karpenter/pkg/cloudprovider"
"knative.dev/pkg/ptr"
)

var (
NotImplementedError = fmt.Errorf("provider is not implemented. Are you running the correct release for your cloud provider?")
)

const (
NodeGroupMessage = "fake factory message"
)

type Factory struct {
WantErr error
// NodeReplicas is use by tests to control observed replicas.
NodeReplicas map[string]int32
// NodeReplicas is used by tests to control observed replicas.
NodeReplicas map[string]*int32
NodeGroupStable bool
}

func NewFactory(options cloudprovider.Options) *Factory {
return &Factory{
NodeReplicas: make(map[string]int32),
NodeReplicas: make(map[string]*int32),
NodeGroupStable: true,
}
}

Expand All @@ -43,9 +48,15 @@ func NewNotImplementedFactory() *Factory {
}

func (f *Factory) NodeGroupFor(sng *v1alpha1.ScalableNodeGroupSpec) cloudprovider.NodeGroup {
msg := ""
if !f.NodeGroupStable {
msg = NodeGroupMessage
}
return &NodeGroup{
WantErr: f.WantErr,
Replicas: ptr.Int32(f.NodeReplicas[sng.ID]),
Stable: f.NodeGroupStable,
Message: msg,
Replicas: f.NodeReplicas[sng.ID],
}
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/cloudprovider/fake/nodegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (

type NodeGroup struct {
WantErr error
Stable bool
ellistarn marked this conversation as resolved.
Show resolved Hide resolved
Message string
Replicas *int32
}

Expand All @@ -41,5 +43,5 @@ func (n *NodeGroup) SetReplicas(count int32) error {
}

func (n *NodeGroup) Stabilized() (bool, string, error) {
return true, "", nil
return n.Stable, n.Message, nil
}
4 changes: 2 additions & 2 deletions pkg/controllers/horizontalautoscaler/v1alpha1/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ var _ = Describe("Examples", func() {
It("should scale to average utilization target, metric=85, target=60, replicas=5, want=8", func() {
Expect(ns.ParseResources("docs/examples/reserved-capacity-utilization.yaml", ha, sng)).To(Succeed())
sng.Spec.Replicas = ptr.Int32(5)
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = *sng.Spec.Replicas
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = ptr.Int32(*sng.Spec.Replicas) // create a new pointer to avoid races with the controller
MockMetricValue(fakeServer, .85)

ExpectCreated(ns.Client, sng, ha)
Expand All @@ -105,7 +105,7 @@ var _ = Describe("Examples", func() {
It("should scale to average value target, metric=41, target=4, want=11", func() {
Expect(ns.ParseResources("docs/examples/queue-length-average-value.yaml", ha, sng)).To(Succeed())
sng.Spec.Replicas = ptr.Int32(1)
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = *sng.Spec.Replicas
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = ptr.Int32(*sng.Spec.Replicas) // create a new pointer to avoid races with the controller
MockMetricValue(fakeServer, 41)

ExpectCreated(ns.Client, sng, ha)
Expand Down
59 changes: 54 additions & 5 deletions pkg/controllers/scalablenodegroup/v1alpha1/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
package v1alpha1

import (
"fmt"
"testing"

v1alpha1 "github.com/awslabs/karpenter/pkg/apis/autoscaling/v1alpha1"
Expand All @@ -38,9 +39,9 @@ func TestAPIs(t *testing.T) {
}

var fakeCloudProvider = fake.NewFactory(cloudprovider.Options{})

var fakeController = &Controller{CloudProvider: fakeCloudProvider}
var env environment.Environment = environment.NewLocal(func(e *environment.Local) {
e.Manager.Register(&Controller{CloudProvider: fakeCloudProvider})
e.Manager.Register(fakeController)
})

var _ = BeforeSuite(func() {
Expand All @@ -53,24 +54,72 @@ var _ = AfterSuite(func() {

var _ = Describe("Examples", func() {
var ns *environment.Namespace
var defaultReplicaCount = int32(3)

var sng *v1alpha1.ScalableNodeGroup

BeforeEach(func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a little trouble following the flow of the BeforeEach, Test, AfterEach logic. It looks like some logic is duplicated (i.e. sng.Spec.Replicas being set).

When I write these, I typically try to do everything in a BeforeEach loop, unless my test has mutated something that needs to be cleaned up after all of the tests.

I also think it might be cleaner to modify the fields of the CloudProvider factory, rather than using a singleton pattern on nodegroup and modifying that object directly. I think all you'd have to do is wire through the behavior of NodeGroupStabilized and NodeGroupStabilizedMessage to the CloudProvider factory, and then before each test, you can just do something like

factory.NodeGroupStabilized = false 
factory.NodeGroupStabilizedMessage = "still trying"

Then when the factory creates the object, it will inject the behavior into the node group and should clean up most of your singleton/casting logic.

Let me know what you think to this approach.

var err error
ns, err = env.NewNamespace()
Expect(err).NotTo(HaveOccurred())
sng = &v1alpha1.ScalableNodeGroup{}
fakeCloudProvider.NodeGroupStable = true
ellistarn marked this conversation as resolved.
Show resolved Hide resolved
fakeCloudProvider.WantErr = nil
sng.Spec.Replicas = &defaultReplicaCount
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = ptr.Int32(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

if I understand correctly, sng.Spec.ID is nil here, so this line is just muddying the water.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sng.Spec.ID is an empty string.
This is required here, since, in my tests also sng.Spec.ID is always an empty string and NodeReplicas[""] is a Nil value if I don't set it which causes nodegroup.Replicas to be also Nil.

})

Context("ScalableNodeGroup", func() {
It("should be created", func() {
Expect(ns.ParseResources("docs/examples/reserved-capacity-utilization.yaml", sng)).To(Succeed())
sng.Spec.Replicas = ptr.Int32(5)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1


fakeCloudProvider.NodeReplicas[sng.Spec.ID] = ptr.Int32(*sng.Spec.Replicas)
ExpectCreated(ns.Client, sng)
ExpectEventuallyHappy(ns.Client, sng)

ExpectDeleted(ns.Client, sng)
})
})

Context("ScalableNodeGroup Reconcile tests", func() {
It("Test reconciler to scale up nodes", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider more idiomatic test naming with "It".

e.g. "it should scale up nodes", where the test name is "should scale up nodes".

Expect(fakeController.Reconcile(sng)).To(Succeed())
Expect(*fakeCloudProvider.NodeReplicas[sng.Spec.ID]).To(Equal(defaultReplicaCount))
})

It("Test reconciler to scale down nodes", func() {
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = ptr.Int32(10) // set existing replicas higher than desired
Expect(fakeController.Reconcile(sng)).To(Succeed())
Expect(*fakeCloudProvider.NodeReplicas[sng.Spec.ID]).To(Equal(defaultReplicaCount))
})

It("Test reconciler to make no change to node count", func() {
fakeCloudProvider.NodeReplicas[sng.Spec.ID] = &defaultReplicaCount // set existing replicas equal to desired
Expect(fakeController.Reconcile(sng)).To(Succeed())
Expect(*fakeCloudProvider.NodeReplicas[sng.Spec.ID]).To(Equal(defaultReplicaCount))
})

It("Scale up nodes when not node group is stabilized and check status condition", func() {
Expect(fakeController.Reconcile(sng)).To(Succeed())
Expect(*fakeCloudProvider.NodeReplicas[sng.Spec.ID]).To(Equal(defaultReplicaCount))
Expect(sng.StatusConditions().GetCondition(v1alpha1.Stabilized).IsTrue()).To(Equal(true))
Expect(sng.StatusConditions().GetCondition(v1alpha1.Stabilized).Message).To(Equal(""))
})

It("Scale up nodes when not node group is NOT stabilized and check status condition", func() {
fakeCloudProvider.NodeGroupStable = false
Expect(fakeController.Reconcile(sng)).To(Succeed())
Expect(*fakeCloudProvider.NodeReplicas[sng.Spec.ID]).To(Equal(defaultReplicaCount))
Expect(sng.StatusConditions().GetCondition(v1alpha1.Stabilized).IsFalse()).To(Equal(true))
Expect(sng.StatusConditions().GetCondition(v1alpha1.Stabilized).Message).To(Equal(fake.NodeGroupMessage))
})

It("Retryable error while reconciling", func() {
fakeCloudProvider.WantErr = fake.RetryableError(fmt.Errorf(fake.NodeGroupMessage)) // retryable error
existingReplicas := fakeCloudProvider.NodeReplicas[sng.Spec.ID]
Expect(fakeController.Reconcile(sng)).To(Succeed())
Expect(fakeCloudProvider.NodeReplicas[sng.Spec.ID]).To(Equal(existingReplicas))
Expect(sng.StatusConditions().GetCondition(v1alpha1.AbleToScale).IsFalse()).To(Equal(true))
Expect(sng.StatusConditions().GetCondition(v1alpha1.AbleToScale).Message).To(Equal(fake.NodeGroupMessage))
})

})
})