Refactoring cluster manager to invoke provider's SpecChanged method #1808

maxdrib · 2022-04-11T15:03:46Z

Issue #, if available: #1669

Description of changes:
This PR removes the cloudstack-specific logic (in a backwards compatible way) from the cluster-manager and moves it to the provider for both CloudStack and VSphere

Testing (if applicable):
Unit tests pass

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

eks-distro-bot · 2022-04-11T15:03:49Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

maxdrib · 2022-04-11T15:03:59Z

/cc @vivek-koppuru @chrisdoherty4

maxdrib · 2022-04-12T15:13:24Z

pkg/providers/vsphere/vsphere.go

+			logger.V(3).Info(fmt.Sprintf("Old machine config spec %s not found in the existing spec", oldMcRef.Name))
+			return true, nil
+		}
+		if !reflect.DeepEqual(existingVmc.Spec, csmc.Spec) {


I recognize that using reflect is non-ideal, but this is what the cluster manager was using previously and to keep the changes smaller and minimize blast radius, I'd like to continue using it for now. The vsphere machine config spec's Equal method is not implemented. Defining that Equal method may require some VSphere context which I don't feel fully equipped to do

abhay-krishna · 2022-04-12T19:05:57Z

/override eks-anywhere-release-tooling-test-presubmit

eks-distro-bot · 2022-04-12T19:06:00Z

@abhay-krishna: Overrode contexts on behalf of abhay-krishna: eks-anywhere-release-tooling-test-presubmit

In response to this:

/override eks-anywhere-release-tooling-test-presubmit

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

eks-distro-bot · 2022-04-12T19:27:58Z

@maxdrib: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
eks-anywhere-release-tooling-test-presubmit	`5c36390`	link	`/test eks-anywhere-release-tooling-test-presubmit`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

g-gaston

Looks good! just one suggestion that I think would make it a bit cleaner

pkg/clustermanager/cluster_manager.go

pkg/providers/provider.go

pkg/providers/cloudstack/cloudstack.go

g-gaston

Looks good, just minor comments

pkg/providers/cloudstack/cloudstack.go

pkg/providers/docker/docker.go

pkg/providers/tinkerbell/tinkerbell.go

vivek-koppuru

/lgtm

vivek-koppuru · 2022-04-13T20:18:54Z

pkg/providers/vsphere/vsphere.go

@@ -199,6 +200,32 @@ func (p *vsphereProvider) UpdateKubeConfig(_ *[]byte, _ string) error {
 	return nil
 }

+func (p *vsphereProvider) machineConfigsSpecChanged(ctx context.Context, cc *v1alpha1.Cluster, cluster *types.Cluster, newClusterSpec *cluster.Spec) (bool, error) {
+	machineConfigMap := make(map[string]*v1alpha1.VSphereMachineConfig)
+	for _, config := range p.MachineConfigs(nil) {


We need to clean this up, either remove the parameter from the function or actually use it instead of referencing the values from the struct.

The parameter is currently being used in the snow provider, but nowhere else I believe. Is this the model all the providers should adopt? It may be worth creating an issue about it

Since this is not an exported method, we have control on what parameters to pass in based on the provider right?

g-gaston · 2022-04-13T20:59:11Z

/approve

eks-distro-bot · 2022-04-13T20:59:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: g-gaston

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [g-gaston]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Refactoring cluster manager to invoke provider's SpecChanged method

b317362

eks-distro-bot added the do-not-merge/work-in-progress label Apr 11, 2022

eks-distro-bot requested review from chrisdoherty4 and vivek-koppuru April 11, 2022 15:04

eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 11, 2022

maxdrib added 2 commits April 11, 2022 11:24

Linting

dfefb5e

Adding unit tests for provider SpecChanged method

23bf3c8

maxdrib marked this pull request as ready for review April 11, 2022 17:43

eks-distro-bot removed the do-not-merge/work-in-progress label Apr 11, 2022

maxdrib added 2 commits April 11, 2022 14:07

Make test independent of machineconfig order

4176078

Moving vsphere provider spec comparison logic into provider component

360aa8a

eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2022

Fixing unit test comments and naming

5c36390

maxdrib commented Apr 12, 2022

View reviewed changes

eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 12, 2022

g-gaston self-requested a review April 12, 2022 18:17

g-gaston reviewed Apr 12, 2022

View reviewed changes

pkg/clustermanager/cluster_manager.go Outdated Show resolved Hide resolved

Refactoring to use existing UpgradeNeeded provider method

3d8bae1

eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2022

maxdrib requested a review from g-gaston April 13, 2022 14:43

g-gaston reviewed Apr 13, 2022

View reviewed changes

pkg/providers/provider.go Outdated Show resolved Hide resolved

pkg/providers/cloudstack/cloudstack.go Outdated Show resolved Hide resolved

Simplifying changes to avoid unnecessary calls

41966a8

maxdrib requested a review from g-gaston April 13, 2022 15:25

g-gaston reviewed Apr 13, 2022

View reviewed changes

pkg/providers/cloudstack/cloudstack.go Outdated Show resolved Hide resolved

pkg/providers/docker/docker.go Outdated Show resolved Hide resolved

pkg/providers/tinkerbell/tinkerbell.go Outdated Show resolved Hide resolved

pkg/providers/tinkerbell/tinkerbell.go Outdated Show resolved Hide resolved

Renaming unused variables and removing unused function

f076339

eks-distro-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 13, 2022

Removing unnecessary capi manager validation logic

b5035d7

eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2022

maxdrib requested a review from g-gaston April 13, 2022 19:05

g-gaston approved these changes Apr 13, 2022

View reviewed changes

eks-distro-bot assigned g-gaston Apr 13, 2022

eks-distro-bot added the lgtm label Apr 13, 2022

vivek-koppuru approved these changes Apr 13, 2022

View reviewed changes

eks-distro-bot assigned vivek-koppuru Apr 13, 2022

eks-distro-bot added the approved label Apr 13, 2022

eks-distro-bot merged commit 1b065ac into aws:main Apr 13, 2022

maxdrib deleted the remove-cloudstack-from-manager branch April 14, 2022 13:20

snyk-bot mentioned this pull request Mar 12, 2023

[Snyk] Upgrade postcss from 8.4.5 to 8.4.21 cloudgeek7/eks-anywhere#2

Open

snyk-bot mentioned this pull request Mar 26, 2023

[Snyk] Upgrade postcss from 8.4.5 to 8.4.21 cloudgeek7/eks-anywhere#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring cluster manager to invoke provider's SpecChanged method #1808

Refactoring cluster manager to invoke provider's SpecChanged method #1808

maxdrib commented Apr 11, 2022 •

edited

Loading

eks-distro-bot commented Apr 11, 2022

maxdrib commented Apr 11, 2022

maxdrib Apr 12, 2022 •

edited

Loading

abhay-krishna commented Apr 12, 2022

eks-distro-bot commented Apr 12, 2022

eks-distro-bot commented Apr 12, 2022

g-gaston left a comment

g-gaston left a comment

vivek-koppuru left a comment

vivek-koppuru Apr 13, 2022

maxdrib Apr 14, 2022

vivek-koppuru Apr 18, 2022

g-gaston commented Apr 13, 2022

eks-distro-bot commented Apr 13, 2022

Refactoring cluster manager to invoke provider's SpecChanged method #1808

Refactoring cluster manager to invoke provider's SpecChanged method #1808

Conversation

maxdrib commented Apr 11, 2022 • edited Loading

eks-distro-bot commented Apr 11, 2022

maxdrib commented Apr 11, 2022

maxdrib Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

abhay-krishna commented Apr 12, 2022

eks-distro-bot commented Apr 12, 2022

eks-distro-bot commented Apr 12, 2022

g-gaston left a comment

Choose a reason for hiding this comment

g-gaston left a comment

Choose a reason for hiding this comment

vivek-koppuru left a comment

Choose a reason for hiding this comment

vivek-koppuru Apr 13, 2022

Choose a reason for hiding this comment

maxdrib Apr 14, 2022

Choose a reason for hiding this comment

vivek-koppuru Apr 18, 2022

Choose a reason for hiding this comment

g-gaston commented Apr 13, 2022

eks-distro-bot commented Apr 13, 2022

maxdrib commented Apr 11, 2022 •

edited

Loading

maxdrib Apr 12, 2022 •

edited

Loading