[chart] Node update strategy & auto driver image tag #988

stevehipwell · 2021-07-23T09:06:04Z

Is this a bug fix or adding new feature?
This PR adds the ability to configure the node update strategy and uses the chart app version for the image tag.

Fixes #985.

What is this PR about? / Why do we need it?
With the default update strategy the more nodes added to the cluster the longer it takes to upgrade, this PR makes this customisable and sets the default to the 10% unavailable strategy used by the aws-vpc-cni and the EKS kube-proxy daemonsets.

What testing is done?
Helm template output validated.

k8s-ci-robot · 2021-07-23T09:06:12Z

Welcome @stevehipwell!

It looks like this is your first PR to kubernetes-sigs/aws-ebs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-ebs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-07-23T09:06:12Z

Hi @stevehipwell. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

krmichel · 2021-07-23T16:57:03Z

/approve

vdhanan · 2021-07-23T17:48:32Z

/ok-to-test

stevehipwell · 2021-07-26T08:00:01Z

@vdhanan any chance this could be merged?

vdhanan · 2021-07-26T17:32:28Z

thanks for the PR, i'll review it today

stevehipwell · 2021-07-28T08:37:04Z

@vdhanan I've rebased to capture the last chart release and updated the changelog again for the last chart release. Is there any chance of a review today, I need the update strategy feature ASAP.

krmichel · 2021-07-29T01:06:54Z

charts/aws-ebs-csi-driver/Chart.yaml

@@ -1,8 +1,8 @@
 apiVersion: v2
-appVersion: "1.1.3"
+appVersion: 1.1.3


Is there a reason to add the v elsewhere instead of just setting this to "v1.1.3"

Although the spec says that appVersion needn't be a valid SemVer version, there are tools that either don't work correctly or error if it's not. The comment about quotes being recommended is also misleading as they're only relevant if you have a version that is a different data type.

Additionally, for many reasons which I won't bother listing, sticking with a valid SemVer 2 version here will cause the least potential for disruption as tools and technologies move on.

Fair enough

stevehipwell · 2021-07-29T08:10:22Z

@krmichel @vdhanan I've rebased and updated the changelog again.

krmichel · 2021-07-29T12:12:08Z

@stevehipwell I am not an owner, so I can't trigger a merge. @vdhanan can you get this merged for Steve?

vdhanan · 2021-07-30T00:10:39Z

charts/aws-ebs-csi-driver/Chart.yaml

+annotations:
+  artifacthub.io/changes: |
+    - kind: changed
+      description: Use chart app version as default image tag
+    - kind: changed
+      description: Add updateStrategy to daemonsets


i'm curious what purpose these annotations serve when we already have a helm chart changelog

They will show up on artifact.io when the issue to publish there gets completed

vdhanan · 2021-07-30T00:19:40Z

sorry for the delayed review, and thanks for the PR -- could you please run make generate-kustomize so that the updateStrategy is available for kustomize users as well? otherwise, LGTM. i'll get this merged immediately once that change is made

AndyXiangLi · 2021-07-30T00:52:51Z

charts/aws-ebs-csi-driver/templates/node-windows.yaml

@@ -54,7 +56,7 @@ spec:
        {{- end }}
      containers:
        - name: ebs-plugin
-          image: {{ .Values.image.repository }}:{{ .Values.image.tag }}
+          image: {{ printf "%s:%s" .Values.image.repository (default (printf "v%s" .Chart.AppVersion) (toString .Values.image.tag)) }}


Will .Values.image.tag be empty after change?
Can we use the v{Chart.appversion} in values.yaml continue use the tag field in the template?

If image.tag is not set it will fall back to the value in appVersion. When running a helm install/upgrade you can either use --values to pass a yaml file of values that could contain image.tag to provide a version or use --set image.tag to set the version to use.

https://helm.sh/docs/helm/helm_install/

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell · 2021-07-30T08:04:20Z

Thanks @vdhanan, I've re-generated the Kustomize maifests.

vdhanan · 2021-07-30T20:21:56Z

/lgtm

vdhanan · 2021-07-30T20:42:44Z

/approve

