From 58caa9327bbab4e8b41a50c5028e82587da30213 Mon Sep 17 00:00:00 2001 From: Rob Kernick Date: Fri, 16 Mar 2018 14:15:35 -0400 Subject: [PATCH 1/3] Add review guidelines around pvcs --- REVIEW_GUIDELINES.md | 51 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/REVIEW_GUIDELINES.md b/REVIEW_GUIDELINES.md index 0419a07aaf64..581a6cab3f89 100644 --- a/REVIEW_GUIDELINES.md +++ b/REVIEW_GUIDELINES.md @@ -76,6 +76,45 @@ image: * The use of the `default` function should be avoided if possible in favor of defaults in `values.yaml`. * It is usually best to not specify defaults for resources and to just provide sensible values that are commented out as a recommendation, especially when resources are rather high. This makes it easier to test charts on small clusters or Minikube. Setting resources should generally be a conscious choice of the user. +## Persistence + +* Persistence should be turned off by default +* PVCs should support specifying an existing claim +* Use multi-attach PVCs instead of several small pvcs + + +```yaml +volumeMounts: + - mountPath: /opt/sonarqube/conf + name: sonarqube + subPath: conf + - mountPath: /opt/sonarqube/data + name: sonarqube + subPath: data + - mountPath: /opt/sonarqube/extensions + name: sonarqube + subPath: extensions +``` + +* Storage class should not be overriden by default +* All options should be shown in README.md +* Example persistence section in values.yaml: + +```yaml +persistence: + enabled: false + ## If defined, storageClassName: + ## If set to "-", storageClassName: "", which disables dynamic provisioning + ## If undefined (the default) or set to null, no storageClassName spec is + ## set, choosing the default provisioner. (gp2 on AWS, standard on + ## GKE, AWS & OpenStack) + ## + # storageClass: "-" + # accessMode: ReadWriteOnce + # size: 10Gi + # existingClaim: "" +``` + ## Documentation `README.md` and `NOTES.txt` are mandatory. `README.md` should contain a table listing all configuration options. `NOTES.txt` should provide accurate and useful information how the chart can be used/accessed. @@ -87,16 +126,16 @@ We officially support compatibility with the current and the previous minor vers ## Kubernetes Native Workloads. While reviewing Charts that contain workloads such as [Deployments](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [StatefulSets](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [DaemonSets](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) the below points should be considered. These are to be seen as best practices rather than strict enforcement. - + 1. Any workload that are stateless and long running (servers) in nature are to be created as Deployments. Deployments in turn create ReplicaSets. -2. Unless there is a compelling reason, ReplicaSets or ReplicationControllers should be avoided as workload types. +2. Unless there is a compelling reason, ReplicaSets or ReplicationControllers should be avoided as workload types. 3. Workloads that are stateful in nature such as databases, key-value stores, message queues, in-memory caches are to be created as StatefulSets 4. It is recommended that Deployments and StatefulSets configure their workloads with a [Pod Disruption Budget](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/) for high availability. 5. For workloads such as databases, KV stores, etc., that replicate data, it is recommended to configure interpod anti-affinity. 6. It is recommended to have a default workload update strategy configured that is suitable for this chart. -7. Batch workloads are to be created using Jobs. -8. It is best to always create workloads with the latest supported [api version](https://v1-8.docs.kubernetes.io/docs/api-reference/v1.8/) as older version are either deprecated or soon to be deprecated. -9. It is generally not advisable to provide hard [resource limits](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container) to workloads and leave it configurable unless the workload requires such quantity bare minimum to function. -10. As much as possible complex pre-app setups are configured using [init contianers](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). +7. Batch workloads are to be created using Jobs. +8. It is best to always create workloads with the latest supported [api version](https://v1-8.docs.kubernetes.io/docs/api-reference/v1.8/) as older version are either deprecated or soon to be deprecated. +9. It is generally not advisable to provide hard [resource limits](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container) to workloads and leave it configurable unless the workload requires such quantity bare minimum to function. +10. As much as possible complex pre-app setups are configured using [init contianers](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). More [configuration](https://kubernetes.io/docs/concepts/configuration/overview/) best practices. From eed10d719535de773c311dee7bff84bc34c001a4 Mon Sep 17 00:00:00 2001 From: Rob Kernick Date: Mon, 19 Mar 2018 16:11:10 -0400 Subject: [PATCH 2/3] Updates to pvc guidelines --- REVIEW_GUIDELINES.md | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/REVIEW_GUIDELINES.md b/REVIEW_GUIDELINES.md index 581a6cab3f89..9dd541c8b983 100644 --- a/REVIEW_GUIDELINES.md +++ b/REVIEW_GUIDELINES.md @@ -80,23 +80,7 @@ image: * Persistence should be turned off by default * PVCs should support specifying an existing claim -* Use multi-attach PVCs instead of several small pvcs - - -```yaml -volumeMounts: - - mountPath: /opt/sonarqube/conf - name: sonarqube - subPath: conf - - mountPath: /opt/sonarqube/data - name: sonarqube - subPath: data - - mountPath: /opt/sonarqube/extensions - name: sonarqube - subPath: extensions -``` - -* Storage class should not be overriden by default +* Storage class should be empty by default * All options should be shown in README.md * Example persistence section in values.yaml: @@ -109,9 +93,9 @@ persistence: ## set, choosing the default provisioner. (gp2 on AWS, standard on ## GKE, AWS & OpenStack) ## - # storageClass: "-" - # accessMode: ReadWriteOnce - # size: 10Gi + storageClass: "" + accessMode: ReadWriteOnce + size: 10Gi # existingClaim: "" ``` From eaa1304c1fb2e1486911c7023ef2d78c37f3cb3f Mon Sep 17 00:00:00 2001 From: Rob Kernick Date: Fri, 23 Mar 2018 08:25:52 -0400 Subject: [PATCH 3/3] Updates to match existing best practices and examples --- REVIEW_GUIDELINES.md | 48 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/REVIEW_GUIDELINES.md b/REVIEW_GUIDELINES.md index 9dd541c8b983..40660b8b0e91 100644 --- a/REVIEW_GUIDELINES.md +++ b/REVIEW_GUIDELINES.md @@ -78,15 +78,15 @@ image: ## Persistence -* Persistence should be turned off by default +* Persistence should be enabled by default * PVCs should support specifying an existing claim -* Storage class should be empty by default +* Storage class should be empty by default so that the default storage class is used * All options should be shown in README.md * Example persistence section in values.yaml: ```yaml persistence: - enabled: false + enabled: true ## If defined, storageClassName: ## If set to "-", storageClassName: "", which disables dynamic provisioning ## If undefined (the default) or set to null, no storageClassName spec is @@ -99,6 +99,48 @@ persistence: # existingClaim: "" ``` +* Example pod spec within a deployment: + +```yaml +volumes: + - name: data + {{- if .Values.persistence.enabled }} + persistentVolumeClaim: + claimName: {{ .Values.persistence.existingClaim | default (include "fullname" .) }} + {{- else }} + emptyDir: {} + {{- end -}} +``` + +* Example pvc.yaml + +```yaml +{{- if and .Values.persistence.enabled (not .Values.persistence.existingClaim) }} +kind: PersistentVolumeClaim +apiVersion: v1 +metadata: + name: {{ template "fullname" . }} + labels: + app: {{ template "name" . }} + chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" + release: "{{ .Release.Name }}" + heritage: "{{ .Release.Service }}" +spec: + accessModes: + - {{ .Values.persistence.accessMode | quote }} + resources: + requests: + storage: {{ .Values.persistence.size | quote }} +{{- if .Values.persistence.storageClass }} +{{- if (eq "-" .Values.persistence.storageClass) }} + storageClassName: "" +{{- else }} + storageClassName: "{{ .Values.persistence.storageClass }}" +{{- end }} +{{- end }} +{{- end }} +``` + ## Documentation `README.md` and `NOTES.txt` are mandatory. `README.md` should contain a table listing all configuration options. `NOTES.txt` should provide accurate and useful information how the chart can be used/accessed.