Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Setting up MetricBeat on Digital Ocean Kubernetes service. Even with … #315

Closed
wants to merge 4 commits into from

Conversation

pbecotte
Copy link
Contributor

@pbecotte pbecotte commented Oct 5, 2019

host: ${NODE} the metrics were coming through with the pod names. I wound up making a bunch of config changes to update things in accordance with the latest documentation. The final issue was no insecure kubelet port on Digital Ocean, so I added a bit of comments about that situation. Let me know if I misunderstood something here.

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jmlrt
Copy link
Member

jmlrt commented Oct 10, 2019

jenkins test this please

@pbecotte
Copy link
Contributor Author

Had to see the integration tests run to understand how they worked- should be working now.

@jmlrt
Copy link
Member

jmlrt commented Oct 14, 2019

jenkins test this please

replicas: 1

# Use host networking so that metricbeat can get hostname and talk to kubelet
hostNetworking: true
Copy link
Member

Choose a reason for hiding this comment

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

default value should be false (see #321 (comment))

@@ -7,19 +7,36 @@ metricbeatConfig:
hostfs: /hostfs
metricbeat.modules:
- module: kubernetes
labels.dedot: true
annotations.dedot: true
add_metadata: true
Copy link
Member

Choose a reason for hiding this comment

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

any specific reason to replace add_kubernetes_metadata processor by module kubernetes metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR come from following the given directions and not getting any metrics. In the process of trying to get it working, I noticed that the config in the helmchart didn't match https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-module-kubernetes.html and changing it was one of a number of things I did to get it working. I know that the dedot stuff was necessary. It is possible that I just misunderstood something though- I assumed the helm config simply hadn't kept up to date with the metricbeat settings... but if this isn't right, happy to change it back :)

@jmlrt jmlrt added the enhancement New feature or request label Dec 5, 2019
@mitchellmaler
Copy link

When we are deploying metricbeat we would only need the hostNetwork enabled on the daemonset and not the deployment since that is just pulling metrics from remote endpoints.
Could this configuration be split up so it doesn't set it to true for both?

@pbecotte
Copy link
Contributor Author

Good point- I'll get in there and update this a bit today.

@pbecotte
Copy link
Contributor Author

So, I switched the default to false and split the setting in two. Let me know if the actual default config should be changed back- I don't know well enough to have an opinion, just copy/paste from the docs :)

As a note- the latest version of attrs 19.2 has a conflict with the version of pytest here

@skotu-vida
Copy link

skotu-vida commented Dec 16, 2019

hey guys, just wondering when is this change going to get merged?

@jmlrt
Copy link
Member

jmlrt commented Dec 18, 2019

jenkins test this please

…`host: ${NODE}` the metrics were coming through incorrectly. I wound up collecting a bunch of config changes as I read the documentation more carefully. The manifest from the beat repo suggests Host Networking. DO requires the secured kubelet port with the hostname for certificate validation. The dedot processer is necessary if you have labels or annotations with dots in the name. Not being a metricbeat expert, let me know if something is incorrect.
@pbecotte
Copy link
Contributor Author

fixed the merge conflict

@jmlrt
Copy link
Member

jmlrt commented Dec 19, 2019

jenkins test this please

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

@pbecotte,
Many thanks for your PR. However, there is too much change in this PR.

I tried a few time to analyze the impacts of these changes for every current users to decide if we should include them and have good arguments if we shouldn't include them but it's beyond my Metricbeat knowledge.

I think this PR should include the changes related to hostNetworking as you already did in #321 (with hostNetwork default value set to false for daemonset and deployment off course).

As for all the other changes (disable add_kubernetes_metadata processor and manage add_metadata inside kubernetes module, add dedots option, ...), I don't think they will fit every use cases for current users.

As you're not sure about what is really needed in your use case, I would recommend you to try using default values with only hostNetworking enabled in a first time, then to apply the other changes (one by one and only if it resolve your issues) in your overrided value file.

If we have other feedbacks mentioning that these values are mandatory for metricbeat specifically on DOKS, we may provide a DOKS example config in the same way that we are providing docker-for-mac, kubernetes-kind, minikube or openshift examples for Elasticsearch chart

| --------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- |
| `metricbeatConfig` | Allows you to add any config files in `/usr/share/metricbeat` such as `metricbeat.yml`. See [values.yaml](./values.yaml) for an example of the formatting with the default configuration. | see [values.yaml](./values.yaml) |
| `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 | `[]` |
| `hostNetworking` | Use host networking for the deployment and daemonset to give better visibility into where the system is running.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this one could also be renamed to hostDaemonsetNetworking

@@ -1,2 +1,4 @@
pytest==4.1.0
PyYAML==4.2b4
# attrs 19.2 not compatable with pytest 4.1
attrs < 19.2
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't need that as pip automatically install a version compatible with pytest 4.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, except pytest 4.1 has its dependency at "attrs>=17.4.0", so it will happily pull in the latest version, which happened to introduce a breaking change.

@pbecotte
Copy link
Contributor Author

That is obviously up to you- I had the changes to get this working already, so I figured I'd open a PR. If you don't want to use them, it is completely possible that you know better than I do :)

However- I didn't just go in and randomly start changing stuff. When I brought up the elastic stack, I had no metrics of any sort related to my k8s cluster. It wasn't one thing wrong, there was a series.

  1. Metricbeat daemonset couldn't communicate with the kubelet. It tries to use $NODE_NAME:10255. This only works if the node name is DNS resolvable and reachable, and the kubelet can be controlled over the insecure port. This took a really long time to troubleshoot, so at least adding the comments (or something like them) seems like it would be useful. For the environment with the kubelet secured, I had to pull in the ssl certs, switch the target port, and make sure I was using NODE_NAME (since localhost doesn't work with the ssl cert)

  2. Once it did, the daemonset metrics started coming in- with the docker container hostname instead of the server host name. The variable HOST was correctly set, but so far as I could tell, metricbeat was ignoring it. Switching to host networking fixed that. Maybe that is a bug or there is another setting somewhere? But- as written, I did not get hostname in any metrics until I switched to host networking. This doesn't seem like a digital ocean specific thing. (You will notice there are ac

  3. That got node metrics in- but now I didn't have any container or k8s metrics. The main problem is the annotations getting broken by the dots in the names. While trying to figure that out, I just went and copied and pasted the recommended configuration from the "kubernetes section" of the metricbeat docs into the config. That made it work as expected. I understand if you don't want to make the change- but I would want to understand under what circumstances the existing settings actually work? In an empty cluster with nothing else deployed I had this issue.

So, that's a long way of saying- the two changes I made (pulling in the config from the metricbeat docs, enabling host networking) were both necessary.

(The third change was pulling the NODE_NAME stuff over to the kube-state-metrics deployment. Probably could be removed, it was kind of a hail mary on getting the daemonset metric hostnames to work correctly, but it also seemed like it should be there to match filebeat and the like).

Either way, thanks for your time- and even if it doesn't get merged, having this in here will probably help someone who hits similar issues in the future.

@jmlrt
Copy link
Member

jmlrt commented Dec 19, 2019

Despite a lot of use cases don't require / aren't compatible with hostNetwork, adding option to activate it for use case requiring it is mandatory as it's not something that can be overrided in custom values.

As for the other changes:

  • While having instructions to use metricbeat chart with hostnames not dns resolvable and kubelet reachable only via TLS, I would prefer this to be in a FAQ section of the README like we have in Elasticsearch README and/or in an example configuration as we have example configuration to use TLS with Elasticsearch for example. I don't feel like a comment in the default value file is the right place for that.

  • As for other parameter changes, labels.dedot and annotations.dedot (at least for annotations.dedot) aren't mandatory and shouldn't be part of the default config => the status for these parameters could be confirmed with Beats developers however it could be easier to validate that with them in a standalone PR instead of having these changes mixed with other.

  • The add_kubernetes_metadata processor is working well for standard use cases so you can remove it by using your custom values but there is no reason to remove it for everyone in default values.yaml.

The existing settings are working well with GKE which is the provider we are using for our automated tests as well as for using this Helm chart in production on 20-30 GKE clusters.

These default settings don't try to be compatible with every use cases and k8s providers, that's why Helm default values are overridable and the whole metricbeat config can be and should be fully customized to fit specific needs using metricbeatConfig value.

I stay with the recommendation of keeping only hostNetwork in this PR. A PR for documenting settings for TLS kubelet could be nice also as well as a PR to add an example of working config for DOKS. However the other changes shouldn't be applied in default values.

@pbecotte
Copy link
Contributor Author

Fair enough, I'm revisiting this today.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants