-
Notifications
You must be signed in to change notification settings - Fork 592
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
fix: metrics test is independent from execution of unrelated tests #14422
Conversation
Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Thanks for opening a PR! 💯
Howto
More infoPlease take a moment to read through the Magma project's
If this is your first Magma PR, also consider reading
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A description in the pr instead of TBD be nice
lte/gateway/python/integ_tests/s1aptests/test_gateway_metrics_attach_detach.py
Show resolved
Hide resolved
"mme_new_association", | ||
label_values_success, | ||
) | ||
assert (mme_new_association == mme_new_association_before + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this counter measure? I would have assumed that this value is 2 after the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not completely sure ... I started my local test with +2, but it seems to be +1. The doc says "New SCTP association". I.e., maybe something like a session based counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are sctp association 1 makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, on new sctp association, for label "mme_new_association" count gets incremented.
@@ -39,11 +39,10 @@ def test_gateway_metrics_attach_detach(self): | |||
""" Basic gateway metrics with attach/detach for a single UE """ | |||
|
|||
label_values_success = {"result": "success"} | |||
mme_new_association = self._getMetricValueGivenLabel( | |||
mme_new_association_before = self._getMetricValueGivenLabel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every test case, new sctp association is created as part Setup() and every test case has Teardown() when test cases exits. In TearDown(), s1ap tester sends sctp shutdown which eventually clears the association. As part of mme restart, is connection closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsarwad aww sorry, I already mergede the PR - do you see an issue with this? Then I will create a new PR.
My observation was that the metric counter remained after test_attach_detach.py
was executed and test_gateway_metrics_attach_detach
actually expected this by an assert counter > 0
. I.e., I would expect that test_attach_detach.py
does not clean up the counter correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like counter for label "mme_new_association" is not getting decremented on sctp shutdown at mme. Changes in PR make sure that only one association is created before test case terminates
…agma#14422) Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com> Signed-off-by: Nils Semmelrock <nils.semmelrock@tngtech.com>
Signed-off-by: Nils Semmelrock nils.semmelrock@tngtech.com
Summary
#14351 moved
test_gateway_metrics_attach_detach.py
in the execution order. This made an early assert fail because nomme_new_association
event happened yet (see, e.g., https://github.com/magma/magma/runs/9277319986). Another attach/detach test needs to be executed firs, e.g.,test_attach_detach.py
if mme was restarted in the mean time.Here: move the assert to the end of the test and check that the event counter actually increased during the test - dynamically so that possible events from other tests do not matter.
Test Plan
Execute
test_gateway_metrics_attach_detach.py
after a freshly restarted mme.Additional Information