Add Helm compatibility for --reuse-values upgrades #1789

ConnorJC3 · 2023-10-18T18:40:13Z

Is this a bug fix or adding new feature?

Adds support for upgrading with --reuse-values with Helm from any prior version

What is this PR about? / Why do we need it?

The team decided we want to support upgrades when using --reuse-values. This PR adds support for that from any prior version. Unfortunately, already released versions cannot be fixed, but this will fix any upgrades to the next or any later version.

What testing is done?

Testing on master (doesn't work):

# Oldest release of the EBS CSI Driver chart, thus the worst case scenario for --reuse-values
$ git checkout aws-ebs-csi-driver-0.6.2

# Note: depending on your Kubernetes version, you may need to make some changes to the Chart to bump removed API versions
$ helm install aws-ebs-csi-driver .
NAME: aws-ebs-csi-driver
LAST DEPLOYED: Wed Oct 18 18:29:05 2023
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
To verify that aws-ebs-csi-driver has started, run:

    kubectl get pod -n kube-system -l "app.kubernetes.io/name=aws-ebs-csi-driver,app.kubernetes.io/instance=aws-ebs-csi-driver"
    
# Switch to latest release
$ git checkout release-1.23

# Note: useOldCSIDriver=true is required to prevent immutable change (although it won't matter here because the upgrade is broken anyways)
# It breaks!
$ helm upgrade --reuse-values --set useOldCSIDriver=true aws-ebs-csi-driver .
Error: UPGRADE FAILED: template: aws-ebs-csi-driver/templates/volumesnapshotclass.yaml:1:17: executing "aws-ebs-csi-driver/templates/volumesnapshotclass.yaml" at <.Values.sidecars.snapshotter.forceEnable>: nil pointer evaluating interface {}.forceEnable

Testing with PR (works):

# Oldest release of the EBS CSI Driver chart, thus the worst case scenario for --reuse-values
$ git checkout aws-ebs-csi-driver-0.6.2

# Note: depending on your Kubernetes version, you may need to make some changes to the Chart to bump removed API versions
$ helm install aws-ebs-csi-driver .
NAME: aws-ebs-csi-driver
LAST DEPLOYED: Wed Oct 18 18:31:53 2023
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
To verify that aws-ebs-csi-driver has started, run:

    kubectl get pod -n kube-system -l "app.kubernetes.io/name=aws-ebs-csi-driver,app.kubernetes.io/instance=aws-ebs-csi-driver"

# Switch to PR
$ git checkout helm-reuse-values-support

# Note: useOldCSIDriver=true is required to prevent immutable change
# It works!
$ helm upgrade --reuse-values --set useOldCSIDriver=true aws-ebs-csi-driver .          
Release "aws-ebs-csi-driver" has been upgraded. Happy Helming!
NAME: aws-ebs-csi-driver
LAST DEPLOYED: Wed Oct 18 18:33:57 2023
NAMESPACE: default
STATUS: deployed
REVISION: 3
NOTES:
To verify that aws-ebs-csi-driver has started, run:

    kubectl get pod -n default -l "app.kubernetes.io/name=aws-ebs-csi-driver,app.kubernetes.io/instance=aws-ebs-csi-driver"

NOTE: The [CSI Snapshotter](https://github.com/kubernetes-csi/external-snapshotter) controller and CRDs will no longer be installed as part of this chart and moving forward will be a prerequisite of using the snap shotting functionality.

# All pods are running etc
$ kubectl -n kube-system get pods                              
NAME                                            READY   STATUS    RESTARTS      AGE
aws-cloud-controller-manager-dt766              1/1     Running   0             41m
aws-node-termination-handler-68d9cbd5dc-xfzcx   1/1     Running   0             41m
coredns-78c4944598-8zs9h                        1/1     Running   0             40m
coredns-78c4944598-bbhqv                        1/1     Running   0             41m
coredns-autoscaler-fcf87bf56-h5j9h              1/1     Running   0             41m
dns-controller-c848f4d4d-kvxx4                  1/1     Running   0             41m
etcd-manager-events-i-00dc16e2d485fab66         1/1     Running   0             42m
etcd-manager-main-i-00dc16e2d485fab66           1/1     Running   0             42m
kops-controller-vmfll                           1/1     Running   0             41m
kube-apiserver-i-00dc16e2d485fab66              2/2     Running   2 (43m ago)   41m
kube-controller-manager-i-00dc16e2d485fab66     1/1     Running   4 (42m ago)   42m
kube-proxy-i-008703fa7cb2d77b7                  1/1     Running   0             39m
kube-proxy-i-00dc16e2d485fab66                  1/1     Running   0             41m
kube-scheduler-i-00dc16e2d485fab66              1/1     Running   0             41m

k8s-ci-robot · 2023-10-18T18:40:22Z

[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 connorjc3. 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

Signed-off-by: Connor Catlett <[email protected]>

k8s-ci-robot · 2023-10-19T15:54:29Z

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

Test name	Commit	Details	Required	Rerun command
pull-aws-ebs-csi-driver-external-test-eks	`8344c69`	link	true	`/test pull-aws-ebs-csi-driver-external-test-eks`
pull-aws-ebs-csi-driver-test-e2e-external-eks-windows	`8344c69`	link	true	`/test pull-aws-ebs-csi-driver-test-e2e-external-eks-windows`
pull-aws-ebs-csi-driver-e2e-single-az	`8344c69`	link	true	`/test pull-aws-ebs-csi-driver-e2e-single-az`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

AndrewSirenko · 2023-10-20T22:44:47Z

charts/aws-ebs-csi-driver/templates/_node-windows.tpl

@@ -1,5 +1,6 @@
 {{- define "node-windows" }}
-{{- if .Values.node.enableWindows }}
+{{- if (.Values.node).enableWindows }}
+---


This --- is just for styling yes?

No, it's for separating multiple YAML documents in the same file. It used to be inside node.yaml and node-windows.yaml but I moved it here so it's only rendered if the relevant DaemonSet is enabled, otherwise extra ---s will be rendered when the DaemonSet is disabled.

AndrewSirenko · 2023-10-20T22:49:19Z

charts/aws-ebs-csi-driver/templates/_node-windows.tpl

@@ -117,23 +148,27 @@ spec:
              exec:
                command: ["/bin/aws-ebs-csi-driver", "pre-stop-hook"]
        - name: node-driver-registrar
-          image: {{ printf "%s%s:%s" (default "" .Values.image.containerRegistry) .Values.sidecars.nodeDriverRegistrar.image.repository .Values.sidecars.nodeDriverRegistrar.image.tag }}
-          imagePullPolicy: {{ default .Values.image.pullPolicy .Values.sidecars.nodeDriverRegistrar.image.pullPolicy }}
+          image: {{ printf "%s%s:%s" (default "" (.Values.image).containerRegistry) (default "public.ecr.aws/eks-distro/kubernetes-csi/node-driver-registrar" (((.Values.sidecars).nodeDriverRegistrar).image).repository) (default "v2.9.0-eks-1-28-6" (((.Values.sidecars).nodeDriverRegistrar).image).tag) }}


I'm against putting default sidecar tags in the chart that we would have to risk changing every release.

It seems like this default value is not relevant to the chart tester CI job failing: Additional DaemonSets feature #1722

I dislike it, but all default values (including tags) must be duplicated into the chart for this support to work. Otherwise, users upgrading with --reuse-values will end up with an inconsistent (or in the case of tags, broken) experience.

AndrewSirenko · 2023-10-20T22:52:10Z

deploy/kubernetes/base/serviceaccount-csi-controller.yaml

@@ -6,7 +6,4 @@ metadata:
  name: ebs-csi-controller-sa
  labels:
    app.kubernetes.io/name: aws-ebs-csi-driver
-  #Enable if EKS IAM roles for service accounts (IRSA) is used. See https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html for details.


Why are we removing this comment? Is it no longer relevant?

They aren't critical to this change, so if you feel strongly I can put them back in, but I took the opportunity to remove all remaining if eq .Release.Name "kustomize" invocations in the chart as they were directly next to what I was already working on.

This particular comment is bad because:

It encourages manually editing the manifests, which is bad practice and we should not be encouraging (instead, users should use Helm parameters or Kustomize to make changes)

It creates a small additional burden on maintainers that they must understand the special properties of the kustomize release name

Explaining how IAM permissions interact with the driver is better left to install.md, where we document the various methods of granting IAM permissions, what policies the role needs, the existence of the managed policy, and other crucial information this short comment does not contain

k8s-ci-robot · 2023-10-28T03:16:10Z

PR needs rebase.

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.

ConnorJC3 · 2023-12-13T18:24:54Z

/close

After further evaluation, the team decided to deprecate --reuse-values in the helm chart in the future instead. Closing this PR out.

k8s-ci-robot · 2023-12-13T18:25:00Z

@ConnorJC3: Closed this PR.

In response to this:

/close

After further evaluation, the team decided to deprecate --reuse-values in the helm chart in the future instead. Closing this PR out.

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 18, 2023

k8s-ci-robot requested review from AndrewSirenko and torredil October 18, 2023 18:40

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 18, 2023

ConnorJC3 force-pushed the helm-reuse-values-support branch from 5bd9f7d to 894e5b2 Compare October 18, 2023 18:44

Add Helm compatibility for --reuse-values upgrades

8344c69

Signed-off-by: Connor Catlett <[email protected]>

ConnorJC3 force-pushed the helm-reuse-values-support branch from 894e5b2 to 8344c69 Compare October 19, 2023 15:17

AndrewSirenko requested changes Oct 20, 2023

View reviewed changes

k8s-ci-robot assigned AndrewSirenko Oct 20, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2023

ConnorJC3 force-pushed the master branch 2 times, most recently from 24a8e7b to bddbe0b Compare November 1, 2023 18:08

k8s-ci-robot closed this Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Helm compatibility for --reuse-values upgrades #1789

Add Helm compatibility for --reuse-values upgrades #1789

ConnorJC3 commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

k8s-ci-robot commented Oct 19, 2023 •

edited

Loading

AndrewSirenko Oct 20, 2023

ConnorJC3 Oct 21, 2023

AndrewSirenko Oct 20, 2023

AndrewSirenko Oct 20, 2023

ConnorJC3 Oct 21, 2023

AndrewSirenko Oct 20, 2023

ConnorJC3 Oct 21, 2023 •

edited

Loading

k8s-ci-robot commented Oct 28, 2023

ConnorJC3 commented Dec 13, 2023

k8s-ci-robot commented Dec 13, 2023

Add Helm compatibility for --reuse-values upgrades #1789

Add Helm compatibility for --reuse-values upgrades #1789

Conversation

ConnorJC3 commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

k8s-ci-robot commented Oct 19, 2023 • edited Loading

AndrewSirenko Oct 20, 2023

Choose a reason for hiding this comment

ConnorJC3 Oct 21, 2023

Choose a reason for hiding this comment

AndrewSirenko Oct 20, 2023

Choose a reason for hiding this comment

AndrewSirenko Oct 20, 2023

Choose a reason for hiding this comment

ConnorJC3 Oct 21, 2023

Choose a reason for hiding this comment

AndrewSirenko Oct 20, 2023

Choose a reason for hiding this comment

ConnorJC3 Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2023

ConnorJC3 commented Dec 13, 2023

k8s-ci-robot commented Dec 13, 2023

k8s-ci-robot commented Oct 19, 2023 •

edited

Loading

ConnorJC3 Oct 21, 2023 •

edited

Loading