Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

update Review Guidelines for workloads #3334

Merged
merged 3 commits into from
Jan 25, 2018
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions REVIEW_GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,22 @@ image:
## Compatibility

We officially support compatibility with the current and the previous minor version of Kubernetes. Generated resources should use the latest possible API versions compatible with these versions. For extended backwards compatibility conditional logic based on capalilities may be used (see [built-in objects](https://github.com/kubernetes/helm/blob/master/docs/chart_template_guide/builtin_objects.md)).

## Kubernetes Native Workloads.

While reviewing Charts that contain 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/) 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 creates replica sets.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workload should be plural as workloads

2. Unless there is a compelling reason replica sets or replication 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 a [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 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 workload with latest supported [api version](https://v1-8.docs.kubernetes.io/docs/api-reference/v1.8/) as older version are either depcicated or soon to be depreciated.
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 practises.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might I suggest we handling the example in 2 steps.

  1. Remove them from here, as we don't have a complete best practice set, so we can get this committed.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.