openconfig / gribi

A gRPC Interface to a Network Element RIB.
Apache License 2.0
57 stars 14 forks source link

Response data set of GET() RPC #44

Closed ashpatcisco closed 2 years ago

ashpatcisco commented 2 years ago

Hi

Assuming session parameters has been set to AFTResultStatusType.RIB_AND_FIB_ACK.

The client programs the server a set of objects using a sequence of AFTOperations. It could be that some of these operations fail in FIB (or dataplane). What should GET() respond with when there are errors from FIB ?

a) If the server's response set consists of all objects programmed by the client, then client does not know which objects have been successfully programmed in FIB b) If the server responds only with objects that have been programmed successfully in FIB (and dataplane), the client does not know the full set of objects previously programmed to the device, leaving device with potentially stale objects - if the object is missing/not needed in the new life of the client.

We suggest that the Get() response should consist of: a) complete set of objects programmed through GRIBI that server has - for the requested AFTType b) The response message per object should also carry the current status of RIB or FIB programming

If above is acceptable, it should also be clarified that with AFTPersistence.PRESERVE mode, the GET() would respond based on the last AFTResultStatusType used to program the object. Perhaps the GET() response should also carry the AFTResultStatusType if there can be uses where clients were to switch from one AFTResultStatusType to another while still maintaining AFTPersistence.PRESERVE.

thanks

nflath commented 2 years ago

I just encountered this problem actually :). This sounds good to me. Otherwise, we need some other method of returning this data.

xw-g commented 2 years ago

Thanks, @ashpatcisco. This sounds good to me too.

One quick thought: how about we make the response message always carry the FIB programing status, regardless of the AFTResultStatusType of the session? It's up to the clients whether to consume it. This might simplify scenarios that involves the changes of AFTResultStatusType with AFTPersistence.PRESERVE?

cc: @robshakir

ashpatcisco commented 2 years ago

Hi Xiao

If the proposal is something like

enum Status { UNAVAILABLE = 1, PROGRAMMED = 2, FAILED = 3, } Status rib; Status fib;

then that would work, as the server can set the "fib" attribute to UNAVAILABLE for where the object was last programmed using a RIB_ACK session.

Given that RIB_ACK is a defined operating mode of the API, if for a use case, client and server have agreed that RIB_ACK is good enough, expecting a FIB programming result "always" for GET() response may be considered as a burden on server.

robshakir commented 2 years ago

This sounds reasonable to me. I think that we had not considered the case that the entry was still in-flight.

@ashpatcisco w.r.t the proposal here, perhaps just:

enum Status {
  UNAVAILABLE = 0;
  PROGRAMMED = 1;
  FAILED = 2;
}
Status rib = XX;
Status fib = XX;

since then UNAVAILABLE becomes the default zero value of the enum.

@nandanarista PTAL to see whether you have any thoughts here please.

@xw-g - will we change #36 here, or open a different PR?

ashpatcisco commented 2 years ago

@robshakir , agreed.

xw-g commented 2 years ago

I have closed #36 per the discussion in this issue. A new PR is better.

xw-g commented 2 years ago

@ashpatcisco Per https://github.com/openconfig/gribi/issues/40#issuecomment-1095520883, I assume you also expect me to open a PR. I'll prepare a PR later and add you for review.

xw-g commented 2 years ago

@ashpatcisco can you help review #46 ? Thanks.

xw-g commented 2 years ago

Closing this issue since #46 is merged.