First cut of Diag gNOI micro service #9

aghaffarkhah · 2018-05-22T02:58:03Z

This PR defines a new gNOI micro service called Diag to perform diagnostic operations on a
switching box. At the moment the service provides APIs for BERT operation only. BURNIN will be added later.

The APIs defined here are meant o deprecate the BERT-related APIs defined in gnoi/layer2/layer2.proto. The new APIs are more extensive than the old ones and match use cases in a real prod environment.

ejbrever · 2018-05-22T17:42:01Z

diag/diag.proto

+//    operations on a set of ports.
+// 2- BURNING related RPCs: Used to perform a vendor-provided Burnin test on the
+//    switch to ensure the box is ready to start serving traffic. Burnin tests
+//    are typically run by HwOps in the datacenter, as part of turnup or repair


I think we should remove this sentence about HwOps as this is Google specific. Different companies will do things differently.

Done. Thanks for noticing.

ejbrever · 2018-05-22T17:46:43Z

diag/diag.proto

+//    switch to ensure the box is ready to start serving traffic. Burnin tests
+//    are typically run by HwOps in the datacenter, as part of turnup or repair
+//    workflow.
+service Diag {


We should probably note that these are only for stateless diagnostics activities (in other words, these activities will all end on their own). None of these should hold any state on the device. Stateful configuration should use the OpenConfig model with GNMI. I only mention this as other vendors support a stateful internal BERT and I am hoping we can expand the current OpenConfig models to fully support this. There is a test-signal boolean leaf defined at the moment though.

Sure these are stateless operations and them failing/passing should not have a permanent artifact on the switch (unless there is something wrong HW-wise). Added a point on that to the comments.

However I am also interested to learn more about the statefull BERT you mentioned here. What is it and how/when it is used? Any pointers?

Here is a pointer to the leaf:
https://github.com/openconfig/public/blob/e8981a183157a5f730f3d054da63f9314f43efb4/release/models/optical-transport/openconfig-terminal-device.yang#L754

Essentially it is very similar, but the devices support permanently turning on a test signal which sends PRBS from the port. This is useful during capacity adds and sometimes troubleshooting to see if a link or portion of a link is error free. The optical vendors I have seen just do this as stateful config instead of running for specified time.

ejbrever · 2018-05-22T17:55:43Z

diag/diag.proto

+  // - When BERT is already in progress on any port specified by the request.
+  // - In case of any low-level HW/SW internal errors.
+  // The RPC returns an OK status of none of these situations is encountered.
+  rpc StartBERT(StartBERTRequest) returns(StartBERTResponse) {}


Curious on your thoughts, but in another PR, I am thinking we should make sure there is an OpenConfig state leaf to describe that an interface is being used for testing. Since this is a service affecting activity, we would want a way for streaming telemetry to indicate that even though the interface may be UP/UP, it is currently under test.

oh definitely agree with that. This was actually in our todo. Since BERT operation are long lived operations, the oper state of an interface MUST be able to capture this.

Internally we actually do have a specific state for a port when it is in so called "diag mode". We can definitely extend gnmi to capture that as well.

We actually have a TESTING state in /interfaces/interface/state/oper-status already. Would this be OK for this use case?

For optical devices, I think we also need this in the terminal-device/logical channels. We can expand this enum possibly:
https://github.com/openconfig/public/blob/e8981a183157a5f730f3d054da63f9314f43efb4/release/models/optical-transport/openconfig-terminal-device.yang#L767

Thanks, for our use case TESTING state should be enough I think.

aghaffarkhah · 2018-05-22T19:07:45Z

Addressed the comment. Thanks for the review.

ejbrever

Thanks for making the changes. LGTM, but please wait for Rob/Anees to review.

robshakir · 2018-05-24T02:29:59Z

diag/diag.proto

+// The Diag service exports to main set of RPCs:
+// 1- BERT related RPCs: Used to perform Bit Error Rate Test (BERT)
+//    operations on a set of ports.
+// 2- BURNING related RPCs: Used to perform a vendor-provided Burnin test on the


s/BURNING/BURNIN/?

robshakir · 2018-05-24T02:30:37Z

diag/diag.proto

+//    operations on a set of ports.
+// 2- BURNING related RPCs: Used to perform a vendor-provided Burnin test on the
+//    switch to ensure the box is ready to start serving traffic. Burnin tests
+//    are typically run in the datacenter, as part of turnup or repair workflow.


Maybe say typically run in the field, since not all gNOI deployed devices will live in datacentres :-)

robshakir · 2018-05-24T02:31:18Z

diag/diag.proto

+  // use this ID (as well as the list of the ports) to stop the BERT operation
+  // and/or get the BERT results. This RPC is expected to return an error status
+  // in the following situations:
+  // - When BERT operation is not supported on any of the ports specified by


Does this mean "not supported on one or more of the ports", or "not supported on all the ports"? I assume the former, but it might be worth clarifying.

Thanks, clarified.

robshakir · 2018-05-24T02:32:19Z

diag/diag.proto

+  // - When BERT is already in progress on any port specified by the request.
+  // - In case of any low-level HW/SW internal errors.
+  // The RPC returns an OK status of none of these situations is encountered.
+  rpc StartBERT(StartBERTRequest) returns(StartBERTResponse) {}


We actually have a TESTING state in /interfaces/interface/state/oper-status already. Would this be OK for this use case?

robshakir · 2018-05-24T02:33:37Z

diag/diag.proto

+  //   BERT operation on any of the ports specified by the request.
+  // The RPC returns an OK status of none of these situations is encountered.
+  // Note that a BERT operation is considered completed if switch has a
+  // record of it. Also note that it is OK to receive a stop request for a port


I'm not clear what "if switch has a record of it" means. Does this mean to say that only BERT operations that the switch has the history of are considered completed. For example, a BERT operation that ran before the last reboot might be unknown?

Yes you got it right. After a BERT operation is done, there is nothing in HW that keeps track of the completed BERTs. So it is up to the agent to keep track of them. The agent typically has a limited resource and keeps track of the last M BERTs. Also the storage used to keep track of BERTs might be volatile (wiped out after reboot). At any point of time if the switch knows about a previous BERT, it can consider this completed. Otherwise it is considered a new BERT.

robshakir · 2018-05-24T02:39:03Z

diag/diag.proto

+    // The selected PRBS generating polynomial for BERT.
+    PrbsPolynomial prbs_polynomial = 2;  // required
+    // BERT duration in mins. Must be a positive number.
+    uint32 test_duration_in_mins = 3;  // required


I'd prefer we specify this in seconds -- is there a reason to only allow integer minutes?

No reason. The duration was given as mins before in CMAL case. But dont think it is a big deal to change the logic this much. Changed to seconds.

robshakir · 2018-05-24T02:40:10Z

diag/diag.proto

+    // Path to the interface corresponding to the port.
+    gnoi.Path interface = 1;  // required
+    // The selected PRBS generating polynomial for BERT.
+    PrbsPolynomial prbs_polynomial = 2;  // required


Is it required to be able to run different polynomials per port? I'm not clear what the use case for this would be.

Chips support this. I see no harm in allowing this.

robshakir · 2018-05-24T02:43:34Z

diag/diag.proto

+  repeated PerPortResponse per_port_responses = 2;
+}
+
+message StopBERTRequest {


Here's where I think things get a bit more complicated by having multiple ports be part of the same BERT request. If I stop a BERT only on one port, but it has a single operation ID for that BERT, then this means that the BERT is cancelled for a subset of the ports, but running for the remainder. The current proto requires the caller to always check all statuses of the underlying ports to know the overall status. It seems like 1:1 would simplify this.

I strongly believe the capability of running BERT on multiple port in one RPC is required for tools. As also explained above.

Regarding your example:
The user can start BERT on multiple ports and put them under the same operation ID. That is true. He/she can also stop BERT on one or more ports which were part of the same operation ID later. The success is determines based on the overall BER values captured on all the ports which we get via GetBERTResult RPC. So if some of the per port BERTs were stopped earlier, things should still be clear. The BERT results will tell how long BERT were run on those ports and what was the BER value on those. To me this seems clear.

Couldn't we simplify the GetBERTResult by just having it return status for all the ports that were associated with the operation id (then don't need the two modes, all and per-port)

So as I understand it, the tools have a use case to get the results for all the ports that might be part of different operation IDs.

As you said we might be able to simplify the logic a bit. I added a TODO there. Happy to simplify in case there is no use case.

robshakir · 2018-05-24T02:43:57Z

diag/diag.proto

+    // Path to the interface corresponding to the port.
+    gnoi.Path interface = 1;
+    // BERT result get status for this port. Only if the status is
+    // BERT_STATUS_OK the rest of the fields below are meaningful.


Suggest Only if the status is BERT_STATUS_OK are the rest of the fields meaninful

robshakir · 2018-05-24T02:45:32Z

diag/diag.proto

+// failing/passing should not leave any permanent artifact on the switch (unless
+// there is something wrong HW-wise).
+service Diag {
+  // Starts BERT operation on a set of ports. Each BERT operation is uniquely


When we say port here, do we mean interface in OpenConfig parlance? Or do we mean a single physical port? I think it'd be good to clarify since it's not necessarily clear whether the gnoi.Path messages should correspond to /interfaces/interface entries or /components/component entries of type PORT.

Ports refer to a channelized port which we previously identified using a (slot, port, channel). In YANG, I think an interface is a good equivalent. I clarified this in this text.

aghaffarkhah · 2018-05-29T18:17:53Z

Addressed Rob's comments. PTAL.

aashaikh · 2018-05-29T21:39:47Z

diag/diag.proto

+//
+
+// This file defines the gNOI APIs used to perform diagnostic operations on a
+// switching box.


s/switching box/network device/

Probably need to read through all of the comments to make sure they are generally applicable where appropriate

In looking through the comments, I think a spec doc is the right place for detailed explanations (e.g., error conditions, etc.).

I replaced switching box and switch in general.

What does the second comment mean though? Are you suggesting to provide a spec doc for BERT? Sure, but is that not good to have some explanation here too?

yes, but my view is that it's good to have some minimal explanation here and reference the spec for more detailed explanation (easier to consume in a doc IMO). I think that change can be done in a subsequent PR.

Having a detailed documentation SGTM. I will definitely take care of it.

Note that this is just a beginning. I will still need to talk to many teams and do necessary changes.

aashaikh · 2018-05-29T21:56:00Z

diag/diag.proto

+// there is something wrong HW-wise).
+// Note: By "port" we refer to a channelized frontpanel or backplane port on a
+// chassis. In OpenConfig YANG models, there is a one-to-one relationship
+// between a port as used here and an "interface". Therefore, the "gnoi.Path"


in this case, shouldn't we avoid the use of port in this service to avoid confusion?

Port is a well-known concept for BERT. I think we should mention it. Just referring to port as interface in the comment is going to be confusing too.

Here we are just trying to make sure people will know what the ports refer to "when they think about YANG models".

I just think it could be a little confusing since we specify which "ports" to test by specifying a path to an interface -- also, the port-to-interface relationship isn't 1:1 as described here for example (when you have channelized ports).

I see what you are saying. Yeah I need to clarify this more. I will make sure to take care if this in the next PR.

aashaikh · 2018-05-30T07:23:59Z

diag/diag.proto

+  // Unique BERT operation ID specified by the client. Multiple BERTs run on
+  // different ports can have the same BERT operation ID. This ID will be used
+  // later to stop the operation and/or get its results.
+  string bert_operation_id = 1;


might it be easier to manage these ids on the client if they are just fixed width numerics ?

I agree with you. So far IDs have been used as strings in CMAL. Wanted to keep them consistent. But I like numerical IDs as well.

For now I add a TODO and see if numbers are acceptable, if so I will change.

aashaikh · 2018-05-30T07:29:53Z

diag/diag.proto

+  repeated PerPortResponse per_port_responses = 2;
+}
+
+message StopBERTRequest {


Couldn't we simplify the GetBERTResult by just having it return status for all the ports that were associated with the operation id (then don't need the two modes, all and per-port)

robshakir · 2018-05-30T20:26:24Z

This LGTM, thanks Ali. Please wait for Anees' comments before merging.

aghaffarkhah · 2018-05-30T21:33:28Z

Hi Anees I tried to address your comments in the last commit.

For some reason I could not comment on the last comment regarding GetBertResult. I am replying here. So as I understand it, the tools have a use case to get the results for all the ports that might be part of different operation IDs.

As you said we might be able to simplify the logic a bit. I added a TODO there. Happy to simplify in case there is no use case.

aashaikh

Approving, but suggest we should circle back on these TODOs (and the spec) soon.

aashaikh · 2018-05-31T19:32:44Z

diag/diag.proto

+//
+
+// This file defines the gNOI APIs used to perform diagnostic operations on a
+// switching box.


yes, but my view is that it's good to have some minimal explanation here and reference the spec for more detailed explanation (easier to consume in a doc IMO). I think that change can be done in a subsequent PR.

aashaikh · 2018-05-31T19:38:45Z

diag/diag.proto

+// there is something wrong HW-wise).
+// Note: By "port" we refer to a channelized frontpanel or backplane port on a
+// chassis. In OpenConfig YANG models, there is a one-to-one relationship
+// between a port as used here and an "interface". Therefore, the "gnoi.Path"


I just think it could be a little confusing since we specify which "ports" to test by specifying a path to an interface -- also, the port-to-interface relationship isn't 1:1 as described here for example (when you have channelized ports).

aghaffarkhah added 2 commits May 21, 2018 19:22

First cut of Diag gNOI micro service

25e96fe

Fix some typos

0ee8631

ejbrever requested changes May 22, 2018

View reviewed changes

Addressed 1st round of comments

d961360

ejbrever reviewed May 23, 2018

View reviewed changes

robshakir requested changes May 24, 2018

View reviewed changes

Addressed Rob's comments

913004e

aashaikh reviewed May 30, 2018

View reviewed changes

robshakir approved these changes May 30, 2018

View reviewed changes

Addressed Anees' comments

f1eb46d

aashaikh approved these changes May 31, 2018

View reviewed changes

ejbrever approved these changes Jun 4, 2018

View reviewed changes

aghaffarkhah merged commit 2f20d5c into openconfig:master Jun 4, 2018

First cut of Diag gNOI micro service #9

First cut of Diag gNOI micro service #9

Conversation

aghaffarkhah commented May 22, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghaffarkhah commented May 22, 2018

ejbrever left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghaffarkhah May 29, 2018 • edited Loading

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

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

aghaffarkhah commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghaffarkhah May 30, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robshakir commented May 30, 2018

aghaffarkhah commented May 30, 2018

aashaikh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghaffarkhah May 29, 2018 •

edited

Loading

aghaffarkhah May 30, 2018 •

edited

Loading