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

Support integration with datadog agent daemonset #25

Open
birgirst opened this issue Sep 11, 2019 · 12 comments
Open

Support integration with datadog agent daemonset #25

birgirst opened this issue Sep 11, 2019 · 12 comments

Comments

@birgirst
Copy link
Contributor

birgirst commented Sep 11, 2019

The current datadog support in fiaas is based around running a dd agent sidecar in each application pod. According to the datadog documentation this may not be the optimal way (https://docs.datadoghq.com/agent/kubernetes/#installation)
"Note: Only one Datadog Agent shound run on each node; a sidecar per pod is not generally recommended and may not function as expected."

When a cluster operator has deployed datadog agent as a daemonset there is a dd agent pod running on each node. For enabling apm and tracing from application pods a couple of environment variables need to be set to enable reporting to the dd agent but currently they cannot be expressed/set via fiaas without making some changes.

Right now it is possible to work around this by manually patching the deployment object generated by fiaas.

spec:
  template:
    spec:
      containers:
      - env:
        - name: DD_AGENT_HOST
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: status.hostIP
        - name: DATADOG_TRACE_AGENT_HOSTNAME
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: status.hostIP
        image: myimage
        name: myapp

Would it make sense to extend the datadog integration to support the behavior described above where the datadog agent host could be exposed to the application and the sidecar avoided altogether?

@mortenlj
Copy link
Member

I guess there are two things here:

  • Add these environment variables when using a datadog integration (is there any reason to not set them when using the sidecar?)
  • Make it possible to configure the datadog integration to use a cluster-wide datadog instead of sidecars. I'm guessing that is implemented by having a service named datadog that points to the daemonset?

@oyvindio
Copy link
Member

If the sidecar-per-pod mode of using Datadog isn't recommended by Datadog, then we should probably look into changing FIAAS's support for Datadog to using it in a way that is recommended. Since the datadog sidecar container image is a instance level configuration flag, and using Datadog in a daemonset mode has some prerequisites that the cluster operator will have to handle, maybe it makes sense to add a separate mode for the datadog support that can be toggled at the FIAAS instance level.

@oyvindio
Copy link
Member

Whoops, I accidentally hit the close button.

@oyvindio oyvindio reopened this Sep 16, 2019
@srvaroa
Copy link
Contributor

srvaroa commented Sep 30, 2019

+1 on this issue, one of the historical problems we've had with the DD integration is precisely related to this topic. We're scaling up users relying on DD so I foresee this will gain priority for us. cc @xavileon

@cdemonchy
Copy link
Contributor

Hi,

I'm interested by this feature. I can provide a patch. I've different ideas for the implementation :

  • As suggested by @oyvindio, we can add a flag in FIAAS that enable the injection of datadog specific env variables

  • We can add another variable managed by FIAAS (like FIAAS_NAMESPACE) : FIAAS_HOST_IP and we can copy the value into the DD_AGENT_HOST variable using the global env configuration parameter. (See https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/)

  • We can add another configuration parameter global-expose (not sure about the name) to let each operator configure the mapping from data exposed by the downward api, for example :

global-expose:
  - DATADOG_TRACE_AGENT_HOSTNAME=status.hostIP
  - DD_AGENT_HOST=status.hostIP
  - MYIP=status.podIP

Do you've a preference or other idea ?

Regards

@gregjones
Copy link
Contributor

2 seems quite consistent with how things work now, and quite simple to implement (and to understand/document). But does it require some change in the datadog config part too, at least for setting the STATSD vars in the app container? Or would this (from the fiaas pov) be as if the app isn't using datadog, and those vars would be the responsibility of the app-owner or fiaas-admin?

@cdemonchy
Copy link
Contributor

With the solution 2, all pods will have an additional env var FIAAS_HOST_IP.

As in pod spec we can create an env variable referencing another env variable. FIAAS admin can add for datadconfig, something like that for example :

global-env:
  - DD_AGENT_HOST=$(FIAAS_HOST_IP)

Regards

@gregjones
Copy link
Contributor

But for the application to send the metrics to the statsd agent, it needs to be told the host/port - currently fiaas sets these env vars in the app's container. I can see how it would make sense for that to become the responsibility of either the app developer or the fiaas-admin instead, but it would be good to make sure that's clear.

@cdemonchy
Copy link
Contributor

I'm not sur to understant the question. As the goad is to use datadog as a daemonset, Datadog will be deployer by the admin. In my opinion the datadog variable and their content should be configurer by the admin too.

@adriengentil
Copy link
Contributor

Since the daemonset is configured by the admin, it makes sense to me to have this configuration at namespace level.

If we go for the option 2, the configuration to use the daemonset agent is set at namespace level, I wonder if it makes sense to have the option to create the sidecar at instance level (if metrics.datadog.enabled=true)?

We could have something where we specify at namespace level if we allow the use of the sidecar or the deamonset, and at instance level we inject the right configuration when metrics.datadog.enabled is set to true.
I think it would help to keep the interface consistent at instance level.

Thoughts?

@cdemonchy
Copy link
Contributor

In my idea, i don't want to introduce a new option to configure datadog. I think there is too many tools for monitoring / tracing / logging and it's will be difficult to support each.

I just had a new generic variable exposing the host ip in a generic variable, after each namespace admin can add a specific variable referencing the generic variable.

I've a poc uploader a poc here : cdemonchy@db94164

@adriengentil
Copy link
Contributor

Hi,

That's great, thanks! I think you can open a PR to get more feedback.
What I was trying to say, is that we may need a little bit more of "polish" to support datadog in daemonset.

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

No branches or pull requests

7 participants