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

Added take_last to EVC #563

Closed
wants to merge 1 commit into from
Closed

Added take_last to EVC #563

wants to merge 1 commit into from

Conversation

Alopalao
Copy link

@Alopalao Alopalao commented Nov 5, 2024

Closes #543

Summary

First iteration of take_last, this PR needs kytos PR to work.
Added option to EVC, take_last which indicates which VLAN will be taken next for current_path and failover_path

Local Tests

Create an EVC and redeploy

End-to-End Tests

To be added.

@Alopalao Alopalao requested a review from a team as a code owner November 5, 2024 17:07
@@ -5,5 +5,5 @@
# pip-compile --output-file requirements/dev.txt requirements/dev.in
#
-e git+https://github.com/kytos-ng/python-openflow.git#egg=python-openflow
-e git+https://github.com/kytos-ng/kytos.git#egg=kytos[dev]
-e git+https://github.com/kytos-ng/kytos.git@feature/last_tag#egg=kytos[dev]
Copy link
Author

Choose a reason for hiding this comment

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

Reminder to remove this.
Also, Is this feature going to be backported?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the underlying issue has the backport_20232 and backport_20241 labels.

Copy link
Member

Choose a reason for hiding this comment

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

btw, I've just merged the related PR kytos-ng/kytos#515. Nicely done.

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, that's a good start. But notice that this doesn't fully meet some of the requirements:

  • If you were to have a static EVC with primary_path, path X, and backup_path, path Y, since take_last is a global attr of the EVC and not per link, after each redeploy for example, notice that it would keep switching and going back to use the same s_vlan for each path, path X would use 1, and paty Y would use 4095. So, in this case, if a network operator wanted to force redeploy to change primary_path path to use a different s_vlan it would be able to accomplish that. See what I mean? The old implementation that wouldn't be a problem since it was fully rotating the allocated s_vlan per link, so it accomplished that indirectly.

You'll need to rethink and refine the proposed solution. Here a few pointers for you to also think about:

  1. I believe we'll need an EVC attr to set the links s_vlan allocation strategy.
  2. I believe we'll also need to store a state on each link metadata regarding the direction (or side) the s_vlan was taken. Since s_vlan is already stored as a metadata, maybe we can also reserve a new metadata for this, since no other NApp will change this since mef_eline is also using unique link refs for now.

@viniarck
Copy link
Member

viniarck commented Nov 6, 2024

@Alopalao, that's a good start. But notice that this doesn't fully meet some of the requirements:

Quick brainstorm

See you can come up with something better:

  • Maybe we can have something like links_svlan_alloc_strategy: str or links_svlan_use_strategy: str (or something along these lines) on EVC, and then this could be a enum values of supported strategies, such as "first|last|alternating", where "alternating" would be the default on. And, then link.choose_vlans would take this parameter too, to take into consideration how to allocate the svlan. If "first" then always try allocate the next, if "last" then always try allocate the last, if "alternating" then you either pick first or last depending on the prior state that link had on its metadata.

Another benefit of structuring this way, in addition to meeting the requirement, is that if network operators want same allocation "direction" again then they can also stick with "first" and "last" strategy, which at some point might be valuable to then, and code maintenance-wise that'd be reasonable to maintain. The main reason why they want alternating for now as you remember from recent discussion is due to potentially being able to fix s_vlans conflicts, which in normal circumstances we don't expect anymore. Also, in the very far future, this could allow us to set allocate explicit s_vlans on link (I doubt we'll need to do this in the next years, but at least it'd wouldn't be too far away, and then the global link strategy in this case could be "explicit|null|None", and then we could expose another way of allowing it to be set)

Finally, once you hash out the final solution, I'll recommend that you summarize it in the issue, and then let's see if it meets the requirement and then go for it. OK?

@Alopalao Alopalao marked this pull request as draft November 19, 2024 14:14
@Alopalao
Copy link
Author

Alopalao commented Dec 3, 2024

Closing this PR in favor of avoid VLAN

@Alopalao Alopalao closed this Dec 3, 2024
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.

Redeploy of an EVC no longer fixes inconsistencies
2 participants