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

remove unnecessary restart using Exec #562

Merged

Conversation

jvanbrunschot
Copy link

@jvanbrunschot jvanbrunschot commented Sep 19, 2019

Currently the restart breaks whenever Datadog is not running yet.

Looking at the code, the restart seems to be unnecessary. No config changes are made between the installation of the package and the definition of the service.

If the Datadog service is not running (but installed) the exec will return a failure.

Notice: /Stage[main]/Datadog_agent::Windows::Agent6/Exec[DatadogRestart]/returns: could not send control=1: The service has not been started.

@albertvaka
Copy link
Contributor

Indeed, this can be removed. The agent is already restarted automatically when needed by notifying the Service.

Thanks for catching this, and thanks for testing on Windows :)

@albertvaka albertvaka merged commit ab4cbce into DataDog:master Sep 19, 2019
@jvanbrunschot
Copy link
Author

@albertvaka

We were actually waiting for Windows support when we saw the "add windows support" PR pending. :)

I wish we could disable the installation part in Windows:

We create a pre-build AMI with all software installed and do the configuration part (like adding the DD api key and tags) whenever we AutoScale. This keeps the configuration part on boot small. Currently the Windows support downloads the MSI which is not ideal for us.

But I/we appreciate the effort that's put into getting Windows support for the DD agent 👍

@albertvaka
Copy link
Contributor

Puppet is smart enough to not install the agent again if it's already installed, but it's true that we still always download the MSI anyway.

If we find a way to only download the MSI only if the agent needs to be installed, I think that would cover your use case.

@albertvaka
Copy link
Contributor

Something like this: #563

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

Successfully merging this pull request may close these issues.

None yet

2 participants