feat(agw): all AGW containeres have health checks #13918

mpfirrmann · 2022-09-13T16:53:09Z

Signed-off-by: Marco Pfirrmann marco.pfirrmann@tngtech.com

Summary

This PR is embedded into the larger issue #13684.

The health checks for the magmad and sctpd containers are added
The time between health checks, interval, is reduced from its default 30s to 4s.

Test Plan

Follow the README to build the containers and check their status with docker ps.

Additional Information

This change is backwards-breaking

github-actions · 2022-09-13T16:53:28Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

Howto

Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
Checks. All required CI checks must pass before merge.
Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

github-actions · 2022-09-13T17:02:23Z

feg-workflow

    2 files 203 suites 40s ⏱️
374 tests 374 ✔️ 0 💤 0 ❌
388 runs 388 ✔️ 0 💤 0 ❌

Results for commit 4565ec9.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-13T17:17:51Z

dp-workflow

14 tests 14 ✔️ 2m 16s ⏱️
  1 suites   0 💤
  1 files   0 ❌

Results for commit 4565ec9.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-13T18:01:59Z

agw-workflow

473 tests 468 ✔️ 1m 12s ⏱️
    1 suites     4 💤
    1 files     1 ❌

For more details on these failures, see this check.

Results for commit 9059b9a.

♻️ This comment has been updated with latest results.

Neudrino

If we have this for all the components, should we abstract this in x-generic-service:? At least the timeout and retries part, such what we only configure the target for the components?

Neudrino · 2022-09-14T08:40:21Z

lte/gateway/docker/docker-compose.yaml

@@ -178,6 +182,10 @@ services:
    container_name: sctpd
    ulimits:
      core: -1
+    healthcheck:
+      test: ["CMD", "bash", "-c", "[ -S /tmp/sctpd_downstream.sock ]"]
+      timeout: "5s"


There is a different timeout as for everything else configured here. Why?

It failed with the 4s timeout.

What was the test setup that made it fail? On my machine it worked with a timeout of one second. To be fair I only started sctpd and not the other containers, but then again a simple bash test command should usually take way less than a second I think.

It worked for me as well with 4s (I tested starting all containers).

mpfirrmann · 2022-09-14T11:01:33Z

If we have this for all the components, should we abstract this in x-generic-service:? At least the timeout and retries part, such what we only configure the target for the components?

Changed.

jheidbrink · 2022-09-14T11:41:16Z

lte/gateway/docker/docker-compose.yaml

    command: /usr/bin/env python3 -m magma.ctraced.main

  sctpd:
    <<: *ltecservice
    container_name: sctpd
    ulimits:
      core: -1
+    healthcheck:
+      test: ["CMD", "bash", "-c", "[ -S /tmp/sctpd_downstream.sock ]"]


nitpick: We could also write test: "test -S /tmp/sctpd_downstream.sock". Personally I find that more readable, especially the combination of square brackets for Yaml lists and for the bash test command are a bit hard to parse.

And as a compromise if we want to stay consistent with the ["CMD", ...] syntax used in this file:

test: ["CMD", "test", "-S", "/tmp/sctpd_downstream.sock"]

disclaimer: This one I didn't test, but I think it should work.

jheidbrink · 2022-09-14T12:01:01Z

lte/gateway/docker/docker-compose.yaml

@@ -25,6 +25,9 @@ x-generic-service: &service
  logging: *logging_anchor
  restart: always
  network_mode: host
+  healthcheck:
+    timeout: "4s"
+    retries: 3


I'm not sure if we should deduplicate this. Currently deduplicating is possible because the parameters are not fine-tuned to the individual checks. Once we start to fine-tune the timeout (like further down in this PR with sctpd) then we actually have two places where we define the timeout: One default and one overwrite. Defaults + overwrites are more complex than just having one definite place to look. Also, by deduplicating, we now split the healthcheck blocks into two: This part that defines the timeouts and retries, and the part in the respective services that defines the tests. Without de-duplicating you can easily see the whole healthcheck definition for a service at a glance.

The classic discussion on code abstraction vs duplication. I am sure there are heaps of internet resources arguing for both sides.

Once we start: But we do not at the moment. Would it not be sensible to focus on the current state, on the here and now, instead of anticipating what we might or might not want to do in future?

But we do not at the moment

Yes we do, in this very PR, with sctpd. That's what I wrote in paranthesis.

For me it works if we set all timeouts to 4s, so maybe there is no need for fine-tuning at this point.

Even if it's possible to use the same values everywhere, I wouldn't say this is a good candidate for deduplication. I think de-duplicating those values would be important if those values need to be the same for all checks, and if we want to prevent ourselves from accidentally modifying the setting for only one service. I don't see the need for that here. So I think this would basically save us some lines of code, and I don't think that justifies having to look up the healthcheck definitions in two different places.

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

Neudrino · 2022-09-15T12:31:39Z

After merge of #13852 a rebase with appropriate changes to align would probably be good.

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

… from magma#13918" This reverts commit 063a426.

Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>

mpfirrmann requested a review from a team September 13, 2022 16:53

mpfirrmann requested a review from a team as a code owner September 13, 2022 16:53

mpfirrmann requested a review from talkhasib September 13, 2022 16:53

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Sep 13, 2022

github-actions bot added the component: ci All updates on CI (Jenkins/CircleCi/Github Action) label Sep 13, 2022

Neudrino reviewed Sep 14, 2022

View reviewed changes

mpfirrmann force-pushed the pr/enable_health_checks branch from afd16f3 to e3f7c83 Compare September 14, 2022 10:58

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Sep 14, 2022

jheidbrink reviewed Sep 14, 2022

View reviewed changes

Neudrino mentioned this pull request Sep 14, 2022

feat(agw): dockerize the agw envoy controller #13852

Merged

1 task

Neudrino added a commit to alexzurbonsen/magma that referenced this pull request Sep 14, 2022

chore(ci): wait for healthy pods workaround - remove services from ma…

2c054c6

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

Neudrino added a commit to alexzurbonsen/magma that referenced this pull request Sep 15, 2022

chore(ci): wait for healthy pods workaround - remove services from ma…

ff44dda

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

Neudrino added a commit to alexzurbonsen/magma that referenced this pull request Sep 15, 2022

chore(ci): wait for healthy pods workaround - remove services from ma…

b51bf86

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

mpfirrmann force-pushed the pr/enable_health_checks branch from e3f7c83 to 9059b9a Compare September 19, 2022 11:06

jheidbrink approved these changes Sep 19, 2022

View reviewed changes

Neudrino approved these changes Sep 19, 2022

View reviewed changes

Neudrino enabled auto-merge (squash) September 19, 2022 14:07

Neudrino added a commit to alexzurbonsen/magma that referenced this pull request Sep 19, 2022

chore(ci): wait for healthy pods workaround - remove services from ma…

518eeea

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

feat(agw): all AGW containeres have health checks

4565ec9

Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>

mpfirrmann force-pushed the pr/enable_health_checks branch from 9059b9a to 4565ec9 Compare September 19, 2022 21:37

mpfirrmann self-assigned this Sep 19, 2022

mpfirrmann linked an issue Sep 19, 2022 that may be closed by this pull request

Fully test containerized AGW #13684

Closed

10 tasks

Neudrino merged commit dc69a34 into magma:master Sep 19, 2022

mpfirrmann deleted the pr/enable_health_checks branch September 20, 2022 06:31

Neudrino added a commit to alexzurbonsen/magma that referenced this pull request Sep 20, 2022

chore(ci): wait for healthy pods workaround - remove services from ma…

063a426

…gma#13918 Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>

Neudrino added a commit to alexzurbonsen/magma that referenced this pull request Sep 20, 2022

Revert "chore(ci): wait for healthy pods workaround - remove services…

b251582

… from magma#13918" This reverts commit 063a426.

mpfirrmann added a commit to wolfseb/magma that referenced this pull request Sep 21, 2022

feat(agw): all AGW containeres have health checks (magma#13918)

894a7e7

Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>

wolfseb mentioned this pull request Oct 17, 2022

chore(agw): cleanup agw nonsanity tests #14183

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(agw): all AGW containeres have health checks #13918

feat(agw): all AGW containeres have health checks #13918

mpfirrmann commented Sep 13, 2022 •

edited

github-actions bot commented Sep 13, 2022

github-actions bot commented Sep 13, 2022 •

edited

github-actions bot commented Sep 13, 2022 •

edited

github-actions bot commented Sep 13, 2022 •

edited

Neudrino left a comment

Neudrino Sep 14, 2022

mpfirrmann Sep 14, 2022

jheidbrink Sep 14, 2022

wolfseb Sep 15, 2022

mpfirrmann commented Sep 14, 2022

jheidbrink Sep 14, 2022

jheidbrink Sep 15, 2022

jheidbrink Sep 14, 2022

Neudrino Sep 14, 2022

jheidbrink Sep 14, 2022 •

edited

wolfseb Sep 15, 2022

jheidbrink Sep 15, 2022

Neudrino commented Sep 15, 2022

feat(agw): all AGW containeres have health checks #13918

feat(agw): all AGW containeres have health checks #13918

Conversation

mpfirrmann commented Sep 13, 2022 • edited

Summary

Test Plan

Additional Information

github-actions bot commented Sep 13, 2022

Howto

More info

github-actions bot commented Sep 13, 2022 • edited

feg-workflow

github-actions bot commented Sep 13, 2022 • edited

dp-workflow

github-actions bot commented Sep 13, 2022 • edited

agw-workflow

Neudrino 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

mpfirrmann commented Sep 14, 2022

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

jheidbrink Sep 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neudrino commented Sep 15, 2022

mpfirrmann commented Sep 13, 2022 •

edited

github-actions bot commented Sep 13, 2022 •

edited

github-actions bot commented Sep 13, 2022 •

edited

github-actions bot commented Sep 13, 2022 •

edited

jheidbrink Sep 14, 2022 •

edited