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

chore(agw): Python dependency upgrades #13482

Merged
merged 13 commits into from
Aug 30, 2022

Conversation

MoritzThomasHuebner
Copy link
Contributor

@MoritzThomasHuebner MoritzThomasHuebner commented Aug 3, 2022

Fixes #13426

In pairing with @wolfseb

Summary

Done:

  • Upgraded Python versions where possible. We used pipdeptree to analyze packages and dependencies.
  • Temporary change to upgrade fpm during provisioning. This is necessary to build cryptography.
  • Some packages (such as docker) are not built by pydep, since a python3-docker exists in http://archive.ubuntu.com/ubuntu/.
  • Updated lockfile

Test Plan

  • Run fab release package:destroy_vm=True on the host VM to trigger new magma and dependency *.deb files to be built.
  • Install local *.deb files, and any other dependencies that have not been newly packaged from the artifactory.
  • Remove /etc/magma.
  • Install magma*.deb file.
  • Spin up magma_test and magma_trfserver
  • Run S1AP precommit and integration tests.
  • Run build-all script on a fork: https://github.com/MoritzThomasHuebner/magma/runs/7759753398?check_suite_focus=true
    • For this run we needed to make a temporary change that disables the check that we are on the origin repo.
    • The check shows that all packages are actually being built.
  • Run Bazel build and test workflow on fork: https://github.com/MoritzThomasHuebner/magma/actions/runs/2830383274
    • Ensures versions in bazel/external/requirements.in do not cause any version conflicts

Additional Information

There are a number of packages we were not able to upgrade:

  • fpm requires that there is a setup.py, thus some of the newest versions of pyparsing, jsonschema, pyroute2 can no longer be built since these packages now utilize other build methods.

  • setuptools: The version 49.6.0 is hardcoded in some places.

  • docker: Ubuntu focal is not compatible with docker>=5.

    • requests: The latest versions require docker>=5.
  • flask is restricted to 1.1.4.

    • This restricts click, itsdangerous, Jinja2, and markupsafe.
  • redis is restricted to 3.5.3.

  • cryptography is restricted to 3.2.1 because newer versions cannot be built with fpm.

  • PyYAML is restricted to <6.0

  • This change is backwards-breaking

@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Aug 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 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: orc8r Orchestrator-related issue labels Aug 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

✔️ The Semantic PR check ended with status success. See instructions on formatting your commit and pull request titles.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

feg-workflow

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

Results for commit 86b098c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

cloud-workflow

1 017 tests   1 017 ✔️  2m 6s ⏱️
   356 suites         0 💤
       7 files           0

Results for commit 86b098c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

dp-workflow

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

Results for commit 86b098c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

agw-workflow

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

Results for commit 86b098c.

♻️ This comment has been updated with latest results.

@github-actions github-actions bot added the component: ci All updates on CI (Jenkins/CircleCi/Github Action) label Aug 5, 2022
@alexzurbonsen
Copy link
Contributor

For documentation purposes: running fab release package:destroy_vm=True creates the following python3 packages

[VM] ~/magma-packages $ ls -1 | grep python3
python3-aiodns_3.0.0_all.deb
python3-aiohttp_3.8.1_amd64.deb
python3-aiosignal_1.2.0_all.deb
python3-async-timeout_4.0.2_all.deb
python3-bravado-core_5.17.0_all.deb
python3-certifi_2022.6.15_all.deb
python3-cffi_1.14.5_amd64.deb
python3-charset-normalizer_2.0.0_all.deb
python3-click_8.1.3_all.deb
python3-cryptography_3.2.1_amd64.deb
python3-cython_0.29.32_amd64.deb
python3-fire_0.4.0_all.deb
python3-freezegun_1.2.1_all.deb
python3-frozenlist_1.3.1_amd64.deb
python3-grpcio_1.47.0_amd64.deb
python3-idna_3.3_all.deb
python3-itsdangerous_2.1.2_all.deb
python3-jinja2_3.1.2_all.deb
python3-json-pointer_2.3_all.deb
python3-jsonpickle_2.2.0_all.deb
python3-lxml_4.9.1_amd64.deb
python3-markupsafe_2.1.1_amd64.deb
python3-multidict_6.0.2_amd64.deb
python3-netifaces_0.11.0_amd64.deb
python3-psutil_5.9.1_amd64.deb
python3-pycares_4.2.1_amd64.deb
python3-pycparser_2.20_all.deb
python3-pycryptodome_3.15.0_amd64.deb
python3-pyroute2.core_0.6.13_all.deb
python3-pyroute2.ethtool_0.6.13_all.deb
python3-pyroute2.ipdb_0.6.13_all.deb
python3-pyroute2.ipset_0.6.13_all.deb
python3-pyroute2.ndb_0.6.13_all.deb
python3-pyroute2.nftables_0.6.13_all.deb
python3-pyroute2.nslink_0.6.13_all.deb
python3-pyroute2_0.6.13_all.deb
python3-python-dateutil_2.8.2_all.deb
python3-redis-collections_0.11.0_all.deb
python3-requests_2.28.1_all.deb
python3-scapy_2.4.5_all.deb
python3-sentry-sdk_1.9.0_all.deb
python3-six_1.16.0_all.deb
python3-spyne_2.14.0_all.deb
python3-urllib3_1.25.3_all.deb
python3-webcolors_1.12_all.deb
python3-yaml_6.0_amd64.deb
python3-yarl_1.8.1_amd64.deb

@alexzurbonsen
Copy link
Contributor

alexzurbonsen commented Aug 8, 2022

Tried to install the magma package and python3 dependencies on a regular VM per @nstng instructions (as in the PR description):

  • delete /etc/magma
  • apt install python3 dependencies and magma*.deb package
  • run a set of commands that copies systemd service files, does some linking etc (not yet committed)

Afterwards services where not running properly since packages could not be found. Had to reinstall some packages (e.g. in magmad sudo apt --reinstall install python3-h2). In that way I got magmad running.

Next, I tried pipelined. Turns out that we seem to use a jinja2 version that is too high.[1] Instead of 3.1.2 we should probably try the highest v2, that is 2.11.3.

It might also be a better idea, to use the prod VM for installation and testing. May be, there we don't face all the issues with reinstalling packages.

[1] ImportError: cannot import name 'escape' from 'jinja2' (/usr/local/lib/python3.8/dist-packages/jinja2/__init__.py)

@pull-request-size pull-request-size bot added size/XXL Denotes a Pull Request that changes 1000+ lines. and removed size/L Denotes a Pull Request that changes 100-499 lines. labels Aug 9, 2022
.github/workflows/bazel.yml Outdated Show resolved Hide resolved
.github/workflows/bazel.yml Outdated Show resolved Hide resolved
@MoritzThomasHuebner
Copy link
Contributor Author

Tried to install the magma package and python3 dependencies on a regular VM per @nstng instructions (as in the PR description):

* delete `/etc/magma`

* apt install python3 dependencies and magma*.deb package

* run a set of commands that copies systemd service files, does some linking etc (not yet committed)

Afterwards services where not running properly since packages could not be found. Had to reinstall some packages (e.g. in magmad sudo apt --reinstall install python3-h2). In that way I got magmad running.

Next, I tried pipelined. Turns out that we seem to use a jinja2 version that is too high.[1] Instead of 3.1.2 we should probably try the highest v2, that is 2.11.3.

It might also be a better idea, to use the prod VM for installation and testing. May be, there we don't face all the issues with reinstalling packages.

[1] ImportError: cannot import name 'escape' from 'jinja2' (/usr/local/lib/python3.8/dist-packages/jinja2/__init__.py)

Thanks for the comment @alexzurbonsen, @wolfseb and I have roughly the same situation. It is still not entirely clear to us why some of the services are not running. Packages like h2 should have been installed from artifactory with the installation of the python3 dependencies.

@wolfseb
Copy link
Contributor

wolfseb commented Aug 9, 2022

Update: we got all magma services running by reinstalling the following Python packages:

  • prometheus-client
  • h2
  • jinja2
  • eventlet
  • redis
  • ovs
  • envoy
  • click

via

sudo apt reinstall python3-*

It is then possible to run the integ tests via the magma_test VM

@wolfseb
Copy link
Contributor

wolfseb commented Aug 9, 2022

Integ tests have been run for the new VM. The following tests fail:

Precommit: None

Extended: None

Nonsanity:

  • test_activate_deactivate_multiple_dedicated.py
  • test_sctp_shutdown_while_mme_is_stopped.py
  • test_3495_timer_for_dedicated_bearer_with_mme_restart.py
  • test_no_attach_complete_with_mme_restart.py

@MoritzThomasHuebner MoritzThomasHuebner changed the title Python dependency upgrades ci: Python dependency upgrades Aug 10, 2022
@MoritzThomasHuebner MoritzThomasHuebner marked this pull request as ready for review August 15, 2022 00:47
@MoritzThomasHuebner MoritzThomasHuebner requested review from a team August 15, 2022 00:47
@MoritzThomasHuebner MoritzThomasHuebner requested a review from a team as a code owner August 15, 2022 00:47
@MoritzThomasHuebner MoritzThomasHuebner requested a review from a team August 15, 2022 00:47
@MoritzThomasHuebner
Copy link
Contributor Author

I triggered a run for the lte integ test
https://github.com/MoritzThomasHuebner/magma/actions/runs/2917075867

@jheidbrink
Copy link
Contributor

The changes look good to me. I verified i could build all py_binarys with Bazel, and run the subscriberdb binary which uses the new systemd-python package. I did not double-check that I can fab package as that was already done. Did someone test the new .deb on a freshly destroyed VM so we can be sure that the .deb didn't just work because of some things still laying around in the system?

@MoritzThomasHuebner
Copy link
Contributor Author

The changes look good to me. I verified i could build all py_binarys with Bazel, and run the subscriberdb binary which uses the new systemd-python package. I did not double-check that I can fab package as that was already done. Did someone test the new .deb on a freshly destroyed VM so we can be sure that the .deb didn't just work because of some things still laying around in the system?

@jheidbrink yes we tested this in the process and installed magma just from the debian packages.

@sebathomas
Copy link
Contributor

I think the integration test run is mostly looking good, only one red test from what I can see. Is test_tau_periodic_active known to be flaky?

@jheidbrink
Copy link
Contributor

Is test_tau_periodic_active known to be flaky?

At least I remember that I commented that one out locally a while ago.

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.

bazel + pipelined lgtm

@sebathomas
Copy link
Contributor

Needs another rebase because of changes to the required jobs, then it should finally be ready to merge.

MoritzThomasHuebner and others added 13 commits August 30, 2022 11:03
- Upgraded versions in `lte/gateway/setup.py` and `orc8r/gateway/setup.py` where possible.
- Added `--python-pip /home/vagrant/build/python/bin/pip` to the `fpm` command in `pydep`. This option ensures that we are not using `easy_install` inadvertently.
- Added a step to upgrade `fpm` upon VM provisioning. This step should be removed after the next preburn is created. The `fpm` upgrade is necessary to build a `.deb` file for `cryptography`.
- Updated the `magma.lockfile.ubuntu` by running `fab release package`.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
- Added pylint exception. The linter erroneously flags this line otherwise.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
- Updated bazel files.
- Also replaced `systemd` with `systemd-python` in bazel files since these are different packages

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
- Updated bazel files.
- Also replaced `systemd` with `systemd-python` in bazel files since these are different packages

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
Signed-off-by: Alexander zur Bonsen <alexander.zur.bonsen@tngtech.com>
Signed-off-by: Alexander zur Bonsen <alexander.zur.bonsen@tngtech.com>
Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
This change makes bazel consistent with the setup.py files. The two packages have similar interfaces but are different and cannot just be interchanged. The use of `systemd.daemon.notify` in `sdwatchdog.py` also indicates that `systemd_python` is the intended package one should use.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
Using pip to download some packages for fpm fails currently since fpm uses adds the `--no-binary :all:` flag to pip download.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
fpm is now already on the latest version when you spin up the VM.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
@MoritzThomasHuebner MoritzThomasHuebner added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Aug 30, 2022
@sebathomas sebathomas merged commit e3cb3cf into magma:master Aug 30, 2022
rsarwad pushed a commit to rsarwad/magma that referenced this pull request Sep 4, 2022
* chore(agw): Updating python dependencies

- Upgraded versions in `lte/gateway/setup.py` and `orc8r/gateway/setup.py` where possible.
- Added `--python-pip /home/vagrant/build/python/bin/pip` to the `fpm` command in `pydep`. This option ensures that we are not using `easy_install` inadvertently.
- Updated the `magma.lockfile.ubuntu` by running `fab release package`.
- Added pylint exception. The linter erroneously flags this line otherwise.
- Updated bazel files.
- Also replaced `systemd` with `systemd-python` in bazel files since these are different packages

* bazel: downgrade pyyaml and generate requirements.txt changes

* chore(agw): Replace systemd with systemd_python

This change makes bazel consistent with the setup.py files. The two packages have similar interfaces but are different and cannot just be interchanged. The use of `systemd.daemon.notify` in `sdwatchdog.py` also indicates that `systemd_python` is the intended package one should use.

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>
Signed-off-by: Alexander zur Bonsen <alexander.zur.bonsen@tngtech.com>
Co-authored-by: Alexander zur Bonsen <alexander.zur.bonsen@tngtech.com>
@MoritzThomasHuebner MoritzThomasHuebner deleted the py_deps_upgrade branch February 23, 2023 01:20
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) component: orc8r Orchestrator-related issue ready2merge This PR is ready to be merged (is approved and passes all required checks) size/XXL Denotes a Pull Request that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Python dependencies after python 3.8 update
6 participants