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

Jump to bottom

Update command can update multiple nodegroups #3914

Merged
merged 5 commits into from Jul 1, 2021
Merged

Update command can update multiple nodegroups #3914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c5b687
Update multiple nodegroups
nikimanoledaki Jun 29, 2021
d393d7f
Add unit tests for updating multiple ng
nikimanoledaki Jun 29, 2021
65e9cc8
Unpend updateConfig integration test
nikimanoledaki Jul 1, 2021
87dee89
Remove validation because UpdateConfig is never nil
nikimanoledaki Jul 1, 2021
ef534c7
Refactor Update ng into private funcs
nikimanoledaki Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration/tests/managed/managed_nodegroup_test.go
View file
Open in desktop
Expand Up @@ -423,7 +423,7 @@ var _ = Describe("(Integration) Create Managed Nodegroups", func() {
})

Context("and creating a nodegroup with an update config", func() {
PIt("defining the UpdateConfig field in the cluster config", func() {
It("defining the UpdateConfig field in the cluster config", func() {
By("creating it")
updateConfig := &api.NodeGroupUpdateConfig{
MaxUnavailable: aws.Int(2),
Expand Down
49 changes: 32 additions & 17 deletions pkg/actions/nodegroup/update.go
View file
Open in desktop
Expand Up @@ -6,14 +6,24 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/eks"
"github.com/kris-nova/logger"
"github.com/pkg/errors"

api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/managed"
)

func (m *Manager) Update() error {
ng := m.cfg.ManagedNodeGroups[0]
describeNodegroupOutput, err := m.ctl.Provider.EKS().DescribeNodegroup(&eks.DescribeNodegroupInput{
for _, ng := range m.cfg.ManagedNodeGroups {
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, just one small nit which is more of a personal preference:

can we move the contents of this for into another private func? simply because the process is quite long so it is easy to lose sight that this is all in a loop 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I split the contents into updateNodegroup and the strictly updateConfig bit into updateUpdateConfig (I know, cringe)

Copy link
Contributor

Choose a reason for hiding this comment

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

😂 excellent

if err := m.updateNodegroup(ng); err != nil {
return err
}
}
return nil
}

func (m *Manager) updateNodegroup(ng *api.ManagedNodeGroup) error {
logger.Info("checking that nodegroup %s is a managed nodegroup", ng.Name)

_, err := m.ctl.Provider.EKS().DescribeNodegroup(&eks.DescribeNodegroupInput{
ClusterName: &m.cfg.Metadata.Name,
NodegroupName: &ng.Name,
})
Expand All @@ -26,22 +36,12 @@ func (m *Manager) Update() error {
}

if ng.UpdateConfig == nil {
return fmt.Errorf("the submitted config didn't contain changes for nodegroup %s", ng.Name)
}

if describeNodegroupOutput.Nodegroup.UpdateConfig == nil {
return errors.New("cannot update updateConfig because the nodegroup is not configured to use one")
return fmt.Errorf("the submitted config does not contain any changes for nodegroup %s", ng.Name)
}

logger.Info("updating nodegroup %s's UpdateConfig", ng.Name)
updateConfig := &eks.NodegroupUpdateConfig{}

if ng.UpdateConfig.MaxUnavailable != nil {
updateConfig.MaxUnavailable = aws.Int64(int64(*ng.UpdateConfig.MaxUnavailable))
}

if ng.UpdateConfig.MaxUnavailablePercentage != nil {
updateConfig.MaxUnavailablePercentage = aws.Int64(int64(*ng.UpdateConfig.MaxUnavailablePercentage))
updateConfig, err := updateUpdateConfig(ng)
if err != nil {
return err
}

_, err = m.ctl.Provider.EKS().UpdateNodegroupConfig(&eks.UpdateNodegroupConfigInput{
Expand All @@ -56,3 +56,18 @@ func (m *Manager) Update() error {
logger.Info("nodegroup %s successfully updated", ng.Name)
return nil
}

func updateUpdateConfig(ng *api.ManagedNodeGroup) (*eks.NodegroupUpdateConfig, error) {
logger.Info("updating nodegroup %s's UpdateConfig", ng.Name)
updateConfig := &eks.NodegroupUpdateConfig{}

if ng.UpdateConfig.MaxUnavailable != nil {
updateConfig.MaxUnavailable = aws.Int64(int64(*ng.UpdateConfig.MaxUnavailable))
}

if ng.UpdateConfig.MaxUnavailablePercentage != nil {
updateConfig.MaxUnavailablePercentage = aws.Int64(int64(*ng.UpdateConfig.MaxUnavailablePercentage))
}

return updateConfig, nil
}
61 changes: 60 additions & 1 deletion pkg/actions/nodegroup/update_test.go
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@ var _ = Describe("Update", func() {
Expect(err).To(MatchError(ContainSubstring("could not find managed nodegroup with name \"my-ng\"")))
})

It("[happy path] successfully updates nodegroup with updateConfig and maxUnavailable", func() {
It("[happy path] successfully updates a nodegroup with updateConfig and maxUnavailable", func() {
p.MockEKS().On("DescribeNodegroup", &awseks.DescribeNodegroupInput{
ClusterName: &clusterName,
NodegroupName: &ngName,
Expand Down Expand Up @@ -77,4 +77,63 @@ var _ = Describe("Update", func() {
err := m.Update()
Expect(err).NotTo(HaveOccurred())
})

It("[happy path] successfully updates multiple nodegroups with updateConfig and maxUnavailable", func() {
p.MockEKS().On("DescribeNodegroup", &awseks.DescribeNodegroupInput{
ClusterName: &clusterName,
NodegroupName: &ngName,
}).Return(&awseks.DescribeNodegroupOutput{
Nodegroup: &awseks.Nodegroup{
UpdateConfig: &awseks.NodegroupUpdateConfig{
MaxUnavailable: aws.Int64(4),
},
},
}, nil)

p.MockEKS().On("DescribeNodegroup", &awseks.DescribeNodegroupInput{
ClusterName: &clusterName,
NodegroupName: aws.String("ng-2"),
}).Return(&awseks.DescribeNodegroupOutput{
Nodegroup: &awseks.Nodegroup{
UpdateConfig: &awseks.NodegroupUpdateConfig{
MaxUnavailable: aws.Int64(4),
},
},
}, nil)

p.MockEKS().On("UpdateNodegroupConfig", &awseks.UpdateNodegroupConfigInput{
UpdateConfig: &awseks.NodegroupUpdateConfig{
MaxUnavailable: aws.Int64(6),
},
ClusterName: &clusterName,
NodegroupName: &ngName,
}).Return(nil, nil)

p.MockEKS().On("UpdateNodegroupConfig", &awseks.UpdateNodegroupConfigInput{
UpdateConfig: &awseks.NodegroupUpdateConfig{
MaxUnavailable: aws.Int64(6),
},
ClusterName: &clusterName,
NodegroupName: aws.String("ng-2"),
}).Return(nil, nil)

cfg.ManagedNodeGroups[0].UpdateConfig = &api.NodeGroupUpdateConfig{
MaxUnavailable: aws.Int(6),
}

newNg := &api.ManagedNodeGroup{
NodeGroupBase: &api.NodeGroupBase{
Name: "ng-2",
},
UpdateConfig: &api.NodeGroupUpdateConfig{
MaxUnavailable: aws.Int(6),
},
}

cfg.ManagedNodeGroups = append(cfg.ManagedNodeGroups, newNg)

m = New(cfg, &eks.ClusterProvider{Provider: p}, nil)
err := m.Update()
Expect(err).NotTo(HaveOccurred())
})
})
26 changes: 14 additions & 12 deletions pkg/ctl/cmdutils/configfile.go
View file
Open in desktop
Expand Up @@ -750,22 +750,24 @@ func NewUpdateNodegroupLoader(cmd *Cmd) ClusterConfigLoader {
return ErrMustBeSet("managedNodeGroups field")
}

logger.Info("validating nodegroup %q", l.ClusterConfig.ManagedNodeGroups[0].Name)
for _, ng := range l.ClusterConfig.ManagedNodeGroups {
logger.Info("validating nodegroup %q", ng.Name)

var unsupportedFields []string
ng := l.ClusterConfig.ManagedNodeGroups[0]
var err error
if unsupportedFields, err = validateSupportedConfigFields(*ng.NodeGroupBase, []string{"Name"}, unsupportedFields); err != nil {
return err
}
var unsupportedFields []string
var err error
if unsupportedFields, err = validateSupportedConfigFields(*ng.NodeGroupBase, []string{"Name"}, unsupportedFields); err != nil {
return err
}

if unsupportedFields, err = validateSupportedConfigFields(*ng, []string{"NodeGroupBase", "UpdateConfig"}, unsupportedFields); err != nil {
return err
}
if unsupportedFields, err = validateSupportedConfigFields(*ng, []string{"NodeGroupBase", "UpdateConfig"}, unsupportedFields); err != nil {
return err
}

if len(unsupportedFields) > 0 {
logger.Warning("unchanged fields: the following fields remain unchanged; they are not supported by `eksctl update nodegroup`: %s", strings.Join(unsupportedFields[:], ", "))
if len(unsupportedFields) > 0 {
logger.Warning("unchanged fields for nodegroup %s: the following fields remain unchanged; they are not supported by `eksctl update nodegroup`: %s", ng.Name, strings.Join(unsupportedFields[:], ", "))
}
}

return nil
}

Expand Down