-
Notifications
You must be signed in to change notification settings - Fork 147
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
Cisco transceiver 3 #3457
Cisco transceiver 3 #3457
Conversation
Pull Request Test Coverage Report for Build 11071143372Details
💛 - Coveralls |
feature/platform/transceiver/tests/zr_firmware_version_test/zr_firmware_version_test.go
Outdated
Show resolved
Hide resolved
feature/platform/transceiver/tests/zr_firmware_version_test/zr_firmware_version_test.go
Outdated
Show resolved
Hide resolved
feature/platform/transceiver/tests/zr_firmware_version_test/zr_firmware_version_test.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
// ConfigETHChannel configures the ETH channel. | ||
func ConfigETHChannel(t *testing.T, dut *ondatra.DUTDevice, interfaceName, transceiverName string, otnIndex, ethIndex uint32) { | ||
t.Helper() | ||
gnmi.Replace(t, dut, gnmi.OC().TerminalDevice().Channel(ethIndex).Config(), &oc.TerminalDevice_Channel{ |
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.
Would you mind if we write this slightly differently to reduce some repeated lines?
Something like:
channnel := &oc.TerminalDevice_Channel{
Description: ygot.String("ETH Logical Channel"),
Index: ygot.Uint32(ethIndex),
LogicalChannelType: oc.TransportTypes_LOGICAL_ELEMENT_PROTOCOL_TYPE_PROT_ETHERNET,
TribProtocol: oc.TransportTypes_TRIBUTARY_PROTOCOL_TYPE_PROT_400GE,
Ingress: &oc.TerminalDevice_Channel_Ingress{
Interface: ygot.String(interfaceName),
Transceiver: ygot.String(transceiverName),
},
Assignment: map[uint32]*oc.TerminalDevice_Channel_Assignment{
0: {
Index: ygot.Uint32(0),
LogicalChannel: ygot.Uint32(otnIndex),
Description: ygot.String("ETH to OTN"),
Allocation: ygot.Float64(400),
AssignmentType: oc.Assignment_AssignmentType_LOGICAL_CHANNEL,
},
},
}
if !deviations.ETHChannelIngressParametersUnsupported(dut) {
channnel.Ingress = &oc.TerminalDevice_Channel_Ingress{
Interface: ygot.String(interfaceName),
Transceiver: ygot.String(transceiverName),
}
}
gnmi.Replace(t, dut, gnmi.OC().TerminalDevice().Channel(ethIndex).Config(), channnel)
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.
Hi @ejbrever, thank you for your review. I incorporated above suggestions.
Regarding this block, there are 3 differences in Cisco's ZR implementation:
- Cisco channel indexing starts from 1 instead of 0
- RateClass parameter = oc.TransportTypes_TRIBUTARY_RATE_CLASS_TYPE_TRIB_RATE_400G is required
- Ingress parameters are not supported.
3 deviations for a single usecase might be an overkill, so I put them under the above code block for readability.
Could you please suggest how we might implement this?
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 the rate class, can you please add this as the default in both cases. I think that should always actually be there, so then for that item we don't have to worry about the diff.
The other two I do think are unique deviations IMHO. Can you create deviations for those two please?
As a variant of the initial restructure then, maybe we could have something like:
var ingress &oc.TerminalDevice_Channel_Ingress
if deviations.ETHChannelIngressParametersUnsupported(dut) {
ingress = <...>
}
assignments := <default>
if deviations.ETHChannelAssignmentCiscoNumbering(dut) {
assignments = <...>
}
channnel := &oc.TerminalDevice_Channel{
Description: ygot.String("ETH Logical Channel"),
Index: ygot.Uint32(ethIndex),
LogicalChannelType: oc.TransportTypes_LOGICAL_ELEMENT_PROTOCOL_TYPE_PROT_ETHERNET,
TribProtocol: oc.TransportTypes_TRIBUTARY_PROTOCOL_TYPE_PROT_400GE,
Ingress: ingress,
Assignment: assignments,
}
gnmi.Replace(t, dut, gnmi.OC().TerminalDevice().Channel(ethIndex).Config(), channnel)
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.
ack - implemented something similar to your suggestion. PTAL.
Added an if-else block for the deviation for readability.
} | ||
|
||
// ConfigETHChannel configures the ETH channel. | ||
func ConfigETHChannel(t *testing.T, dut *ondatra.DUTDevice, interfaceName, transceiverName string, otnIndex, ethIndex uint32) { | ||
t.Helper() | ||
gnmi.Replace(t, dut, gnmi.OC().TerminalDevice().Channel(ethIndex).Config(), &oc.TerminalDevice_Channel{ |
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 the rate class, can you please add this as the default in both cases. I think that should always actually be there, so then for that item we don't have to worry about the diff.
The other two I do think are unique deviations IMHO. Can you create deviations for those two please?
As a variant of the initial restructure then, maybe we could have something like:
var ingress &oc.TerminalDevice_Channel_Ingress
if deviations.ETHChannelIngressParametersUnsupported(dut) {
ingress = <...>
}
assignments := <default>
if deviations.ETHChannelAssignmentCiscoNumbering(dut) {
assignments = <...>
}
channnel := &oc.TerminalDevice_Channel{
Description: ygot.String("ETH Logical Channel"),
Index: ygot.Uint32(ethIndex),
LogicalChannelType: oc.TransportTypes_LOGICAL_ELEMENT_PROTOCOL_TYPE_PROT_ETHERNET,
TribProtocol: oc.TransportTypes_TRIBUTARY_PROTOCOL_TYPE_PROT_400GE,
Ingress: ingress,
Assignment: assignments,
}
gnmi.Replace(t, dut, gnmi.OC().TerminalDevice().Channel(ethIndex).Config(), channnel)
Changes for the tests: