-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conversation
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.
LGTM. This is awesome work 🏅 🥇
I left some comments on some improvements/changes. However I'm approving and merging this in so we can continue to iterate on it.
@@ -65,11 +65,11 @@ helm install --name elasticsearch elastic/elasticsearch --version 7.0.0-alpha1 - | |||
| `minimumMasterNodes` | The value for [discovery.zen.minimum_master_nodes](https://www.elastic.co/guide/en/elasticsearch/reference/6.7/discovery-settings.html#minimum_master_nodes). Should be set to `(master_eligible_nodes / 2) + 1`. Ignored in Elasticsearch versions >= 7. | `2` | | |||
| `esMajorVersion` | Used to set major version specific configuration | `7` | | |||
| `esConfig` | Allows you to add any config files in `/usr/share/elasticsearch/config/` such as `elasticsearch.yml` and `log4j2.properties`. See [values.yaml](./values.yaml) for an example of the formatting. | `{}` | | |||
| `extraEnvs` | Extra [environment variables](https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/#using-environment-variables-inside-of-your-config) which will be appended to the `env:` definition for the container | `{}` | | |||
| `extraEnvs` | Extra [environment variables](https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/#using-environment-variables-inside-of-your-config) which will be appended to the `env:` definition for the container | `[]` | |
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.
Thanks for catching these Mr Eagle eyes.
| `secretMounts` | Allows you easily mount a secret as a file inside the `DaemonSet`. Useful for mounting certificates and other secrets. See [values.yaml](./values.yaml) for an example | `[]` | | ||
| `terminationGracePeriod` | Termination period (in seconds) to wait before killing Filebeat pod process on pod shutdown | `30` | | ||
| `tolerations` | Configurable [tolerations](https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/) | `[]` | | ||
| `updateStrategy` | The [updateStrategy](https://kubernetes.io/docs/tutorials/stateful-application/basic-stateful-set/#updating-statefulsets) for the `DaemonSet`. By default Kubernetes will kill and recreate pods on updates. Setting this to `OnDelete` will require that pods be deleted manually. | `RollingUpdate` | |
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.
The link here should be pointing to the daemonset docs instead of the statefulset. https://kubernetes.io/docs/tasks/manage-daemon/update-daemon-set/#daemonset-update-strategy
@@ -0,0 +1,17 @@ | |||
--- | |||
filebeatConfig: |
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.
As discussed we are going to make these values the defaults so that the default helm install filebeat
experience will already send logs to the default elasticsearch chart address.
@@ -0,0 +1,4 @@ | |||
1. Watch all cluster members come up. |
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.
Think this is a copy/paste thing but I'll update this to remove "cluster members".
{{- end }} | ||
- name: data | ||
hostPath: | ||
path: {{ .Values.hostPathRoot }}/{{ template "fullname" . }}-data |
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 think adding the namespace as part of the path would be a good idea too. Without it there will be collisions if there are multiple filebeat releases running in different namespaces but on the same physical host.
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.
Release names need to be unique in helm but it is possible to have multiple helm/tiller installations in a cluster, and with helm 3 tiller will be gone so I suspect that release names will be namespaced too.
port: monitor | ||
ports: | ||
- name: monitor | ||
containerPort: 5066 |
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.
Should probably make this configurable since it can be changed in filebeat. Not sure why but I'm sure someone has a valid usecase for wanting to customize it so may as well make it configurable.
This sounds good to me. I'm going to play with it a bit to see how it responds when the endpoint (such as elasticsearch) is down. It was only added in 6.4 but I don't see that being a problem as long as it is working in the latest 6.x and 7.x releases. |
${CHART}/tests/*.py
${CHART}/examples/*/test/goss.yaml
@Crazybus pushing what I have thus far. I think the actual body of the yaml templates, template tests, and docs are mostly complete, the remaining steps from my perspective are:
I've pushed this to a branch on this repo instead of my fork so feel free to tinker with the branch if you want, I'm fine with either of us writing those tests and CI jobs, whoever gets to it first.
Implementation notes:
emptyDir
, while the newer reference that the beats team maintains prefershostPath
, which I've reflected here. See the associated commit that made this change for additional information.-c
flag, I leaned this way because it's a more definitive check in case the go process became unresponsive for whatever reason. The downside is that the docs mark this feature as experimental.value
.