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: integ tests always wait for pipelined #14813

Merged
merged 1 commit into from Jan 9, 2023

Conversation

nstng
Copy link
Contributor

@nstng nstng commented Jan 6, 2023

Signed-off-by: Nils Semmelrock nils.semmelrock@tngtech.com

Summary

This change tries to tackle multiple issues. The main idea is to not fetch the current datapath during static initialization of the s1ap_utils, but in the constructor when an object is created.

Tackling #14068

Working towards enabling again an ipv6 tests that was disable in #14682

  • With the s1ap_utils initalization moved after initializing magmad_util we can check if nat is not enabled - and enabling it in this case (this can happen if a test that run with disabled nat failed with an error that prevents the re-enabling of nat in the teardown).
  • If a test with disabled nat tries to call pipelined, then we see 6h workflow timeouts in ci.

Tackling #14699

  • This is basically the main issue that tracks the two issues above.

Failures in re-runs of tests that re-initialize the datapath

  • I have not seen this problem in recent logs - but iirc then we have seen issues where the current datapath id was not the expected id
  • hypothesis: a test fails and the datapath is re-initialized - but the s1ap_util does not fetch the id again (because this happens during the static initialization)
  • moving the datapath id fetch to the constructor should help with this issue

Test Plan

executed precommit and extended tests locally

Additional Information

Closes #14699
Closes #14068

  • This change is backwards-breaking

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines. label Jan 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

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 the component: agw Access gateway-related issue label Jan 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

Oops! Looks like you failed the AGW Build & Format Python.

Howto

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

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

FeG Lint & Test

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

Results for commit f41698a.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

DP Lint & Test

14 tests   14 ✔️  3m 20s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit f41698a.

♻️ This comment has been updated with latest results.

@nstng nstng force-pushed the wait_for_datapath_in_integ_tests branch from 255e32c to 2252623 Compare January 6, 2023 14:50
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
@nstng nstng force-pushed the wait_for_datapath_in_integ_tests branch from 2252623 to f41698a Compare January 6, 2023 14:56
@nstng nstng marked this pull request as ready for review January 6, 2023 15:04
@nstng nstng requested review from a team and VinashakAnkitAman January 6, 2023 15:05
@nstng nstng self-assigned this Jan 9, 2023
Copy link
Contributor

@mpfirrmann mpfirrmann left a comment

Choose a reason for hiding this comment

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

Just for my understanding: we would explicitly enable NAT by default for all tests (if of course not disabled during execution) but this would be mainly to counteract failing non-NAT tests?

@nstng
Copy link
Contributor Author

nstng commented Jan 9, 2023

Just for my understanding: we would explicitly enable NAT by default for all tests (if of course not disabled during execution) but this would be mainly to counteract failing non-NAT tests?

Correct, the current state (before this change) is that NAT is enabled by default. But if a non-NAT test fails hard, i.e., NAT is not enabled again in the test tear down, then the next test start also with non-NAT. (Actually one could argue, that we then could remove the tear down steps in all non-NAT test, but I would leave them because switching the NAT mode takes some time, i.e., we should try to avoid this at test start time).

@nstng nstng merged commit 350b440 into magma:master Jan 9, 2023
lucasgonze pushed a commit to lucasgonze/magma that referenced this pull request Feb 29, 2024
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>

Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
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 size/S Denotes a PR that changes 10-29 lines.
Projects
None yet
2 participants