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

Add bit indicating whether orig packet used VXLAN or VXLAN-GPE encap #58

Merged
merged 6 commits into from
Feb 12, 2020
Merged

Add bit indicating whether orig packet used VXLAN or VXLAN-GPE encap #58

merged 6 commits into from
Feb 12, 2020

Conversation

mickeyspiegel
Copy link
Contributor

Include guidelines suggesting that the sink should ignore this bit in
mixed v1.0 / v1.1 environments.

@mickeyspiegel
Add bit indicating whether orig packet used VXLAN or VXLAN-GPE encap
11176ff 
Include guidelines suggesting that the sink should ignore this bit in
mixed v1.0 / v1.1 environments.
@mickeyspiegel mickeyspiegel requested review from mhira1 and jklr January 17, 2019 23:28
mhira1
mhira1 reviewed Jan 18, 2019
View reviewed changes
Copy link
Contributor

@mhira1 mhira1 left a comment

Choose a reason for hiding this comment

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

Looks good, minor comment inline.

telemetry/specs/INT.mdk Outdated
* G: Indicates whether the original packet (before insertion of INT headers
and metadata) used a VXLAN or VXLAN GPE encapsulation.
- 0: Original packet used VXLAN encapsulation.
- 1: Original packet used VXLAN GPE encapsulation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering whether we should flip this and use 0 to indicate the original packet was VXLAN GPE, i.e. no conversion is required and 1 to indicate that original packet was VXLAN, i.e. yes conversion is required at INT sink. Thus INT sink does VXLAN GPE to VXLAN conversion if this bit is set to 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a very strong preference but using 1 to indicate the need for conversion seems a bit more indicative and useful for debugging. Again, no strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jklr
Copy link
Contributor

jklr commented Oct 28, 2019

@mickeyspiegel
This is a reminder of the AR from the May meeting: All text around 1.0/1.x interoperability needs to be modified as this is now going into version 2.0. Mickey to revise, Mukesh to review and merge.

mickeyspiegel
mickeyspiegel commented Jan 15, 2020
View reviewed changes
telemetry/specs/INT.mdk Outdated
- In an all v1.1 environment, both packets received with VXLAN encapsulation
and VXLAN GPE encapsulation could have INT inserted, with the INT sink
relying on the value of this bit to determine whether to carry out the
conversion from VXLAN GPE back to VXLAN.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is now aimed at the INT v2.0 spec, the two bullet points about v1.0, v1.1 and interop can just be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mickeyspiegel
Merge remote-tracking branch 'upstream/master' into vxlan_gpe_bit
c977449
@mickeyspiegel
Address comments
c9e1b42
@mickeyspiegel
Align VXLAN GPE format with VXLAN GPE shim header format
e98b238 
The VXLAN GPE draft defines a shim header format that is very similar
to the shim header format proposed for INT over VXLAN GPE. This commit
aligns the INT over VXLAN GPE shim header format with the VXLAN GPE
draft shim header by:
- Swapping the length and reserved bytes in the shim header, which
  also aligns with other INT shim header formats.
- Changing the length definition in the shim header to exclude the shim
  header itself.
- Changing the proposed VXLAN GPE Next Protocol field value for INT
  to 0x80, the first value in the range set aside for shim headers.
@mickeyspiegel
Minor formatting cleanup
878a927
@mickeyspiegel
Copy link
Contributor Author

Open Issues:

  1. Should we reduce the INT-Type field to 4 bits as JK recently proposed?
  2. Should we move the G bit to the Type byte, or leave it in the Reserved byte?

Note that the VXLAN GPE shim header format in draft-ietf-nvo3-vxlan-gpe-09 defines:

   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |      Type     |    Length     |    Reserved   | Next Protocol |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

where
Type: This field MAY be used to identify different messages of this protocol.
Reserved: The use of this field is reserved to the protocol defined in this message.

@jklr
Copy link
Contributor

jklr commented Jan 24, 2020

@mickeyspiegel as for the shim hdr format, I think the consensus from the last meeting is to follow the GPE convention, deviating from the INT convention.

@jklr jklr mentioned this pull request Jan 28, 2020
Merged
@jklr jklr merged commit dd0d2c9 into p4lang:master Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhira1 mhira1 mhira1 left review comments

@jklr jklr jklr 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
@mickeyspiegel @jklr @mhira1