Skip to content

Implement support for PodSandboxStatus RPC #267

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.

Sign up for GitHub

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

Open
MikeZappa87 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implement support for PodSandboxStatus RPC #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions pkg/adaptation/adaptation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,47 @@ func (r *Adaptation) UpdatePodSandbox(ctx context.Context, req *UpdatePodSandbox
return &UpdatePodSandboxResponse{}, nil
}

// PodSandboxStatus relays the corresponding CRI request to plugins.
// If a plugin returns IP addresses, those will be returned to the caller.
// An error is returned if multiple plugins attempt to set the same field.
Copy link
Contributor

@aojea aojea Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be a nightmare to troubleshoot in prod on kubernetes clusters, since the error surfaced will most probably do not indicate something, the CNI will say it is correct, since it assignes the IP, and this NRI plugin will be probably opaque to the user ... I think we need to define first a better authoritative model for the IP assignment in the runtimes

Copy link
Author

@MikeZappa87 MikeZappa87 Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a couple things here. If this PR merges, the CNI would be optional so people would need to take that into consideration. I believe this concern is shared across NRI as well? NRI can modify fields on the Pod and containers without the user knowing. The way I was going to implement this in containerd at least was to not allow this RPC to be modified if the CNI was enabled, at least that was just a thought to prevent these conditions however open to all the ideas here.

Copy link
Author

@MikeZappa87 MikeZappa87 Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on the authority for ip assignments?

Copy link

@pfl pfl Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some thoughts. If the NRI plugin can specify all necessary networking details for a pre-existing interface, then the container runtime could perhaps configure everything itself. But CNIs exist to set up an optimal (to them or the problem at hand) overlay network between nodes, so involving CNIs should perhaps not be removed from the equation altogether?

Copy link
Contributor

@aojea aojea Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NRI can modify fields on the Pod and containers without the user knowing

Yeah, but those are properties of the application itself, rlimits, cpu, and assigned by the runtime IIUIC .. the IP assigned is a property required to communicate with the rest of the world, and also assigned by another actor, so you are changing a convention that will generate a lot of issues in prod

@MikeZappa87 this idea of a dedicated hook

explicit Pre- and PostSetupNetwork

Makes it easier to think about, so runtime can choose

