[stable/datalog] Do not enable the `cri` check when running on a `docker` setup #21476

L3n41c · 2020-03-16T12:58:53Z

What this PR does / why we need it:

Make a clear distinction between the docker and the CRI setup so that only one of docker or cri check is scheduled by the agent.

Which issue this PR fixes

With the current chart, datadog.criSocketPath defaults to /var/run/docker.sock.
As datadog.criSocketPath is always setup, the Helm chart defines the environment variable DD_CRI_SOCKET_PATH which makes the datadog agent initialisation scripts setup the cri check.
But the docker socket doesn’t implement the CRI API so that, the cri check is always setup and fails.

Special notes for your reviewer:

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
Title of the PR starts with chart name (e.g. [stable/mychartname])

k8s-ci-robot · 2020-03-16T12:59:07Z

Hi @L3n41c. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…ker` setup Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

clamoriniere · 2020-03-17T19:32:53Z

/ok-to-test

clamoriniere · 2020-03-17T19:51:38Z

/lgtm

k8s-ci-robot · 2020-03-17T19:51:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clamoriniere, L3n41c

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~~ [L3n41c,clamoriniere]

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

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com>

…template (#21510) * enable deployment annotations, bump chart version (#21502) Signed-off-by: Ryan Holt <ryan@ryanholt.net> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datalog] Do not enable the `cri` check when running on a `docker` setup (#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datadog] Add some missing syscalls to the `system-probe` seccomp profile (#21456) The added syscalls are syscalls that an unconfined `system-probe` would do. Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/minio] corrected syntax error in statefulset (#21503) * corrected syntax error in statefulset Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> * chart version bump Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/redis-ha] Make emptyDir configurable from values (#21489) Signed-off-by: Jeroen Castelein <jeroencastelein11@gmail.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Set DD_APM_ENABLED With the new Helm chart, even if `datadog.apm.enabled` is set to false, it reverts to the docker defaults (true). Having the trace-agent running in the background is pretty harmless from a resource overhead standpoint, however, the logic of the helm chart will automatically do the 8126 port-forwarding, and since we don't want non-apm customers to have this port exposed, we need to respect the chart settings. Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Bumped version Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * 2.0.12 Fixed a bug where datadog.apm.enabled was not being respected Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * re-applying the changes Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> Co-authored-by: Ryan Holt <ryan@ryanholt.net> Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com> Co-authored-by: Thomas Wilkinson <thomas@capnajax.com> Co-authored-by: Jeroen Castelein <jeroen.castelein@kpn.com>

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

…template (helm#21510) * enable deployment annotations, bump chart version (helm#21502) Signed-off-by: Ryan Holt <ryan@ryanholt.net> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datalog] Do not enable the `cri` check when running on a `docker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datadog] Add some missing syscalls to the `system-probe` seccomp profile (helm#21456) The added syscalls are syscalls that an unconfined `system-probe` would do. Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/minio] corrected syntax error in statefulset (helm#21503) * corrected syntax error in statefulset Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> * chart version bump Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/redis-ha] Make emptyDir configurable from values (helm#21489) Signed-off-by: Jeroen Castelein <jeroencastelein11@gmail.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Set DD_APM_ENABLED With the new Helm chart, even if `datadog.apm.enabled` is set to false, it reverts to the docker defaults (true). Having the trace-agent running in the background is pretty harmless from a resource overhead standpoint, however, the logic of the helm chart will automatically do the 8126 port-forwarding, and since we don't want non-apm customers to have this port exposed, we need to respect the chart settings. Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Bumped version Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * 2.0.12 Fixed a bug where datadog.apm.enabled was not being respected Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * re-applying the changes Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> Co-authored-by: Ryan Holt <ryan@ryanholt.net> Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com> Co-authored-by: Thomas Wilkinson <thomas@capnajax.com> Co-authored-by: Jeroen Castelein <jeroen.castelein@kpn.com>

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

…template (helm#21510) * enable deployment annotations, bump chart version (helm#21502) Signed-off-by: Ryan Holt <ryan@ryanholt.net> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datalog] Do not enable the `cri` check when running on a `docker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datadog] Add some missing syscalls to the `system-probe` seccomp profile (helm#21456) The added syscalls are syscalls that an unconfined `system-probe` would do. Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/minio] corrected syntax error in statefulset (helm#21503) * corrected syntax error in statefulset Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> * chart version bump Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/redis-ha] Make emptyDir configurable from values (helm#21489) Signed-off-by: Jeroen Castelein <jeroencastelein11@gmail.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Set DD_APM_ENABLED With the new Helm chart, even if `datadog.apm.enabled` is set to false, it reverts to the docker defaults (true). Having the trace-agent running in the background is pretty harmless from a resource overhead standpoint, however, the logic of the helm chart will automatically do the 8126 port-forwarding, and since we don't want non-apm customers to have this port exposed, we need to respect the chart settings. Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Bumped version Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * 2.0.12 Fixed a bug where datadog.apm.enabled was not being respected Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * re-applying the changes Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> Co-authored-by: Ryan Holt <ryan@ryanholt.net> Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com> Co-authored-by: Thomas Wilkinson <thomas@capnajax.com> Co-authored-by: Jeroen Castelein <jeroen.castelein@kpn.com>

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

…template (helm#21510) * enable deployment annotations, bump chart version (helm#21502) Signed-off-by: Ryan Holt <ryan@ryanholt.net> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datalog] Do not enable the `cri` check when running on a `docker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datadog] Add some missing syscalls to the `system-probe` seccomp profile (helm#21456) The added syscalls are syscalls that an unconfined `system-probe` would do. Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/minio] corrected syntax error in statefulset (helm#21503) * corrected syntax error in statefulset Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> * chart version bump Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/redis-ha] Make emptyDir configurable from values (helm#21489) Signed-off-by: Jeroen Castelein <jeroencastelein11@gmail.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Set DD_APM_ENABLED With the new Helm chart, even if `datadog.apm.enabled` is set to false, it reverts to the docker defaults (true). Having the trace-agent running in the background is pretty harmless from a resource overhead standpoint, however, the logic of the helm chart will automatically do the 8126 port-forwarding, and since we don't want non-apm customers to have this port exposed, we need to respect the chart settings. Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Bumped version Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * 2.0.12 Fixed a bug where datadog.apm.enabled was not being respected Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * re-applying the changes Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> Co-authored-by: Ryan Holt <ryan@ryanholt.net> Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com> Co-authored-by: Thomas Wilkinson <thomas@capnajax.com> Co-authored-by: Jeroen Castelein <jeroen.castelein@kpn.com>

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Adrien Loiseau <adrien.loiseau@logic-immo.com>

…template (helm#21510) * enable deployment annotations, bump chart version (helm#21502) Signed-off-by: Ryan Holt <ryan@ryanholt.net> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datalog] Do not enable the `cri` check when running on a `docker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datadog] Add some missing syscalls to the `system-probe` seccomp profile (helm#21456) The added syscalls are syscalls that an unconfined `system-probe` would do. Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/minio] corrected syntax error in statefulset (helm#21503) * corrected syntax error in statefulset Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> * chart version bump Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/redis-ha] Make emptyDir configurable from values (helm#21489) Signed-off-by: Jeroen Castelein <jeroencastelein11@gmail.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Set DD_APM_ENABLED With the new Helm chart, even if `datadog.apm.enabled` is set to false, it reverts to the docker defaults (true). Having the trace-agent running in the background is pretty harmless from a resource overhead standpoint, however, the logic of the helm chart will automatically do the 8126 port-forwarding, and since we don't want non-apm customers to have this port exposed, we need to respect the chart settings. Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Bumped version Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * 2.0.12 Fixed a bug where datadog.apm.enabled was not being respected Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * re-applying the changes Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> Co-authored-by: Ryan Holt <ryan@ryanholt.net> Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com> Co-authored-by: Thomas Wilkinson <thomas@capnajax.com> Co-authored-by: Jeroen Castelein <jeroen.castelein@kpn.com> Signed-off-by: Adrien Loiseau <adrien.loiseau@logic-immo.com>

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Miguel Mingorance <miguel.mingorance@deliveryhero.com>

…template (helm#21510) * enable deployment annotations, bump chart version (helm#21502) Signed-off-by: Ryan Holt <ryan@ryanholt.net> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datalog] Do not enable the `cri` check when running on a `docker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/datadog] Add some missing syscalls to the `system-probe` seccomp profile (helm#21456) The added syscalls are syscalls that an unconfined `system-probe` would do. Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/minio] corrected syntax error in statefulset (helm#21503) * corrected syntax error in statefulset Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> * chart version bump Signed-off-by: Thomas Wilkinson <thomas.wilkinson@us.ibm.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * [stable/redis-ha] Make emptyDir configurable from values (helm#21489) Signed-off-by: Jeroen Castelein <jeroencastelein11@gmail.com> Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Set DD_APM_ENABLED With the new Helm chart, even if `datadog.apm.enabled` is set to false, it reverts to the docker defaults (true). Having the trace-agent running in the background is pretty harmless from a resource overhead standpoint, however, the logic of the helm chart will automatically do the 8126 port-forwarding, and since we don't want non-apm customers to have this port exposed, we need to respect the chart settings. Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * Bumped version Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * 2.0.12 Fixed a bug where datadog.apm.enabled was not being respected Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> * re-applying the changes Signed-off-by: Omer Lifshitz <omer.lifshitz@datadoghq.com> Co-authored-by: Ryan Holt <ryan@ryanholt.net> Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com> Co-authored-by: Thomas Wilkinson <thomas@capnajax.com> Co-authored-by: Jeroen Castelein <jeroen.castelein@kpn.com> Signed-off-by: Miguel Mingorance <miguel.mingorance@deliveryhero.com>

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Mar 16, 2020

k8s-ci-robot requested a review from hkaj March 16, 2020 12:58

helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2020

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 Mar 16, 2020

L3n41c mentioned this pull request Mar 17, 2020

Do not enable the cri check when running on a docker setup DataDog/datadog-operator#51

Merged

[stable/datalog] Do not enable the cri check when running on a `doc…

bf0ed52

…ker` setup Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

L3n41c force-pushed the docker_vs_cri branch from da7b357 to bf0ed52 Compare March 17, 2020 16:44

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2020

clamoriniere approved these changes Mar 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2020

k8s-ci-robot merged commit b181353 into helm:master Mar 17, 2020

L3n41c deleted the docker_vs_cri branch March 18, 2020 08:05

Vincoll mentioned this pull request Mar 18, 2020

[stable/datadog] criutil failure #21338

Closed

L3n41c mentioned this pull request Mar 19, 2020

[stable/datadog] disabling CRI also disables log collection for docker runtime #21461

Closed

fowlie pushed a commit to fowlie/charts that referenced this pull request Mar 20, 2020

[stable/datalog] Do not enable the cri check when running on a `doc…

83715b9

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

ppennanen mentioned this pull request Apr 6, 2020

cri check fails when running on docker DataDog/datadog-operator#66

Closed

irlevesque pushed a commit to quantopian/charts that referenced this pull request Jul 13, 2020

[stable/datalog] Do not enable the cri check when running on a `doc…

caeedf6

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

includerandom pushed a commit to includerandom/helm_charts that referenced this pull request Jul 19, 2020

[stable/datalog] Do not enable the cri check when running on a `doc…

362f198

…ker` setup (helm#21476) Signed-off-by: Lénaïc Huard <lenaic.huard@datadoghq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/datalog] Do not enable the `cri` check when running on a `docker` setup #21476

[stable/datalog] Do not enable the `cri` check when running on a `docker` setup #21476

L3n41c commented Mar 16, 2020 •

edited

Loading

k8s-ci-robot commented Mar 16, 2020

clamoriniere commented Mar 17, 2020

clamoriniere commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

[stable/datalog] Do not enable the cri check when running on a docker setup #21476

[stable/datalog] Do not enable the cri check when running on a docker setup #21476

Conversation

L3n41c commented Mar 16, 2020 • edited Loading

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Mar 16, 2020

clamoriniere commented Mar 17, 2020

clamoriniere commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

[stable/datalog] Do not enable the `cri` check when running on a `docker` setup #21476

[stable/datalog] Do not enable the `cri` check when running on a `docker` setup #21476

L3n41c commented Mar 16, 2020 •

edited

Loading