-
Notifications
You must be signed in to change notification settings - Fork 16.7k
Harden elasticsearch chart for Kube 1.5 #1062
Conversation
Hi @icereval. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
stable/elasticsearch/values.yaml
Outdated
DataStorageClass: "anything" | ||
DataStorageClassVersion: "alpha" | ||
# DataStorageClass: "ssd" | ||
DataTerminationGracePeriodSeconds: 900 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not move the chart to stable
as long as config values start with an uppercase letter. In this case, I would also switch to nesting first. I think this make sense here, e. g.:
client:
replicas: 2
port: 2379
heapSize: "128m"
master:
replicas: 2
heapSize: "128m"
data:
replicas: 3
storage: "30Gi"
Generally, I think a move to stable should be a separate PR so it is explicit.
Regarding resources, it's becoming increasingly popular to specify resources like so (without specifying defaults): https://github.com/kubernetes/charts/blob/master/stable/nginx-ingress/templates/controller-deployment.yaml#L93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree and all are very good points. Would you mind informing the base PR #890, if we can get this merged, even if still in incubator status one of us can follow up with a new PR and the final changes needed to make this stable
.
fd939ad
to
918f888
Compare
918f888
to
38dc3f0
Compare
@simonswine @unguiculus any update on this review? Were the latest changes enough to merge? |
As suggested in #890, a move to stable should only happen after applying best practices, e. g. regarding values. See https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/values.md. |
* Add environment variable KUBERNETES_MASTER, resolves issue documented here: fabric8io/fabric8#6229 (comment) * Rename PetSet to StatefulSet, rename template file * Add initialDelay and increase timesouts to all liveness and readiness checks. This was the only way I could get it to deploy reliably in my environment. * Update to a newer image version
* Added configmap to explicitly provide cluster configurations and scripts * Replace depreciating `ES_HEAP_SIZE` with `ES_JAVA_OPTS` to position for ES v5 support * Removed alpha storage class operators * Removed catastrophic liveness probe checking entire clusters health * Readiness probe now inspects local node health * Added termination grace period (defaults to 15m) to allow pre-stop-script.sh time to gracefully migrate shards * Added init container to configure `vm.max_map_count` * Updated elasticsearch.yaml: * Added `PROCESSOR` configuration to prevent large cluster garbage collection issues leading to node eviction * Added configurable gateway defaults to help avoid a split brain, requiring two masters online and in consensus before recovery can continue * Updated pre-stop-script.sh: * Check `v1beta1` `statefulset` endpoint * Evalute `.spec.replicas` for statefulset desired size * Clear `_cluster/settings` ip exclusion prior to shutdown to avoid a possible (random) ip match scenario on expansion of the clsuter * Data nodes now use default storage class if once is not specified
38dc3f0
to
b62ccae
Compare
@unguiculus & @prydonius, rebase complete and a first pass at helm best practices applied to the incubator chart. |
c1e43a6
to
c425ab8
Compare
Excellent. This goes in the right direction. Now, please have a look at https://github.com/kubernetes/charts/blob/master/stable/nginx-ingress/templates/controller-deployment.yaml#L6 |
@icereval Are you working on an update? |
@unguiculus, yep, I'll have my updates ready soon |
c425ab8
to
e1ed701
Compare
@unguiculus, best practice changes pushed up |
Would you mind adding a NOTES.txt? Otherwise it looks nice but I have yet to review more thoroughly. I installed it on GKE and everything came up nicely. |
@k8s-bot ok to test |
Added NOTES.txt and |
ea779d1
to
5b6139f
Compare
* Update elasticsearch chart to work with Kube 1.5 * Add environment variable KUBERNETES_MASTER, resolves issue documented here: fabric8io/fabric8#6229 (comment) * Rename PetSet to StatefulSet, rename template file * Add initialDelay and increase timesouts to all liveness and readiness checks. This was the only way I could get it to deploy reliably in my environment. * Update to a newer image version * Harden aspects of the elasticsearch chart * Added configmap to explicitly provide cluster configurations and scripts * Replace depreciating `ES_HEAP_SIZE` with `ES_JAVA_OPTS` to position for ES v5 support * Removed alpha storage class operators * Removed catastrophic liveness probe checking entire clusters health * Readiness probe now inspects local node health * Added termination grace period (defaults to 15m) to allow pre-stop-script.sh time to gracefully migrate shards * Added init container to configure `vm.max_map_count` * Updated elasticsearch.yaml: * Added `PROCESSOR` configuration to prevent large cluster garbage collection issues leading to node eviction * Added configurable gateway defaults to help avoid a split brain, requiring two masters online and in consensus before recovery can continue * Updated pre-stop-script.sh: * Check `v1beta1` `statefulset` endpoint * Evalute `.spec.replicas` for statefulset desired size * Clear `_cluster/settings` ip exclusion prior to shutdown to avoid a possible (random) ip match scenario on expansion of the clsuter * Data nodes now use default storage class if once is not specified * Apply best practices * Add Notes for client service types, and warnings
* Update elasticsearch chart to work with Kube 1.5 * Add environment variable KUBERNETES_MASTER, resolves issue documented here: fabric8io/fabric8#6229 (comment) * Rename PetSet to StatefulSet, rename template file * Add initialDelay and increase timesouts to all liveness and readiness checks. This was the only way I could get it to deploy reliably in my environment. * Update to a newer image version * Harden aspects of the elasticsearch chart * Added configmap to explicitly provide cluster configurations and scripts * Replace depreciating `ES_HEAP_SIZE` with `ES_JAVA_OPTS` to position for ES v5 support * Removed alpha storage class operators * Removed catastrophic liveness probe checking entire clusters health * Readiness probe now inspects local node health * Added termination grace period (defaults to 15m) to allow pre-stop-script.sh time to gracefully migrate shards * Added init container to configure `vm.max_map_count` * Updated elasticsearch.yaml: * Added `PROCESSOR` configuration to prevent large cluster garbage collection issues leading to node eviction * Added configurable gateway defaults to help avoid a split brain, requiring two masters online and in consensus before recovery can continue * Updated pre-stop-script.sh: * Check `v1beta1` `statefulset` endpoint * Evalute `.spec.replicas` for statefulset desired size * Clear `_cluster/settings` ip exclusion prior to shutdown to avoid a possible (random) ip match scenario on expansion of the clsuter * Data nodes now use default storage class if once is not specified * Apply best practices * Add Notes for client service types, and warnings
Further extends upon PR #890
Added configmap to explicitly provide cluster configurations and scripts
Replace depreciating
ES_HEAP_SIZE
withES_JAVA_OPTS
to position for ES v5 supportRemoved alpha storage class operators
Removed catastrophic liveness probe checking entire clusters health
Readiness probe now inspects local node health
Added termination grace period (defaults to 60m) to allow pre-stop-script.sh time to gracefully migrate shards
Added init container to configure
vm.max_map_count
Updated elasticsearch.yaml:
PROCESSOR
configuration to prevent large cluster garbage collection issues leading to node evictionUpdated pre-stop-script.sh:
v1beta1
statefulset
endpoint.spec.replicas
for statefulset desired size_cluster/settings
ip exclusion prior to shutdown to avoid a possible (random) ip match scenario on expansion of the clsuterData nodes now use default storage class if one is not specified
Apply Helm best practices to prep for stable