if NRISetupNetwork {
  callNRINetworkPlugin
} if CNISetupNetwork {
  call CNI // current state
} else {
  return error, no network plugin configured

This way there is no risk someone assigns an IP and later it realizes is not being used

Copy link
Author

@MikeZappa87 MikeZappa87 Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if NRISetupNetwork {
callNRINetworkPlugin
} if CNISetupNetwork {
call CNI // current state
} else {
return error, no network plugin configured

The POC works in this way. However I wonder if I want to move this PR forward or not. I debate. It might be better to close for now.

Copy link
Contributor

@aojea aojea Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the end of the day is a containerd/crio decision, I can only provide non-binding opinions here

func (r *Adaptation) PodSandboxStatus(ctx context.Context, req *PodSandboxStatusRequest) (*PodSandboxStatusResponse, error) {
r.Lock()
defer r.Unlock()
defer r.removeClosedPlugins()

var (
rsp = &PodSandboxStatusResponse{}
ipOwner = ""
additionalIpsOwner = ""
)

for _, plugin := range r.plugins {
pluginRsp, err := plugin.podSandboxStatus(ctx, req)
if err != nil {
return nil, err
}
if pluginRsp == nil || (pluginRsp.Ip == "" && len(pluginRsp.AdditionalIps) == 0) {
continue
}
if pluginRsp.Ip != "" {
if ipOwner != "" {
return nil, fmt.Errorf("plugins %q and %q both tried to set PodSandboxStatus IP address field", ipOwner, plugin.name())
}
rsp.Ip = pluginRsp.Ip
Copy link
Contributor

@aojea aojea Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already return the pod ips #119 , do you need anything else from the Status?

Copy link
Contributor

@aojea aojea Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, ok, this is not for read, this is for update ... this has a high risk of causing problems with the runtimes, last time I checked I think that crio and containerd both had different hooks for the container IP setting and the NRI hook ... first of all I think that we should define the contract between the NRI hooks and the runtimes of at what point one thing is executed ... I added integration tests in containerd to ensure the current defined order and lifecycle is respected and does not break containerd/containerd#11331 but we should do the same exercise in crio to avoid drifting the runtimes

Copy link
Author

@MikeZappa87 MikeZappa87 Feb 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/containerd/containerd/blob/317286ac00e07bebd7d77925c0fef2af0d620e40/internal/cri/server/sandbox_run.go#L329

Are you talking about this? If you make any changes those are not commited back to the internal store of containerd. I am not 100% following you here? Since kubelet calls PodSandboxStatus, we need to hook into that RPC directly otherwise the change needs to be made in either RunPodSandbox or the kubelet?

Having kubelet use something internal like reading a claim instead of PodSandboxStatus would be an idea for sure. If we update RunPodSandbox, we would need to have that update the internal store for example?

https://github.com/kubernetes/kubernetes/blob/8c9c67c000104450cfc5a5f48053a9a84b73cf93/pkg/kubelet/kuberuntime/kuberuntime_manager.go#L1568 for reference.

ipOwner = plugin.name()
}
if len(pluginRsp.AdditionalIps) > 0 {
if additionalIpsOwner != "" {
return nil, fmt.Errorf("plugins %q and %q both tried to set PodSandboxStatus additional IP addresses field", additionalIpsOwner, plugin.name())
}
rsp.AdditionalIps = pluginRsp.AdditionalIps
additionalIpsOwner = plugin.name()
}
}

return rsp, nil
}

// PostUpdatePodSandbox relays the corresponding CRI event to plugins.
func (r *Adaptation) PostUpdatePodSandbox(ctx context.Context, evt *StateChangeEvent) error {
evt.Event = Event_POST_UPDATE_POD_SANDBOX
Expand Down
3 changes: 3 additions & 0 deletions pkg/adaptation/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ type (
RunPodSandboxRequest = api.RunPodSandboxRequest
UpdatePodSandboxRequest = api.UpdatePodSandboxRequest
UpdatePodSandboxResponse = api.UpdatePodSandboxResponse
PodSandboxStatusRequest = api.PodSandboxStatusRequest
PodSandboxStatusResponse = api.PodSandboxStatusResponse
StopPodSandboxRequest = api.StopPodSandboxRequest
RemovePodSandboxRequest = api.RemovePodSandboxRequest
PostUpdatePodSandboxRequest = api.PostUpdatePodSandboxRequest
Expand Down Expand Up @@ -127,6 +129,7 @@ const (
Event_STOP_CONTAINER = api.Event_STOP_CONTAINER
Event_REMOVE_CONTAINER = api.Event_REMOVE_CONTAINER
Event_VALIDATE_CONTAINER_ADJUSTMENT = api.Event_VALIDATE_CONTAINER_ADJUSTMENT
Event_POD_SANDBOX_STATUS = api.Event_POD_SANDBOX_STATUS
ValidEvents = api.ValidEvents

ContainerState_CONTAINER_UNKNOWN = api.ContainerState_CONTAINER_UNKNOWN
Expand Down
12 changes: 12 additions & 0 deletions pkg/adaptation/builtin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type BuiltinHandlers struct {
RemovePodSandbox func(context.Context, *api.RemovePodSandboxRequest) error
UpdatePodSandbox func(context.Context, *api.UpdatePodSandboxRequest) (*api.UpdatePodSandboxResponse, error)
PostUpdatePodSandbox func(context.Context, *api.PostUpdatePodSandboxRequest) error
PodSandboxStatus func(context.Context, *api.PodSandboxStatusRequest) (*api.PodSandboxStatusResponse, error)

CreateContainer func(context.Context, *api.CreateContainerRequest) (*api.CreateContainerResponse, error)
PostCreateContainer func(context.Context, *api.PostCreateContainerRequest) error
Expand Down Expand Up @@ -84,6 +85,9 @@ func (b *BuiltinPlugin) Configure(ctx context.Context, req *api.ConfigureRequest
if b.Handlers.PostUpdatePodSandbox != nil {
events.Set(api.Event_POST_UPDATE_POD_SANDBOX)
}
if b.Handlers.PodSandboxStatus != nil {
events.Set(api.Event_POD_SANDBOX_STATUS)
}
if b.Handlers.CreateContainer != nil {
events.Set(api.Event_CREATE_CONTAINER)
}
Expand Down Expand Up @@ -204,6 +208,14 @@ func (b *BuiltinPlugin) UpdatePodSandbox(ctx context.Context, req *api.UpdatePod
return &api.UpdatePodSandboxResponse{}, nil
}

// PodSandboxStatus implements PluginService of the NRI API.
func (b *BuiltinPlugin) PodSandboxStatus(ctx context.Context, req *api.PodSandboxStatusRequest) (*api.PodSandboxStatusResponse, error) {
if b.Handlers.PodSandboxStatus != nil {
return b.Handlers.PodSandboxStatus(ctx, req)
}
return &api.PodSandboxStatusResponse{}, nil
}

// PostUpdatePodSandbox is a handler for the PostUpdatePodSandbox event.
func (b *BuiltinPlugin) PostUpdatePodSandbox(ctx context.Context, req *api.PostUpdatePodSandboxRequest) error {
if b.Handlers.PostUpdatePodSandbox != nil {
Expand Down
22 changes: 22 additions & 0 deletions pkg/adaptation/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,28 @@ func (p *plugin) updatePodSandbox(ctx context.Context, req *UpdatePodSandboxRequ
return &UpdatePodSandboxResponse{}, nil
}

func (p *plugin) podSandboxStatus(ctx context.Context, req *PodSandboxStatusRequest) (*PodSandboxStatusResponse, error) {
if !p.events.IsSet(Event_POD_SANDBOX_STATUS) {
return nil, nil
}

ctx, cancel := context.WithTimeout(ctx, getPluginRequestTimeout())
defer cancel()

rsp, err := p.impl.PodSandboxStatus(ctx, req)
if err != nil {
if isFatalError(err) {
log.Errorf(ctx, "closing plugin %s, failed to handle event %d: %v",
p.name(), Event_POD_SANDBOX_STATUS, err)
p.close()
return nil, nil
}
return nil, err
}

return rsp, nil
}

// Relay other pod or container state change events to the plugin.
func (p *plugin) StateChange(ctx context.Context, evt *StateChangeEvent) (err error) {
if !p.events.IsSet(evt.Event) {
Expand Down
13 changes: 13 additions & 0 deletions pkg/adaptation/plugin_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,19 @@ func (p *pluginType) UpdatePodSandbox(ctx context.Context, req *UpdatePodSandbox
return nil, errUnknownImpl
}

func (p *pluginType) PodSandboxStatus(ctx context.Context, req *PodSandboxStatusRequest) (*PodSandboxStatusResponse, error) {
switch {
case p.ttrpcImpl != nil:
return p.ttrpcImpl.PodSandboxStatus(ctx, req)
case p.builtinImpl != nil:
return p.builtinImpl.PodSandboxStatus(ctx, req)
case p.wasmImpl != nil:
return p.wasmImpl.PodSandboxStatus(ctx, req)
}

return nil, errUnknownImpl
}

func (p *pluginType) StateChange(ctx context.Context, req *StateChangeEvent) (err error) {
switch {
case p.ttrpcImpl != nil:
Expand Down
Loading
Loading