Add methods to fetch upgrader image from bundle #7283

taneyland · 2024-01-10T16:08:12Z

Issue #, if available:

Description of changes:
The eks-a controller will be responsible for creating the custom CP/MD upgrade resources for in-place upgrades. We need a way to fetch the upgrader image from the bundle so this adds methods that will be used in the controller to pull the bundle and fetch the images. The images will be stored in a config map. This upgrader config map will then be fetched by the node upgrader when the image is needed.

ConfigMap:

apiVersion: v1
data:
  v1.25.16-eks-1-25-29: public.ecr.aws/l0g8r8j6/aws/upgrader:v1-25-29-eks-a-v0.0.0-dev-build.8103
  v1.26.11-eks-1-26-25: public.ecr.aws/l0g8r8j6/aws/upgrader:v1-26-25-eks-a-v0.0.0-dev-build.8103
  v1.27.8-eks-1-27-19: public.ecr.aws/l0g8r8j6/aws/upgrader:v1-27-19-eks-a-v0.0.0-dev-build.8103
  v1.28.4-eks-1-28-12: public.ecr.aws/l0g8r8j6/aws/upgrader:v1-28-12-eks-a-v0.0.0-dev-build.8103
  v1.29.0-eks-1-29-1: public.ecr.aws/l0g8r8j6/aws/upgrader:v1-29-1-eks-a-v0.0.0-dev-build.8103
kind: ConfigMap

Testing (if applicable):
performed in-place upgrade with image from bundle

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.

codecov · 2024-01-10T16:14:30Z

Codecov Report

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

Comparison is base (413ea04) 71.65% compared to head (0d2b99b) 71.73%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
controllers/nodeupgrade_controller.go	35.71%	6 Missing and 3 partials ⚠️
pkg/clustermanager/cluster_manager.go	89.18%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7283      +/-   ##
==========================================
+ Coverage   71.65%   71.73%   +0.07%     
==========================================
  Files         556      559       +3     
  Lines       43185    43416     +231     
==========================================
+ Hits        30944    31144     +200     
- Misses      10532    10553      +21     
- Partials     1709     1719      +10

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

vivek-koppuru · 2024-01-17T06:15:48Z

controllers/nodeupgrade_controller.go

@@ -168,14 +166,20 @@ func (r *NodeUpgradeReconciler) reconcile(ctx context.Context, log logr.Logger,
 		return ctrl.Result{}, nil
 	}

+	configMap := &corev1.ConfigMap{}
+	if err := remoteClient.Get(ctx, types.NamespacedName{Name: constants.UpgraderConfigMapName, Namespace: constants.EksaSystemNamespace}, configMap); err != nil {


Other option is so pass in the config map from here to node upgrade object, just so that if node upgrader is run on it's own for any reason, we can pass in any config map. If we decide to mount the config map to the upgrader pod directly, then that gives us a different amount of flexibility where we can deploy the pod directly with a different config map mounted. Just some thoughts that I have to consider.

controllers/nodeupgrade_controller.go

pkg/constants/constants.go

pkg/clustermanager/cluster_manager.go

abhinavmpandey08 · 2024-01-18T21:37:20Z

controllers/nodeupgrade_controller.go

+	if configMap.Data == nil {
+		return ctrl.Result{}, errors.New("upgrader config map is empty")
+	}
+	upgraderImage := configMap.Data[nodeUpgrade.Spec.KubernetesVersion]


this check if configMap.Data == nil { just verifies that the configmap data is still nil but not if the configmap contains an entry for the EKS Distro/K8s version we are looking for
We can keep that but can we also add something like this?

Suggested change

upgraderImage := configMap.Data[nodeUpgrade.Spec.KubernetesVersion]

upgraderImage, ok := configMap.Data[nodeUpgrade.Spec.KubernetesVersion]

if !ok {

return ctrl.Result{}, fmt.Errorf("upgrader image corresponding to EKS Distro version %s not found in the configmp", nodeUpgrade.Spec.KubernetesVersion)

}

Ah yes. I totally agree. Thanks!

abhinavmpandey08 · 2024-01-18T21:43:00Z

pkg/clustermanager/cluster_manager.go

+		if _, ok := upgraderConfigMap.Data[version]; !ok {
+			upgraderConfigMap.Data[version] = image
+		}


I think for this case, when a version is already in the configmap, we should override it instead of preserving the older version

Suggested change

if _, ok := upgraderConfigMap.Data[version]; !ok {

upgraderConfigMap.Data[version] = image

}

upgraderConfigMap.Data[version] = image

abhinavmpandey08 · 2024-01-18T22:53:28Z

/approve
/woof

eks-distro-bot · 2024-01-18T22:53:30Z

@abhinavmpandey08:

In response to this:

/approve
/woof

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 · 2024-01-18T22:53:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavmpandey08

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

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

taneyland added the do-not-merge/hold label Jan 10, 2024

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

taneyland force-pushed the fetch_from_bundle branch 7 times, most recently from 007680d to aa260aa Compare January 16, 2024 23:28

taneyland removed the do-not-merge/hold label Jan 16, 2024

vivek-koppuru reviewed Jan 17, 2024

View reviewed changes

taneyland force-pushed the fetch_from_bundle branch 3 times, most recently from c1599f2 to 72f570c Compare January 17, 2024 17:29

abhinavmpandey08 reviewed Jan 17, 2024

View reviewed changes

controllers/nodeupgrade_controller.go Outdated Show resolved Hide resolved

pkg/constants/constants.go Outdated Show resolved Hide resolved

pkg/clustermanager/cluster_manager.go Show resolved Hide resolved

taneyland force-pushed the fetch_from_bundle branch from 72f570c to d16f35a Compare January 18, 2024 16:27

abhinavmpandey08 reviewed Jan 18, 2024

View reviewed changes

Add methods to fetch upgrader image from bundle

0d2b99b

taneyland force-pushed the fetch_from_bundle branch from d16f35a to 0d2b99b Compare January 18, 2024 21:54

abhinavmpandey08 approved these changes Jan 18, 2024

View reviewed changes

eks-distro-bot assigned abhinavmpandey08 Jan 18, 2024

eks-distro-bot added the lgtm label Jan 18, 2024

eks-distro-bot added the approved label Jan 18, 2024

taneyland merged commit 50c4621 into aws:main Jan 18, 2024
11 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to fetch upgrader image from bundle #7283

Add methods to fetch upgrader image from bundle #7283

taneyland commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

vivek-koppuru Jan 17, 2024 •

edited

Loading

abhinavmpandey08 Jan 18, 2024 •

edited

Loading

taneyland Jan 18, 2024

abhinavmpandey08 Jan 18, 2024

abhinavmpandey08 commented Jan 18, 2024

eks-distro-bot commented Jan 18, 2024

eks-distro-bot commented Jan 18, 2024

-	upgraderImage := configMap.Data[nodeUpgrade.Spec.KubernetesVersion]
+	upgraderImage, ok := configMap.Data[nodeUpgrade.Spec.KubernetesVersion]
+	if !ok {
+	    return ctrl.Result{}, fmt.Errorf("upgrader image corresponding to EKS Distro version %s not found in the configmp", nodeUpgrade.Spec.KubernetesVersion)
+	}

Add methods to fetch upgrader image from bundle #7283

Add methods to fetch upgrader image from bundle #7283

Conversation

taneyland commented Jan 10, 2024 • edited Loading

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

vivek-koppuru Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

abhinavmpandey08 Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

taneyland Jan 18, 2024

Choose a reason for hiding this comment

abhinavmpandey08 Jan 18, 2024

Choose a reason for hiding this comment

abhinavmpandey08 commented Jan 18, 2024

eks-distro-bot commented Jan 18, 2024

eks-distro-bot commented Jan 18, 2024

taneyland commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

vivek-koppuru Jan 17, 2024 •

edited

Loading

abhinavmpandey08 Jan 18, 2024 •

edited

Loading