Review the generation of the datadog.conf to make it more flexible and maintainable #222

aerostitch · 2017-08-01T01:03:43Z

What does this PR do?

Provides the ability to add config parameters that are not controlled by environment variables yet

Motivation

I wanted a way to add those without overwriting the entire datadog.conf file.

Additional Notes

The python file has been added to the alpine folder to make it available for both the alpine distro and the classic one.

Edit

The initial content of this PR has evolved. Please read the conversation for more details.

Closes: #213

xvello · 2017-08-01T09:40:12Z

Hi @aerostitch,

Although I'm rooting for generic solutions, we decided to keep the entrypoint as "dumb" as possible to avoid tricky side-effects. But I agree it leads to restrictions on what it can do (for example we refuse mapping list/dict types to avoid parsing errors).

For more advanced usages, our recommendation for kubernetes is storing datadog.yaml in a configmap, could that work for you?

aerostitch · 2017-08-01T15:39:48Z

Hey @xvello , Short answer: no Problems it would generate: This solution would work as a **temporary** solution though I'd rather have a builtin solution because I would need to track down every changes made in the default config file for each upgrade. I don't want to loose a day on an upgrade just because one default parameter has been added to the default config file that I overwrite. Proposed alternatives: - If you want I can make it raise exceptions on bad json format if you're afraid of having it too smart! XD - Worse case I can add an environment variable juste for the percentiles config. But that really looks like overcomplicating the maintenance. (I'm already not sure why you went through the pain of having 2 shell & bash scripts that end up doing the same stuff when you distribute a python environment. - yes I know alpine doesn't have bash built in but if you want to evolve supporting more distros it would seem more logical to have something that works for all like a python script with symlinks in each folder specific for a repo for example) Side note: There's already a ton of logic in the shell scripts. I don't think you can really still qualify it as "dumb" 😉 Other side note: I'd be totally be down to put the full logic of the environment variables management into a python script (that could even use configParser so that we avoid any problems) to lower the amount of shell that need to be manage here. Just tell me if you're interested. Joseph

xvello · 2017-08-02T11:42:32Z

Hi Joseph,

I agree with you, we do need to work on something more maintainable than 5 different entrypoints that get out of sync on a regular basis. I actually created a backlog card for that yesterday between merging #220 & #223 and reading your PR ;)

Indeed, I do think it would be great to use ConfigParser to load the default config, add envvar bindings (including a generic json one) and write the updated configuration. We would lose comments, but I think that's an acceptable tradeoff.

That would also solve our current limitation of not being able to map array/mapping types in the yaml files. A generic json payload that would be parsed and injected in the configuration would indeed solve that.

I'd like to get @hkaj's input on that before going forward though, he'll be back in the office on Tuesday.

In the meantime, do you want me to add that binding, old-style?

aerostitch · 2017-08-02T17:40:46Z

Hey @xvello !

I got the temporary solution in place for now, so I prefer to wait until we get the clean solution directly! ;)
While waiting for @hkaj to be back I can prepare a prototype if you want.

xvello · 2017-08-03T08:53:27Z

Please go forward if you have time.

My main concern is how this would transfer to agent6 later. We designed agent6 with envvar binding out of the box for the main configuration, but that does not cover the integrations' configuration. As we'll continue shipping python 2.7 in agent6, I guess we would still use that script for them.

aerostitch · 2017-08-07T06:16:26Z

Hey @xvello ,

Sorry for the late answer, last days have been very busy.
I've started the script. Should I overwrite this PR with it or should I create a new PR to start fresh?

Thanks
Joseph

xvello · 2017-08-07T09:40:39Z

Hi @aerostitch,

Please go forward on this PR, to keep the full context.

aerostitch · 2017-08-07T19:38:15Z

So I have a slightly different proposition than passing the json around:

We 1st set some default properties we want in the config
Then we look for the variable that have some logic around it like the API key for example
finally we parse the environment looking for variables starting by DD_ (minus an exclusion list) and we consider that their name is DD_<PROPERTY_NAME>. and we set them in the config file (see set_generics in the current commit).
We could have a more distinctive prefix like DD_CONF_<PROPERTY_NAME> for the core agent related ones, then DD_<integration_name>_<property_name> for the integrations
What do you think?

Note: using the ConfigParser makes you loose the comments and also changes the separator from : to = which should not be a big deal as it's read after by ConfigParser but just FYI.

Other node: for now I've only done the datadog.conf management as an example. Not sure what's the status of agent6 and if we could integrate this idea of generic DD_CONF_<property_name> generic variables to it. Creating the other files should be pretty straightforward, but I just wanted to validate the idea 1st.

aerostitch · 2017-08-08T18:41:13Z

Note: the symlink doesn't work but we can use the same idea of calling ADD https://raw.githubusercontent.com/DataDog/dd-agent/master/packaging/datadog-agent/source/config_builder.py /config_builder.py as it is done in other places.

aerostitch · 2017-08-08T23:59:47Z

@hkaj , @xvello I think I finished the refactoring. Let me know if that looks good to you.
The DD_CONF_<property_name> seems only relevant for the datadog.conf but could provide a nice flexible way to serve that purpose.
Let me know what you think.

Adding the capability to use DD_<integration_name>_<property_name> could fit the need of elements requests #115 more safely but I'm afraid of integration name collisions like kubernetes and kubernetes_state for example maybe worth digging.

Thanks

hkaj

Hi @aerostitch

Thanks for the PR! I had a look at it and left a few comments.
Binding settings to env var is a good idea, but we need to be careful about what we bind and what takes precedence between defaults, env vars and files.
For datadog.conf it's okay because most settings have sane defaults, and the scope is small. But for integrations I don't think we should allow individual overrides as that can (and will) get hairy.

I suggest using your approach for datadog.conf, and keeping the old entrypoint approach for integrations and supervisor (we don't do much with supervisor, I don't think it's worth changing it).

The other interesting point you raised is the duplication of the entrypoints. As @xvello discovered earlier, docker hub now builds on docker cloud, so we can specify the dockerfile name for builds. Allowing us to move all entrypoints at the root of docker-dd-agent and thus sharing a single entrypoint between them. That would solve this issue, and also make these good old seds way simpler to read than a Python script that does the same thing (for integrations and supervisor. Again I think the Python script is necessary for datadog.conf).

What do you think?

hkaj · 2017-08-09T17:16:52Z

config_builder.py

+        # This will store the config parser object that is used in the different functions
+        self.config = None
+
+    def set_instance_property(self, integration, property_key, property_value):


This is a pretty hacky way to configure checks, I don't think we should support this interface. There is a plan to allow passing a check config file as json through an environment, but it will be for the whole config file, we don't plan on supporting individual setting overrides.

We could add this check config through env var thing now if that's a blocker for you but mounting them in /conf.d should do the trick in most cases.

Not sure what's hacky in that. It just opens the integration config file (from the example or from the existing one if any) and updates the property for the instance (or the several instances if several ones are already defined).
It looks more complex but in the long term it would be more maintainable than having to change 6 sed in 6 entrypoint.sh no?
That function is generic and is only used in 'build_integration_conf` which is just a rewrite in python of their shell counterpart present in several of the entrypoints. Looks more maintainable on the long term no?

And when we switch to allow environment variables (which we do for some in build_integration_conf) we wouldn't really have to modify it.

Note: the generic DD_<integration_name>_<property_name> idea have not been implemented this PR.

Not sure what's hacky in that

Fair enough, I wasn't clear here. It certainly is more flexible, but this is bound to fail in unexpected ways as config files change, and we observed that this kind of config-level failures tend to confuse users a lot, leading to frustration, and unnecessary support tickets.
It will also greatly limit our freedom in revamping config files in fear that someone is editing them with an ad-hoc system like this one.

It looks more complex but in the long term it would be more maintainable than having to change 6 sed in 6 entrypoint.sh

Again, we don't expect this to grow much (but rather to shrink with agent 6) and we'll reduce the number of entrypoint files. Integration config should be modified by passing whole files, not individual settings. Those we have in the entrypoint now are special cases that we want to get rid of progressively, not generalize.

hkaj · 2017-08-09T17:19:54Z

config_builder.py

+        with open(_target, 'w') as _stream:
+            yaml.dump(_data, _stream, default_flow_style=False)
+
+    def build_integration_conf(self):


This is a rewrite of the check settings we change in the entrypoint in Python. What I like about it is that it's all in one place, what I don't like is that it's more complex than the entrypoint seds.

What we could do to make the entrypoint situation better is to modify the structure of docker-dd-agent to share the entrypoint between all images. If we do that, does keeping the sed-ing in the entrypoint sound reasonable to you?

It looks more clear and flexible to me to do that. And I was thinking about adding the ability to have properties via a hash, not only 1 property at a time. Would that be clearer?

Keeping the sed in the entrypoints will depend on if we want to have a good maintainability or not. All entrypoints do different stuffs. Some start dogstatsd, some start supervisord, it means maintaining several entrypoints, so the less code is in the entrypoint scripts, the more maintainable it will be.

I was thinking about adding the ability to have properties via a hash, not only 1 property at a time

dict or individual, this is not an interface we want to support. Integration configs should be all or nothing. Those we touch in entrypoints are special cases that we'll work on getting rid of, not extending.

All entrypoints do different stuffs

That's true, we can't magically merge them together without adding any complexity at all. That's why I'm okay with the datadog.conf part.

hkaj · 2017-08-09T17:36:51Z

config_builder.py

+        with open(config_file, 'wb') as cfd:
+            self.config.write(cfd)
+
+    def build_supervisor_conf(self):


same here, this is less clear than the 3 seds and 1 echo we have now for supervisor config. I see the added complexity but not the added value.

3 sed in 6 dockerfile and 6 entrypoint.sh (for now)....
Why doing things 3 different ways? If we do a python script for maintaining part of the config, why not reuse the function for the rest?
Right now supervisor.conf has 3 parameters changed, meaning 3 sed.
How maintainable will it be when you want to change the 10 others?

We probably won't, and supervisor will most definitely go away with agent 6 anyway. That's also why adding complexity around it is a no go.

hkaj · 2017-08-09T18:03:23Z

config_builder.py

+
+        self.save_config(self.supervisor_conf_file)
+
+    def build_datadog_conf(self):


For datadog.conf I agree it's a good idea to allow individual overrides, and binding all settings to env var is a good way to do that (that's actually what agent 6 will do).

aerostitch · 2017-08-09T18:55:44Z

Thanks for the review @hkaj .

The different entrypoints start different things so having Dockerfile in just one location (which would be great) won't solve all the maintainability issues.
For the complexity thing I tried to build atomic functions to make it easy to read and reuse.
At first I wanted to reuse the config.y functions from the dd-agent but there's nothing to write in it.

I think keeping things in different shell scripts is what made things difficult to maintain in the 1st place. Not sure it's the best route to go.
If you look at the code of the entrypoints, there is even different ways to do sed to end up with the same result. That doesn't sound really easy to read.

Would better documentation or better comments help?
Do you just want to drop all the supervisor.conf and integration support?
Just configuring datadog.conf would be just useless with the arrival of agent6 from what I hear.

hkaj · 2017-08-10T12:52:15Z

thanks for the details @aerostitch
I agree with you that the current situation for entrypoints is far from ideal, but I think we can improve it by consolidating them.
Extending the config file edits we do here (and are not really proud of 😅 ) into a generic thing doesn't really fit with the direction agent 6 is taking except for datadog.conf, so I would like to avoid adding such a feature since we have no plan to maintain it in the mid-term.

aerostitch · 2017-08-10T15:19:02Z

Ok, so as a summary:

Move the dockerfiles to the root
only touch datadog.conf
refactor the entrypoints to merge some

I'll check that today.

hkaj · 2017-08-10T15:23:05Z

That's a great summary, we'll handle the dockerfile move as it requires changing docker hub settings. We can also take care of the entrypoints if you prefer focusing on datadog.conf.

Thanks again!

aerostitch · 2017-08-10T15:31:30Z

The datadog.conf is pretty much done.
Don't worry about the entrypoint, I'll take care of it. I can also move the Dockerfile and you change the config when merging the PR. It would allow me to test the different cases.
What do you think?

aerostitch · 2017-08-10T19:25:15Z

Note: I did remove the examples from the jmx/examples/ folder as they were identical in all points to the ones in examples/

aerostitch · 2017-08-10T20:08:05Z

Hey @hkaj,

I did the changes we discussed earlier.
Note that there are now 2 entrypoints only (that could probably be merged but I wanted to keep the warning as it was for now).
These entrypoint are using /bin/sh so I had to do some changes to make it accepted by both alpine and debian (conversion of the [[ ]] to [ ] for example).

I removed the examples from the jmx folder as they were exactly matching the ones in the root folder.

Also there's dogstatsd/README.md that I'm not sure if still needed.

Let me know what you think of this version.

Thanks
Joseph

hkaj

Nice, I left a few comments and we'll need to test it carefully for regressions but it looks really good!

