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.

Sign up for GitHub

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

Jump to bottom

Make ECMP hashing for GTP-U traffic TEID-based #232

Merged
merged 13 commits into from
Apr 20, 2021
Merged

Make ECMP hashing for GTP-U traffic TEID-based #232

merged 13 commits into from
Apr 20, 2021

Conversation

osinstom
Copy link
Contributor

@osinstom osinstom commented Apr 13, 2021 •
edited
Loading

This PR implements ECMP hashing for GTP-U traffic.

Closes #232.

ccascone
ccascone reviewed Apr 13, 2021
View reviewed changes
p4src/include/control/hasher.p4 Outdated Show resolved Hide resolved
p4src/include/header.p4 Outdated Show resolved Hide resolved
@osinstom
Copy link
Contributor Author

I have several concerns to discuss on the PDP sync meeting.

@codecov
Copy link

codecov bot commented Apr 15, 2021

The author of this PR, osinstom, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at [email protected] with any questions.

@osinstom osinstom changed the title [WIP] Make ECMP hashing for GTP-U traffic TEID-based Make ECMP hashing for GTP-U traffic TEID-based Apr 15, 2021
@osinstom osinstom requested a review from Yi-Tseng April 15, 2021 15:37
Yi-Tseng
Yi-Tseng reviewed Apr 15, 2021
View reviewed changes
p4src/include/header.p4 Outdated Show resolved Hide resolved
ccascone
ccascone reviewed Apr 16, 2021
View reviewed changes
p4src/include/control/hasher.p4 Outdated Show resolved Hide resolved
p4src/include/control/hasher.p4 Show resolved Hide resolved
p4src/include/control/hasher.p4 Show resolved Hide resolved
@Yi-Tseng Yi-Tseng mentioned this pull request Apr 16, 2021
Closed
ccascone
ccascone reviewed Apr 19, 2021
View reviewed changes
p4src/include/control/hasher.p4 Outdated Show resolved Hide resolved
p4src/include/control/hasher.p4 Outdated Show resolved Hide resolved
p4src/include/control/hasher.p4 Outdated Show resolved Hide resolved
p4src/include/header.p4 Outdated Show resolved Hide resolved
ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
p4src/include/control/hasher.p4 Show resolved Hide resolved
@ccascone
Copy link
Member

It looks like we are hitting the 45m timeout in Jenkins: https://gerrit.onosproject.org/plugins/gitiles/ci-management/+/refs/heads/master/jjb/templates/fabric-tna-jobs.yaml#46

Execution time likely increased because of new SCTP packet type in #124 and the new ECMP tests in this PR (each one sending 50 pkts each). As a temporary workaround, we can disable testing the fabric and fabric-spgw profiles, since we only use fabric-int and fabric-spgw-int in Aether. As a long-term solution, I will open a GH issue about making PTF tests more efficient.

@osinstom osinstom force-pushed the ecmp-hashing-with-teid branch from 3bda971 to 40d6b24 Compare April 19, 2021 16:14
ccascone
ccascone approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@ccascone ccascone left a comment

Choose a reason for hiding this comment

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

LGTM. Left only minor comments.

ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
ptf/tests/ptf/fabric.ptf/test.py Outdated Show resolved Hide resolved
osinstom and others added 2 commits April 20, 2021 16:01
@ccascone
Copy link
Member

I would like @Yi-Tseng to approve as well before merging

@osinstom osinstom mentioned this pull request Apr 20, 2021
Merged
Yi-Tseng
Yi-Tseng reviewed Apr 20, 2021
View reviewed changes
ptf/tests/ptf/fabric.ptf/test.py
# teid_toport list is used to learn the teid that causes the packet
# to be forwarded for each port
teid_toport = [None, None]
for i in range(50):
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ccascone do you remember why we need to run 50 times?

Copy link
Member

@ccascone ccascone Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure. I believe the goal was to generate enough packet header combinations to hash on all buckets. This is consistent with other ECMP-related tests, but I agree it can be optimized. If you agree, please add this to #238, and unless you have other concerns I suggest we merge this change as-is.

Yi-Tseng
Yi-Tseng approved these changes Apr 20, 2021
View reviewed changes
Copy link
Collaborator

@Yi-Tseng Yi-Tseng left a comment

Choose a reason for hiding this comment

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

lgtm overall, just one minor comment about the test

@Yi-Tseng Yi-Tseng merged commit 1c8289d into stratum:main Apr 20, 2021
@Yi-Tseng Yi-Tseng mentioned this pull request Apr 20, 2021
Open
This was referenced Apr 24, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccascone ccascone ccascone approved these changes

@Yi-Tseng Yi-Tseng Yi-Tseng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@osinstom @ccascone @Yi-Tseng