fix(lte): Fixed `detect_init_system` #14034

MoritzThomasHuebner · 2022-09-29T06:30:02Z

Closes #14033

Signed-off-by: Moritz Huebner moritz.huebner@tngtech.com

Summary

Added exec_command_capture_output to allow running commands that return non-zero status codes without raising an exception.
Moved _is_installed function outside detect_init_system.
Simplified docker/systemd_magmad_running logic.
Renamed MagmadUtil._data to MagmadUtil._credentials and removed "command" key.
No longer write "command" to MagmadUtil._credentials, as this was not used anyway.

Test Plan

See if an integration test can be started

with a dockerized setup
with a systemd setup

github-actions · 2022-09-29T06:30:21Z

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-29T06:42:43Z

feg-workflow

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

Results for commit 1ab0f7b.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-29T06:55:46Z

dp-workflow

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

Results for commit 1ab0f7b.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-29T07:24:20Z

agw-workflow

615 tests 611 ✔️ 3m 47s ⏱️
    2 suites     4 💤
    2 files     0 ❌

Results for commit 1ab0f7b.

♻️ This comment has been updated with latest results.

nstng

Thank you for fixing this - I was sure I checked the service not running case :( but yeah, I can reproduce the issue.

Refactoring also lgtm.

NIT I would have preferred a prior check if the tool is installed (was done via type before the change). It feels a little hacky that a 'command not found' is basically ignored and the empty stdout is compared in that case. Raw return of subprocess.run:

CompletedProcess(args=['sshpass', '-p', 'vagrant', 'ssh', '-o', 'UserKnownHostsFile=/dev/null', '-o', 'StrictHostKeyChecking=no', '-o', 'LogLevel=ERROR', 'vagrant@192.168.60.142', "docker ps --filter 'name=magmad' --format '{{.Names}}'"], returncode=127, stdout=b'', stderr=b'bash: docker: command not found\n')

nstng · 2022-09-29T16:01:43Z

restarted failing required jobs - looks like a connection hiccup

MoritzThomasHuebner · 2022-09-30T09:10:14Z

Thank you for fixing this - I was sure I checked the service not running case :( but yeah, I can reproduce the issue.

Refactoring also lgtm.

NIT I would have preferred a prior check if the tool is installed (was done via type before the change). It feels a little hacky that a 'command not found' is basically ignored and the empty stdout is compared in that case. Raw return of subprocess.run:
CompletedProcess(args=['sshpass', '-p', 'vagrant', 'ssh', '-o', 'UserKnownHostsFile=/dev/null', '-o', 'StrictHostKeyChecking=no', '-o', 'LogLevel=ERROR', 'vagrant@192.168.60.142', "docker ps --filter 'name=magmad' --format '{{.Names}}'"], returncode=127, stdout=b'', stderr=b'bash: docker: command not found\n')

I'll have another look at this next week, but I think this is out of scope for this PR.

- Added `exec_command_capture_output` to have an option to capture stdout and continue on non-zero exit codes. - Removed `_is_installed` function. - Simplified docker/systemd_magmad_running logic. - Renamed `MagmadUtil._data` to `MagmadUtil._credentials` and removed "command" key. - No longer write "command" to `MagmadUtil._credentials`, as this was not used anyway. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

MoritzThomasHuebner · 2022-10-03T03:01:52Z

@nstng I added the _is_installed back in as you suggested.

github-actions · 2022-10-03T03:02:13Z

Oops! Looks like you failed the Python Format Check.

Howto

Instructions on running the formatter and linter locally are provided in the format AGW doc
Guide to the different CI checks and resolution guidelines

♻️ Updated: ✅ The check is passing the Python Format Check after the last commit.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

lte/gateway/python/integ_tests/s1aptests/s1ap_utils.py

- replace `&` with `and` in magmad running checks so that second condition is only evaluated if the first suceeds - Improved docstring of `exec_command_capture_output` - Added type hints to `exec_command`, `exec_command_output`, and `exec_command_capture_output` to highlight how these methods are different Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

- Perform sequential checks for installation and if magma is running - Remove exec_command_capture_output again as it is now unused Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

github-actions · 2022-10-04T03:35:26Z

Oops! Looks like you failed the DCO check. Be sure to sign all your commits.

Howto

♻️ Updated: ✅ The check is passing the DCO check after the last commit.

This reverts commit 2bb7908. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

VinashakAnkitAman

LGTM. Please rebase the code as some ci check is failing.

* fix(lte): Fixed `detect_init_system` - Added `exec_command_capture_output` to have an option to capture stdout and continue on non-zero exit codes. - Removed `_is_installed` function. - Simplified docker/systemd_magmad_running logic. - Renamed `MagmadUtil._data` to `MagmadUtil._credentials` and removed "command" key. - No longer write "command" to `MagmadUtil._credentials`, as this was not used anyway. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> * fix(lte): Added _is_installed check back in Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> * fix(lte): Improve changes in previous commits - replace `&` with `and` in magmad running checks so that second condition is only evaluated if the first suceeds - Improved docstring of `exec_command_capture_output` - Added type hints to `exec_command`, `exec_command_output`, and `exec_command_capture_output` to highlight how these methods are different Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> * fix(lte): Improve changes in previous commits - Perform sequential checks for installation and if magma is running - Remove exec_command_capture_output again as it is now unused Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> * Revert "fix(lte): Improve changes in previous commits" This reverts commit 2bb7908. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> * fix(lte): Properly moved installation checks first Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Sep 29, 2022

github-actions bot added the component: agw Access gateway-related issue label Sep 29, 2022

MoritzThomasHuebner force-pushed the fix_detect_init_system branch from c16d263 to b33ab3b Compare September 29, 2022 06:31

MoritzThomasHuebner marked this pull request as ready for review September 29, 2022 07:31

MoritzThomasHuebner requested review from a team and pruthvihebbani September 29, 2022 07:31

MoritzThomasHuebner force-pushed the fix_detect_init_system branch from b33ab3b to 2d1afb4 Compare September 29, 2022 09:02

nstng self-requested a review September 29, 2022 13:23

nstng approved these changes Sep 29, 2022

View reviewed changes

crasu mentioned this pull request Sep 30, 2022

chore(ci): add workflow that runs s1ap integ tests against dockerized… #14023

Closed

MoritzThomasHuebner force-pushed the fix_detect_init_system branch from 2d1afb4 to c0500d3 Compare October 3, 2022 03:00

fix(lte): Added _is_installed check back in

1b77d44

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

MoritzThomasHuebner force-pushed the fix_detect_init_system branch from c0500d3 to 1b77d44 Compare October 3, 2022 03:21

nstng requested changes Oct 3, 2022

View reviewed changes

lte/gateway/python/integ_tests/s1aptests/s1ap_utils.py Outdated Show resolved Hide resolved

VinashakAnkitAman reviewed Oct 3, 2022

View reviewed changes

lte/gateway/python/integ_tests/s1aptests/s1ap_utils.py Outdated Show resolved Hide resolved

MoritzThomasHuebner added 2 commits October 4, 2022 14:12

fix(lte): Improve changes in previous commits

2bb7908

- Perform sequential checks for installation and if magma is running - Remove exec_command_capture_output again as it is now unused Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

Revert "fix(lte): Improve changes in previous commits"

4161b0d

This reverts commit 2bb7908. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

MoritzThomasHuebner force-pushed the fix_detect_init_system branch from 5334a89 to 4161b0d Compare October 4, 2022 03:38

fix(lte): Properly moved installation checks first

1ab0f7b

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

nstng self-requested a review October 4, 2022 07:56

nstng approved these changes Oct 4, 2022

View reviewed changes

VinashakAnkitAman approved these changes Oct 4, 2022

View reviewed changes

MoritzThomasHuebner merged commit eb377c0 into magma:master Oct 5, 2022

MoritzThomasHuebner deleted the fix_detect_init_system branch February 23, 2023 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lte): Fixed `detect_init_system` #14034

fix(lte): Fixed `detect_init_system` #14034

MoritzThomasHuebner commented Sep 29, 2022 •

edited

github-actions bot commented Sep 29, 2022

github-actions bot commented Sep 29, 2022 •

edited

github-actions bot commented Sep 29, 2022 •

edited

github-actions bot commented Sep 29, 2022 •

edited

nstng left a comment

nstng commented Sep 29, 2022

MoritzThomasHuebner commented Sep 30, 2022

MoritzThomasHuebner commented Oct 3, 2022

github-actions bot commented Oct 3, 2022 •

edited

github-actions bot commented Oct 4, 2022 •

edited

VinashakAnkitAman left a comment

fix(lte): Fixed detect_init_system #14034

fix(lte): Fixed detect_init_system #14034

Conversation

MoritzThomasHuebner commented Sep 29, 2022 • edited

Summary

Test Plan

github-actions bot commented Sep 29, 2022

Howto

More info

github-actions bot commented Sep 29, 2022 • edited

feg-workflow

github-actions bot commented Sep 29, 2022 • edited

dp-workflow

github-actions bot commented Sep 29, 2022 • edited

agw-workflow

nstng left a comment

Choose a reason for hiding this comment

nstng commented Sep 29, 2022

MoritzThomasHuebner commented Sep 30, 2022

MoritzThomasHuebner commented Oct 3, 2022

github-actions bot commented Oct 3, 2022 • edited

Howto

github-actions bot commented Oct 4, 2022 • edited

Howto

VinashakAnkitAman left a comment

Choose a reason for hiding this comment

fix(lte): Fixed `detect_init_system` #14034

fix(lte): Fixed `detect_init_system` #14034

MoritzThomasHuebner commented Sep 29, 2022 •

edited

github-actions bot commented Sep 29, 2022 •

edited

github-actions bot commented Sep 29, 2022 •

edited

github-actions bot commented Sep 29, 2022 •

edited

github-actions bot commented Oct 3, 2022 •

edited

github-actions bot commented Oct 4, 2022 •

edited