hkaj · 2017-08-11T11:19:17Z

Dockerfile-dogstatsd

- && sed -i -e"s/^.*non_local_traffic:.*$/non_local_traffic: yes/" /etc/dd-agent/datadog.conf \
- && sed -i -e"s/^.*log_to_syslog:.*$/log_to_syslog: no/" /etc/dd-agent/datadog.conf \
- && mv /supervisor.conf /etc/dd-agent/supervisor.conf
+ && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* \


\ is not needed

Good catch. fixed.

hkaj · 2017-08-11T11:22:26Z

Dockerfile-dogstatsd-alpine

+    AGENT_VERSION=5.16.0 \
+    DD_ETC_ROOT="/opt/datadog-agent/agent" \        
+    PATH="/opt/datadog-agent/venv/bin:/opt/datadog-agent/agent/bin:$PATH" \
+    PATH="${DD_HOME}/venv/bin:${DD_HOME}/bin:${PATH}" \


these 2 path lines can be simplified into PATH="${DD_HOME}/agent/bin:${DD_HOME}/venv/bin:${DD_HOME}/bin:$PATH"

Actually, as we set them in the same ENV command, I had issues where variables were not set in the described order ending up in PATH=/venv/bin:/bin:${PATH}" after evaluation so I've sticked to the full path and removed the other that I forgot to remove there.
Thx

hkaj · 2017-08-11T13:21:23Z

entrypoint-dogstatsd.sh

+else                                               
+  # disable the agent when the env var is absent  
+  export DD_APM_ENABLED=false                     
+fi


should this block be in config_builder?

I didn't find a clean way to set the environment variables from python to the outside shell.
But transformed it to be way simpler here: export DD_APM_ENABLED=${DD_APM_ENABLED:-false}

hkaj · 2017-08-11T13:21:33Z

entrypoint-dogstatsd.sh

+  # To do this, we execute some of dd-agent's python code and expose the hostname
+  # as an env var
+  export DD_HOSTNAME=`python -c "from utils.hostname import get_hostname; print get_hostname()"`
+fi


I didn't find a clean way to set the environment variables from python to the outside shell. Do you have one?

aerostitch · 2017-08-11T18:33:21Z

Rebased from master too.

aerostitch · 2017-08-16T18:30:47Z

Sorry @hkaj I just realized that I didn't say that I updated the PR with your feedback last week. 😶

hkaj · 2017-08-18T14:12:19Z

thanks @aerostitch
I had one more look and it looks great! I think it's ready to go. We'll probably cut a major release for it (there's also a few more merged PRs waiting on a release I think). @xvello is having one more look as well and if it's all good let's 🚢 it!

xvello · 2017-08-18T14:48:10Z

That's a great improvement over master, thanks!

There's a few more cleanup we could do (remove alpine/examples for example), but that can be addressed after merging that.

My only fear is about the jmx variant not building reliably on the docker hub. As images build in parallel, we can't be sure that latest will finish building before latest-jmx starts. That will create an image that is silently outdated. I'd rather avoid it.

While we investigate how to build chained images reliably, I'd go with reverting Dockerfile-jmx to a standalone Dockerfile from jessie. I would have loved to chain the builds, but I think it's too risky on the docker hub, unless I'm missing a feature to ensure that.

aerostitch · 2017-08-18T16:57:44Z

Thans for the review @hkaj and @xvello .
Makes sense, I dropped the commit that moved the jmx part to be based on the based image.
Do you want me to directly include other changes in it to avoid asking everyone to refactor their changes?
Like #228 (only needs to add DD_STATSD_TARGET now), #206 , #135 or #81...
Also do you need me to update the README with the new generic vars support?

Thx

hkaj · 2017-08-21T08:34:19Z

Thanks @aerostitch
The PR is big enough, let's merge it as is.
We'll take care of the documentation update.
Thanks again, one more great PR from you 💪

aerostitch · 2017-08-21T17:49:19Z

thx! :)

aerostitch changed the title ~~Add extra config capabilities via a map~~ Add extra config capabilities via a json string Aug 1, 2017

xvello self-assigned this Aug 1, 2017

xvello added community feature labels Aug 3, 2017

aerostitch force-pushed the add_map_extra_config branch from 3fcc1eb to 86e3fe0 Compare August 7, 2017 19:32

aerostitch force-pushed the add_map_extra_config branch 2 times, most recently from a44e297 to 8538d25 Compare August 8, 2017 00:50

aerostitch changed the title ~~Add extra config capabilities via a json string~~ Review the generation of the datadog.conf to make it more flexible and maintainable Aug 8, 2017

hkaj self-requested a review August 8, 2017 13:52

aerostitch force-pushed the add_map_extra_config branch 3 times, most recently from 4eea2ac to e37973b Compare August 8, 2017 23:57

aerostitch force-pushed the add_map_extra_config branch from e37973b to 28c9e87 Compare August 9, 2017 17:41

hkaj requested changes Aug 9, 2017

View reviewed changes

aerostitch force-pushed the add_map_extra_config branch from 28c9e87 to 1f1eb0c Compare August 9, 2017 18:58

aerostitch force-pushed the add_map_extra_config branch from 8dc4075 to 2c3a26c Compare August 10, 2017 18:53

aerostitch force-pushed the add_map_extra_config branch 2 times, most recently from b8c90df to 5513d29 Compare August 10, 2017 19:59

aerostitch mentioned this pull request Aug 10, 2017

Make sure checks.d directory is present #213

Closed

hkaj requested changes Aug 11, 2017

View reviewed changes

aerostitch added 6 commits August 11, 2017 11:30

Adding datadog.conf builder

10af94f

Refactor root base image to use config_builder.py

a63f056

Refactor alpine dockerfile and unify entrypoints

540621e

Move jmx Dockerfile to root and cleanup duplicated examples

4390599

Move dogstatsd Dockerfile to root and add dogstatsd-specific entrypoint

1362a12

Move alpine dogstatsd Dockerfile to root and supervisor.conf 1 level up

c4bc326

aerostitch force-pushed the add_map_extra_config branch from 497f953 to 201958c Compare August 11, 2017 18:32

hkaj approved these changes Aug 18, 2017

View reviewed changes

Fix based on PR feedback

6790da3

aerostitch force-pushed the add_map_extra_config branch from 201958c to 6790da3 Compare August 18, 2017 16:37

DataDog deleted a comment from aerostitch Aug 21, 2017

hkaj merged commit 77cf970 into DataDog:master Aug 21, 2017

aerostitch deleted the add_map_extra_config branch August 21, 2017 17:48

olivielpeau mentioned this pull request Aug 24, 2017

Fix sd_config_backend and sd_backend_host vars #232

Merged

xvello mentioned this pull request Sep 25, 2017

[dev] Request for major version changelog #240

Closed


		self.save_config(self.supervisor_conf_file)

		def build_datadog_conf(self):

Review the generation of the datadog.conf to make it more flexible and maintainable #222

Review the generation of the datadog.conf to make it more flexible and maintainable #222

Conversation

aerostitch commented Aug 1, 2017 • edited Loading

What does this PR do?

Motivation

Additional Notes

Edit

xvello commented Aug 1, 2017

aerostitch commented Aug 1, 2017 via email • edited Loading

xvello commented Aug 2, 2017

aerostitch commented Aug 2, 2017

xvello commented Aug 3, 2017

aerostitch commented Aug 7, 2017

xvello commented Aug 7, 2017

aerostitch commented Aug 7, 2017 • edited Loading

aerostitch commented Aug 8, 2017

aerostitch commented Aug 8, 2017 • edited Loading

hkaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaj Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaj Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerostitch commented Aug 9, 2017

hkaj commented Aug 10, 2017

aerostitch commented Aug 10, 2017

hkaj commented Aug 10, 2017

aerostitch commented Aug 10, 2017

aerostitch commented Aug 10, 2017

aerostitch commented Aug 10, 2017

hkaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerostitch Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

aerostitch commented Aug 11, 2017

aerostitch commented Aug 16, 2017

hkaj commented Aug 18, 2017

xvello commented Aug 18, 2017 • edited Loading

aerostitch commented Aug 18, 2017

hkaj commented Aug 21, 2017

aerostitch commented Aug 21, 2017

aerostitch commented Aug 1, 2017 •

edited

Loading

aerostitch commented Aug 1, 2017 via email •

edited

Loading

aerostitch commented Aug 7, 2017 •

edited

Loading

aerostitch commented Aug 8, 2017 •

edited

Loading

hkaj Aug 10, 2017 •

edited

Loading

hkaj Aug 10, 2017 •

edited

Loading

aerostitch Aug 11, 2017 •

edited

Loading

xvello commented Aug 18, 2017 •

edited

Loading