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

feature: Added uni tag usage #371

Merged
merged 18 commits into from
Oct 11, 2023
Merged

feature: Added uni tag usage #371

merged 18 commits into from
Oct 11, 2023

Conversation

Alopalao
Copy link

@Alopalao Alopalao commented Aug 29, 2023

Closes #324
Closes #367

Summary

Delete deprecated notify_link_available_tags method from utils.py
Adapted NNI tags to vlan_pool epic contributions (kytos and topology PR)
UNIs now use and make available tags when required (when created, updated, deleted)
Adapted NApp from tag_type interger to string

Local tests

Created, updated and deleted EVC, missing doing these actions in mass.
Uni test added

End-to-End Tests

E2E tests are passing, comment.

@Alopalao
Copy link
Author

Alopalao commented Sep 5, 2023

E2E tests are passing:

============================= test session starts ==============================
platform linux -- Python 3.9.2, pytest-7.2.0, pluggy-1.3.0
rootdir: /tests
plugins: timeout-2.1.0, rerunfailures-10.2, anyio-3.6.2
collected 241 items

tests/test_e2e_01_kytos_startup.py ..                                    [  0%]
tests/test_e2e_05_topology.py ..................                         [  8%]
tests/test_e2e_10_mef_eline.py ..........ss.....x.....x................  [ 24%]
tests/test_e2e_11_mef_eline.py ......                                    [ 27%]
tests/test_e2e_12_mef_eline.py .....Xx.                                  [ 30%]
tests/test_e2e_13_mef_eline.py .....xs.s......xs.s.XXxX.xxxx..X......... [ 47%]
...                                                                      [ 48%]
tests/test_e2e_14_mef_eline.py x                                         [ 49%]
tests/test_e2e_15_mef_eline.py ..                                        [ 50%]
tests/test_e2e_20_flow_manager.py .....................                  [ 58%]
tests/test_e2e_21_flow_manager.py ...                                    [ 60%]
tests/test_e2e_22_flow_manager.py ...............                        [ 66%]
tests/test_e2e_23_flow_manager.py ..............                         [ 72%]
tests/test_e2e_30_of_lldp.py ....                                        [ 73%]
tests/test_e2e_31_of_lldp.py ...                                         [ 75%]
tests/test_e2e_32_of_lldp.py ...                                         [ 76%]
tests/test_e2e_40_sdntrace.py .............                              [ 81%]
tests/test_e2e_41_kytos_auth.py ........                                 [ 85%]
tests/test_e2e_50_maintenance.py ........................                [ 95%]
tests/test_e2e_60_of_multi_table.py .....                                [ 97%]
tests/test_e2e_70_kytos_stats.py ..F.F..                                 [100%]

test_e2e_70_kytos_stats Those two cases are test_015_packet_count and test_025_packet_count_per_flow. These cases are failing even with master branch due to slow hardware process, unrelated to code.

@Alopalao Alopalao marked this pull request as ready for review September 5, 2023 22:08
@Alopalao Alopalao requested a review from a team as a code owner September 5, 2023 22:08
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

It's great how it's shaping up, @Alopalao.

Overall looks good, raised a few minor points though.

cc'ing also @Ktmi to review since he's involved in mef_eline related epics.

models/evc.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
@Alopalao
Copy link
Author

Tag_type changed to string.
E2E tests PR #261 needed. Results:

tests/test_e2e_01_kytos_startup.py ..                                    [  0%]
tests/test_e2e_05_topology.py ..................                         [  8%]
tests/test_e2e_10_mef_eline.py ..........ss.....x.....x................  [ 24%]
tests/test_e2e_11_mef_eline.py ......                                    [ 27%]
tests/test_e2e_12_mef_eline.py .....Xx.                                  [ 30%]
tests/test_e2e_13_mef_eline.py .....Xs.s......Xs.s.XXxX.xxxx..X......... [ 47%]
...                                                                      [ 48%]
tests/test_e2e_14_mef_eline.py x                                         [ 49%]
tests/test_e2e_15_mef_eline.py ..                                        [ 50%]
tests/test_e2e_20_flow_manager.py .....................                  [ 58%]
tests/test_e2e_21_flow_manager.py ...                                    [ 60%]
tests/test_e2e_22_flow_manager.py ...............                        [ 66%]
tests/test_e2e_23_flow_manager.py ..............                         [ 72%]
tests/test_e2e_30_of_lldp.py ....                                        [ 73%]
tests/test_e2e_31_of_lldp.py ...                                         [ 75%]
tests/test_e2e_32_of_lldp.py ...                                         [ 76%]
tests/test_e2e_40_sdntrace.py .............                              [ 81%]
tests/test_e2e_41_kytos_auth.py ........                                 [ 85%]
tests/test_e2e_50_maintenance.py ........................                [ 95%]
tests/test_e2e_60_of_multi_table.py .....                                [ 97%]
tests/test_e2e_70_kytos_stats.py ....F..                                 [100%]

Ignore error test_e2e_70_kytos_stats, error was from stats['duration_sec'] being 0.

@Alopalao Alopalao changed the title Added uni tag usage feature: added uni tag usage Sep 11, 2023
@Alopalao Alopalao changed the title feature: added uni tag usage feature: Added uni tag usage Sep 11, 2023
@viniarck viniarck self-requested a review September 12, 2023 18:43
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

It's shaping up well @Alopalao.

I encountered a traceback though (more details in the comments).

Also, I asked to address front-end, bump v3 on endpoints, and migration script too. Let me know what you think, also for v3, good that we have on our radar vlan range for 2023.2 too so that wouldn't require another bumping and we can consider it's expected for v3, OK?

openapi.yml Outdated Show resolved Hide resolved
requirements/dev.in Outdated Show resolved Hide resolved
tests/helpers.py Outdated Show resolved Hide resolved
db/models.py Show resolved Hide resolved
models/path.py Outdated Show resolved Hide resolved
@viniarck viniarck self-requested a review September 18, 2023 15:16
scripts/README.md Outdated Show resolved Hide resolved
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

Functionality-wise it's pretty much there. Asked minor changes though.

Thanks, Aldo. Nicely done how this is evolving.

models/path.py Show resolved Hide resolved
openapi.yml Outdated Show resolved Hide resolved
models/evc.py Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
models/path.py Outdated Show resolved Hide resolved
- Updated readme
- Updated changelog
- Tag type integer compatibility returned.
- No longer calling `notify_interface_tags()` from Interface
main.py Show resolved Hide resolved
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

@Alopalao, thanks for the updates. I closed many of the threads I opened. The only things remaining are:

  • Enhance the DB script to also use commands and ideally provide aggregate|preview commands as you've done on your topology PR.
  • Make the final decision regarding not bumping v3 and the compatibility that you're already in touch with @italovalcy (since this is a major decision that will require rework on existing PRs, let's try to settle this as soon as we can), thanks guys.

- Downgrade API urls version back to 2
@Alopalao
Copy link
Author

Updated script and changelog. Also downgraded API versioning back to v2, commit c26527a89f2f0e0bd3186806033117f580991c46

@viniarck viniarck self-requested a review September 27, 2023 14:30
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

@Alopalao,

I was also simulating an upgrade with one intra-evpl and one inter-evpl, and the scripts executed idempotently as expected.

I noticed an unexpected side effect though which might be from PR #349, right after the OpenFlow handshake, it ended up activating the EVCs, without running sdntrace_cp (currently all activations after a restart are relying on sdntrace_cp, so I'm going to ask for you to reassess this part too, since it's like a pre-mature activation, notice that in the future though the state of EVC might be stored so this part might change again, @Ktmi is working on something related to this part, anyway, it's not immediately conflicting but something to be aware):

