update Review Guidelines for workloads #3334

dhilipkumars · 2018-01-15T16:07:27Z

Initial update with Best Practises. PTAL

Ref: #3011

Thank you for contributing to kubernetes/charts. Before you submit this PR we'd like to
make sure you are aware of our technical requirements and best practices:

For a quick overview across what we will look at reviewing your PR, please read
our review guidelines:

https://github.com/kubernetes/charts/blob/master/REVIEW_GUIDELINES.md

Following our best practices right from the start will accelerate the review process and
help get your PR merged quicker.

When updates to your PR are requested, please add new commits and do not squash the
history. This will make it easier to identify new changes. The PR will be squashed
anyways when it is merged. Thanks.

Initial update with Best Practises.

dhilipkumars · 2018-01-18T08:02:26Z

cc : @kubernetes/charts-maintainers

mattfarina

Thanks for putting this together!

A little feedback (mostly nits)...

mattfarina · 2018-01-18T16:50:26Z

REVIEW_GUIDELINES.md

+
+## Kubernetes Native Workloads.
+
+While reviewing Charts that contains workloads such as [Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [Statefulset](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [Daemonset](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) below points should be considered.  These are to seen as best practises rather than strict enforcement.


Some nits...

While reviewing Charts that contains workloads

should be contain

and...

below points should be considered.

should be:

the below points should be considered.

I also noticed practises is misspelled. It should be practices.

mattfarina · 2018-01-18T16:52:38Z

REVIEW_GUIDELINES.md

+
+While reviewing Charts that contains workloads such as [Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [Statefulset](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [Daemonset](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) below points should be considered.  These are to seen as best practises 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 creates replica sets.


workload should be plural as workloads

mattfarina · 2018-01-18T16:53:04Z

REVIEW_GUIDELINES.md

+While reviewing Charts that contains workloads such as [Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [Statefulset](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [Daemonset](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) below points should be considered.  These are to seen as best practises 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 creates replica sets.
+2. Unless there is a compelling reason replica sets or replica controllers are are to be avoided as workload types. 


replica controllers should be replication controllers

mattfarina · 2018-01-18T17:28:07Z

REVIEW_GUIDELINES.md

+
+1. Any workload that are stateless and long running (servers) in nature are to be created as deployments.  Deployments in turn creates replica sets.
+2. Unless there is a compelling reason replica sets or replica controllers are are to be avoided as workload types. 
+3. Workloads that are stateful in nature such as Databases, Key Value Store, Messages Queues , In-memory caches are to be created as statefulsets


There's an extra space between Messages Queues and the comma after it.

Might also be useful to call it StatefulSet everywhere to keep the case consistent in the doc.

mattfarina · 2018-01-18T17:29:11Z

REVIEW_GUIDELINES.md

+1. Any workload that are stateless and long running (servers) in nature are to be created as deployments.  Deployments in turn creates replica sets.
+2. Unless there is a compelling reason replica sets or replica controllers are are to be avoided as workload types. 
+3. Workloads that are stateful in nature such as Databases, Key Value Store, Messages Queues , In-memory caches are to be created as statefulsets
+4. It is recommended that Deployments and Statefulsets configure their workloads with [Pod Disruption Budget](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/) for high availability.


It might read better if after "their workloads with" there was an a.

mattfarina · 2018-01-18T17:30:34Z

REVIEW_GUIDELINES.md

+3. Workloads that are stateful in nature such as Databases, Key Value Store, Messages Queues , In-memory caches are to be created as statefulsets
+4. It is recommended that Deployments and Statefulsets configure their workloads with [Pod Disruption Budget](https://kubernetes.io/docs/concepts/workloads/pods/disruptions/) for high availability.
+5. For workloads such as databases, KV store, etc., that replicates data it is recommended to configure interpod anti-affinity.
+6. It is recommended to have default workload update strategy configured that is suitable for this chart


Might read better to update

to have default workload update

to

to have a default workload update

mattfarina · 2018-01-18T17:40:43Z

REVIEW_GUIDELINES.md

+5. For workloads such as databases, KV store, etc., that replicates data it is recommended to configure interpod anti-affinity.
+6. It is recommended to have 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 workload with latest supported [api version](    https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.9/#statefulset-v1-apps) as older version are either depcicated or soon to be depreciated. 


The latest is only supported in 1.9. apps/v1 isn't supporting in 1.8. Because of these we should use the last api version. Many providers don't even provide 1.9 yet (like GKE).

unguiculus · 2018-01-18T18:43:40Z

Thanks for putting this together. Do we really need it this explicit? For my taste it is a little bit too much.

Apply review comments

dhilipkumars · 2018-01-22T14:16:05Z

@mattfarina Thanks for the review PTAL

@unguiculus Sure, its targetted for new reviewers to Chart community who may not be expert with workload apis in K8s, Also do you have any good examples for 'deployment', 'job' and 'DaemonSet' that we can be a good starting point for new contributors/reviewers?

unguiculus · 2018-01-23T18:11:03Z

REVIEW_GUIDELINES.md

+
+More [configuration](https://kubernetes.io/docs/concepts/configuration/overview/) best practises.  
+
+Example Charts that follow above best practices: [Statefulset](https://github.com/kubernetes/charts/tree/master/incubator/zookeeper), [Deployment](), [DaemonSet]() and [Jobs]().


Zookeeper is not the best example. It doesn't fully follow best practices and is in incubator.

Might I suggest we handling the example in 2 steps.

Remove them from here, as we don't have a complete best practice set, so we can get this committed.

Add discussing the examples to the next charts chart meeting so we can figure out which are good to include and then handle that in a second PR.

@dhilipkumars if you remove that line it LGTM. Don't see a reason to not merge it then. Thanks for putting this together.

@mattfarina Done

examples to be handled separately.

mattfarina · 2018-01-25T15:11:26Z

/lgtm

We can iterate on it from here if we like, I think.

k8s-ci-robot · 2018-01-25T15:11:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhilipkumars, mattfarina

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [mattfarina]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

update Review Guidelilnes for workloads

1081057

Initial update with Best Practises.

dhilipkumars requested review from scottrigby and unguiculus January 15, 2018 16:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 15, 2018

mattfarina reviewed Jan 18, 2018

View reviewed changes

Fix review comments

a7b2ac5

Apply review comments

unguiculus reviewed Jan 23, 2018

View reviewed changes

Remove examples

26e7514

examples to be handled separately.

k8s-ci-robot assigned mattfarina Jan 25, 2018

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2018

k8s-ci-robot merged commit 964cdd5 into helm:master Jan 25, 2018

scottrigby mentioned this pull request Mar 25, 2019

Clarify when to use StatefulSet instead of Deployment for Charts with PVC #1863

Closed

scottrigby mentioned this pull request Jan 20, 2022

Document charts best practices (from initiative in helm/charts) helm/helm-www#1258

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update Review Guidelines for workloads #3334

update Review Guidelines for workloads #3334

dhilipkumars commented Jan 15, 2018

dhilipkumars commented Jan 18, 2018

mattfarina left a comment

mattfarina Jan 18, 2018

mattfarina Jan 18, 2018

mattfarina Jan 18, 2018

mattfarina Jan 18, 2018

mattfarina Jan 18, 2018

mattfarina Jan 18, 2018

mattfarina Jan 18, 2018

unguiculus commented Jan 18, 2018

dhilipkumars commented Jan 22, 2018

unguiculus Jan 23, 2018

mattfarina Jan 23, 2018

dhilipkumars Jan 24, 2018

mattfarina Jan 24, 2018

dhilipkumars Jan 25, 2018

mattfarina commented Jan 25, 2018

k8s-ci-robot commented Jan 25, 2018


		## Kubernetes Native Workloads.

		While reviewing Charts that contains workloads such as [Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [Statefulset](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [Daemonset](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) below points should be considered. These are to seen as best practises rather than strict enforcement.


		While reviewing Charts that contains workloads such as [Deployment](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [Statefulset](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [Daemonset](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) below points should be considered. These are to seen as best practises 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 creates replica sets.


		More [configuration](https://kubernetes.io/docs/concepts/configuration/overview/) best practises.

		Example Charts that follow above best practices: [Statefulset](https://github.com/kubernetes/charts/tree/master/incubator/zookeeper), [Deployment](), [DaemonSet]() and [Jobs]().

update Review Guidelines for workloads #3334

update Review Guidelines for workloads #3334

Conversation

dhilipkumars commented Jan 15, 2018

dhilipkumars commented Jan 18, 2018

mattfarina left a 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

Choose a reason for hiding this comment

unguiculus commented Jan 18, 2018

dhilipkumars commented Jan 22, 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

mattfarina commented Jan 25, 2018

k8s-ci-robot commented Jan 25, 2018