Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elasticsearch helm chart #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

renmak
Copy link

@renmak renmak commented Apr 20, 2017

@wilkers-steve @v1k0d3n
Elasticsearch helm chart.

Status: Ready for review.

Copy link
Contributor

@wilkers-steve wilkers-steve left a comment

Choose a reason for hiding this comment

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

This is a solid start @renmak. I've left a few comments inline

@@ -0,0 +1,4 @@
apiVersion: v1
description: A Helm chart for Kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want the description to read "A helm chart for elasticsearch", or some variant

annotations:
pod.beta.kubernetes.io/init-containers: '[
{
"name": "sysctl",
Copy link
Contributor

Choose a reason for hiding this comment

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

This init container isn't necessary. Elasticsearch has two methods for changing heap size: the first is via the ES_HEAP_SIZE environment variable, and the second is with JVM flags. The example used here (ES_JAVA_OPTS) sets these for the container already. We should look to see what the contents of the elasticsearch configuration file include to provide a method to override the default configuration values through a templated config file in a confmap, or we should make sure the environment variables used in the current approach can be customized through values.yaml

role: {{ .Values.labels.es_data.role }}
annotations:
pod.beta.kubernetes.io/init-containers: '[
{
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment regarding heap size wrt this init container

- name: "CLUSTER_NAME"
value: "myesdb"
- name: "NUMBER_OF_MASTERS"
value: "2"
Copy link
Contributor

Choose a reason for hiding this comment

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

These environment variables should be configurable where it makes sense. For example here, we'd want to make sure NUMBER_OF_MASTERS matches the actual replicas of es-master pods.

@renmak renmak changed the title Elasticsearch helm chart (WIP) WIP - Elasticsearch helm chart May 1, 2017
@@ -0,0 +1,16 @@
{{/* vim: set filetype=mustache: */}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see these two partial templates used anywhere. If they're unnecessary, they should be removed

Copy link
Author

Choose a reason for hiding this comment

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

Yes will delete this file if no usage found. Keeping it right now just in case if needed.

# limitations under the License.

apiVersion: apps/v1beta1
kind: StatefulSet
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this to be a StatefulSet. Each of the components should be the same kind (statefulset vs deployment)

volumeMounts:
- name: storage
mountPath: /data
volumeClaimTemplates:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any manifests for the volume claims should be separate templates

securityContext:
privileged: true
capabilities:
add:
Copy link
Contributor

Choose a reason for hiding this comment

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

The SYS_RESOURCE capability will need to be added to lock memory appropriately when elasticsearch is set up

Copy link
Author

Choose a reason for hiding this comment

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

done

- IPC_LOCK
- SYS_RESOURCE
image: {{ .Values.images.elasticsearch }}
imagePullPolicy: {{ .Values.images.pullPolicy }}
Copy link
Contributor

Choose a reason for hiding this comment

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

The ability to enable resources and template the values for limits and requests should be included, as in the other charts

Copy link
Author

Choose a reason for hiding this comment

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

done

cluster_name: myesdb

images:
elasticsearch: quay.io/pires/docker-elasticsearch-kubernetes:5.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

While this image works currently, we need to determine our reference image. Kolla has been the reference for our OpenStack images, but I don't know how well maintained Elasticsearch is on their end. This matters because Elasticsearch and Kibana's versions are tightly coupled, and we don't want either one to be out of sync with the other.

# limitations under the License.

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

StorageClass is v1 now

Copy link
Author

Choose a reason for hiding this comment

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

done

volumeMounts:
- name: storage
mountPath: /data
volumes:
Copy link
Contributor

Choose a reason for hiding this comment

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

To use an empty directory for storage here, it should be

  • name: storage
    emptyDir: {}

Copy link
Author

Choose a reason for hiding this comment

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

done

- name: storage
mountPath: /data
volumes:
- emptyDir:
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, this approach for using an emptyDir needs to change

Copy link
Author

Choose a reason for hiding this comment

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

done

- name: "CLUSTER_NAME"
value: {{ .Values.cluster_name }}
- name: "NUMBER_OF_MASTERS"
value: {{ .Values.replicas.es_master }} #Should we use "" string here or just as number?
Copy link
Contributor

Choose a reason for hiding this comment

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

This inline comment should be removed

@renmak renmak force-pushed the elasticsearch-helm-chart branch from 24a3d9c to d7bac99 Compare May 23, 2017 21:13
@renmak renmak changed the title WIP - Elasticsearch helm chart Elasticsearch helm chart May 23, 2017
@renmak
Copy link
Author

renmak commented May 23, 2017

To reviewer: As of right now we have working version of ES chart. We are using emptyDir for storage. We plan to make further changes in future.

@alanmeadows
Copy link

@renmak, Should this be closed as it would likely be destined for github.com/openstack/... at this point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants