-
Notifications
You must be signed in to change notification settings - Fork 78
Add set_mtu to packet_link_qualification #297
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
base: main
Are you sure you want to change the base?
Conversation
To avoid issues where the L2 interface MTU may be smaller than the packet_size requested for a packet link qualification test, a new field is added to ensure the interface mtu is set to the packet_size. Instead of updating the description of the current packet_size field which would be a breaking change for plaforms that do not already do this, a new field is introduced. For the reflector (asic loopback mode), a packet_size field is also added for the same purpose.
Pull Request Test Coverage Report for Build 16943467466Details
💛 - Coveralls |
LimeHat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate the spirit of this PR, I'm concerned about the fact that it introduces yet another mechanism of config (or state) changes without clear rules.
- how this operation is supposed to work?
- does it update the config (which, for example, is reflected in gnmi)?
- does it update the state?
- what are the rules of interaction with bootz datastores/namespaces (boot specifically, which is supposedly "immutable")?
- there's also gSII..
- what happens if a control module switchover occurs during the test?
and so on and so forth..
I'd suggest leaving interface config aspects to the config management system.
|
|
||
| // If true, the Layer 2 MTU of the underlying interface should be set to the | ||
| // packet size for the duration of the test. | ||
| // If true, the gnoi service must ensure the L2 MTU of the interface is set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gnoi service == device.
Checking what is programmed on dataplane/port and programming port on ASIC if needed.
Hi, thanks for the feedback. This PR is intentionally provoking this kind of feedback. 😄 Here's the operational use case: The operational goal of link qualification is that it can be used as a 'one shot' RPC action that takes care of everything needed to perform the link qualification. This includes taking the interface "out of service", doing the setup, generation and restoration of service. Packet-link-qual is an ephemeral state of operational intent. Operationally we want to avoid "temporary" configuration state (1). So a few options are:
Options 1 and 2 seem viable and don't violate the goal of avoiding 'temporary, ephemeral configuration state'. Option 3 violates the operational goal. More feedback from operators and vendors is appreciated. :) (1) gSII proposes a generalized way to formalize temporary or ephemeral configuration. |
|
Ah, this also involves LAG.. which makes it even more interesting and vendor-specific! :-)
... in some implementations. Have you tried Nokia? :-)
To keep it vendor-neutral and help you (and the vendor in question) achieve the operational goal, may I suggest the following:
|
rszarecki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add min_mtu, max_mtu to message AsicLoopbackCapabilities.
We shall know what system is capable for, befor requesting any configuration.
| // This is where any l2/l3/l4 match criteria would be described. | ||
| // The L2 MTU of the interface must be set to the packet_size for the | ||
| // duration of the test. | ||
| uint32 packet_size = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, can we also have 'set-MTU' Boolean?
It will be bit redundant but IMHO more intuitive.
Also it is optional file,right? Can we declare default 1500B?
|
|
||
| // If true, the Layer 2 MTU of the underlying interface should be set to the | ||
| // packet size for the duration of the test. | ||
| // If true, the gnoi service must ensure the L2 MTU of the interface is set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gnoi service == device.
Checking what is programmed on dataplane/port and programming port on ASIC if needed.
To avoid issues where the L2 interface MTU may be smaller than the packet_size requested for a packet link qualification test, a new field is added to ensure the interface mtu is set to the packet_size.
Instead of updating the description of the current packet_size field which would be a breaking change for plaforms that do not already do this, a new field is introduced.
For the reflector (asic loopback mode), a packet_size field is also added for the same purpose.