fiaas / fiaas-deploy-daemon

fiaas-deploy-daemon is the core component of the FIAAS platform
https://fiaas.github.io/
Apache License 2.0
55 stars 31 forks source link

Support integration with datadog agent daemonset #25

Open birgirst opened 5 years ago

birgirst commented 5 years ago

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 commented 5 years ago

I guess there are two things here:

oyvindio commented 5 years ago

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 commented 5 years ago

Whoops, I accidentally hit the close button.

srvaroa commented 5 years ago

+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 commented 4 years ago

Hi,

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

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 : https://github.com/cdemonchy/fiaas-deploy-daemon/commit/db94164001db353a63e9c5d75ca1b742c0b56488

adriengentil commented 4 years ago

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.