2023-09-27 14:08:16,770 - INFO [kytos.core.atcp_server] (MainThread) New connection from 127.0.0.1:33732
2023-09-27 14:08:16,771 - INFO [kytos.core.atcp_server] (MainThread) New connection from 127.0.0.1:33746
2023-09-27 14:08:16,772 - INFO [kytos.core.atcp_server] (MainThread) New connection from 127.0.0.1:33750
2023-09-27 14:08:17,277 - INFO [kytos.napps.kytos/of_core] (thread_pool_sb_1) Connection ('127.0.0.1', 33732), Switch 00:00:00:00:00:00:00:01: OPENFLOW HANDSHAKE COMPLETE
2023-09-27 14:08:17,280 - INFO [kytos.napps.kytos/of_core] (thread_pool_sb_1) Connection ('127.0.0.1', 33746), Switch 00:00:00:00:00:00:00:03: OPENFLOW HANDSHAKE COMPLETE
2023-09-27 14:08:17,284 - INFO [kytos.napps.kytos/of_core] (thread_pool_sb_0) Connection ('127.0.0.1', 33750), Switch 00:00:00:00:00:00:00:02: OPENFLOW HANDSHAKE COMPLETE
2023-09-27 14:08:17,351 - INFO [kytos.napps.kytos/mef_eline] (thread_pool_app_16) Activating EVC 136a09e1d2a240. Interfaces: {'00:00:00:00:00:00:00:01:1': {'status': 'UP', 'status_reason
': set()}, '00:00:00:00:00:00:00:01:2': {'status': 'UP', 'status_reason': set()}}.
2023-09-27 14:08:17,355 - INFO [kytos.napps.kytos/mef_eline] (thread_pool_app_16) Activating EVC 789c1eeeb1f74f. Interfaces: {'00:00:00:00:00:00:00:01:1': {'status': 'UP', 'status_reason
': set()}, '00:00:00:00:00:00:00:03:1': {'status': 'UP', 'status_reason': set()}}.

Also, as I mentioned on Slack to you, I noticed an unexpected redploy and also unexpected consistency check deleting flows. This didn't happen deterministically, so I'm not sure if it was a left over in my development environment, either way I'll recommend for you to explore and exercise the upgrade a bit more to confirm this part too:

2023-09-27 13:08:45,240 - INFO [kytos.napps.kytos/flow_manager] (thread_pool_sb_1) Consistency check: alien flow on switch 00:00:00:00:00:00:00:03
2023-09-27 13:08:45,241 - INFO [kytos.napps.kytos/flow_manager] (thread_pool_sb_1) Flow forwarded to switch 00:00:00:00:00:00:00:03 to be deleted. Flow: {'flows': [{'switch': '00:00:00:0
0:00:00:00:03', 'table_id': 0, 'match': {'in_port': 3, 'dl_vlan': 1}, 'priority': 20000, 'idle_timeout': 0, 'hard_timeout': 0, 'cookie': 12282418139759057986, 'id': '5c38f29eff18b71d1ac5
d779a1ca75f2', 'stats': {'byte_count': 0, 'duration_sec': 530, 'duration_nsec': 762000000, 'packet_count': 0}, 'cookie_mask': 0, 'instructions': [{'instruction_type': 'apply_actions', 'a
ctions': [{'action_type': 'pop_vlan'}, {'port': 1, 'action_type': 'output'}]}]}]}
2023-09-27 13:08:45,241 - INFO [kytos.napps.kytos/flow_manager] (thread_pool_sb_1) Consistency check: alien flow on switch 00:00:00:00:00:00:00:03
2023-09-27 13:08:45,242 - INFO [kytos.napps.kytos/flow_manager] (thread_pool_sb_1) Flow forwarded to switch 00:00:00:00:00:00:00:03 to be deleted. Flow: {'flows': [{'switch': '00:00:00:0
0:00:00:00:03', 'table_id': 0, 'match': {'in_port': 2, 'dl_vlan': 1}, 'priority': 20000, 'idle_timeout': 0, 'hard_timeout': 0, 'cookie': 12282418139759057986, 'id': 'c9e960a72d0dfa9fc18e
27165873eb1d', 'stats': {'byte_count': 0, 'duration_sec': 530, 'duration_nsec': 677000000, 'packet_count': 0}, 'cookie_mask': 0, 'instructions': [{'instruction_type': 'apply_actions', 'a
ctions': [{'action_type': 'pop_vlan'}, {'port': 1, 'action_type': 'output'}]}]}]}
2023-09-27 13:08:45,384 - INFO [uvicorn.access] (MainThread) 127.0.0.1:60194 - "POST /api/kytos/flow_manager/v2/flows/00%3A00%3A00%3A00%3A00%3A00%3A00%3A01 HTTP/1.1" 202
2023-09-27 13:08:45,390 - INFO [kytos.napps.kytos/flow_manager] (AnyIO worker thread) Send FlowMod from request dpid: 00:00:00:00:00:00:00:03, command: add, force: False,  flows[0, 1]: [
{'match': {'in_port': 2, 'dl_vlan': 1}, 'cookie': 12282418139759057986, 'actions': [{'action_type': 'pop_vlan'}, {'action_type': 'output', 'port': 1}], 'owner': 'mef_eline', 'table_group
': 'evpl', 'table_id': 0, 'priority': 20000}]
2023-09-27 13:08:45,395 - INFO [uvicorn.access] (MainThread) 127.0.0.1:60208 - "POST /api/kytos/flow_manager/v2/flows/00%3A00%3A00%3A00%3A00%3A00%3A00%3A03 HTTP/1.1" 202
2023-09-27 13:08:45,401 - INFO [kytos.napps.kytos/mef_eline] (thread_pool_app_12) Failover path for EVC(73ea391b271c42, evpl) was deployed.

In terms of consistency check, I haven't encountered any unexpected diffs in the DB, so could be indeed a left over on OvS switches that I had, but let's see. Look forward to your feedback and exploratory test too.

scripts/vlan_type_string.py Outdated Show resolved Hide resolved
@viniarck
Copy link
Member

Aldo, I reported the issue here #376, good thing it was not related to the new changes. I'll try to use a new topology and re-run the upgrade and finish the review here, and the other bug is addressed in another PR. Let's try to isolate things just so we can land this PR here soon if possible, let's see.

Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

@Alopalao leaving this PR pre-approved.

Using another topology, I ended up hitting an early activation again, but that's not an issue from this PR, so it can be handled in a new PR with the linked issues from the previous comments:

2023-09-27 16:56:29,554 - INFO [kytos.napps.kytos/mef_eline] (thread_pool_app_1) Activating EVC ef632a9413b04c. Interfaces: {'00:00:00:00:00:00:00:01:1': {'status': 'UP', 'status_reason
': set()}, '00:00:00:00:00:00:00:01:2': {'status': 'UP', 'status_reason': set()}}.
2023-09-27 16:56:29,558 - INFO [kytos.napps.kytos/mef_eline] (thread_pool_app_1) Activating EVC d69490106b4344. Interfaces: {'00:00:00:00:00:00:00:01:1': {'status': 'UP', 'status_reason
': set()}, '00:00:00:00:00:00:00:03:1': {'status': 'UP', 'status_reason': set()}}.
  • I tried out both scripts, they both worked idempotently, deleted evcs too, interface_details available_tags ended up as expected.

If your exploratory tests are also passing and if you simulated other successful upgrades since e2e tests are passing I think we can land this PR (that way we can also land the other 3 related others too, facilitating for you to move forward with the epic), let us know.

Later on, please don't forget to also cover the GET endpoint on topology and augment e2e tests covering the new part (and also add new e2e tests with tag ranges on UNIs and confirming the correct allocation and when it shouldn't). cc'ing also @italovalcy and @Ktmi in case they want to review in the next days since they're involved with other mef_eline PRs too.

scripts/README.md Outdated Show resolved Hide resolved
Copy link

@Ktmi Ktmi left a comment

Choose a reason for hiding this comment

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

Looks great!

@viniarck
Copy link
Member

Let's ship it. @Alopalao check out the last comment.

@italovalcy, btw, in the future when you have time your review is still appreciated, if there's any feedback to be addressed Aldo will discuss and take care in a next one, we'll prioritize merging this one.

@viniarck viniarck merged commit 0abc5ae into master Oct 11, 2023
1 of 2 checks passed
@viniarck viniarck deleted the epic/vlan_pool branch October 11, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants