Refactor control API #255

ashutshkumr · 2022-07-21T12:01:06Z

Check documentation here.

What has changed (and why) ?

Refactored all control APIs into three major categories - control state, control action and control patch
- 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 ?

result keyword is mentioned in a lot of places, and hence would like to replace them (only in select places) if we're ok with new terminology - aka monitor
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.

Backwards Compatibility

This change essentially breaks backwards compatibility because it will affect all control APIs, deprecating which might take longer than expected and might further add to the confusion (e.g. we'll end up having two ways of doing too many things for a long time)
From SDK perspective, breakage shall be on a function call-level, and hence shall only require changes in otg helper library

Examples

Before

 api := gosnappi.NewApi()

 c := api.NewConfig()

 // set config
 api.SetConfig(c)

 // start protocols
 ps := gosnappi.NewProtocolState()
 ps.SetState(gosnappi.ProtocolStateState.START)
 api.SetProtocolState(ps)

 // wait for protocol metrics to be ok ...

 // start transmit
 ts := gosnappi.NewTransmitState()
 ts.SetState(gosnappi.TransmitStateState.START)
 api.SetTransmitState(ts)

 // wait for flow metrics to be ok ...

 // withdraw some routes
 rs := gosnappi.NewRouteState()
 rs.SetNames([]string{"rr1"})
 rs.SetState(gosnappi.RouteStateState.WITHDRAW)
 api.SetRouteState(rs)

After

 api := gosnappi.NewApi()

 c := api.NewConfig()

 // set config
 api.SetConfig(c)

 // start protocols
 ps := gosnappi.NewControlState()
 ps.Protocol().SetState(gosnappi.ProtocolStateState.START)
 api.SetControlState(ps)

 // wait for protocol metrics to be ok ...

 // start transmit
 ts := gosnappi.NewControlState()
 ts.FlowTransmit().SetState(gosnappi.TransmitStateState.START)
 api.SetControlState(ts)

 // wait for flow metrics to be ok ...

 // withdraw some routes
 rs := gosnappi.NewControlState()
 rs.Route().
 	SetNames([]string{"rr1"}).
 	SetState(gosnappi.RouteStateState.WITHDRAW)
 api.SetControlState(rs)

…nerator/models into control-refactor

control/patch.yaml

control/state.yaml

winstonliu1111

can we update the modeling guide as well so it is clear what should go into state/action/patch ??

winstonliu1111 · 2022-08-01T18:10:23Z

control/patch.yaml

+  schemas:
+    Control.Patch:
+      description: >-
+        Request for triggering action on resources in traffic generator


description needs change? this is not triggering action on resources?

agreed, changed.

SuryyaKrJana

Changes look ok to me.

anjan-keysight · 2022-08-02T05:21:31Z

Looks good to me.

api/api.yaml

apratimmukherjee · 2022-08-03T10:32:37Z

control/lacp.yaml

  version: ^0.0.0

 components:
  schemas:
-    LacpMember.State:
+    Lacp.State:


Wondering if every protocol should also have a set of choices in the heirarchy , since in future we might need to add additional triggers in any protocol (e.g. LACP ) based on end-user requirements. Or we can also choose to have separate choices at top level when the name should probably remain LacpMember to distinguish against future additional triggers,

Per the most recent discussion, every protocol shall have its own set of choices, if the need arises to add more triggers for a given protocol.

We probably should avoid adding top level choices unless it's a separate protocol altogether. Let me know if you plan to suggest a change here.

Dont we need to then add a choice under Lacp , with a single choice available now ?

We should make it a choice under LACP from the beginning, to make it explicit. Also, new choices might have a different set of arguments to trigger the event.

@apratimmukherjee @PrasenjitAdhikary kindly review again - I've made the suggested changes.

apratimmukherjee · 2022-08-03T11:59:13Z

control/patch.yaml

+            The list of configured flows for which given property will be updated.
+          type: array
+          items:
+            $ref: '../flow/flow.yaml#/components/schemas/Flow'


Though this is not related to this PR , I could not follow the exact semantics for patch_config . Lets assume at one time we can update only 1 resource e.g flows. We have 5 flows. Since property names is an array , I assume this means the same set of properties will be updated for all specified flows (e.g. only rate or only size or both i.e. array size possibly cant be more than 2 and values should not be duplicate for properties array. After this , I got confused since looks to me that user has to specify entire flow once again . Should user not be allowed to update only rate or size per flow (specified by flowname) and other properties should be hidden in this API . And validation should be done that specified sub-fields match fields chosen in 'property' array. i.e. should the updatable properties have a dedicated different structure such as flow_patch_attribs etc. Or did I miss the intent of the patch API for flows.

The reason there's no dedicated data structure for updating flow properties like size and rate is because from SDK UX POV, it's easier for users to work with.

c = api.config() f1 = c.flows.add(name='f1') f1.size.fixed = 256 f1.rate.pps = 500 f2 = c.flows.add(name='f2') f2.size.fixed = 256 f2.rate.pps = 500 # set config for the first time api.set_config(c) # update rate for flow1 f1.rate.pps = 1000 cp = api.control_patch() cp.flows.property_names = ["rate"] cp.flows.flows = [f1] api.set_control_patch(cp) # fetch newly updated config cpatched = api.get_config() # fetched config shall be equivalent to what user already has assert c == cpatched

It looks like we should probably be explicit that only the values set in specified fields ( one of rate or size for flows ) will be patched with current config only for specified flows. The values of all remaining fields in the provided flows will be ignored ( or validated that no other fields are changed ?) . I assume if no changes are done for corresponding attribute , then no separate warning is expected to be provided.

ajbalogh

lgtm

PrasenjitAdhikary · 2022-08-09T07:36:58Z

control/patch.yaml

+
+components:
+  schemas:
+    Control.Patch:


Unlike any other control API, this action changes the configuration (which will also reflect in get_config). Need to reflect that in the description, to avoid any ambiguity.
On second thought, if the API should be moved under the configuration group - and renamed to update_config - since the other two APIs in the group are also related to configuration and it will be obvious that the configuration would be modified and reflected in get_config?

apratimmukherjee

Looks good to me.

PrasenjitAdhikary

Looks good to me.

apratimmukherjee · 2022-08-23T12:48:10Z

control/lacp.yaml

          type: array
          items:
            type: string
          x-constraint:
          - '/components/schemas/Port/properties/name'
        state:
          description: >-
-            The LACP Member admin state.
+            The LACP member admin state.
            'up' will start transmission of LACPDUs on selected member ports.


I reviewed the descriptions with Subrata/Sourav since I was under the impression that this is no longer accurate.
Can you please change the 'up' and 'down' description to below ( as received from Subrata ) which is more accurate than what was added initially:
'up' will turn on ‘sync’ flag of LACPDUs transmitted from selected member ports.
'down' will turn off ‘sync’ flag of LACPDUs transmitted from selected member ports.

ashutshkumr · 2023-02-27T09:06:18Z

Replaced by #305

ashutshkumr and others added 7 commits July 21, 2022 17:28

init refactor for control API

e4b795a

Update auto generated content

03a532e

results -> monitor

9985fb9

Merge branch 'control-refactor' of https://github.com/open-traffic-ge…

10c1849

…nerator/models into control-refactor

Update auto generated content

eb1362b

set -> control

ff55796

Merge branch 'control-refactor' of https://github.com/open-traffic-ge…

b315f71

…nerator/models into control-refactor

ashutshkumr marked this pull request as ready for review July 25, 2022 10:21

ashutshkumr requested review from PrasenjitAdhikary, ANISH-GOTTAPU, ajbalogh, alakendu, anjan-keysight, ankur-sheth, apratimmukherjee, Rangababu-R, winstonliu1111 and rudranil-das July 25, 2022 10:21

actions-user and others added 3 commits July 25, 2022 10:22

Update auto generated content

b3b899d

control_state -> set_control_state

4c6c19e

Update auto generated content

b213ca7

ashutshkumr changed the title ~~Refactor for control API~~ Refactor control API Jul 25, 2022

ashutshkumr mentioned this pull request Jul 25, 2022

Dev dev api consolidate #248

Closed

ashutshkumr requested a review from SuryyaKrJana July 25, 2022 11:07

ankur-sheth reviewed Aug 1, 2022

View reviewed changes

control/patch.yaml Outdated Show resolved Hide resolved

ankur-sheth reviewed Aug 1, 2022

View reviewed changes

control/state.yaml Outdated Show resolved Hide resolved

control/state.yaml Show resolved Hide resolved

winstonliu1111 requested changes Aug 1, 2022

View reviewed changes

SuryyaKrJana approved these changes Aug 2, 2022

View reviewed changes

anjan-keysight closed this Aug 2, 2022

ashutshkumr reopened this Aug 2, 2022

apratimmukherjee reviewed Aug 3, 2022

View reviewed changes

addressed review comments

686dde1

ajbalogh approved these changes Aug 8, 2022

View reviewed changes

PrasenjitAdhikary reviewed Aug 9, 2022

View reviewed changes

ashutshkumr added 5 commits August 22, 2022 11:58

introduced new hierarchy for lacp states

ea0b976

relocated config path from control to config

ed0900b

relocated config path from control to config

bd378ce

patch -> updatre

b275805

push generated artifacts

e4acfaf

apratimmukherjee approved these changes Aug 23, 2022

View reviewed changes

PrasenjitAdhikary approved these changes Aug 23, 2022

View reviewed changes

apratimmukherjee reviewed Aug 23, 2022

View reviewed changes

ashutshkumr mentioned this pull request Sep 26, 2022

Behavior of SetConfig and SetProtocolState needs to be clearly defined #270

Closed

ashutshkumr mentioned this pull request Nov 11, 2022

Refactor Control API #283

Closed

ankur-sheth mentioned this pull request Nov 11, 2022

Consolidate related control plane state/actions into set_device_state api #245

Closed

ashutshkumr closed this Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor control API #255

Refactor control API #255

ashutshkumr commented Jul 21, 2022 •

edited

Loading

winstonliu1111 left a comment

winstonliu1111 Aug 1, 2022

ashutshkumr Aug 8, 2022

SuryyaKrJana left a comment

anjan-keysight commented Aug 2, 2022

apratimmukherjee Aug 3, 2022

ashutshkumr Aug 8, 2022

apratimmukherjee Aug 8, 2022

PrasenjitAdhikary Aug 9, 2022

ashutshkumr Aug 22, 2022

apratimmukherjee Aug 3, 2022

ashutshkumr Aug 8, 2022

apratimmukherjee Aug 8, 2022 •

edited

Loading

ajbalogh left a comment

PrasenjitAdhikary Aug 9, 2022

apratimmukherjee left a comment

PrasenjitAdhikary left a comment

apratimmukherjee Aug 23, 2022

ashutshkumr commented Feb 27, 2023

Refactor control API #255

Refactor control API #255

Conversation

ashutshkumr commented Jul 21, 2022 • edited Loading

What has changed (and why) ?

What's pending ?

Backwards Compatibility

Examples

winstonliu1111 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuryyaKrJana left a comment

Choose a reason for hiding this comment

anjan-keysight commented Aug 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apratimmukherjee Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

ajbalogh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apratimmukherjee left a comment

Choose a reason for hiding this comment

PrasenjitAdhikary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutshkumr commented Feb 27, 2023

ashutshkumr commented Jul 21, 2022 •

edited

Loading

apratimmukherjee Aug 8, 2022 •

edited

Loading