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 NPM installation switch #784

Merged
2 commits merged into from
Mar 24, 2021
Merged

Support NPM installation switch #784

2 commits merged into from
Mar 24, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 20, 2021

Tested via local change:
default['datadog']['windows_agent_url'] = 'https://s3.amazonaws.com/dd-agent-mstesting/builds/tagged/'
default['datadog']['windows_npm_install'] = true
default['datadog']['system_probe']['enabled'] = true
dd_agent_installer = "datadog-agent-7.27.0-rc.4-1-x86_64.msi"

and running command:
kitchen converge dd-agent-windows-2012r2-13113

Observed the VM provisioned properly, NPM installed and "C:/ProgramData/Datadog/system-probe.yaml" is created.

@ghost ghost added the windows label Mar 20, 2021
@ghost ghost self-requested a review as a code owner March 20, 2021 02:01
@ghost ghost force-pushed the mike.zhu/npm2 branch from d596b41 to e02690b Compare March 20, 2021 02:08
Copy link
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we can improve the user experience:

NPM only runs if system-probe.yaml contains a section like this:

network_config:
    enabled: true

Although it's technically possible to use Windows NPM with the current PR, you need to do three things:

  • Setting windows_npm_install = true to install the driver
  • Setting system_probe.enabled = true to create the system-probe.yaml file
  • Setting system_probe.extra_config: {"network_config": { "enabled": true } } to write the above section in the file.

We can simplify this to a great extent with the addition of a new chef property:

default['datadog']['system_probe']['network_enabled'] = true

This property could have the three effects mentioned: installing the driver, enabling the system-probe.yaml creation and writing the above section to the file. We can maintain the existing properties (eg: windows_npm_install) for users who want full control, but for most cases being able to set a single property will simplify the user experience.

@ghost ghost force-pushed the mike.zhu/npm2 branch 2 times, most recently from a227af4 to 7fcd0c2 Compare March 23, 2021 20:56
@ghost ghost added the in progress label Mar 23, 2021
@ghost ghost force-pushed the mike.zhu/npm2 branch from 7fcd0c2 to fabaed3 Compare March 23, 2021 21:11
@ghost ghost requested a review from albertvaka March 23, 2021 21:29
@ghost
Copy link
Author

ghost commented Mar 23, 2021

Found and fixed some issues:

  1. Sysprobe recipe only worked on Linux with agent version > 6.11. This has been now fixed to allow it work on Windows with agent version > 7.26. In the first commit, system_probe.yaml appeared during test, but it was from the MSI.
  2. system_probe.yaml.erb loads values directly (except for extra config), so the code in system-probe.rb setting the config file never worked. This has been fixed to load values from ruby file.

Tested via local changes below:
default['datadog']['agent_version'] = '7.27.0'
default['datadog']['windows_agent_url'] = 'https://s3.amazonaws.com/dd-agent-mstesting/builds/tagged/'
default['datadog']['system_probe']['network_enabled'] = true
dd_agent_installer = "datadog-agent-7.27.0-rc.4-1-x86_64.msi"

@ghost ghost dismissed albertvaka’s stale review March 23, 2021 21:37

The comment has been addressed in the latest commit.

@ghost ghost force-pushed the mike.zhu/npm2 branch from fabaed3 to 03ce470 Compare March 24, 2021 04:57
@ghost ghost force-pushed the mike.zhu/npm2 branch from 03ce470 to c51c8a4 Compare March 24, 2021 05:07
@ghost ghost requested a review from albertvaka March 24, 2021 05:27
Copy link
Contributor

@albertvaka albertvaka left a comment

Choose a reason for hiding this comment

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

Left one comment but otherwise LGTM.

Co-authored-by: Albert Vaca Cintora <albert.vaca@datadoghq.com>
@ghost ghost merged commit 7a5d1ac into master Mar 24, 2021
@ghost ghost deleted the mike.zhu/npm2 branch March 24, 2021 15:11
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants