Better document cluster agent & Update chart #8786

CharlyF · 2018-10-26T11:55:47Z

What this PR does / why we need it:

Better document the Install for the Datadog Cluster Agent
Add a codeowner
Move default log level to info

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

hkaj · 2018-10-26T12:14:38Z

stable/datadog/values.yaml

@@ -3,7 +3,7 @@ image:
  # This chart is compatible with different images, please choose one
  repository: datadog/agent               # Agent6
  # repository: datadog/dogstatsd         # Standalone DogStatsD6
-  tag: 6.5.2  # Use 6.5.2-jmx to enable jmx fetch collection
+  tag: 6.6.0  # Use 6.5.2-jmx to enable jmx fetch collection


could you update the comment as well?

hkaj · 2018-10-26T12:18:19Z

stable/datadog/OWNERS

@@ -2,7 +2,9 @@ approvers:
 - hkaj
 - irabinovitch
 - xvello
+- charlyf


Yes please! You'll need to open an issue to become a trusted collaborator first though.

Signed-off-by: charlyF <charly@datadoghq.com>

hkaj · 2018-11-02T13:40:43Z

stable/datadog/README.md

+Read about the Datadog Cluster Agent in the [official documentation](https://docs.datadoghq.com/agent/kubernetes/cluster/).
+
+Run the following if you want to deploy the chart with the Datadog Cluster Agent.
+You can also specify `clusterAgent.metricsProvider.enabled=true` if you want to enable the External Metrics Server.


This is already set in the command below. Either remove this line or remove the option in the bash snippet.

I think we should keep both, just to stress that this is the specific option that enables the external metrics provider.

Fair enough, then we should change the wording.
Right now it reads as Run the following [...], you can also specify <option> if you want to enable [...] but the option is already in the snippet.

What about Note that specifying <option> will enable [...] instead?

hkaj · 2018-11-02T13:43:46Z

stable/datadog/README.md

+If you want to learn to use this feature, you can check out this [walkthrough](https://github.com/DataDog/datadog-agent/blob/master/docs/cluster-agent/CUSTOM_METRICS_SERVER.md).
+```bash
+helm install --name datadog-monitoring \
+    --set clusterAgent.token=YOUR-32-CHARACTERS-TOKEN \


actually i think we should just get rid of this setting and generate a random token ourselves with {{ randAlphaNum 32 | quote }}. WDYT?

hkaj · 2018-11-02T13:44:24Z

stable/datadog/README.md

+    --set clusterAgent.token=YOUR-32-CHARACTERS-TOKEN \
+    --set datadog.apiKey=YOUR-API-KEY-HERE \
+    --set datadog.appKey=YOUR-APP-KEY-HERE \
+    --set datadog.leaderElection=true \


Should we just turn leader election on for the cluster agent by default and get rid of this option? What could go wrong if we do? (of course we'd keep the option for when the cluster agent is not enabled).

Signed-off-by: charlyF <charly@datadoghq.com>

hkaj

values.yaml has leaderElection: false uncommented. That means the default value for the cluster agent won't apply and leaderElection won't be enabled by default. I think we should comment it out, and make it false by default for the agent but true for the cluster agent. Does that make sense?

hkaj · 2018-11-02T15:43:00Z

stable/datadog/README.md

+Run the following if you want to deploy the chart with the Datadog Cluster Agent.
+Note that specifying `clusterAgent.metricsProvider.enabled=true` will enable the External Metrics Server.
+If you want to learn to use this feature, you can check out this [walkthrough](https://github.com/DataDog/datadog-agent/blob/master/docs/cluster-agent/CUSTOM_METRICS_SERVER.md).
+The Leader Election is enabled by default in the chart for the Cluster Agent. Only the Cluster Agent(s) participate in the election, in case you have several replicas configred (using `clusterAgent.replicas`.


configred --> configured

hkaj · 2018-11-02T15:44:12Z

stable/datadog/templates/NOTES.txt

@@ -47,11 +47,12 @@ Create an application key at https://app.datadoghq.com/account/settings#api
  {{- if not .Values.clusterAgent.token }}

 ##############################################################################
-####               ERROR: You did not set a clusterAgent.token            ####
+####               WARNING: You did not set a clusterAgent.token            ####


i'm fine with notifying it, but INFO is enough IMHO? There's nothing wrong here.

hkaj · 2018-11-02T15:49:33Z

stable/datadog/templates/agent-deployment.yaml

          - name: DD_LEADER_ELECTION
-            value: {{ .Values.datadog.leaderElection | quote}}
-          {{- end }}
+            value: {{ .Values.datadog.leaderElection | default "true" | quote}}


As an aside: could we rename this file to cluster-agent-deployment.yaml? This confuses me every time.

I agree, I'll do it here

Signed-off-by: charlyF <charly@datadoghq.com>

CharlyF · 2018-11-02T16:10:12Z

I'm ok with commenting it out in the docs to be more clear but from my testings and given that:
https://github.com/helm/charts/pull/8786/files#diff-19938b6253581d6defa2db7d73496176R61
The leader election will be ON if the Cluster Agent is enabled. Unless set to false.
If the Cluster Agent is not enabled however, the default value will be the one from datadog-agent/pkg/config/config.go which is false. So the value in values.yaml does not do anything.

Signed-off-by: charlyF <charly@datadoghq.com>

hkaj · 2018-11-02T20:06:30Z

/approve
/ok-to-test

hkaj · 2018-11-02T20:31:01Z

/lgtm

k8s-ci-robot · 2018-11-02T20:31:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CharlyF, hkaj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~stable/datadog/OWNERS~~ [hkaj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* updating README Signed-off-by: charlyF <charly@datadoghq.com> * changing log level to warning Signed-off-by: charlyF <charly@datadoghq.com> * bumping chart, lint, updating agent version Signed-off-by: charlyF <charly@datadoghq.com> * updating versions in readme and comment Signed-off-by: charlyF <charly@datadoghq.com> * leader election is set to true by default Signed-off-by: charlyF <charly@datadoghq.com> * adding option and doc to generate and use random token if not provided Signed-off-by: charlyF <charly@datadoghq.com> * rephrase README Signed-off-by: charlyF <charly@datadoghq.com> * feedback Signed-off-by: charlyF <charly@datadoghq.com> * commenting leaderelection option in values.yaml Signed-off-by: charlyF <charly@datadoghq.com>

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2018

k8s-ci-robot requested review from hkaj and xvello October 26, 2018 11:55

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 26, 2018

hkaj suggested changes Oct 26, 2018

View reviewed changes

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2018

CharlyF force-pushed the document-cluster-agent branch from e4965cf to 30ddbc5 Compare October 30, 2018 15:11

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2018

CharlyF force-pushed the document-cluster-agent branch from 30ddbc5 to a1f638b Compare October 30, 2018 20:47

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2018

charlyF added 4 commits October 30, 2018 16:49

updating README

89f50c3

Signed-off-by: charlyF <charly@datadoghq.com>

changing log level to warning

6ae1df2

Signed-off-by: charlyF <charly@datadoghq.com>

bumping chart, lint, updating agent version

f8b159a

Signed-off-by: charlyF <charly@datadoghq.com>

updating versions in readme and comment

2dc2d42

Signed-off-by: charlyF <charly@datadoghq.com>

CharlyF force-pushed the document-cluster-agent branch from a1f638b to 2dc2d42 Compare October 30, 2018 20:49

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2018

Merge branch 'master' into document-cluster-agent

63f6465

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2018

hkaj suggested changes Nov 2, 2018

View reviewed changes

charlyF added 3 commits November 2, 2018 10:11

leader election is set to true by default

bb4a422

Signed-off-by: charlyF <charly@datadoghq.com>

adding option and doc to generate and use random token if not provided

871d6bd

Signed-off-by: charlyF <charly@datadoghq.com>

rephrase README

d8dad6b

Signed-off-by: charlyF <charly@datadoghq.com>

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2018

hkaj suggested changes Nov 2, 2018

View reviewed changes

feedback

621aa57

Signed-off-by: charlyF <charly@datadoghq.com>

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2018

commenting leaderelection option in values.yaml

7982139

Signed-off-by: charlyF <charly@datadoghq.com>

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 2, 2018

hkaj approved these changes Nov 2, 2018

View reviewed changes

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2018

k8s-ci-robot assigned hkaj Nov 2, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2018

k8s-ci-robot merged commit 3bdff56 into helm:master Nov 2, 2018

CharlyF deleted the document-cluster-agent branch November 2, 2018 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better document cluster agent & Update chart #8786

Better document cluster agent & Update chart #8786

CharlyF commented Oct 26, 2018

hkaj Oct 26, 2018

hkaj Oct 26, 2018

hkaj Nov 2, 2018

CharlyF Nov 2, 2018

hkaj Nov 2, 2018

hkaj Nov 2, 2018

CharlyF Nov 2, 2018

hkaj Nov 2, 2018

CharlyF Nov 2, 2018

hkaj left a comment

hkaj Nov 2, 2018

hkaj Nov 2, 2018 •

edited

Loading

hkaj Nov 2, 2018

CharlyF Nov 2, 2018

CharlyF commented Nov 2, 2018

hkaj commented Nov 2, 2018

hkaj commented Nov 2, 2018

k8s-ci-robot commented Nov 2, 2018

Better document cluster agent & Update chart #8786

Better document cluster agent & Update chart #8786

Conversation

CharlyF commented Oct 26, 2018

What this PR does / why we need it:

Checklist

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

Choose a reason for hiding this comment

hkaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaj Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlyF commented Nov 2, 2018

hkaj commented Nov 2, 2018

hkaj commented Nov 2, 2018

k8s-ci-robot commented Nov 2, 2018

hkaj Nov 2, 2018 •

edited

Loading