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

feat(agw): Create magma VM based on debian package #13552

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

voisey
Copy link
Contributor

@voisey voisey commented Aug 9, 2022

Summary

Closes #13263.

Known issues associated with this PR:

  • In some cases, we see that pipelined is not reachable. This seems to be due to a problem with restarting this service in this set-up. This could be related to Fix mme memory leaks in debian production builds #13551.
  • agw_install_ubuntu.sh is basically duplicated into agw_install_ubuntu_vm.sh and agw_network_ubuntu.sh. The reason is that the agw_install_ubuntu.sh is the official installation script and it seems wrong to mix this with planned CI set-up. A flag was introduced into the magma_deploy role to get around this.

Test Plan

  • lte/gateway $ vagrant up magma_deb + same for magma_test + magma_trfserver + execute all relevant integration tests
  • All precommit and extended LTE integ tests ran green locally at least once

@voisey voisey requested a review from a team as a code owner August 9, 2022 15:52
@voisey voisey requested a review from uri200 August 9, 2022 15:52
@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Aug 9, 2022
@voisey voisey mentioned this pull request Aug 9, 2022
3 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

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

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added component: agw Access gateway-related issue component: ci All updates on CI (Jenkins/CircleCi/Github Action) labels Aug 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

feg-workflow

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

Results for commit 893a16d.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

dp-workflow

13 tests   13 ✔️  2m 52s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit 893a16d.

♻️ This comment has been updated with latest results.

Comment on lines 55 to 56
# TODO: GH13551. Ugly workaround to prevent service failures because of memory leaks
sed -i 's/\[Service\]/[Service]\nEnvironment="LSAN_OPTIONS=detect_leaks=0"/' /etc/systemd/system/magma@mme.service
Copy link
Collaborator

Choose a reason for hiding this comment

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

The workaround suppresses all memory leaks, which is not a good idea. It will be better to suppress only the two known issues in libfluid and libczmq, but highlight if new memory leaks come up.

Copy link
Contributor Author

@voisey voisey Aug 10, 2022

Choose a reason for hiding this comment

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

Thanks for the comment. In the end, we've decided to remove this line entirely since we've found that connection is lost with pipelined even with it. We propose to merge this PR as a first attempt and then to have a more detailed look at this in future PRs (already planned), when we try to use this in the CI to execute integration tests. Does this sound okay to you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@voisey , do you plan to remove this line as part of this PR, or future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now removed it from this PR.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

agw-workflow

615 tests   611 ✔️  4m 41s ⏱️
    2 suites      4 💤
    2 files        0

Results for commit 893a16d.

♻️ This comment has been updated with latest results.

@nstng nstng self-requested a review August 9, 2022 17:48
@voisey voisey force-pushed the create_magma_prod_vm branch 2 times, most recently from 03611f1 to 6112bf0 Compare August 11, 2022 14:39
@voisey voisey force-pushed the create_magma_prod_vm branch 2 times, most recently from 6f38419 to 7e2cead Compare August 12, 2022 08:33
@voisey
Copy link
Contributor Author

voisey commented Aug 12, 2022

@nstng @ssanadhya The migration of the shell script to ansible is done, so this should be ready for review now.

@nstng nstng self-requested a review August 15, 2022 12:37
Copy link
Contributor

@nstng nstng left a comment

Choose a reason for hiding this comment

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

lgtm - I can run integration tests vs magma_deb. I can reproduce that (very often) when one test fails then a couple follow up tests will fail because pipelined is not reachable. I agree that this issue can and shout be better analyzed when a setup is created to use magma_deb in CI.

@voisey voisey requested review from ssanadhya and removed request for uri200 August 16, 2022 07:32
- { src: 'orc8r/tools/ansible/roles/fluent_bit/files/magma_td-agent-bit.service', dest: 'magma@td-agent-bit.service' }
- { src: 'lte/gateway/deploy/roles/magma/files/systemd/magma_dp_envoy.service', dest: 'magma_dp@envoy.service' }

- name: Reload magmad to apply service file changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

This step is redundant if you are eventually rebooting the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting that, I've just removed it.

@ssanadhya
Copy link
Collaborator

@voisey , what's the purpose of 'magma_deploy' role?

@ssanadhya
Copy link
Collaborator

@voisey @nstng , FYI, this PR is also a step towards fixing #12075 . Can you add #12075 to your backlog too?

@nstng
Copy link
Contributor

nstng commented Aug 16, 2022

@voisey , what's the purpose of 'magma_deploy' role?

It's for installing a magma debian package (and some additional dependencies and configuration). The role is used in the official magma install script lte/gateway/deploy/agw_install_ubuntu.sh where the latest production magma.deb is installed (i.e. currently 1.7). Here the role is used to install the latest magma.deb created by the build_all workflow (i.e., the latest version in the magma artifactory in focal-ci).

@voisey voisey mentioned this pull request Aug 18, 2022
4 tasks
Copy link
Collaborator

@ssanadhya ssanadhya left a comment

Choose a reason for hiding this comment

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

Lgtm, with minor comments

lte/gateway/Vagrantfile Show resolved Hide resolved
Signed-off-by: Cameron Voisey <cameron.voisey@tngtech.com>
@voisey voisey merged commit 338e259 into magma:master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue component: ci All updates on CI (Jenkins/CircleCi/Github Action) size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run integration tests against a Magma deployment build
4 participants