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

Update description for forwarding-viable #1032

Merged
merged 27 commits into from
Feb 22, 2024
Merged

Conversation

sachendras
Copy link
Contributor

@sachendras sachendras commented Jan 5, 2024

Add more clarity/rules for the forwarding-viable leaf.

  1. When a controller "voluntarily" changes the state of all member-links on a LAG bundle or the bundle itself to forwarding-viable=False, it expects that the port/bundle is not used to transmit data-plane traffic even when that is the port of last resort.

a. This requirement can be easily be achieved by an implementation if they follow the rule that, when a port/bundle is set to forwarding-viable=false, do not transmit L3 packets. This is true for both control-plane or data-plane packets. As a result, it would be a graceful transition for L3 traffic on that connection because, the other end of the connection will have its L3 protocol's dead-interval/hold-down timer expired leading for it to migrate away from the connection eventually.

  1. I feel minimum-link calculation for aggregate bundle should be ignored for member ports whose forwarding-viable=false. Minimum-Links add unnecessary complications on the controller end. Also, that will result in an implementation to disable a bundle if the min-links requirement is not met and that is not the expectation. Expectation is that, the implementation must continue to Receive and Transmit L2 and only Receive L3 packets on such ports.

  2. Finally, in order to remove confusions around IS-IS, the pull calls out that IS-IS should be treated as L3 protocols from this leaf's perspective.

More details in go/forwarding_viable_expected_behavior

Add more clarity/rules for the forwarding-viable leaf and how an implementation should implement it.
@sachendras sachendras requested a review from a team as a code owner January 5, 2024 19:22
@OpenConfigBot
Copy link

OpenConfigBot commented Jan 5, 2024

No major YANG version changes in commit 856d272

@xw-g xw-g requested a review from rszarecki January 8, 2024 20:33
@dplore dplore changed the title Update openconfig-if-sdn-ext.yang Update description for forwarding-viable Jan 16, 2024
sachendras and others added 2 commits January 17, 2024 14:22
Co-authored-by: rszarecki <46606165+rszarecki@users.noreply.github.com>
Comment on lines 63 to 67
3. L3 packets, control-plane as well as data-plane must be received
but not transmitted on such ports/bundles. By doing so, expectation
is that the other end of the connection ceases to use its respective
ports for transmit and receive when either the dead-timer or the
hold-down timer of its L3 protocol expires.
Copy link
Member

Choose a reason for hiding this comment

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

We should define what the expectation is for received L3 packets. I would guess the device is supposed to forward L3 packets, right? If yes, then stating something like the following might make this more clear?

  1. Received L3 packets should be evaluated by the data plane as usual. For example, when a dataplane packet is received, a lookup should be performed and the packet forwarded as usual. If an L3 control plane packet is received, it should be processed as usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated as per the internal doc, ptal.

Choose a reason for hiding this comment

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

Hi Sach, this statement appears tailored for a protocol like IS-IS. The peer does not have to stop sending traffic to this device on the non-viable ports based on a timeout even if all the trunk members are not-viable. So, can we modify the statement to talk only about DUT and not enforce expectations on the peer? Specifically,

  1. Stop L3 transmit for both control and data-plane packets.
  2. L3 forwarding should work as usual on Rx without any timeout considerations. The peer can choose to continue sending this packets here indefinitely.

If you prefer, you can call out IS-IS as an example, where the timeout will happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nachikethas I have updated rule#3 to include other use cases, ptal

sachendras and others added 2 commits February 12, 2024 08:12
Co-authored-by: Darren Loher <dloher@google.com>
Co-authored-by: Darren Loher <dloher@google.com>
dplore
dplore previously approved these changes Feb 15, 2024
Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

The text makes sense to me as is. I've suggested some formatting and style changes which I hope makes it easier to read. PTAL.

@dplore
Copy link
Member

dplore commented Feb 15, 2024

/gcbrun

Co-authored-by: Darren Loher <dloher@google.com>
@sachendras
Copy link
Contributor Author

/gcbrun

@sachendras
Copy link
Contributor Author

/gcbrun

1 similar comment
@dplore
Copy link
Member

dplore commented Feb 21, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Feb 21, 2024

/gcbrun

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

LGTM! @nachikethas can you please give a final review?

@dplore dplore merged commit fca6245 into openconfig:master Feb 22, 2024
14 checks passed
romeyod pushed a commit to romeyod/aftNextHop that referenced this pull request Sep 19, 2024
…it. (openconfig#1032)

* Update openconfig-if-sdn-ext.yang

Clarify the forwarding-viable leaf and how an implementation should implement it.

---------

Co-authored-by: rszarecki <46606165+rszarecki@users.noreply.github.com>
Co-authored-by: Darren Loher <dloher@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants