local volume provisioner: improve helm chart #789

cofyc · 2018-06-04T07:48:13Z

Get rid of engine thing, it's better to use differnt values files for different environments.
Rename jobsrole and local-storage-provisioner-job-binding to local-storage-provisioner, and only create them when necessary.
Able to create StorageClass object and configure it.
Add docs for all chart configurations.
- https://github.com/cofyc/external-storage/blob/refactor_heml_chart/local-volume/helm/README.md#configurations
Add some examples.
- https://github.com/cofyc/external-storage/blob/refactor_heml_chart/local-volume/helm/README.md#examples

cofyc · 2018-06-04T07:55:50Z

/assign @msau42
/area local-volume

msau42 · 2018-06-04T22:27:39Z

local-volume/helm/README.md

+| common.useJobForCleaning               | Is set to true, provisioner will use jobs-based block cleaning.                                       | bool     | `false`                                                    |
+| common.configMapName                   | Provisioner ConfigMap name.                                                                           | str      | `local-provisioner-config`                                 |
+| classes.[n].name                       | StorageClass name.                                                                                    | str      | `-`                                                        |
+| classes.[n].hostDir                    | Path on the host where local volume of this storage class are mounted.                                | str      | `-`                                                        |


local volumes of this storage class are mounted under

msau42 · 2018-06-04T22:31:28Z

local-volume/helm/README.md

+| classes.[n].storageClass.reclaimPolicy | Specify reclaimPolicy of storage class, available: Delete/Retain.                                     | str      | `Delete`                                                   |
+| daemonset.name                         | Provisioner DaemonSet name.                                                                           | str      | `local-volume-provisioner`                                 |
+| daemonset.image                        | Provisioner image.                                                                                    | str      | `quay.io/external_storage/local-volume-provisioner:v2.1.0` |
+| daemonset.imagePullPolicy              | Provisioner DaemonSet image pull policy.                                                              | str      | `Always`                                                   |


I'm thinking it may be better to just use the default imagePullPolicy if not set.

msau42 · 2018-06-04T22:34:13Z

local-volume/helm/examples/gce-cleanbyjobs.yaml

+classes:
+- name: local-scsi
+  hostDir: "/mnt/disks/by-uuid/google-local-ssds-scsi-fs"
+  blockCleanerCommand:


On gce/gke, we actually separate block and preformatted volumes into different directories. So this block cleaning config doesn't do anything. It may be better to put this in a baremetal example.

Should I remove all blockCleanerCommand sections from other gce/gke example yamls? I never run kubernetes on gce/gke, could you help test these examples if possible?

Yeah I would leave it out for now and add it to the baremetal example instead. I have an issue open on me to integrate and test the block feature on gce/gke

Done in d1d0ef9.

msau42 · 2018-06-04T22:35:39Z

local-volume/helm/examples/baremetal-without-rbac.yaml

+  blockCleanerCommand:
+  - "/scripts/quick_reset.sh"
+  storageClass:
+    reclaimPolicy: Delete


I would leave out reclaim policy if you're going to use the default

cofyc · 2018-06-05T03:33:28Z

Updated in 616d77d.

Update docs.
By default, don't set imagePullPolicy (will use default kubernetes imagePullPolicy).
Remove gce-cleanbyjobs.yaml, add baremetal-cleanbyjobs.yaml.
Comment other avaialbe block cleaner commands in baremetal.yaml.
Leave out reclaimPolicy: Delete if we're going to use the default.

cofyc · 2018-06-05T08:23:43Z

Add update-helm-values utility in b210c6c.

There is a problem vendoring "k8s.io/helm/pkg/chartutil" package with glide, so I simply copied utilities I need into this repo. They are all k8s code, I think it's fine to copy here. How about fixing it later?

glide up -v always fails with this error:

...
[ERROR]	Error scanning github.com/digitalocean/godo/context: cannot find package "." in:
	/home/vagrant/.glide/cache/src/https-git.luolix.top-digitalocean-godo/context
...

found a related issue here: Masterminds/glide#968

msau42 · 2018-06-05T17:30:14Z

local-volume/utils/update-helm-values/README.md

@@ -0,0 +1,9 @@
+## update helm values


can you make this directory name be explicit about the before/after versions that this tool supports?

It supports its first version to local-volume-provisioner-v2.1.0, so I renamed it to update-helm-values-pre-v2.2.0. If you have a better name, let me change to it.

msau42 · 2018-06-05T17:49:07Z

local-volume/helm/examples/gce-retain.yaml

+common:
+  # Beta PV.NodeAffinity field is used by default. If running against pre-1.10
+  # k8s version, the `useAlphaAPI` flag must be enabled in the configMap.
+  useAlphaAPI: false


Let's leave out any default values.

msau42 · 2018-06-05T17:56:33Z

local-volume/helm/provisioner/templates/provisioner-cluster-role-binding.yaml

 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
-  name: jobsrole
+  name: local-storage-provisioner


A more descriptive name could be "local-storage-provisioner-jobs-role"

msau42 · 2018-06-06T00:28:30Z

local-volume/helm/provisioner/templates/provisioner.yaml

-{{- range $tmp, $val := $classPath }}
-   {{- range $storageClass, $classConfig := $val }}
-{{ $storageClass }}:
+{{- range $classConfig := . -}}


Is it possible to put all this inline with the rest of the spec template instead of at the beginning?

Done, I removed variables aliases which I found not necessary too. It's hard to read diff of provisioner.yaml now, you can simply check final version of provisioner.

I also added a helm chart test script here: https://github.com/cofyc/external-storage/tree/refactor_heml_chart/local-volume/helm/test

cofyc · 2018-06-06T03:51:03Z

Updated in e6f6fe9:

Rename update-helm-values to update-helm-values-pre-v2.2.0
Add more tests in update-helm-values-pre-v2.2.0
Leave out any default values in examples
Rename role local-storage-provisioner to local-storage-provisioner-jobs-role
Rename rolebinding local-storage-provisioner to local-storage-provisioner-jobs-rolebinding
Inline all template definitions
Remove all template varaibles aliases
Add helm tests and make test-local-volume/helm

cofyc · 2018-06-06T14:30:20Z

I missed daemonset.kubeConfigEnv configuration, pushed a new commit 3958fb0.

msau42

This lgtm! Please squash

msau42 · 2018-06-06T21:03:17Z

local-volume/helm/provisioner/templates/provisioner.yaml

-
-{{- if $kubeConfigEnv }}
+            value: "{{ .Values.daemonset.image }}"
+{{- if .Values.daemonset.kubeConfigEnv }}


Can this be indented?

… for different environments - Create job RBAC when necessary - Able to create StorageClass object and configure it - Add docs and examples for helm chart - By default, don't set imagePullPolicy (will use default kubernetes imagePullPolicy) - Add update-helm-values-pre-v2.2.0 for migrating helm chart values - Inline all template definitions and remove all template varaibles aliases

cofyc · 2018-06-07T04:03:54Z

@msau42 Done. I squashed it into two commits:

Improvements and refactorings: 68d5d15
Tests: cc6db8a

msau42 · 2018-06-07T16:44:54Z

/retest

cofyc · 2018-06-08T01:33:23Z

seems like /retest does not trigger travis rebuild, I pushed again.

msau42 · 2018-06-08T17:31:45Z

/lgtm
/assign @childsb

local volume provisioner: improve helm chart

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 4, 2018

cofyc force-pushed the refactor_heml_chart branch 3 times, most recently from 45ad26d to 9d704dd Compare June 4, 2018 07:54

k8s-ci-robot assigned msau42 Jun 4, 2018

k8s-ci-robot added the area/local-volume label Jun 4, 2018

cofyc force-pushed the refactor_heml_chart branch 2 times, most recently from 1a8b012 to 1db9336 Compare June 4, 2018 08:14

msau42 reviewed Jun 4, 2018

View reviewed changes

cofyc force-pushed the refactor_heml_chart branch 3 times, most recently from 7c36e01 to 616d77d Compare June 5, 2018 03:20

cofyc force-pushed the refactor_heml_chart branch from 427e967 to d1d0ef9 Compare June 5, 2018 05:21

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 Jun 5, 2018

cofyc force-pushed the refactor_heml_chart branch from 8613b32 to b210c6c Compare June 5, 2018 08:25

msau42 reviewed Jun 5, 2018

View reviewed changes

msau42 reviewed Jun 6, 2018

View reviewed changes

k8s-ci-robot 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 Jun 6, 2018

cofyc force-pushed the refactor_heml_chart branch 4 times, most recently from 036a77f to e6f6fe9 Compare June 6, 2018 06:43

msau42 reviewed Jun 6, 2018

View reviewed changes

cofyc force-pushed the refactor_heml_chart branch from 3958fb0 to 4ac87aa Compare June 7, 2018 03:59

cofyc mentioned this pull request Jun 7, 2018

Added support for tolerations in local-volume provisioner daemonset helm template #792

Merged

Add helm tests and make test-local-volume/helm

cc6db8a

cofyc force-pushed the refactor_heml_chart branch from 4ac87aa to cc6db8a Compare June 8, 2018 01:17

k8s-ci-robot assigned childsb Jun 8, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2018

childsb merged commit b05b770 into kubernetes-retired:master Jun 13, 2018

cofyc deleted the refactor_heml_chart branch June 14, 2018 02:14

cofyc pushed a commit to cofyc/external-storage that referenced this pull request Dec 17, 2018

Merge pull request kubernetes-retired#789 from cofyc/refactor_heml_chart

e09f9b0

local volume provisioner: improve helm chart

nikhita unassigned childsb Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local volume provisioner: improve helm chart #789

local volume provisioner: improve helm chart #789

cofyc commented Jun 4, 2018 •

edited

Loading

cofyc commented Jun 4, 2018

msau42 Jun 4, 2018

msau42 Jun 4, 2018

msau42 Jun 4, 2018

cofyc Jun 5, 2018

msau42 Jun 5, 2018

cofyc Jun 5, 2018 •

edited

Loading

msau42 Jun 4, 2018

cofyc commented Jun 5, 2018

cofyc commented Jun 5, 2018 •

edited

Loading

msau42 Jun 5, 2018

cofyc Jun 6, 2018

msau42 Jun 5, 2018

cofyc Jun 6, 2018

msau42 Jun 5, 2018

cofyc Jun 6, 2018

msau42 Jun 6, 2018

cofyc Jun 6, 2018

cofyc commented Jun 6, 2018 •

edited

Loading

cofyc commented Jun 6, 2018

msau42 left a comment

msau42 Jun 6, 2018

cofyc Jun 7, 2018

cofyc commented Jun 7, 2018 •

edited

Loading

msau42 commented Jun 7, 2018

cofyc commented Jun 8, 2018

msau42 commented Jun 8, 2018

local volume provisioner: improve helm chart #789

local volume provisioner: improve helm chart #789

Conversation

cofyc commented Jun 4, 2018 • edited Loading

cofyc commented Jun 4, 2018

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

cofyc Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Jun 5, 2018

cofyc commented Jun 5, 2018 • 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

cofyc commented Jun 6, 2018 • edited Loading

cofyc commented Jun 6, 2018

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Jun 7, 2018 • edited Loading

msau42 commented Jun 7, 2018

cofyc commented Jun 8, 2018

msau42 commented Jun 8, 2018

cofyc commented Jun 4, 2018 •

edited

Loading

cofyc Jun 5, 2018 •

edited

Loading

cofyc commented Jun 5, 2018 •

edited

Loading

cofyc commented Jun 6, 2018 •

edited

Loading

cofyc commented Jun 7, 2018 •

edited

Loading