Skip to content

Reduce API calls in upgrade cluster #4366

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

Merged
merged 4 commits into from
Nov 1, 2021
Merged

Reduce API calls in upgrade cluster #4366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7656979
reduce API calls in upgrade cluster
Oct 25, 2021
cb38463
Update pkg/eks/eks.go
Oct 25, 2021
a7919f3
Merge branch 'main' into upgrade-reduce-calls
Nov 1, 2021
0346306
Merge branch 'main' into upgrade-reduce-calls
Nov 1, 2021
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
8 changes: 4 additions & 4 deletions pkg/actions/cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type Cluster interface {

func New(cfg *api.ClusterConfig, ctl *eks.ClusterProvider) (Cluster, error) {
clusterExists := true
err := ctl.RefreshClusterStatus(cfg)
err := ctl.RefreshClusterStatusIfStale(cfg)
Copy link
Contributor

@nikimanoledaki nikimanoledaki Oct 25, 2021

Choose a reason for hiding this comment

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

wb RefreshStaleClusterStatus?

Copy link
Contributor Author

@aclevername aclevername Oct 25, 2021

Choose a reason for hiding this comment

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

mhmm that implies it will always refresh, rather than only sometimes (it used to be maybe)

Copy link
Contributor

@nikimanoledaki nikimanoledaki Oct 25, 2021

Choose a reason for hiding this comment

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

ok yeah that's true, ifX is a step up frommaybe!

if err != nil {
if awsError, ok := errors.Unwrap(errors.Unwrap(err)).(awserr.Error); ok &&
awsError.Code() == awseks.ErrCodeResourceNotFoundException {
Expand All @@ -31,14 +31,14 @@ func New(cfg *api.ClusterConfig, ctl *eks.ClusterProvider) (Cluster, error) {
}

stackManager := ctl.NewStackManager(cfg)
hasClusterStack, err := stackManager.HasClusterStack()
clusterStack, err := stackManager.GetClusterStackIfExists()
Copy link
Contributor

@nikimanoledaki nikimanoledaki Oct 25, 2021

Choose a reason for hiding this comment

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

does GetClusterStackIfExists return an error if the cluster stack does not exist? in that case, it could just be called GetClusterStack, right? (I'm not a fan of this func name 😅 )

Copy link
Contributor Author

@aclevername aclevername Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

does GetClusterStackIfExists return an error if the cluster stack does not exist?

No. It only returns an error if one of the API call fails. Thats why its the IfExists, if the clusterStack is nil thats to indicate it doesn't exist.

I don't love the code but I wanted a way to re-use the stack so that we make less API calls

Copy link
Contributor Author

@aclevername aclevername Oct 28, 2021

Choose a reason for hiding this comment

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

@nikimanoledaki does that make sense?

Copy link
Contributor

@nikimanoledaki nikimanoledaki Oct 29, 2021

Choose a reason for hiding this comment

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

makes sense, I don't love it either but can't think of anything else!

if err != nil {
return nil, err
}

if hasClusterStack {
if clusterStack != nil {
logger.Debug("cluster %q was created by eksctl", cfg.Metadata.Name)
return NewOwnedCluster(cfg, ctl, stackManager), nil
return NewOwnedCluster(cfg, ctl, clusterStack, stackManager), nil
}

if !clusterExists {
Expand Down
6 changes: 4 additions & 2 deletions pkg/actions/cluster/owned.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,16 @@ import (
type OwnedCluster struct {
cfg *api.ClusterConfig
ctl *eks.ClusterProvider
clusterStack *manager.Stack
stackManager manager.StackManager
newClientSet func() (kubernetes.Interface, error)
}

func NewOwnedCluster(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, stackManager manager.StackManager) *OwnedCluster {
func NewOwnedCluster(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, clusterStack *manager.Stack, stackManager manager.StackManager) *OwnedCluster {
return &OwnedCluster{
cfg: cfg,
ctl: ctl,
clusterStack: clusterStack,
stackManager: stackManager,
newClientSet: func() (kubernetes.Interface, error) {
return ctl.NewStdClientSet(cfg)
Expand All @@ -34,7 +36,7 @@ func NewOwnedCluster(cfg *api.ClusterConfig, ctl *eks.ClusterProvider, stackMana
}

func (c *OwnedCluster) Upgrade(dryRun bool) error {
if err := c.ctl.LoadClusterVPC(c.cfg, c.stackManager); err != nil {
if err := vpc.UseFromClusterStack(c.ctl.Provider, c.clusterStack, c.cfg); err != nil {
return errors.Wrapf(err, "getting VPC configuration for cluster %q", c.cfg.Metadata.Name)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/actions/cluster/owned_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("Delete", func() {
}}},
}, nil)

c := cluster.NewOwnedCluster(cfg, ctl, fakeStackManager)
c := cluster.NewOwnedCluster(cfg, ctl, nil, fakeStackManager)
fakeClientSet = fake.NewSimpleClientset()

c.SetNewClientSet(func() (kubernetes.Interface, error) {
Expand Down Expand Up @@ -135,7 +135,7 @@ var _ = Describe("Delete", func() {
}}},
}, nil)

c := cluster.NewOwnedCluster(cfg, ctl, fakeStackManager)
c := cluster.NewOwnedCluster(cfg, ctl, nil, fakeStackManager)

err := c.Delete(time.Microsecond, false, false)
Expect(err).NotTo(HaveOccurred())
Expand Down
4 changes: 0 additions & 4 deletions pkg/actions/fargate/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ var _ = Describe("Fargate", func() {
})

Context("owned cluster", func() {
BeforeEach(func() {
fakeStackManager.HasClusterStackReturns(true, nil)
})

When("creating a farage role without specifying a role", func() {
When("the fargate role doesn't exist", func() {
BeforeEach(func() {
Expand Down
17 changes: 11 additions & 6 deletions pkg/cfn/manager/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,30 +555,35 @@ func (c *StackCollection) DescribeStacks() ([]*Stack, error) {
return stacks, nil
}

func (c *StackCollection) HasClusterStack() (bool, error) {
func (c *StackCollection) GetClusterStackIfExists() (*Stack, error) {
clusterStackNames, err := c.ListClusterStackNames()
if err != nil {
return false, err
return nil, err
}
return c.HasClusterStackUsingCachedList(clusterStackNames)
return c.getClusterStackUsingCachedList(clusterStackNames)
}

func (c *StackCollection) HasClusterStackUsingCachedList(clusterStackNames []string) (bool, error) {
stack, err := c.getClusterStackUsingCachedList(clusterStackNames)
return stack != nil, err
}

func (c *StackCollection) getClusterStackUsingCachedList(clusterStackNames []string) (*Stack, error) {
clusterStackName := c.MakeClusterStackName()
for _, stack := range clusterStackNames {
if stack == clusterStackName {
stack, err := c.DescribeStack(&cloudformation.Stack{StackName: &clusterStackName})
if err != nil {
return false, err
return nil, err
}
for _, tag := range stack.Tags {
if matchesClusterName(*tag.Key, *tag.Value, c.spec.Metadata.Name) {
return true, nil
return stack, nil
}
}
}
}
return false, nil
return nil, nil
}

// DescribeStackEvents describes the events that have occurred on the stack
Expand Down
140 changes: 70 additions & 70 deletions pkg/cfn/manager/fakes/fake_stack_manager.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/cfn/manager/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ type StackManager interface {
DeleteStackBySpec(s *Stack) (*Stack, error)
DeleteStackBySpecSync(s *Stack, errs chan error) error
DescribeStacks() ([]*Stack, error)
HasClusterStack() (bool, error)
GetClusterStackIfExists() (*Stack, error)
HasClusterStackUsingCachedList(clusterStackNames []string) (bool, error)
DescribeStackEvents(i *Stack) ([]*cloudformation.StackEvent, error)
LookupCloudTrailEvents(i *Stack) ([]*cloudtrail.Event, error)
Expand Down
13 changes: 7 additions & 6 deletions pkg/eks/eks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (c *ClusterProvider) RefreshClusterStatus(spec *api.ClusterConfig) error {
// SupportsManagedNodes reports whether an existing cluster supports Managed Nodes
// The minimum required control plane version and platform version are 1.14 and eks.3 respectively
func (c *ClusterProvider) SupportsManagedNodes(clusterConfig *api.ClusterConfig) (bool, error) {
if err := c.maybeRefreshClusterStatus(clusterConfig); err != nil {
if err := c.RefreshClusterStatusIfStale(clusterConfig); err != nil {
return false, err
}

Expand Down Expand Up @@ -126,7 +126,7 @@ func ClusterSupportsManagedNodes(cluster *awseks.Cluster) (bool, error) {

// SupportsFargate reports whether an existing cluster supports Fargate.
func (c *ClusterProvider) SupportsFargate(clusterConfig *api.ClusterConfig) (bool, error) {
if err := c.maybeRefreshClusterStatus(clusterConfig); err != nil {
if err := c.RefreshClusterStatusIfStale(clusterConfig); err != nil {
return false, err
}
return ClusterSupportsFargate(c.Status.ClusterInfo.Cluster)
Expand Down Expand Up @@ -179,7 +179,8 @@ func PlatformVersion(platformVersion string) (int, error) {
return version, nil
}

func (c *ClusterProvider) maybeRefreshClusterStatus(spec *api.ClusterConfig) error {
// RefreshClusterStatusIfStale refreshes the cluster status if enough time has passed since the last refresh
func (c *ClusterProvider) RefreshClusterStatusIfStale(spec *api.ClusterConfig) error {
if c.clusterInfoNeedsUpdate() {
return c.RefreshClusterStatus(spec)
}
Expand All @@ -188,7 +189,7 @@ func (c *ClusterProvider) maybeRefreshClusterStatus(spec *api.ClusterConfig) err

// CanDelete return true when a cluster can be deleted, otherwise it returns false along with an error explaining the reason
func (c *ClusterProvider) CanDelete(spec *api.ClusterConfig) (bool, error) {
err := c.maybeRefreshClusterStatus(spec)
err := c.RefreshClusterStatusIfStale(spec)
if err != nil {
if awsError, ok := errors.Unwrap(errors.Unwrap(err)).(awserr.Error); ok &&
awsError.Code() == awseks.ErrCodeResourceNotFoundException {
Expand All @@ -202,7 +203,7 @@ func (c *ClusterProvider) CanDelete(spec *api.ClusterConfig) (bool, error) {

// CanOperate returns true when a cluster can be operated, otherwise it returns false along with an error explaining the reason
func (c *ClusterProvider) CanOperate(spec *api.ClusterConfig) (bool, error) {
err := c.maybeRefreshClusterStatus(spec)
err := c.RefreshClusterStatusIfStale(spec)
if err != nil {
return false, errors.Wrapf(err, "unable to fetch cluster status to determine operability")
}
Expand All @@ -217,7 +218,7 @@ func (c *ClusterProvider) CanOperate(spec *api.ClusterConfig) (bool, error) {

// CanUpdate return true when a cluster or add-ons can be updated, otherwise it returns false along with an error explaining the reason
func (c *ClusterProvider) CanUpdate(spec *api.ClusterConfig) (bool, error) {
err := c.maybeRefreshClusterStatus(spec)
err := c.RefreshClusterStatusIfStale(spec)
if err != nil {
return false, errors.Wrapf(err, "fetching cluster status to determine update status")
}
Expand Down