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: Allow creation for "any" and "untagged" as EVCs #258

Merged
merged 36 commits into from
Mar 14, 2023

Conversation

Alopalao
Copy link

@Alopalao Alopalao commented Feb 2, 2023

Closes #219

Summary

Removed required property from #/components/schemas/Tag
Allow string values in uni.tag.value, (only "any" and "untagged" for now)

Local Tests

New uni allowed in EVCs requests:

{"uni_a": {"tag": {"value": "any", "tag_type": 1}, "interface_id": "00:00:00:00:00:00:00:01:1"}}

{"uni_z": {"tag": {"value": "untagged", "tag_type": 1}, "interface_id": "00:00:00:00:00:00:00:02:2"}}'

Comparing results to OVS
Case: Matching only packets with a VLAN tag regardless of its value (any to "4096/4096")
OVS flow created:
cookie=0xaae4abdd793f8e41, duration=15.738s, table=0, n_packets=0, n_bytes=0, send_fTlow_rem priority=20000,in_port="s1-eth1",vlan_tci=0x1000/0x1000 actions=push_vlan:0x88a8,output:"s1-eth3"

Case: Matching only packets without a VLAN tag (untagged to 0)
OVS flow created:
cookie=0xaaa16d05a1189a4f, duration=2.930s, table=0, n_packets=0, n_bytes=0, send_flow_rem priority=20000,in_port="s1-eth1",vlan_tci=0x0000/0x1fff actions=push_vlan:0x88a8,output:"s1-eth3"

Packets reception tested with mininet ping feature.
Cases are working with CONSISTENCY_CHECK enabled.

End-to-End Tests

These test include the e2e test recently committed.
Also this new PR.

tests/test_e2e_01_kytos_startup.py ..                                    [  0%]
tests/test_e2e_05_topology.py ..................                         [  9%]
tests/test_e2e_10_mef_eline.py ..........ss.....x.....x...............   [ 27%]
tests/test_e2e_11_mef_eline.py .....                                     [ 29%]
tests/test_e2e_12_mef_eline.py .....xx.                                  [ 33%]
tests/test_e2e_13_mef_eline.py .....xxXx......xxXx.XXxX.xxxx..x......... [ 52%]
...                                                                      [ 53%]
tests/test_e2e_14_mef_eline.py x                                         [ 53%]
tests/test_e2e_15_mef_eline.py ..                                        [ 54%]
tests/test_e2e_20_flow_manager.py ....................                   [ 64%]
tests/test_e2e_21_flow_manager.py ..                                     [ 64%]
tests/test_e2e_22_flow_manager.py ...............                        [ 71%]
tests/test_e2e_23_flow_manager.py ..............                         [ 78%]
tests/test_e2e_30_of_lldp.py ....                                        [ 80%]
tests/test_e2e_31_of_lldp.py ...                                         [ 81%]
tests/test_e2e_32_of_lldp.py ...                                         [ 82%]
tests/test_e2e_40_sdntrace.py .....                                      [ 85%]
tests/test_e2e_41_kytos_auth.py ........                                 [ 88%]
tests/test_e2e_50_maintenance.py ........................                [100%]

Helpful document

A Google sheet was created. It contains the every result for all the possibles uni.tag.value's for both intra and inter switch EVCs. The results shown on this documents reflect the expected results on kytos after this pull request is merged.

@viniarck viniarck requested a review from a team February 3, 2023 14:29
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 this PR.

On issue #219, we'll need to align the approach that we're going go for on of_core as well just so this change works as intended. We'll need to give a semantics of probably value 0 or None to give the semantics of any VLAN tag since currently, of_core is using an logical OR with VlanId.OFPVID_PRESENT

@italovalcy did an initial assessment about this, I'll suggest that you discuss with him and document on that issue the approach we'll go for, and then after then you can adapt this PR here and map the rest of the changes.

Thank you

@Alopalao Alopalao marked this pull request as draft February 7, 2023 21:29
@Alopalao
Copy link
Author

Alopalao commented Feb 7, 2023

As discussed:
Since having tag.value be either 0 or -1 is not the best option, they are replaced with any and untagged respectively.
On commit d6772b39f12f35aa3493b498da74494b6aaa9801:

  • mef_eline is change so it supports strings in uni.tag.value. For now, the string values can only be any and untagged. This restriction is validated on TAGDoc.
  • For any, the action {"action_type": "set_vlan", "vlan_id": "any"} is not being added to flow_mods.

@Alopalao Alopalao marked this pull request as ready for review February 8, 2023 20:39
@viniarck
Copy link
Member

viniarck commented Feb 9, 2023

@Alopalao, thanks for anticipating some PRs, as we've just briefly talked on the planning meeting today, let's also make sure that the approach we end up is aligned with MEF vlan range, here's some initial points to include in the discussion:

  1. In the short-term, dl_vlan with a mask will need to be supported on Add support to VLAN range #18, currently of_core is structured to handle either an int or a string containing an integer and its mask "int/int". 0's in the mask means don't care, so "an int/0" means any vlan. flow_manager exposes a relatively low-level OpenFlow API, so we should model in a way that facilitates for building the messages. On higher level APIs, such as mef_eline's since it's also driving the range of VLANs, we could use higher level values there such as "untagged" or "any" but it'll need to translate to what flow_manager expects, and it also needs to be able to express a range in the vlan range feature, let's assess the trade offs and try to keep as simple and compatible as we can.

  2. Regarding "untagged", I've noticed that OvS - the default switches platform that we use on mininet uses vlan 0, vlan_tci=0 Match only packets without an 802.1Q header, (vlan_tci is 16 bits in total 12 bits for vlan and 4 for priority + one extra bit). This also resembles the concept of a native vlan that network operators are familiar with as well. Maybe this is something that we could consider and see if it can simplify on our end.

  3. sdntrace_cp relies on stored flows, so whatever is stored on dl_vlan is important to sdntrace_cp. @gretelliz will also need to adapt sdntrace_cp. Maybe we could always have a mask all 1s (0xfff) if it's not set? So this would also be aligned with vlan range? Something like str "int/mask". We don't expect to have direct queries on the flows.flow.match.dl_type document attribute. So not storing the result int might not be an issue, and having it as str "int/mask"? would also facilitate for sdntrace_cp? Something to think about.

  4. Get more insights about this functionality exploring on OpenFlow switches first. I'd also recommend to explore on OvS (and later on NoviFlow) to install dl_vlan flows that means untagged or any there and see how they ended up shown in the control-plane (dump-flows on OvS for instance) and generate some traffic. Having this data and information at least on OvS initially would be very helpful for us to ensure that our implementation end up installing equivalent flows.

  5. Double check and document which modes are supported to be installed in the same UNI, probably check with @italovalcy the valid combinations that we'd need to support in prod, for instance, should it be allowed to have an EPL and untagged in the same interface? Also check the default priorities flows implications we want to set.

cc'ing also @ajoaoff since he'll work on mef eline vlan range

@Alopalao Alopalao changed the title Removed required property from UNI tag Allow creation for "any" and "untagged" as EVCs Feb 21, 2023
@Alopalao Alopalao changed the title Allow creation for "any" and "untagged" as EVCs feature: Allow creation for "any" and "untagged" as EVCs Feb 21, 2023
tests/unit/models/test_evc_deploy.py Outdated Show resolved Hide resolved
tests/unit/models/test_evc_deploy.py Outdated Show resolved Hide resolved
@Alopalao Alopalao changed the base branch from master to backport_branch_2022.3 March 6, 2023 17:10
@Alopalao Alopalao changed the base branch from backport_branch_2022.3 to master March 6, 2023 17:10
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.

This feature turned out really great @Alopalao, congrats.

I've asked small changes though. Also, could you update the changelog accordingly?

controllers/__init__.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
openapi.yml Show resolved Hide resolved
tests/unit/models/test_evc_deploy.py Outdated Show resolved Hide resolved
db/models.py Outdated Show resolved Hide resolved
-Added new function `get_priority`
-Added enum to yml `Tag.value`
-Added `self.subTest` to for loop
-Deleted leftover string
models/evc.py Outdated Show resolved Hide resolved
@viniarck
Copy link
Member

Aldo, the chagelog aldo needs to be updated if you could updated it.

I've just reviewed your e2e tests too, nicely done.

Once these minor suggestions here are addressed I'll approve this PR, and once @italovalcy approves the e2e too since he was also interested in ensuring some flows being asserted then we'll merge this PR here OK?

@viniarck viniarck self-requested a review March 13, 2023 13:43
main.py Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
models/evc.py Outdated Show resolved Hide resolved
- Replace leftover dictionaries with `special_cases`
@viniarck viniarck self-requested a review March 14, 2023 13:47
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.

Nicely done, @Alopalao. Thanks for pushing these commits. I'll leave the PR approved, once e2e is also approved feel free to merge this PR.

Also, when this is about to get merged, please notify on #kytos-ng-dev channel, that until sdntrace_cp issue 79 is addressed, this feature here is available, but after a controller restart consistency check will for any/untagged won't succeed and as a result, unnecessary periodic redeploys will end up happening, which is OK for now in master during development since that sdntrace_cp issue is also scheduled for 2023.1, but let's make that clear for developers when they're using master version. Maybe we could have a marked xfail e2e test for this too, and once sdntrace_cp fixes it, then we remove the @pytest.mark.xfail decorator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow creation of EVCs for untagged UNI - support matching packets without a VLAN tag
2 participants