Support returning ip addresses with RunPodSandbox#295
Conversation
6c570d2 to
dba324e
Compare
Signed-off-by: Michael Zappa <michael.zappa@gmail.com>
dba324e to
dde870f
Compare
|
|
||
| message RunPodSandboxResponse{} | ||
| message RunPodSandboxResponse{ | ||
| repeated string ips = 1; |
There was a problem hiding this comment.
Should we add anything else? Interfaces? Routes?
There was a problem hiding this comment.
I created an envelope msg that sets the stage for additional fields. Right now containerd has fields for the ip, additional ips but the fields you mention are part of the CNI result string. We could add them.
There was a problem hiding this comment.
nod we can add the info map record for verbose status reporting later on.. with a follow-up
There was a problem hiding this comment.
In the current we have a PodSandboxNetwork message which could provide typed fields instead? That might be preferable as we evolve the networking capabilities in NRI.
There was a problem hiding this comment.
nod i presumed either map[key]=any json string... or typed fields if there's agreement on the definition
There was a problem hiding this comment.
I think we should do both. The same convention in cri-api helps with extensibility and luckily for networking, a lot of fields have been well defined for some time. I think once this is merged, we can look to adding those fields.
There was a problem hiding this comment.
i like our pattern of supporting annotation meta for optional/experimental projects, and typed fields where we have solid ground
There was a problem hiding this comment.
+1 for that as well. Perhaps we can just go ahead and add
There was a problem hiding this comment.
Line 402 in 520641e
Signed-off-by: Michael Zappa <michael.zappa@gmail.com>
c2cb517 to
da4f0ae
Compare
This PR came out of a discussion with @mikebrow regarding if an alternative exists to adding PodSandboxStatus to NRI. He proposed adding ip addresses to the RunPodSandboxResponse.
#267