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.

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

Adding an option for --disable-nodegroup-eviction when deleting a cluster #4659

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
2800550
Ignoring errors from nodegroups draining when deleting a cluster
AmitBenAmi Nov 5, 2021
251448f
Using `force` as `disableEviction` when draining the cluster's nodegr…
AmitBenAmi Nov 5, 2021
e986809
Adding the `disableNodegroupEviction` flag when deleting a cluster, t…
AmitBenAmi Nov 5, 2021
b6b7d8e
Using the cluster deletion flag usage for disabling the eviction when…
AmitBenAmi Nov 5, 2021
ae728e3
Fixing the tests with the new argument
AmitBenAmi Nov 7, 2021
0ce5258
Adding mocking for the cluster deletion
AmitBenAmi Nov 7, 2021
9fb1bb7
Adding tests for the cluster deletion command
AmitBenAmi Nov 7, 2021
954bf08
Refactoring the `DescribeTable` usage in the Cluster deletion tests
AmitBenAmi Jan 13, 2022
273023a
Refactoring the code for easier mock implementation
AmitBenAmi Jan 16, 2022
2dce551
Passing a `NodeGroupManager` to the `drainAllNodeGroups` function fro…
AmitBenAmi Jan 16, 2022
2d2b09d
Adding the `newNodeGroupManager` as exported for easier mocking
AmitBenAmi Jan 16, 2022
c84cc31
Adding the `VpcCniDeleter` to mock more easily
AmitBenAmi Jan 17, 2022
b44e321
Adding the `NodeGroupDrainer` to mock more easily
AmitBenAmi Jan 17, 2022
51eeac1
Fixing the typo in the function name
AmitBenAmi Jan 17, 2022
0300fec
Removing the unnecessary function
AmitBenAmi Jan 17, 2022
f01e0fa
Adding the first test of the drain
AmitBenAmi Jan 17, 2022
0bc238e
Removing the unnecessary function
AmitBenAmi Jan 17, 2022
b25254f
Adding more tests for the `drainAllNodeGroups` function
AmitBenAmi Jan 17, 2022
99ea9e9
Refactoring the drain method, and using an `interface` to easily mock…
AmitBenAmi Jan 18, 2022
44e57b5
Adding tests for using/not using the `force` flag when deleting the c…
AmitBenAmi Jan 18, 2022
f0d8967
Fixing the rebase
AmitBenAmi Jan 18, 2022
17c1b2d
Using all params
AmitBenAmi Jan 18, 2022
77a599c
Fixing the imports order to fix the lint errors
AmitBenAmi Jan 19, 2022
deb6b22
Update pkg/actions/cluster/cluster.go
AmitBenAmi Jan 19, 2022
c6a6a53
Removing the `bool` type parameter
AmitBenAmi Jan 19, 2022
4cdfd0f
Changing the `disableEviction` from pointer to regular `bool`
AmitBenAmi Jan 19, 2022
28b6cd5
Refactoring the tests
AmitBenAmi Jan 19, 2022
d6aa582
Changing the name of internal type`
AmitBenAmi Jan 19, 2022
8617b9f
Refactoring the tests to show the mocking parameters
AmitBenAmi Jan 19, 2022
a611589
Adding more context to unit tests with the `When` function
AmitBenAmi Jan 19, 2022
a887e09
Changing the `Describe` for clearer context
AmitBenAmi Jan 19, 2022
9df3916
Changing the draining error message
AmitBenAmi Jan 24, 2022
a104d5f
Changing the tests to match the error
AmitBenAmi Jan 24, 2022
2731e7d
Changing the `if` order to ease the code reading
AmitBenAmi Jan 25, 2022
d52a2b3
Refactoring the `Drain` method since it has multiple parameters
AmitBenAmi Jan 25, 2022
b49d9d0
Changing the name of the new `DrainInput` struct
AmitBenAmi Jan 25, 2022
a193625
Fixing the tests
AmitBenAmi Jan 25, 2022
5e9d7a9
Making testing private functions possible
AmitBenAmi Jan 25, 2022
cf4730a
Merge branch 'main' into disabling-nodes-eviction-when-deleting-clust…
Himangini Jan 31, 2022
d1d787e
Fixing lint errors
AmitBenAmi Jan 31, 2022
d961f13
Merge branch 'disabling-nodes-eviction-when-deleting-cluster-with-for…
AmitBenAmi Jan 31, 2022
0c4230e
Merge branch 'main' into disabling-nodes-eviction-when-deleting-clust…
Skarlso Jan 31, 2022
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
2 changes: 1 addition & 1 deletion pkg/actions/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (

type Cluster interface {
Upgrade(dryRun bool) error
Delete(waitInterval time.Duration, wait, force bool) error
Delete(waitInterval time.Duration, wait, force, disableNodegroupEviction bool) error
}

func New(cfg *api.ClusterConfig, ctl *eks.ClusterProvider) (Cluster, error) {
Expand Down
21 changes: 16 additions & 5 deletions pkg/actions/cluster/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import (
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/cloudformation"
"github.com/pkg/errors"

"github.com/weaveworks/eksctl/pkg/actions/nodegroup"

"github.com/weaveworks/eksctl/pkg/cfn/manager"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/fargate"
Expand All @@ -29,6 +29,11 @@ import (
"github.com/kris-nova/logger"
)

type NodeGroupDrainer interface {
Drain(input *nodegroup.DrainInput) error
}
type vpcCniDeleter func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface)

func deleteSharedResources(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, stackManager manager.StackManager, clusterOperable bool, clientSet kubernetes.Interface) error {
if clusterOperable {
if err := deleteFargateProfiles(cfg.Metadata, ctl, stackManager); err != nil {
Expand Down Expand Up @@ -159,7 +164,7 @@ func checkForUndeletedStacks(stackManager manager.StackManager) error {
return nil
}

func drainAllNodegroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, stackManager manager.StackManager, clientSet kubernetes.Interface, allStacks []manager.NodeGroupStack) error {
func drainAllNodeGroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface, allStacks []manager.NodeGroupStack, disableEviction bool, nodeGroupDrainer NodeGroupDrainer, vpcCniDeleter vpcCniDeleter) error {
if len(allStacks) == 0 {
return nil
}
Expand All @@ -172,11 +177,17 @@ func drainAllNodegroups(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, stackM
}

logger.Info("will drain %d unmanaged nodegroup(s) in cluster %q", len(cfg.NodeGroups), cfg.Metadata.Name)
nodeGroupManager := nodegroup.New(cfg, ctl, clientSet)
if err := nodeGroupManager.Drain(cmdutils.ToKubeNodeGroups(cfg), false, ctl.Provider.WaitTimeout(), 0, false, false); err != nil {

drainInput := &nodegroup.DrainInput{
NodeGroups: cmdutils.ToKubeNodeGroups(cfg),
MaxGracePeriod: ctl.Provider.WaitTimeout(),
DisableEviction: disableEviction,
}
if err := nodeGroupDrainer.Drain(drainInput); err != nil {
return err
}
attemptVpcCniDeletion(cfg.Metadata.Name, ctl, clientSet)

vpcCniDeleter(cfg.Metadata.Name, ctl, clientSet)
return nil
}

Expand Down
132 changes: 132 additions & 0 deletions pkg/actions/cluster/delete_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package cluster_test

import (
"github.com/weaveworks/eksctl/pkg/actions/nodegroup"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/stretchr/testify/mock"
"github.com/weaveworks/eksctl/pkg/actions/cluster"
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/cfn/manager"
"github.com/weaveworks/eksctl/pkg/cfn/manager/fakes"
"github.com/weaveworks/eksctl/pkg/eks"
"github.com/weaveworks/eksctl/pkg/kubernetes"
"github.com/weaveworks/eksctl/pkg/testutils/mockprovider"
"k8s.io/client-go/kubernetes/fake"
)

type drainerMock struct {
mock.Mock
}

func (drainer *drainerMock) Drain(input *nodegroup.DrainInput) error {
args := drainer.Called(input)
return args.Error(0)
}

var _ = Describe("DrainAllNodeGroups", func() {
var (
clusterName string
p *mockprovider.MockProvider
cfg *api.ClusterConfig
fakeStackManager *fakes.FakeStackManager
fakeClientSet *fake.Clientset
ctl *eks.ClusterProvider
)

BeforeEach(func() {
clusterName = "my-cluster"
p = mockprovider.NewMockProvider()
cfg = api.NewClusterConfig()
cfg.Metadata.Name = clusterName
fakeStackManager = new(fakes.FakeStackManager)
fakeClientSet = fake.NewSimpleClientset()
ctl = &eks.ClusterProvider{Provider: p, Status: &eks.ProviderStatus{}}
})

Context("draining node groups", func() {
When("disable eviction flag is set to false", func() {
It("drain the node groups", func() {
c := cluster.NewOwnedCluster(cfg, ctl, nil, fakeStackManager)
c.SetNewClientSet(func() (kubernetes.Interface, error) {
return fakeClientSet, nil
})

nodeGroupStacks := []manager.NodeGroupStack{{NodeGroupName: "ng-1"}}
mockedDrainInput := &nodegroup.DrainInput{
NodeGroups: cmdutils.ToKubeNodeGroups(cfg),
MaxGracePeriod: ctl.Provider.WaitTimeout(),
}

mockedDrainer := &drainerMock{}
mockedDrainer.On("Drain", mockedDrainInput).Return(nil)
vpcCniDeleterCalled := 0
vpcCniDeleter := func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) {
vpcCniDeleterCalled++
}

err := cluster.DrainAllNodeGroups(cfg, ctl, fakeClientSet, nodeGroupStacks, false, mockedDrainer, vpcCniDeleter)
Expect(err).NotTo(HaveOccurred())
mockedDrainer.AssertNumberOfCalls(GinkgoT(), "Drain", 1)
Expect(vpcCniDeleterCalled).To(Equal(1))
})
})

When("disable eviction flag is set to true", func() {
It("drain the node groups", func() {
c := cluster.NewOwnedCluster(cfg, ctl, nil, fakeStackManager)
c.SetNewClientSet(func() (kubernetes.Interface, error) {
return fakeClientSet, nil
})

nodeGroupStacks := []manager.NodeGroupStack{{NodeGroupName: "ng-1"}}
mockedDrainInput := &nodegroup.DrainInput{
NodeGroups: cmdutils.ToKubeNodeGroups(cfg),
MaxGracePeriod: ctl.Provider.WaitTimeout(),
DisableEviction: true,
}

mockedDrainer := &drainerMock{}
mockedDrainer.On("Drain", mockedDrainInput).Return(nil)
vpcCniDeleterCalled := 0
vpcCniDeleter := func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) {
vpcCniDeleterCalled++
}

err := cluster.DrainAllNodeGroups(cfg, ctl, fakeClientSet, nodeGroupStacks, true, mockedDrainer, vpcCniDeleter)
Expect(err).NotTo(HaveOccurred())
mockedDrainer.AssertNumberOfCalls(GinkgoT(), "Drain", 1)
Expect(vpcCniDeleterCalled).To(Equal(1))
})
})

When("no node group stacks exist", func() {
It("does no draining at all", func() {
c := cluster.NewOwnedCluster(cfg, ctl, nil, fakeStackManager)
c.SetNewClientSet(func() (kubernetes.Interface, error) {
return fakeClientSet, nil
})

var nodeGroupStacks []manager.NodeGroupStack
mockedDrainInput := &nodegroup.DrainInput{
NodeGroups: cmdutils.ToKubeNodeGroups(cfg),
MaxGracePeriod: ctl.Provider.WaitTimeout(),
}

mockedDrainer := &drainerMock{}
mockedDrainer.On("Drain", mockedDrainInput).Return(nil)
vpcCniDeleterCalled := 0
vpcCniDeleter := func(clusterName string, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) {
vpcCniDeleterCalled++
}

err := cluster.DrainAllNodeGroups(cfg, ctl, fakeClientSet, nodeGroupStacks, false, mockedDrainer, vpcCniDeleter)
Expect(err).NotTo(HaveOccurred())
mockedDrainer.AssertNotCalled(GinkgoT(), "Drain")
Expect(vpcCniDeleterCalled).To(Equal(0))
})
})
})
})
18 changes: 17 additions & 1 deletion pkg/actions/cluster/export_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
package cluster

import "github.com/weaveworks/eksctl/pkg/kubernetes"
import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/eks"
"github.com/weaveworks/eksctl/pkg/kubernetes"
)

var (
DrainAllNodeGroups = drainAllNodeGroups
)

func (c *UnownedCluster) SetNewClientSet(newClientSet func() (kubernetes.Interface, error)) {
c.newClientSet = newClientSet
Expand All @@ -9,3 +17,11 @@ func (c *UnownedCluster) SetNewClientSet(newClientSet func() (kubernetes.Interfa
func (c *OwnedCluster) SetNewClientSet(newClientSet func() (kubernetes.Interface, error)) {
c.newClientSet = newClientSet
}

func (c *UnownedCluster) SetNewNodeGroupManager(newNodeGroupManager func(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) NodeGroupDrainer) {
c.newNodeGroupManager = newNodeGroupManager
}

func (c *OwnedCluster) SetNewNodeGroupManager(newNodeGroupManager func(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) NodeGroupDrainer) {
c.newNodeGroupManager = newNodeGroupManager
}
27 changes: 19 additions & 8 deletions pkg/actions/cluster/owned.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/kris-nova/logger"
"github.com/pkg/errors"
"github.com/weaveworks/eksctl/pkg/actions/nodegroup"
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/cfn/manager"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
Expand All @@ -15,11 +16,12 @@ import (
)

type OwnedCluster struct {
cfg *api.ClusterConfig
ctl *eks.ClusterProvider
clusterStack *manager.Stack
stackManager manager.StackManager
newClientSet func() (kubernetes.Interface, error)
cfg *api.ClusterConfig
ctl *eks.ClusterProvider
clusterStack *manager.Stack
stackManager manager.StackManager
newClientSet func() (kubernetes.Interface, error)
newNodeGroupManager func(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) NodeGroupDrainer
}

func NewOwnedCluster(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clusterStack *manager.Stack, stackManager manager.StackManager) *OwnedCluster {
Expand All @@ -31,6 +33,9 @@ func NewOwnedCluster(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clusterSt
newClientSet: func() (kubernetes.Interface, error) {
return ctl.NewStdClientSet(cfg)
},
newNodeGroupManager: func(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clientSet kubernetes.Interface) NodeGroupDrainer {
return nodegroup.New(cfg, ctl, clientSet)
},
}
}

Expand Down Expand Up @@ -67,7 +72,7 @@ func (c *OwnedCluster) Upgrade(dryRun bool) error {
return nil
}

func (c *OwnedCluster) Delete(_ time.Duration, wait, force bool) error {
func (c *OwnedCluster) Delete(_ time.Duration, wait, force, disableNodegroupEviction bool) error {
var (
clientSet kubernetes.Interface
oidc *iamoidc.OpenIDConnectManager
Expand Down Expand Up @@ -105,8 +110,14 @@ func (c *OwnedCluster) Delete(_ time.Duration, wait, force bool) error {
if err != nil {
return err
}
if err := drainAllNodegroups(c.cfg, c.ctl, c.stackManager, clientSet, allStacks); err != nil {
return err

nodeGroupManager := c.newNodeGroupManager(c.cfg, c.ctl, clientSet)
if err := drainAllNodeGroups(c.cfg, c.ctl, clientSet, allStacks, disableNodegroupEviction, nodeGroupManager, attemptVpcCniDeletion); err != nil {
if !force {
return err
}

logger.Warning("an error occurred during nodegroups draining, force=true so proceeding with deletion: %q", err.Error())
}
}

Expand Down
Loading