Adding initial draft of the Containerz yang model#1389
Adding initial draft of the Containerz yang model#1389alshabib wants to merge 2 commits intoopenconfig:masterfrom
Conversation
3ea4065 to
3f56831
Compare
| description | ||
| "gRPC server containerz freshness-related data."; | ||
|
|
||
| container containers { |
There was a problem hiding this comment.
Similar suggestion as above - consider grouping under containerz hierarchy first. If this is meant to be similar to docker ps outputs then can align as such. Any counters should go under child counters container at relevant hierarchy.
|
|
||
| augment "/oc-sys:system/oc-sys-grpc:grpc-servers/oc-sys-grpc:grpc-server/" + | ||
| "oc-sys-grpc:state" { | ||
| when "../config[contains(services, 'oc-containerz:CONTAINERZ')]/enable = 'true'"; |
There was a problem hiding this comment.
We don't allow for this kind of when statement using contains AFAIK.
There was a problem hiding this comment.
This concept was introduced in the gNSI related models
https://github.com/openconfig/public/blob/master/release/models/gnsi/openconfig-gnsi-authz.yang#L205
But this identity does not exist and each gRPC service domain should really bring in a distinct related container and group everything related underneath (commented above)
There was a problem hiding this comment.
nit: if you're using this when statement, oc-containerz:CONTAINERZ should be oc-gnoi-containerz:CONTAINERZ to match the module prefix oc-gnoi-containerz you defined at the top of the file here
| "gRPC server containerz freshness-related data."; | ||
|
|
||
| container containers { | ||
| list container { |
There was a problem hiding this comment.
I see this as duplicative to the ListContainer RPC, which is fine in principle, but how do we expect the two to be used? I presume the thinking here is that some clients will just subscribe through gNMI to understand this state?
There was a problem hiding this comment.
Yes - the idea is to export base container telemetry to such that we do not depend on containers to export base telemetry that may cause a circular dependency.
| "A timestamp of the last time a gRPC client succeeded | ||
| in establishing a connection to the server."; | ||
| } | ||
| leaf running-containers { |
There was a problem hiding this comment.
Is this a useful metric if we also have the list of containers?
|
/gcbrun |
|
No major YANG version changes in commit 6d0ec50 |
| list container { | ||
| key "name"; | ||
|
|
||
| leaf name { |
There was a problem hiding this comment.
For openconfig style compliance, this needs to be a leafref to a leaf of the same name in a "state container"
for example:
public/release/models/system/openconfig-system-grpc.yang
Lines 75 to 129 in 1463f23
There was a problem hiding this comment.
I remove the other container here.
| description | ||
| "gRPC server containerz freshness-related data."; | ||
|
|
||
| container containers { |
There was a problem hiding this comment.
Consider the same fields from a docker ps (or docker ls if the intent is only to show running containers)?
There was a problem hiding this comment.
That is not entirely possible as restarts or build-label are not directly returned from ps
|
FYI: set project review status to "waiting for author". |
| description | ||
| "Configuration data for Containerz"; | ||
|
|
||
| uses containerz-config; |
There was a problem hiding this comment.
this config is going to be directly underneath the /oc-sys:system/oc-sys-grpc:grpc-servers/oc-sys-grpc:grpc-server path - should we put it in a container so that it doesnt appear to be config for the gprc-server specifically?
e.g. right now the vrf leaf is at /system/grpc-servers/grpc-server[name=X]/config/vrf, and based on the path people could be misled into thinking that this should be the VRF for the grpc-server itself, rather than for the container runtime.
something like:
/system/grpc-servers/grpc-server[name=X]/containerz/config/vrf
would make it more clear that this is not for the grpc-server specifically
There was a problem hiding this comment.
I still dont think this should be on a per-grpc-server basis though - could you take a look at #1389 (comment) ?
having this config be under a global /system path (e.g. /system/containerz/config and /system/containerz/state) would make a lot more sense imo, and remove the ability for the client to push config that is conflicting across each grpc-server.
Change Scope
This is a yang model for the gNOI.Containerz service.
This change is backwards compatible.