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

Don't shuffle next tags #310

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Don't shuffle next tags #310

merged 1 commit into from
Nov 30, 2022

Conversation

ajoaoff
Copy link

@ajoaoff ajoaoff commented Nov 28, 2022

When getting the next available tag, remove the shuffling of the available tags.
To avoid race conditions, add a lock to the method.
The lock is unique in the Link class, avoiding possible deadlocks.

@ajoaoff ajoaoff requested a review from a team November 28, 2022 23:56
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.

@ajoaoff nicely done. Looks good to me.

Very helpful for network operators as @italovalcy mentioned. Currently we have a rotating behavior as EVCs get deleted since available VLANs get appended, the rotating behavior also looks good to me:

kytos $> controller.switches['00:00:00:00:00:00:00:01'].interfaces[4].available_tags[:3]
Out[8]:
[TAG(<TAGType.VLAN: 1>, 4),
 TAG(<TAGType.VLAN: 1>, 5),
 TAG(<TAGType.VLAN: 1>, 6)]

kytos $> controller.switches['00:00:00:00:00:00:00:01'].interfaces[4].available_tags[-3:]
Out[9]:
[TAG(<TAGType.VLAN: 1>, 4094),
 TAG(<TAGType.VLAN: 1>, 4095),
 TAG(<TAGType.VLAN: 1>, 1)]

Before you merge this PR, could you run the e2e tests to make sure it's all set? Recently we also got an error on this test case test_085_create_and_remove_ten_circuit_concurrently kytos-ng/kytos-end-to-end-tests#182, and that one exercises core points of this change.

@ajoaoff ajoaoff closed this Nov 30, 2022
@ajoaoff ajoaoff reopened this Nov 30, 2022
@ajoaoff
Copy link
Author

ajoaoff commented Nov 30, 2022

I've run the end to end tests with success, merging the PR.

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.

2 participants