curated packages full cluster lifecycle adaptations #4970

ewollesen · 2023-02-14T22:42:16Z

Issue #, if available:

Description of changes:

Testing (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.

ewollesen · 2023-02-14T22:42:22Z

/hold

ewollesen · 2023-02-14T22:43:08Z

Related to aws/eks-anywhere-packages#807

codecov · 2023-02-16T16:17:30Z

Codecov Report

Merging #4970 (cf44c4e) into main (f2dc386) will increase coverage by 0.05%.
The diff coverage is 88.31%.

@@            Coverage Diff             @@
##             main    #4970      +/-   ##
==========================================
+ Coverage   72.24%   72.30%   +0.05%     
==========================================
  Files         435      435              
  Lines       35518    35659     +141     
==========================================
+ Hits        25661    25782     +121     
- Misses       8316     8332      +16     
- Partials     1541     1545       +4

Impacted Files	Coverage Δ
controllers/cluster_controller_legacy.go	`0.00% <ø> (ø)`
pkg/api/v1alpha1/cluster_types.go	`80.37% <0.00%> (-0.31%)`	⬇️
controllers/cluster_controller.go	`77.77% <70.00%> (-0.45%)`	⬇️
pkg/executables/helm.go	`70.86% <82.75%> (+2.35%)`	⬆️
pkg/curatedpackages/packagecontrollerclient.go	`96.93% <91.57%> (-1.95%)`	⬇️
controllers/factory.go	`96.81% <100.00%> (+0.16%)`	⬆️
pkg/curatedpackages/packageinstaller.go	`93.18% <100.00%> (ø)`
pkg/dependencies/factory.go	`74.30% <100.00%> (+0.02%)`	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ewollesen · 2023-03-01T23:06:15Z

/unhold

cmd/eksctl-anywhere/cmd/installpackagecontroller.go

controllers/cluster_controller.go

pkg/executables/helm.go

danbudris

looks good from the perspective of the RBAC configuration scope!

ewollesen · 2023-03-15T20:21:31Z

/approve

eks-distro-bot · 2023-03-15T20:21:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ewollesen

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~~ [ewollesen]

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

vivek-koppuru · 2023-03-15T20:17:30Z

controllers/cluster_controller_legacy.go

+// resources, which will trigger the curated packages controller to do the
+// rest.
+//
+// +kubebuilder:rbac:groups=packages.eks.amazonaws.com,resources=packagebundlecontrollers,verbs=create;delete;get;list;patch;update;watch;


What is the purpose of adding this comment then? Generally we don't need to backport to legacy controller anyways.

This was the only place I found to add these comments that was actually reflected in the final output. Can you suggest a better place to put them?

This file is for the old controller, so I would suggest to try in cluster_controller.go. Or if it's not applicable then omit.

I'll try again, but the last time I made modifications there, they plain just didn't show up... Which seemed weird, but maybe I was using the wrong makefile target... or something.

vivek-koppuru · 2023-03-15T20:27:20Z

pkg/api/v1alpha1/cluster_types.go

+// IsPackagesEnabled checks if the configuration supports curated packages
+// installation.
+func (c *Cluster) IsPackagesEnabled() bool {
+	return !c.IsSelfManaged() && (c.Spec.Packages == nil || !c.Spec.Packages.Disable)


This is confusing to me then, why is there a method for IsPackagesEnabled then? That makes it sound like it should apply for any cluster that has packages enabled or not, regardless of if it's handled by the CLI.

Is IsPackagesSupported a better name based on the comment? I think understanding the use case better will help explain it.

vivek-koppuru · 2023-03-15T20:30:45Z

pkg/curatedpackages/packagecontrollerclient.go

+	chart          *releasev1.Image
+	chartInstaller ChartInstaller
+	// deleter handles helm chart install deletion.
+	deleter               ChartInstallationDeleter


Can we just call this ChartUninstaller?

I guess? Helm has weird wording for a chart installation or whatever. I guess I don't have a strong feeling on it, it's just all confusing.

vivek-koppuru · 2023-03-15T20:31:49Z

pkg/curatedpackages/packagecontrollerclient.go

+//
+// This method fills in the gaps between the original CLI use case, where all
+// information is known at PackageControllerClient initialization, and the
+// Full Cluster Lifecycle use case, where there's limited information at


What are some examples of this limited information?

Basically, if you look at NewPackageControllerClient's parameters, and subtract these parameters out, you're left with the set of parameters that full cluster lifecycle knows at instantiation. Things like the cluster name aren't known at instantiation for full cluster lifecycle. Some of this is because of different timings, and some is because when used by full cluster lifecycle, this package controller client might be reused to build multiple clusters, whereas in a CLI usage, it builds a single cluster, then exits.

There's probably a lot of refactoring that could be done here, but this PR was big enough as is.

I'll add something to the comment.

pkg/curatedpackages/packagecontrollerclient.go

vivek-koppuru · 2023-03-15T20:34:23Z

pkg/curatedpackages/packagecontrollerclient.go

+	}(&err)
+	pc.mu.Lock()
+	// This anonymous function ensures that the pc.mu is unlocked before
+	// Enable is called, preventing deadlocks in the event that Enable tries


Not sure if I quite understand why we need to use a lock here. Just curious on the use case.

Guillermo pointed out that this isn't thread safe. While we don't have any concurrent code at the moment, he asked that I ensure thread-safety if possible, as there are designs towards concurrency for the future. Since it was relatively simple to add, I did so. This is far from the only thing in our codebase that isn't thread-safe though. ;)

vivek-koppuru · 2023-03-15T20:35:50Z

pkg/dependencies/factory.go

@@ -1049,6 +1049,7 @@ func (f *Factory) WithPackageControllerClient(spec *cluster.Spec, kubeConfig str
 			return err
 		}
 		f.dependencies.PackageControllerClient = curatedpackages.NewPackageControllerClient(
+			f.dependencies.Helm,


Why do we have two helm dependencies being passed in here?

The first is for installing helm charts, the second is for removing helm charts. They can require different parameters when they're instantiated, so you can't assume that you can use the same object to do both. Kinda annoying, but grants flexibility.

Adding comments

What's a case where different need to be specified? I am wondering if we should only stick to one right now if we don't have that use case for two different right now. Whoever needs two separate can then decide to separate this.

Removing via followup. There was a case during development, but then that case was eliminated, so they can be collapsed into one now.

vivek-koppuru · 2023-03-15T20:36:53Z

pkg/executables/helm.go

@@ -116,19 +118,26 @@ func (h *Helm) SaveChart(ctx context.Context, ociURI, version, folder string) er
 }

 func (h *Helm) InstallChartFromName(ctx context.Context, ociURI, kubeConfig, name, version string) error {
-	params := []string{"install", name, ociURI, "--version", version, "--kubeconfig", kubeConfig}
+	params := []string{"upgrade", "--install", name, ociURI, "--version", version, "--kubeconfig", kubeConfig}


Can we have a comment explaining that this will install if it doesn't exist and upgrade if it does, and any side effects of that?

This will install the chart if it doesn't exist, and (if necessary) upgrade it if it does.

This change introduces the creation and deletion of a namespaced package bundle controller for workload clusters created via the full cluster lifecycle. Future commits will add packages installation/deletion via full cluster lifecycle.

Co-authored-by: Guillermo Gaston <guillermogastonlorente@gmail.com>

Co-authored-by: Vivek Koppuru <vivek.koppuru@gmail.com>

eks-distro-bot added the do-not-merge/work-in-progress label Feb 14, 2023

eks-distro-bot added do-not-merge/hold size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2023

ewollesen force-pushed the fclapi-support branch 2 times, most recently from 7eca270 to e9e5017 Compare February 22, 2023 23:55

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 Feb 22, 2023

ewollesen force-pushed the fclapi-support branch 2 times, most recently from 66630e5 to d0f3cf1 Compare March 1, 2023 17:04

eks-distro-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 1, 2023

ewollesen force-pushed the fclapi-support branch 6 times, most recently from d3329a0 to 412df1e Compare March 1, 2023 23:01

ewollesen changed the title ~~WIP curated packages full cluster lifecycle adaptations~~ curated packages full cluster lifecycle adaptations Mar 1, 2023

eks-distro-bot removed the do-not-merge/work-in-progress label Mar 1, 2023

eks-distro-bot removed the do-not-merge/hold label Mar 1, 2023

ewollesen force-pushed the fclapi-support branch from 412df1e to b932576 Compare March 1, 2023 23:09

ewollesen requested review from lewisdiamond and TerryHowe March 2, 2023 15:06

a-cool-train reviewed Mar 2, 2023

View reviewed changes

cmd/eksctl-anywhere/cmd/installpackagecontroller.go Show resolved Hide resolved

a-cool-train reviewed Mar 2, 2023

View reviewed changes

controllers/cluster_controller.go Show resolved Hide resolved

a-cool-train reviewed Mar 2, 2023

View reviewed changes

controllers/cluster_controller.go Outdated Show resolved Hide resolved

ewollesen force-pushed the fclapi-support branch 4 times, most recently from 49428fa to 14609e3 Compare March 14, 2023 18:26

a-cool-train reviewed Mar 14, 2023

View reviewed changes

pkg/executables/helm.go Outdated Show resolved Hide resolved

ewollesen force-pushed the fclapi-support branch from 14609e3 to 3be6109 Compare March 15, 2023 02:22

ewollesen requested review from danbudris and g-gaston March 15, 2023 02:30

danbudris approved these changes Mar 15, 2023

View reviewed changes

eks-distro-bot assigned danbudris Mar 15, 2023

eks-distro-bot added the lgtm label Mar 15, 2023

eks-distro-bot added the approved label Mar 15, 2023

vivek-koppuru reviewed Mar 15, 2023

View reviewed changes

eks-distro-bot removed the lgtm label Mar 15, 2023

Use helm upgrade --install for improved idempotency.

b80e51f

This will install the chart if it doesn't exist, and (if necessary) upgrade it if it does.

vivek-koppuru approved these changes Mar 15, 2023

View reviewed changes

eks-distro-bot assigned vivek-koppuru Mar 15, 2023

eks-distro-bot added the lgtm label Mar 15, 2023

Eric Wollesen and others added 4 commits March 15, 2023 16:09

Update pkg/curatedpackages/packagecontrollerclient.go

21f008e

Co-authored-by: Guillermo Gaston <guillermogastonlorente@gmail.com>

Update pkg/curatedpackages/packagecontrollerclient.go

efa476c

Co-authored-by: Guillermo Gaston <guillermogastonlorente@gmail.com>

Update pkg/curatedpackages/packagecontrollerclient.go

cf44c4e

Co-authored-by: Vivek Koppuru <vivek.koppuru@gmail.com>

ewollesen force-pushed the fclapi-support branch from 57d6965 to cf44c4e Compare March 15, 2023 22:10

eks-distro-bot removed the lgtm label Mar 15, 2023

vivek-koppuru approved these changes Mar 15, 2023

View reviewed changes

eks-distro-bot added the lgtm label Mar 15, 2023

eks-distro-bot merged commit ce9f924 into aws:main Mar 15, 2023

ewollesen deleted the fclapi-support branch March 15, 2023 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

curated packages full cluster lifecycle adaptations #4970

curated packages full cluster lifecycle adaptations #4970

ewollesen commented Feb 14, 2023

ewollesen commented Feb 14, 2023

ewollesen commented Feb 14, 2023

codecov bot commented Feb 16, 2023 •

edited

Loading

ewollesen commented Mar 1, 2023

danbudris left a comment

ewollesen commented Mar 15, 2023

eks-distro-bot commented Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023 •

edited

Loading

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

vivek-koppuru Mar 15, 2023

ewollesen Mar 15, 2023

curated packages full cluster lifecycle adaptations #4970

curated packages full cluster lifecycle adaptations #4970

Conversation

ewollesen commented Feb 14, 2023

ewollesen commented Feb 14, 2023

ewollesen commented Feb 14, 2023

codecov bot commented Feb 16, 2023 • edited Loading

Codecov Report

ewollesen commented Mar 1, 2023

danbudris left a comment

Choose a reason for hiding this comment

ewollesen commented Mar 15, 2023

eks-distro-bot commented Mar 15, 2023

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

Choose a reason for hiding this comment

ewollesen Mar 15, 2023 • 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

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

codecov bot commented Feb 16, 2023 •

edited

Loading

ewollesen Mar 15, 2023 •

edited

Loading