Create management cluster using controller bootstrap task #7319

mitalipaygude · 2024-01-17T19:34:14Z

Issue #, if available:

Description of changes:
Create management cluster using controller bootstrap task
The original #7110 is being split into smaller PRs. This PR is the first one.

Testing (if applicable):

The whole workflow has been tested end to end for Docker, vSphere, and Cloudstack providers.
Unit tests

Documentation added/planned (if applicable):

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 · 2024-01-17T19:34:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mitalipaygude. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2024-01-17T19:38:12Z

Codecov Report

Attention: 84 lines in your changes are missing coverage. Please review.

Comparison is base (f323b9f) 71.77% compared to head (0145636) 71.63%.
Report is 11 commits behind head on main.

❗ Current head 0145636 differs from pull request most recent head f4f36cc. Consider uploading reports for the commit f4f36cc to get more accurate results

Files	Patch %	Lines
pkg/workflows/management/create_install_eksa.go	44.00%	28 Missing ⚠️
pkg/clustermanager/eksa_installer.go	0.00%	26 Missing ⚠️
pkg/dependencies/factory.go	0.00%	26 Missing ⚠️
pkg/workflows/management/create_bootstrap.go	90.90%	2 Missing ⚠️
pkg/workflows/management/create_install_capi.go	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7319      +/-   ##
==========================================
- Coverage   71.77%   71.63%   -0.15%     
==========================================
  Files         560      562       +2     
  Lines       43495    43517      +22     
==========================================
- Hits        31220    31173      -47     
- Misses      10556    10630      +74     
+ Partials     1719     1714       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

g-gaston · 2024-01-17T20:29:46Z

pkg/clustermanager/cluster_manager.go

+		}
+	}
+
+	clusterSpec.Cluster.AddManagedByCLIAnnotation()


what is this for?

This condition let's the webhook create management clusters using the eksa controller.
Here's that condition https://github.com/aws/eks-anywhere/blob/main/pkg/api/v1alpha1/cluster_webhook.go#L64

ah good point
then could we set it before applying the cluster spec? so in a different task, whichever uses the cluster creator

Also maybe add a comment since not even me remember what this was for 🤦🏻

In that case I'll add it before the create workload cluster task and remove it from this PR.

g-gaston · 2024-01-17T20:30:06Z

pkg/clustermanager/cluster_manager.go

@@ -523,6 +523,22 @@ func (c *ClusterManager) getWorkloadClusterKubeconfig(ctx context.Context, clust
 	return nil
 }

+// CreateEKSAReleaseBundle applies the eks-a release bundle to the cluster.
+func (c *ClusterManager) CreateEKSAReleaseBundle(ctx context.Context, cluster *types.Cluster, clusterSpec *cluster.Spec) error {


can this be done from the eks-a installer?

I ended up using this wrapper because while creating the cluster's namespace we use the cluster Client that is a private Retrier Client field used by the cluster manager. I won't be able to use the cluster client if I were to move this code.

why do you need "the clusterctl client"? we should only need a k8s client wrapped by a retrier
which the installer should already have?

My bad, I meant the cluster client.

mitalipaygude · 2024-01-18T08:59:28Z

/test eks-anywhere-generate-files-presubmits

mitalipaygude · 2024-01-19T20:47:01Z

/retest

g-gaston · 2024-01-19T21:04:49Z

pkg/clustermanager/eksa_installer.go

+// CreateNamespaceIfNotPresent creates the cluster namespace if it doesn not exist.
+func (i *EKSAInstaller) CreateNamespaceIfNotPresent(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error {
+	if spec.Cluster.Namespace != "" {
+		if err := i.client.CreateNamespaceIfNotPresent(ctx, cluster.KubeconfigFile, spec.Cluster.Namespace); err != nil {


IMO we should not do this
It should be the use the one creating the namespace before calling eks-a if they want to create a cluster in it

g-gaston · 2024-01-19T21:08:43Z

pkg/dependencies/factory.go

+
+		client := clustermanager.NewRetrierClient(
+			&clusterManagerClient{
+				f.dependencies.Clusterctl,


why do we need clusterctl for the eksa installer? shouldn't it be enough with a k8s client?

g-gaston · 2024-01-19T21:09:11Z

pkg/dependencies/factory.go

@@ -1103,6 +1104,39 @@ func (f *Factory) WithClusterManager(clusterConfig *v1alpha1.Cluster, timeoutOpt
 	return f
 }

+// WithEKSAInstaller builds a cluster manager based on the cluster config and timeout options.
+func (f *Factory) WithEKSAInstaller(clusterConfig *v1alpha1.Cluster, timeoutOpts *ClusterManagerTimeoutOptions) *Factory {


nit: can we update the clustermanager build method to make use of this one?

g-gaston · 2024-01-19T21:11:00Z

pkg/workflows/interfaces/interfaces.go

+type EksaInstaller interface {
+	ApplyBundles(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error
+	ApplyReleases(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error
+	CreateNamespaceIfNotPresent(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error
+}


ideally we should have

Suggested change

type EksaInstaller interface {

ApplyBundles(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error

ApplyReleases(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error

CreateNamespaceIfNotPresent(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error

}

type EksaInstaller interface {

Install(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error

}

if we can't do that for some reason, let's talk about why and try to figure out a solution

g-gaston · 2024-01-19T21:12:44Z

pkg/workflows/management/create_test.go

+	)
+}
+
+// func (c *createTestSetup) expectInstallEksaComponentsBootstrap(err1, err2, err3, err4 error) {


is this not needed? if so, let's remove it

g-gaston · 2024-01-19T21:12:53Z

pkg/workflows/management/create_test.go

+	}
+}
+
+// func TestCreateEKSAReleaseBundleFailure(t *testing.T) {


g-gaston · 2024-01-19T21:13:59Z

pkg/clustermanager/eksa_installer.go

@@ -108,6 +109,47 @@ func (i *EKSAInstaller) Upgrade(ctx context.Context, log logr.Logger, c *types.C
 	return changeDiff, nil
 }

+// ApplyBundles applies the bundles to the cluster.
+func (i *EKSAInstaller) ApplyBundles(ctx context.Context, log logr.Logger, cluster *types.Cluster, spec *cluster.Spec) error {


Can all these methods be part of Install? (you can make them unexported and then call them from Install)

eks-distro-bot · 2024-01-24T00:46:44Z

@mitalipaygude: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
eks-anywhere-presubmit	`f4f36cc`	link	true	`/test eks-anywhere-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.

eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 17, 2024

g-gaston reviewed Jan 17, 2024

View reviewed changes

mitalipaygude force-pushed the mgmt_split branch from fb0ae0e to 0145636 Compare January 18, 2024 08:40

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 Jan 18, 2024

g-gaston reviewed Jan 19, 2024

View reviewed changes

mitalipaygude force-pushed the mgmt_split branch from 8e5273a to 0528213 Compare January 23, 2024 01:05

Create management cluster using controller bootstrap task

b7d3bfc

mitalipaygude force-pushed the mgmt_split branch from 0528213 to b7d3bfc Compare January 23, 2024 01:09

unit test

f4f36cc

mitalipaygude closed this Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create management cluster using controller bootstrap task #7319

Create management cluster using controller bootstrap task #7319

mitalipaygude commented Jan 17, 2024

eks-distro-bot commented Jan 17, 2024

codecov bot commented Jan 17, 2024 •

edited

Loading

g-gaston Jan 17, 2024

mitalipaygude Jan 17, 2024 •

edited

Loading

g-gaston Jan 17, 2024

mitalipaygude Jan 18, 2024

g-gaston Jan 17, 2024

mitalipaygude Jan 17, 2024 •

edited

Loading

g-gaston Jan 17, 2024

mitalipaygude Jan 18, 2024

mitalipaygude commented Jan 18, 2024

mitalipaygude commented Jan 19, 2024

g-gaston Jan 19, 2024

g-gaston Jan 19, 2024

g-gaston Jan 19, 2024

g-gaston Jan 19, 2024

g-gaston Jan 19, 2024

g-gaston Jan 19, 2024

g-gaston Jan 19, 2024

eks-distro-bot commented Jan 24, 2024

Create management cluster using controller bootstrap task #7319

Create management cluster using controller bootstrap task #7319

Conversation

mitalipaygude commented Jan 17, 2024

eks-distro-bot commented Jan 17, 2024

codecov bot commented Jan 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mitalipaygude Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitalipaygude Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitalipaygude commented Jan 18, 2024

mitalipaygude commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eks-distro-bot commented Jan 24, 2024

codecov bot commented Jan 17, 2024 •

edited

Loading

mitalipaygude Jan 17, 2024 •

edited

Loading

mitalipaygude Jan 17, 2024 •

edited

Loading