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

AFTResult status code for a Modify session w/o param negotiation #88

Open
LimeHat opened this issue May 11, 2023 · 3 comments
Open

AFTResult status code for a Modify session w/o param negotiation #88

LimeHat opened this issue May 11, 2023 · 3 comments

Comments

@LimeHat
Copy link

LimeHat commented May 11, 2023

In a scenario where a client skips session parameters negotiation, and starts with a ModifyRequest with an AFToperation only, should the successful operation be acknowledged with a deprecated OK status as suggested by these lines in .proto

// If the client does not send session_parameters, then the network element
// assumes the following defaults:
// - ALL_PRIMARY for client redundancy
// - A client disconnect is treated as if the client issued DELETE
// AFTOperation for all the AFT entries that client ADDed.
// - Each AFTOperation is acknowledged with OK or FAILED in
// in the AFTResult of the ModifyResponse.

Or, as could be implied from the specification.md, we should use RIB_PROGRAMMED instead (since this is the default ack mode)

gribi/doc/specification.md

Lines 151 to 163 in 0f369fb

It is possible that the client skips the negotiation step. In this case, the
first `ModifyRequest` message from the client contains `operation` but not
`params`. The device will assume the default parameters are requested by the
client. If any of the default parameters is not supported by the device, the
device should close the `Modify` RPC upon receiving the first `ModifyRequest`
message, and the device should set the generic gRPC
[`Status.code`][gRPC status code] to `UNIMPLEMENTED`. The `Status.details`
should contain `ModifyRPCErrorDetails` message with `reason` set to
`UNSUPPORTED_PARAMS`. The following are the default parameters:
* `redundancy` = `ALL_PRIMARY`.
* `persistence` = `DELETE`
* `ack_type` = `RIB_ACK`

?

@robshakir
Copy link
Contributor

@LimeHat Apologies, missed this issue -- with the deprecation of OK, then RIB_PROGRAMMED should be used IMHO. Any thoughts?

@robshakir
Copy link
Contributor

Currently this is not tested in the compliance test suite, because we always assume that we specify some persistence mode, and SINGLE_PRIMARY. Tying adding the compliance test in openconfig/gribigo@689fdad to adding support for non-leader elected, non-persistent clients to gribigo.

@LimeHat
Copy link
Author

LimeHat commented Oct 27, 2024

Thanks Rob. I agree with the behavioral part and thank you for adding the test.

Perhaps the proto comment can also be updated to remove the ambiguity?

// - Each AFTOperation is acknowledged with OK or FAILED in

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

No branches or pull requests

2 participants