openconfig / entity-naming

Libraries for mapping canonical names of OpenConfig entities to platform specific implementations
Apache License 2.0
2 stars 4 forks source link

Implementing Line Card Port function #33

Open nleiva opened 3 weeks ago

nleiva commented 3 weeks ago

Hi,

How do you envision implementing other Line Cards for a given vendor? Do you expand the Vendor Port function matching on HardwareModel?

Let's say I want to add support for other Juniper interfaces:

Then, I'd have to check HardwareModel before nameBuilder.WriteString("et-") and also change nameBuilder.WriteString("/0") to nameBuilder.WriteString(fmt.Sprintf("%d", pp.PICIndex)). Does that sound correct to you?

unc (n *Namer) Port(pp *namer.PortParams) (string, error) {
    if !pp.Channelizable {
        return "", fmt.Errorf("Juniper does not support unchannelizable ports")
    }

    var nameBuilder strings.Builder
    nameBuilder.WriteString("et-")
    if pp.SlotIndex == nil {
        nameBuilder.WriteString("0/")
        nameBuilder.WriteString(fmt.Sprintf("%d", pp.PICIndex))
    } else {
        nameBuilder.WriteString(fmt.Sprintf("%d", *pp.SlotIndex))
        nameBuilder.WriteString("/0")
    }
    nameBuilder.WriteString(fmt.Sprintf("/%d", pp.PortIndex))
    if pp.ChannelIndex != nil {
        nameBuilder.WriteString(fmt.Sprintf(":%d", *pp.ChannelIndex))
    }
    return nameBuilder.String(), nil
}

nleiva commented 2 weeks ago

Before I submit a PR for Juniper devices, how would something like this look to you:

  1. Add a file to store a LineCard to interface name mapping.
const (
    GE = "ge"
    ET = "et"
    TE = "te"
    XE = "xe"
)

var speedStrings = map[string]string{
    "MX304-750-122718": ET,
    "MX960-10X-1GE": GE,
    "MX960-20X10GE-SFPP": TE,
    "MX960-1X100GE-CFP": ET,
    "MX960-1X100GE-CFP2-OTN": ET,
    "MX960-12X10GE-SFPP-OTN": ET,
    "MX960-3X40GE-QSFPP": TE,
    "QFX10002-36Q-36X40G": ET,
    "QFX10002-72Q-72X40G": ET,
    "QFX3500": XE,
    "QFX3500-48S4Q": XE,
    "SRX340":  GE,
    "SRX340-8XGE8XGE-SFP-BASE-PIC": GE,
}
  1. Modify Juniper's Port function to check if we have a match for HardwareModel, otherwise use a default value (as is today). I also removed the current hardcoded PIC value (0).
// Port is an implementation of namer.Port.
func (n *Namer) Port(pp *namer.PortParams) (string, error) {
    if !pp.Channelizable {
        return "", fmt.Errorf("Juniper does not support unchannelizable ports")
    }
    speed := "et"
    ifsymb, ok := speedStrings[n.HardwareModel]
    if ok {
        speed = ifsymb
    }

    var nameBuilder strings.Builder
    nameBuilder.WriteString(speed + "-")
    if pp.SlotIndex == nil {
        nameBuilder.WriteString("0/")
    } else {
        nameBuilder.WriteString(fmt.Sprintf("%d/", *pp.SlotIndex))
    }
    nameBuilder.WriteString(fmt.Sprintf("%d", pp.PICIndex))
    nameBuilder.WriteString(fmt.Sprintf("/%d", pp.PortIndex))
    if pp.ChannelIndex != nil {
        nameBuilder.WriteString(fmt.Sprintf(":%d", *pp.ChannelIndex))
    }
    return nameBuilder.String(), nil
}

Thanks

lgomez9 commented 2 days ago

Hi Nicolas,

That looks like a good solution to me.

As a small nit, that I realize is not coming from you - we can also put together the namebuilder lines after the if statement:

nameBuilder.WriteString(fmt.Sprintf("%d/%d", pp.PICIndex, pp.PortIndex)
dplore commented 1 day ago

This is a bit of a tangent,but I think we should define what hardware_model in the entity-naming maps to in OpenConfig data models. My thought is it should map to /components/component/state/model-name

Further, the PORT component probably does not have a useful model-name. Rather one could use the component tree to resolve the parent of the port, recursing up to the first component of type LINECARD or CHASSIS to determine the model-name. I think we need some specification like this to make using the entity-naming library deterministic across multiple vendors. This could start out just as markdown documentation. It's not clear if the code for this should be in entity-naming or elsewhere, but I do think we should at least document this recommendation.

nleiva commented 5 hours ago

Hi Nicolas,

That looks like a good solution to me.

As a small nit, that I realize is not coming from you - we can also put together the namebuilder lines after the if statement:

nameBuilder.WriteString(fmt.Sprintf("%d/%d", pp.PICIndex, pp.PortIndex)

Thanks!

Yeah, I didn't modify the name-building pattern to keep the example as close as the current implementation.

nleiva commented 5 hours ago

This is a bit of a tangent,but I think we should define what hardware_model in the entity-naming maps to in OpenConfig data models. My thought is it should map to /components/component/state/model-name

Further, the PORT component probably does not have a useful model-name. Rather one could use the component tree to resolve the parent of the port, recursing up to the first component of type LINECARD or CHASSIS to determine the model-name. I think we need some specification like this to make using the entity-naming library deterministic across multiple vendors. This could start out just as markdown documentation. It's not clear if the code for this should be in entity-naming or elsewhere, but I do think we should at least document this recommendation.

This is a great point. I see the model also has an install-position they want to use instead of slot-id, which we could return based on this project's SlotIndex, PICIndex, and PortIndex.

// / openconfig-platform/components/component 
type Component struct {
    Chassis *Component_Chassis  `path:"chassis" module:"openconfig-platform"`
    InstallPosition *string `path:"state/install-position" module:"openconfig-platform/openconfig-platform"`
    Linecard    *Component_Linecard `path:"linecard" module:"openconfig-platform-linecard"`
    ModelName   *string `path:"state/model-name" module:"openconfig-platform/openconfig-platform"`
    Port    *Component_Port `path:"port" module:"openconfig-platform"`
}

type Component_Chassis struct {
    Utilization *Component_Chassis_Utilization  `path:"utilization" module:"openconfig-platform"`
}

type Component_Linecard struct {
    PowerAdminState E_PlatformTypes_ComponentPowerType  `path:"config/power-admin-state" module:"openconfig-platform-linecard/openconfig-platform-linecard"`
    SlotId  *string `path:"state/slot-id" module:"openconfig-platform-linecard/openconfig-platform-linecard"`
    Utilization *Component_Linecard_Utilization `path:"utilization" module:"openconfig-platform-linecard"`
}

type Component_Port struct {
}
dplore commented 4 hours ago

This is a bit of a tangent,but I think we should define what hardware_model in the entity-naming maps to in OpenConfig data models. My thought is it should map to /components/component/state/model-name Further, the PORT component probably does not have a useful model-name. Rather one could use the component tree to resolve the parent of the port, recursing up to the first component of type LINECARD or CHASSIS to determine the model-name. I think we need some specification like this to make using the entity-naming library deterministic across multiple vendors. This could start out just as markdown documentation. It's not clear if the code for this should be in entity-naming or elsewhere, but I do think we should at least document this recommendation.

This is a great point. I see the model also has an install-position they want to use instead of slot-id we might want to return based on this project's SlotIndex, PICIndex, and PortIndex.

Yes, install-position is a very recent addition which deprecates slot-id and generalizes the use case for resolving the physical location where a component is installed in the hardware / component tree. I expect to see support for install-position appear in implementations over the next 6-18 months. Since slot-id still exists (deprecated paths are expected to be supported until they are actually removed from the model) we should be flexible and use it when install-position is not available.