k8s-ci-robot · 2021-07-30T20:42:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krmichel, stevehipwell, vdhanan

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

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 23, 2021

k8s-ci-robot requested review from ddebroy and vdhanan July 23, 2021 09:06

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 23, 2021

krmichel approved these changes Jul 23, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2021

stevehipwell force-pushed the add-update-strategy branch from 2a17cbc to 95cd6f5 Compare July 28, 2021 08:21

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

stevehipwell changed the title ~~[chart] Add updateStrategy to daemonsets~~ [chart] Update driver to v1.2.0, auto image tag & node update strategy Jul 28, 2021

stevehipwell force-pushed the add-update-strategy branch 2 times, most recently from 19df27f to 26a652d Compare July 28, 2021 08:26

stevehipwell changed the title ~~[chart] Update driver to v1.2.0, auto image tag & node update strategy~~ [chart] Node update strategy & auto driver image tag Jul 28, 2021

stevehipwell force-pushed the add-update-strategy branch from 26a652d to fa02d70 Compare July 28, 2021 08:29

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

stevehipwell force-pushed the add-update-strategy branch from fa02d70 to 16de75b Compare July 28, 2021 09:07

krmichel suggested changes Jul 29, 2021

View reviewed changes

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

krmichel mentioned this pull request Jul 29, 2021

Helm chart 1.0 kubernetes-sigs/aws-fsx-csi-driver#194

Merged

stevehipwell force-pushed the add-update-strategy branch from 16de75b to 951de5f Compare July 29, 2021 08:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2021

krmichel approved these changes Jul 29, 2021

View reviewed changes

vdhanan reviewed Jul 30, 2021

View reviewed changes

AndyXiangLi reviewed Jul 30, 2021

View reviewed changes

[chart] Node update strategy & auto driver image tag

10af684

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell force-pushed the add-update-strategy branch from 951de5f to 10af684 Compare July 30, 2021 07:58

k8s-ci-robot assigned vdhanan Jul 30, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2021

k8s-ci-robot merged commit 2c9ad2b into kubernetes-sigs:master Jul 30, 2021

stevehipwell mentioned this pull request Oct 18, 2021

REQUEST: New membership for stevehipwell kubernetes/org#3047

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chart] Node update strategy & auto driver image tag #988

[chart] Node update strategy & auto driver image tag #988

stevehipwell commented Jul 23, 2021 •

edited

Loading

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

krmichel commented Jul 23, 2021 •

edited

Loading

vdhanan commented Jul 23, 2021

stevehipwell commented Jul 26, 2021

vdhanan commented Jul 26, 2021

stevehipwell commented Jul 28, 2021

krmichel Jul 29, 2021

stevehipwell Jul 29, 2021

stevehipwell Jul 29, 2021

krmichel Jul 29, 2021

stevehipwell commented Jul 29, 2021

krmichel commented Jul 29, 2021

vdhanan Jul 30, 2021

krmichel Jul 30, 2021

vdhanan commented Jul 30, 2021

AndyXiangLi Jul 30, 2021

krmichel Jul 30, 2021

stevehipwell commented Jul 30, 2021

vdhanan commented Jul 30, 2021

vdhanan commented Jul 30, 2021

k8s-ci-robot commented Jul 30, 2021

[chart] Node update strategy & auto driver image tag #988

[chart] Node update strategy & auto driver image tag #988

Conversation

stevehipwell commented Jul 23, 2021 • edited Loading

k8s-ci-robot commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

krmichel commented Jul 23, 2021 • edited Loading

vdhanan commented Jul 23, 2021

stevehipwell commented Jul 26, 2021

vdhanan commented Jul 26, 2021

stevehipwell commented Jul 28, 2021

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

stevehipwell commented Jul 29, 2021

krmichel commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdhanan commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevehipwell commented Jul 30, 2021

vdhanan commented Jul 30, 2021

vdhanan commented Jul 30, 2021

k8s-ci-robot commented Jul 30, 2021

stevehipwell commented Jul 23, 2021 •

edited

Loading

krmichel commented Jul 23, 2021 •

edited

Loading