Helm chart 1.0 #194

wongma7 · 2021-06-17T22:51:54Z

Is this a bug fix or adding new feature? fixes #191

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

What testing is done?

wongma7 · 2021-07-16T20:08:45Z

/test pull-aws-fsx-csi-driver-e2e

wongma7 · 2021-07-16T21:58:02Z

/test pull-aws-fsx-csi-driver-e2e

…ready

wongma7 · 2021-07-17T00:38:53Z

• Failure [604.565 seconds]
[fsx-csi-e2e] Dynamic Provisioning with s3 data repository
/home/prow/go/src/github.com/kubernetes-sigs/aws-fsx-csi-driver/tests/e2e/dynamic_provisioning_test.go:111
should create a volume on demand with s3 as data repository [It]
/home/prow/go/src/github.com/kubernetes-sigs/aws-fsx-csi-driver/tests/e2e/dynamic_provisioning_test.go:151

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-fsx-csi-driver/194/pull-aws-fsx-csi-driver-e2e/1416172104553861120

/retest

Makefile

krmichel · 2021-07-29T00:42:12Z

charts/aws-fsx-csi-driver/templates/controller-deployment.yaml

+  replicas: {{ .Values.controller.replicaCount }}
+  selector:
+    matchLabels:
+      app: fsx-csi-controller


Adding this will cause upgrade issues since matchLabels is immutable. It looks like maybe it is being added to ensure that is continues to be included on the kustomize manifests. If that is the case, I would move if from here to an else in the selectorLabels helper template.

I'll test out the upgrade case, since I am bumping major version of helm chart I am not sure without testing how helm deals with it. I just cargo culted how ebs does it TBH: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/charts/aws-ebs-csi-driver/templates/controller.yaml#L12

this change is safe because helm somehow knows to delete and recreate the deployment which also deletes and recreates the pods..

I installed the current helm chart v0.1.0 using the README command
a. (I manually edited the deployment and daemonset to have hostNetwork and to point to image 0.4.0 instead of 0.3.0 because they were crashlooping as I have instance metadata disabled)

I ran helm upgrade --install aws-fsx-csi-driver ./charts/aws-fsx-csi-driver

helm recreated the deployment

k get deployment NAME READY UP-TO-DATE AVAILABLE AGE fsx-csi-controller 1/1 1 1 57s

I didn't notice that the name of the deployment was changing. That explains the delete and create behavior.

charts/aws-fsx-csi-driver/templates/controller-deployment.yaml

charts/aws-fsx-csi-driver/templates/node-serviceaccount.yaml

krmichel · 2021-07-29T01:40:53Z

deploy/kubernetes/base/controller-deployment.yaml

 spec:
  replicas: 2
  selector:
    matchLabels:
      app: fsx-csi-controller
+      app.kubernetes.io/name: aws-fsx-csi-driver


I don't know if this is a concern but this will need intervention on a new apply since matchLabels are immutable.

This may still be the case since the deployment name isn't changing. Did you test the upgrade with kustomize?

charts/aws-fsx-csi-driver/templates/controller-deployment.yaml

deploy/kubernetes/base/controller-deployment.yaml

krmichel · 2021-07-29T01:45:15Z

deploy/kubernetes/base/node-daemonset.yaml

 spec:
  selector:
    matchLabels:
      app: fsx-csi-node
+      app.kubernetes.io/name: aws-fsx-csi-driver


I don't know if this is a concern but this will need intervention on a new apply since matchLabels are immutable.

wongma7 · 2021-08-03T20:55:19Z

I resolved comments I think have been addressed by latest 5 commits, the one open question is how to deal with matchLabels which I will test out. appreciate the thorough review

krmichel

Let me know what you think about trying to validate that the service account exists. Did you test the kustomize based upgrade? It looks like the names of those resources are changing. I haven't used kustomize but based on past kubectl behavior it seems like the match labels could be an issue.

I would seem my comment about the validation is part of a resolved conversation so you might have to look for it :(

krmichel · 2021-08-04T12:31:03Z

charts/aws-fsx-csi-driver/templates/controller-serviceaccount.yaml

  annotations:
    {{- toYaml . | nindent 4 }}
  {{- end }}
+{{- end }}


Personally, I would just add the annotation to controller.serviceAccount.annotations, which is what I did for the EBS chart. I do, however, see what you are saying about some users potentially wanting to use a pre-existing service account.

If this chart isn't creating a service account and therefore expects that one already exists it might be worth using the lookup function to validate that it does exist and fail if it doesn't.
https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-lookup-function

krmichel · 2021-08-04T12:33:23Z

deploy/kubernetes/base/controller-deployment.yaml

 spec:
  replicas: 2
  selector:
    matchLabels:
      app: fsx-csi-controller
+      app.kubernetes.io/name: aws-fsx-csi-driver


This may still be the case since the deployment name isn't changing. Did you test the upgrade with kustomize?

…r own serviceAccount

wongma7 · 2021-08-04T20:02:34Z

latest commit gives lookup function a try. it seems to work but the error message is quite ugly, I'm not sure if I called fail correctly?

$ helm upgrade --install aws-fsx-csi-driver  ./charts/aws-fsx-csi-driver --set controller.serviceAccount.create=false
Release "aws-fsx-csi-driver" does not exist. Installing it now.
Error: template: aws-fsx-csi-driver/templates/controller-serviceaccount.yaml:15:8: executing "aws-fsx-csi-driver/templates/controller-serviceaccount.yaml" at <fail (printf "create serviceaccount %s/%s or set .controller.serviceaccount.create true" .Release.Namespace .Values.controller.serviceAccount.name)>: error calling fail: create serviceaccount default/fsx-csi-controller-sa or set .controller.serviceaccount.create true
~/g/s/g/k/aws-fsx-csi-driver (helmchart1.0|✚1) [1] $ k create serviceaccount fsx-csi-controller-sa
serviceaccount/fsx-csi-controller-sa created
~/g/s/g/k/aws-fsx-csi-driver (helmchart1.0|✚1) $ helm upgrade --install aws-fsx-csi-driver  ./charts/aws-fsx-csi-driver --set controller.serviceAccount.create=false
Release "aws-fsx-csi-driver" does not exist. Installing it now.
NAME: aws-fsx-csi-driver
LAST DEPLOYED: Wed Aug  4 12:59:31 2021
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None

wongma7 · 2021-08-04T20:05:57Z

the kustomize upgrade indeed failed to apply. however it worked with --force, I suppose I can add this to the readme installation instructions as upgrade note, same as for helm (working on that now)

…and helm chart

krmichel · 2021-08-05T00:41:23Z

latest commit gives lookup function a try. it seems to work but the error message is quite ugly, I'm not sure if I called fail correctly?

$ helm upgrade --install aws-fsx-csi-driver  ./charts/aws-fsx-csi-driver --set controller.serviceAccount.create=false
Release "aws-fsx-csi-driver" does not exist. Installing it now.
Error: template: aws-fsx-csi-driver/templates/controller-serviceaccount.yaml:15:8: executing "aws-fsx-csi-driver/templates/controller-serviceaccount.yaml" at <fail (printf "create serviceaccount %s/%s or set .controller.serviceaccount.create true" .Release.Namespace .Values.controller.serviceAccount.name)>: error calling fail: create serviceaccount default/fsx-csi-controller-sa or set .controller.serviceaccount.create true
~/g/s/g/k/aws-fsx-csi-driver (helmchart1.0|✚1) [1] $ k create serviceaccount fsx-csi-controller-sa
serviceaccount/fsx-csi-controller-sa created
~/g/s/g/k/aws-fsx-csi-driver (helmchart1.0|✚1) $ helm upgrade --install aws-fsx-csi-driver  ./charts/aws-fsx-csi-driver --set controller.serviceAccount.create=false
Release "aws-fsx-csi-driver" does not exist. Installing it now.
NAME: aws-fsx-csi-driver
LAST DEPLOYED: Wed Aug  4 12:59:31 2021
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None

Looks like a correct use of fail.

nckturner · 2021-08-05T18:36:36Z

/lgtm
/approve

k8s-ci-robot · 2021-08-05T18:36:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krmichel, nckturner, wongma7

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

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

Use chart-releaser github action

ead1645

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 17, 2021

k8s-ci-robot requested review from justinsb and leakingtapan June 17, 2021 22:52

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2021

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 15, 2021

wongma7 force-pushed the helmchart1.0 branch 4 times, most recently from e94dd40 to 295421f Compare July 15, 2021 23:45

wongma7 force-pushed the helmchart1.0 branch from 295421f to bc9e87e Compare July 16, 2021 20:48

Helm chart 1.0: shuffle values to better match ebs/efs charts

745ca5a

wongma7 force-pushed the helmchart1.0 branch from bc9e87e to 745ca5a Compare July 16, 2021 22:10

wongma7 added 3 commits July 16, 2021 15:16

Update README, helm CHANGELOG

c99a8cc

Add make generate-kustomize

a4674b2

Run make generate-kustomize and delete old yamls

9f76863

k8s-ci-robot 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 Jul 16, 2021

Set replicaCount=1 since NODE_COUNT=1, else controller never becomes …

8355ae3

…ready

wongma7 changed the title ~~WIP: Helm chart 1.0~~ Helm chart 1.0 Jul 16, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2021

nckturner reviewed Jul 20, 2021

View reviewed changes

Makefile Show resolved Hide resolved

Makefile Show resolved Hide resolved

krmichel suggested changes Jul 29, 2021

View reviewed changes

Add Values.controller.tolerations

de19088

wongma7 added 4 commits August 3, 2021 13:50

Default driver image tag to .Chart.AppVersion

e63dda3

Add Values.node.tolerations and Values.node.tolerateAllTaints

69de92c

Add extra-create-metadata and leader-election flags to csi-provisioner

f9c1b50

Run make generate-kustomize

1702134

wongma7 added 2 commits August 3, 2021 14:00

Update helm CHANGELOG

3fd96fc

Wipe 0.x.0 helm chart README

0200261

krmichel suggested changes Aug 4, 2021

View reviewed changes

Fail if *.serviceAccount.create is false and user hasn't created thei…

a6e7303

…r own serviceAccount

Update README to mention breaking changes in kustomize configuration …

34ec8f5

…and helm chart

krmichel approved these changes Aug 5, 2021

View reviewed changes

k8s-ci-robot assigned nckturner Aug 5, 2021

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

k8s-ci-robot merged commit 70a9ac3 into kubernetes-sigs:master Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm chart 1.0 #194

Helm chart 1.0 #194

wongma7 commented Jun 17, 2021

wongma7 commented Jul 16, 2021

wongma7 commented Jul 16, 2021

wongma7 commented Jul 17, 2021

krmichel Jul 29, 2021

wongma7 Aug 3, 2021

wongma7 Aug 3, 2021

krmichel Aug 4, 2021

krmichel Jul 29, 2021

krmichel Aug 4, 2021

krmichel Jul 29, 2021

wongma7 commented Aug 3, 2021

krmichel left a comment •

edited

Loading

krmichel Aug 4, 2021

krmichel Aug 4, 2021

wongma7 commented Aug 4, 2021

wongma7 commented Aug 4, 2021

krmichel commented Aug 5, 2021

nckturner commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

Helm chart 1.0 #194

Helm chart 1.0 #194

Conversation

wongma7 commented Jun 17, 2021

wongma7 commented Jul 16, 2021

wongma7 commented Jul 16, 2021

wongma7 commented Jul 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wongma7 commented Aug 3, 2021

krmichel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wongma7 commented Aug 4, 2021

wongma7 commented Aug 4, 2021

krmichel commented Aug 5, 2021

nckturner commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

krmichel left a comment •

edited

Loading