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

Refactor Control API #305

Merged
merged 24 commits into from
Mar 6, 2023
Merged

Refactor Control API #305

merged 24 commits into from
Mar 6, 2023

Conversation

ashutshkumr
Copy link
Contributor

@ashutshkumr ashutshkumr commented Feb 27, 2023

Check documentation here.

What has changed (and why) ?

  • Refactored all control APIs into two major categories - control state and control action
    • Previously we had too many API endpoints for controlling states/actions - resulting in different function calls at SDK level for slightly related operations
    • There was no clear distinction between intent of exposed APIs - i.e. lack of organization
    • We know already that there are lots more to come, and hence it helps to keep the top level API less polluted/noisy
  • Grouped capture, states and metrics under one category called Monitor.
    • Its meaning resembles that of telemetry, but encompasses things like captured packets which traditionally doesn't fit the openconfig's interpretation of telemetry.
    • result is no longer an appropriate terminology because they more closely imply "one-off output" instead of "ever changing output".

What's pending ?

  • Need to discuss if we should expose warnings as an additional output for metrics/states. As of today, there's no way to return warning in those scenarios.
  • Changes due to error handling UX improvement

Backwards Compatibility

  • No breakage in existing tests (existing API and schemas used by it are deprecated)
  • snappi version used by tests will still need to be updated because internally URLs have changed from /results/* -> /monitor/*
  • When deprecated API is removed, breakage shall be on a function call-level, and hence shall only require changes in otg helper library

Examples

  • Before

     api := gosnappi.NewApi()
    
     api.SetConfig(gosnappi.NewConfig())
    
     // start all protocols
     ps := gosnappi.NewProtocolState()
     ps.SetState(gosnappi.ProtocolStateState.START)
     api.SetProtocolState(ps)
    
     // start traffic
     ts := gosnappi.NewTransmitState()
     ts.SetState(gosnappi.TransmitStateState.START)
     api.SetTransmitState(ts)
    
     // withdraw all routes
     rs := gosnappi.NewRouteState()
     rs.SetState(gosnappi.RouteStateState.WITHDRAW)
     api.SetRouteState(rs)
    
     // set lacp admin state on all LAG members
     la := gosnappi.NewDeviceState()
     la.LacpMemberState().SetState(gosnappi.LacpMemberStateState.UP)
     api.SetDeviceState(la)
    
     // send IPv4 ping and check responses
     pr := gosnappi.NewPingRequest()
     req := pr.Endpoints()
     req.Add().Ipv4().SetSrcName("ip1").SetDstIp("1.1.1.1")
     req.Add().Ipv4().SetSrcName("ip2").SetDstIp("1.1.1.2")
     res, _ := api.SendPing(pr)
     // ceck ping response
     fmt.Println(res.Warnings())
     fmt.Println(res.Responses())
  • After

     api := gosnappi.NewApi()
    
     api.SetConfig(gosnappi.NewConfig())
    
     // start all protocols
     ps := gosnappi.NewControlState()
     ps.Protocol().All().SetState(gosnappi.StateProtocolAllState.START)
     api.SetControlState(ps)
    
     // start traffic
     ts := gosnappi.NewControlState()
     ts.Traffic().FlowTransmit().SetState(gosnappi.StateTrafficFlowTransmitState.START)
     api.SetControlState(ts)
    
     // withdraw all routes
     rs := gosnappi.NewControlState()
     rs.Protocol().Route().SetState(gosnappi.StateProtocolRouteState.WITHDRAW)
     api.SetControlState(rs)
    
     // set lacp admin state on all LAG members
     la := gosnappi.NewControlState()
     la.Protocol().Lacp().Admin().SetState(gosnappi.StateProtocolLacpAdminState.UP)
         api.SetControlState(la)
    
     // send IPv4 ping and check responses
     pr := gosnappi.NewControlAction()
     req := pr.Protocol().Ipv4().Ping().Requests()
     req.Add().SetSrcName("ip1").SetDstIp("1.1.1.1")
     req.Add().SetSrcName("ip2").SetDstIp("1.1.1.2")
     res, _ := api.SetControlAction(pr)
     // ceck ping response
     fmt.Println(res.Warnings())
     fmt.Println(res.Response().Protocol().Ipv4().Ping().Responses())

@ashutshkumr ashutshkumr mentioned this pull request Feb 27, 2023
api/api.yaml Outdated Show resolved Hide resolved
control/state.yaml Outdated Show resolved Hide resolved
api/api.yaml Show resolved Hide resolved
@ashutshkumr
Copy link
Contributor Author

I've added a description for what is the configuration of ping request used (e.g. ping packet count, timeout value, etc.), but unfortunately it's not showing up in redocly.

Kindly review the YAML for same.

Copy link
Contributor

@apratimmukherjee apratimmukherjee left a comment

Choose a reason for hiding this comment

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

Couple of new comments. Rest looks good.

control/readme.md Outdated Show resolved Hide resolved
control/readme.md Outdated Show resolved Hide resolved
control/ipv4.yaml Outdated Show resolved Hide resolved
Request for initiating ping between a single source and destination pair.

For ping request, 1 IPv4 ICMP Echo Request shall be sent and wait for ping response to
either succeed or time out. The API wait timeout for each request shall be 300ms.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we pick 300ms?

Also, the description doesn't specify if the pings are sent serially or in parallel in case more than one are initiated in the request. Is it up to the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value 300ms was arbitrarily picked as currently we're not allowing users to pick any other ping parameters. There's only one ping packet sent as per the description (instead of multiple packages, which would entail description about whether it's serial or concurrent).

I have also marked this schema with x-status: under-review as the parameters to be provided for ping request is still TBD. Ideally the parameters needn't be mentioned in schema description.

control/ipv4.yaml Outdated Show resolved Hide resolved
control/ipv6.yaml Outdated Show resolved Hide resolved
control/ipv6.yaml Outdated Show resolved Hide resolved
control/ipv6.yaml Show resolved Hide resolved
control/ipv6.yaml Outdated Show resolved Hide resolved
description: >-
Sets all configured protocols to `start` or `stop` state.

Setting protocol state to `start` shall be a no-op if preceding `set_config` API call
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it also result in a warning?

Copy link
Contributor Author

@ashutshkumr ashutshkumr Mar 6, 2023

Choose a reason for hiding this comment

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

Indeed, I've opened a separate issue to document warnings in other places too (including this one).

#309

properties:
choice:
type: string
x-enum:
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we have any other option besides admin in the future? I mean do we need the choice here or can we just expose the properties here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could have session state which is different from admin state. In general, we'll end up having a separate schema object for every protocol which in-turn shall encapsulate different kinds of operational states.

@ashutshkumr ashutshkumr merged commit 04c748e into master Mar 6, 2023
@ashutshkumr ashutshkumr deleted the control-api-refactor branch March 6, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants