openconfig / gnsi

Apache License 2.0
20 stars 16 forks source link

Description of GrpcService.config_metadata is too vague #92

Closed haussli closed 1 year ago

haussli commented 1 year ago

https://github.com/openconfig/gnsi/blob/main/accounting/acct.proto#L178-L180

How is it used to track set requests? What is the format and/or content of this field?

morrowc commented 1 year ago

I believe the intent here was that the client would send along in metadata something like: uuid

that would be set uniquely per 'change'. If the change happened to come across as multiple set (or get) commands. For instance, setting new certificates through gNSI requires at least 2 RPC requests, so linking those 2 things together over a period of time is useful.

haussli commented 1 year ago

So, each set would have an uuid and all uuids that constituted that atomic operation set would be included in config_metadata, separated by \r\n?

I do not see anything in the specs that indicates an implementation must assign an ID of any kind to a transaction.

morrowc commented 1 year ago

hey there, I think I misunderstood my self :( This is the accounting record sent for collection/storage.

I can't recall, actually, what the point of this config_metadata was originally.

I do think that if your gRPC change is something like: "gnsi.certz.Rotate() -> Probe -> Finalize"

having the device create a UUID for each RPC might be handy... but actually I'd want 1 UUID for the whole operation, not N UUID's as the process toggles the toggles here. I expect that as a device is in operations you'd see many gRPC things happening, for instance:

In short I don't think I can find a usecase today :(

haussli commented 1 year ago

OK. Shall we change the description to something like: config_metadata should contain a per-transaction (1 per stream) UUID.

Or, is supposed to contain grpc metadata (https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md)?

morrowc commented 1 year ago

descr change seems fine, yes. I don't think metadata is what we meant here.

haussli commented 1 year ago

Does https://github.com/openconfig/gnsi/pull/114 match your expectation?

If it does, metadata_istruncated should be removed, since there is no way it should ever be truncated.

if it does not, perhaps you expected it to be a list of UUIDs, one per transaction? In which case, metadata_istruncated should still be removed and config_metadata should be a repeated string?

earies commented 1 year ago

I'll admit I'm having a bit of a hard time following here and would probably suggest this start at a workflow + specification definition level before any sort of IDL comment updates because it's not clear to me at least the full intention of metadata/UUIDs in this context.

As far as anything publicly defined here in relation to a gNMI Set - there is client generated metadata that can be encoded into an OpenConfig origin attached to / (not an actual schema node but rather the root of the OpenConfig schema tree) - link - this is however not really what you are referring to here since that is more client specified data for bookkeeping, transaction ID associations offline, etc..

AFAIK there have been no discussions around any UUID generation/association expectations on the endpoint side for various RPC behaviors.

If it isn't clear at this time, I would suggest removing ambiguous message fields and building up over time taking into account possible future expansions

morrowc commented 1 year ago

AFAIK there have been no discussions around any UUID generation/association expectations on the endpoint side for various RPC behaviors.

ok.

If it isn't clear at this time, I would suggest removing ambiguous message fields and building up over time taking into account possible future expansions

sure there's some yagni bits here... which seems totally fine to delete and re-add when we need them. I believe the original intent of this field "config_metadata" (we could have picked a better name, for sure) was to act like the SessionID does in Tac+. "All work done during ebben's ssh to device X is identified as session-id-12" My suggestion (the comment) was that a UUID would be fine for this sort of thing... because otherwise: "What is this? a string? an integer? float? int64?" I didn't really want to get into 'what is this fields content' and suggested: "a uuid seems great as a unique thing, and easy to generate..."

Again, though, if we dont' see a need for this 'right now', delete and move along is a totally fine answer.