Implement Stream* RPCs (KEP-5825)#9761
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors resource listing operations by extracting shared filtering logic into internal helpers and introduces new streaming RPC methods for containers, images, and pod sandboxes across multiple server implementations. It also updates Kubernetes, gRPC, and protobuf dependencies, and adjusts health check context handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Helper
participant Stream as gRPC Stream
Client->>Server: StreamResource(filter)
Server->>Helper: listResource(ctx, filter)
Helper->>Helper: Apply filtering logic
Helper-->>Server: []*Resource (filtered)
loop Send in chunks (streamChunkSize)
Server->>Stream: Send(Response{Resource: chunk})
Stream-->>Client: Receives chunk
end
alt Error during Send
Server-->>Client: Return error
else Success
Server-->>Client: Return nil
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9761 +/- ##
==========================================
- Coverage 67.78% 67.56% -0.23%
==========================================
Files 212 212
Lines 29366 29469 +103
==========================================
+ Hits 19907 19910 +3
- Misses 7763 7858 +95
- Partials 1696 1701 +5 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/sandbox_metrics_list.go (1)
35-49:⚠️ Potential issue | 🔴 CriticalFix the inverted condition: the
elsebranch attempts to access fields from anilresult.On line 36 you check
if current := metrics.GetMetric(); current != niland only enter theelsebranch whenGetMetric()returnsnil. Then on line 40 you callmetrics.GetMetric().GetContainerMetrics()— butGetMetric()isnil, so this returns an empty slice and theforloop on line 41 never executes. Lines 42–46 are unreachable dead code.The intended logic appears to be splitting a pod's container metrics into separate entries. However, you cannot retrieve container metrics from a
nilGetMetric()result. Check whether the container metrics should be accessed directly frommetricsor another source, and fix the condition to correctly distinguish between cases where you append the top-level metric versus when you need to iterate over container-level entries.
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 261: The go.mod replace directive currently points to a personal fork
(github.com/bitoku/kubernetes) for k8s.io/cri-api using a pseudo-version that is
unavailable and unacceptable for production; update this by either replacing the
directive with an official k8s.io/cri-api release version (preferred), or remove
the replace directive and vendor the dependency into the repo (go mod vendor) so
builds are reproducible, or upstream the streaming RPC changes to the official
Kubernetes repo and switch the replace to the resulting official version. Locate
the replace line for k8s.io/cri-api in go.mod and implement one of these
remedies, ensuring the chosen version is an official tag or vendored copy before
merging.
🧹 Nitpick comments (4)
server/container_stats_list.go (1)
23-41: Streaming collects all results into memory before sending.The current implementation gathers every stat into a slice, then iterates to stream them. This is functionally correct but doesn't provide backpressure or reduce peak memory — the caller still buffers the full result set. For a WIP/initial implementation this is fine, but to realize the full benefit of KEP-5825 streaming, consider yielding items as they are produced (e.g., via a callback or channel-based approach in the helper) in a follow-up.
server/sandbox_metrics_list.go (1)
16-27:ctxunused — context not propagated tolistPodSandboxMetrics.
listPodSandboxMetrics()takes nocontext.Contextparameter, so thectxextracted from the stream on line 18 (if it were extracted — it's actually not even extracted here) is unused. However,ListPodSandboxMetricson line 10 receives actxbut also doesn't pass it. IfMetricsForPodSandboxListorListSandboxesever needs context (for tracing, cancellation, etc.), this will need updating. As a minor nit, the signature oflistPodSandboxMetricscould acceptcontext.Contextfor consistency with the other helpers (listContainerStats,listPodSandboxStats,listContainers,listImagesall takectx).server/container_list.go (1)
87-105:StreamContainersdoesn't propagate tracing spans unlikeListContainers.
ListContainers(line 74) starts a span vialog.StartSpan(ctx), butStreamContainersdoesn't. For observability parity, consider starting a span in the streaming path as well (or in the sharedlistContainershelper).server/sandbox_list.go (1)
38-57: Duplicate filtering:Created(), state, and label checks run twice.
filterSandboxList(lines 61–114) already filters out sandboxes that aren'tCreated(), don't match the requested state, and don't match label selectors. ThenlistPodSandboxesre-checksCreated()on line 45 and re-applies state+label filtering viafilterSandboxon line 51. The second pass is redundant.This appears to be pre-existing, but the helper extraction makes it a good time to clean it up — either remove the checks from
listPodSandboxesor fromfilterSandboxList.
649c1bc to
fc76d03
Compare
Contributor
Author
|
I will follow up to create integration tests once cri-tools has implementations for them. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/storage/image.go (1)
877-885:⚠️ Potential issue | 🔴 CriticalUse
new(uint)here.Line 883 will not compile:
newtakes a type, not an expression likeuint(0). Usenew(uint)to allocate a pointer to a zero-initialized uint for the retry count.Proposed fix
- MaxRetries: new(uint(0)), + MaxRetries: new(uint),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/image.go` around lines 877 - 885, The MaxRetries field in the libimage.CopyOptions passed to artifactStore.Pull is using an invalid expression (`new(uint(0))`) which won't compile; change it to allocate a pointer to a zero-initialized uint (use `new(uint)`) when setting MaxRetries in the CopyOptions struct passed to artifactStore.Pull so MaxRetries is a *uint pointing to 0.server/inspect.go (1)
117-131:⚠️ Potential issue | 🔴 CriticalTake the address of the
HostNetworkvalue.Line 130 will not compile. The
new()builtin expects a type argument, butsb.HostNetwork()is a method call returning aboolvalue. Capture theboolvalue in a local variable and return its address instead.Proposed fix
imageRef := ctr.CRIContainer().GetImageRef() + hostNetwork := sb.HostNetwork() return types.ContainerInfo{ Name: ctr.Name(), Pid: pidToReturn, @@ LogPath: ctr.LogPath(), Sandbox: ctr.Sandbox(), IPs: sb.IPs(), - HostNetwork: new(sb.HostNetwork()), + HostNetwork: &hostNetwork, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/inspect.go` around lines 117 - 131, The returned ContainerInfo currently tries to call new(sb.HostNetwork()) which is invalid because new expects a type and sb.HostNetwork() is a method returning a bool; fix by calling sb.HostNetwork() into a local bool (e.g., hostNet := sb.HostNetwork()) and then set HostNetwork: &hostNet in the types.ContainerInfo literal (update the struct construction in the function that returns types.ContainerInfo so HostNetwork receives the address of that local variable).
♻️ Duplicate comments (1)
go.mod (1)
267-267:⚠️ Potential issue | 🔴 CriticalThe
replacedirective still blocks the verify job.CI is already failing on Line 267 because
gomoddirectivesforbids this override, so the branch cannot merge with this line present. If the streaming CRI API has to stay ahead of upstream for now, vendor the generated API instead of overriding module resolution.#!/bin/bash set -euo pipefail # Show the replace directive and search repo config for the gomoddirectives rule. sed -n '262,267p' go.mod printf '\nGolangCI configuration referencing gomoddirectives:\n' fd -t f 'golangci.*' . -H | xargs -r rg -n 'gomoddirectives|replace'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 267, The replace directive "replace k8s.io/cri-api => github.com/bitoku/kubernetes/staging/src/k8s.io/cri-api v0.0.0-20260311153604-3ba7af3bb754" in go.mod violates the repository's gomoddirectives policy and blocks CI; remove that replace and instead vendor the streaming CRI API sources into the repo (e.g., commit the generated API package under your module tree or vendor/), update imports if needed to point to the vendored package, regenerate modules (so go.mod/go.sum no longer contain the replace) and ensure the gomoddirectives rule no longer flags the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 8: Add an inline comment next to the G703 entry in the gosec exclusion
list (the GOSEC_EXCLUDE/GOSEC_FLAGS setting) explaining why G703 is being
suppressed (e.g., documented false positives for deferred error handling, a
referenced issue/PR, or an explicit policy decision) and include a pointer to
the justification (issue/PR URL or team doc) so future reviewers can validate
the rationale; update the Makefile's gosec exclusion block to include that brief
comment adjacent to "G703".
In `@pkg/config/config.go`:
- Around line 836-842: In SetSystemContext, the code wrongly uses new() with
constant names (ShortNameModeEnforcing/ShortNameModeDisabled); create a variable
of type types.ShortNameMode initialized to the appropriate constant and assign
its pointer to c.SystemContext.ShortNameMode (or take the address of a
short-lived variable with the constant value) instead of calling new() on the
constant; update the assignments in tomlConfig.SetSystemContext that set
c.SystemContext.ShortNameMode based on c.ShortNameMode accordingly.
---
Outside diff comments:
In `@internal/storage/image.go`:
- Around line 877-885: The MaxRetries field in the libimage.CopyOptions passed
to artifactStore.Pull is using an invalid expression (`new(uint(0))`) which
won't compile; change it to allocate a pointer to a zero-initialized uint (use
`new(uint)`) when setting MaxRetries in the CopyOptions struct passed to
artifactStore.Pull so MaxRetries is a *uint pointing to 0.
In `@server/inspect.go`:
- Around line 117-131: The returned ContainerInfo currently tries to call
new(sb.HostNetwork()) which is invalid because new expects a type and
sb.HostNetwork() is a method returning a bool; fix by calling sb.HostNetwork()
into a local bool (e.g., hostNet := sb.HostNetwork()) and then set HostNetwork:
&hostNet in the types.ContainerInfo literal (update the struct construction in
the function that returns types.ContainerInfo so HostNetwork receives the
address of that local variable).
---
Duplicate comments:
In `@go.mod`:
- Line 267: The replace directive "replace k8s.io/cri-api =>
github.com/bitoku/kubernetes/staging/src/k8s.io/cri-api
v0.0.0-20260311153604-3ba7af3bb754" in go.mod violates the repository's
gomoddirectives policy and blocks CI; remove that replace and instead vendor the
streaming CRI API sources into the repo (e.g., commit the generated API package
under your module tree or vendor/), update imports if needed to point to the
vendored package, regenerate modules (so go.mod/go.sum no longer contain the
replace) and ensure the gomoddirectives rule no longer flags the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee156c51-420c-4667-a6cd-5c6ab3dbeb11
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.github/workflows/nixpkgs.yml.github/workflows/test.ymlMakefiledependencies.yamlgo.modinternal/hostport/hostport_nftables.gointernal/oci/runtime_oci.gointernal/storage/image.gonix/derivation.nixnix/nixpkgs.jsonpkg/config/config.gopkg/config/workloads_test.goserver/container_list.goserver/container_stats_list.goserver/container_status_test.goserver/container_update_resources.goserver/image_list.goserver/inspect.goserver/sandbox_list.goserver/sandbox_metrics_list.goserver/sandbox_stats_list.goserver/server.gotest/docs-validation/main.goutils/utils.go
💤 Files with no reviewable changes (1)
- utils/utils.go
✅ Files skipped from review due to trivial changes (10)
- nix/derivation.nix
- server/server.go
- .github/workflows/test.yml
- nix/nixpkgs.json
- server/container_status_test.go
- server/container_update_resources.go
- dependencies.yaml
- pkg/config/workloads_test.go
- .github/workflows/nixpkgs.yml
- internal/hostport/hostport_nftables.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/sandbox_metrics_list.go
- server/container_stats_list.go
- server/container_list.go
- server/image_list.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/sandbox_metrics_list.go (1)
31-53:⚠️ Potential issue | 🟠 MajorThe fallback branch currently drops every nil-metric entry.
elseonly runs whenmetrics.GetMetric()is nil, but inside that branch the code readsGetContainerMetrics()andGetPodSandboxId()from the same nil metric again. On CRI generated getters that yields zero values, so nothing is appended and that case disappears from both the unary and stream responses.Based on learnings "protobuf-generated Get* methods for k8s.io/cri-api types are nil-safe: if the receiver is nil, GetX() returns the zero value instead of panicking."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/sandbox_metrics_list.go` around lines 31 - 53, The code in listPodSandboxMetrics calls metrics.GetMetric() twice and tries to access container fields inside the else branch when GetMetric() is nil, which results in zero-values and drops entries; change the logic to only access container metrics from a non-nil PodSandboxMetrics: in listPodSandboxMetrics call current := metrics.GetMetric(); if current != nil append current to responseMetricsList and, if you need to expand per-container entries, iterate current.GetContainerMetrics() and use current.GetPodSandboxId() there; remove the else branch that dereferences a nil metric and ensure all accesses to GetContainerMetrics()/GetPodSandboxId() are performed on the non-nil current.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/container_stats_list.go`:
- Around line 24-42: StreamContainerStats currently calls listContainerStats
which builds the entire []*types.ContainerStats before sending, causing high
memory/latency and preventing early cancellation; change to a streaming approach
by adding a streaming variant (e.g., listContainerStatsStream(ctx, filter, emit
func([]*types.ContainerStats) error)) or modify listContainerStats to accept a
callback/iterator, then update StreamContainerStats to invoke that streamer and
directly call stream.Send(&types.StreamContainerStatsResponse{ContainerStats:
batch}) for each emitted batch (use existing streamChunkSize batching inside the
streamer or emit per-item), and ensure you respect the stream.Context() and
propagate errors from stream.Send to stop iteration immediately.
In `@server/sandbox_metrics_list.go`:
- Around line 10-14: The helper listPodSandboxMetrics currently ignores the
caller's context so cancellation isn't observed; modify listPodSandboxMetrics to
accept a context.Context parameter (e.g., func (s *Server)
listPodSandboxMetrics(ctx context.Context) ...) and update callers
ListPodSandboxMetrics and StreamPodSandboxMetrics to pass their ctx through
(call s.listPodSandboxMetrics(ctx)), ensuring any internal loops or blocking
operations in listPodSandboxMetrics periodically check ctx.Done() and return
early on cancellation or propagate context-aware calls.
---
Outside diff comments:
In `@server/sandbox_metrics_list.go`:
- Around line 31-53: The code in listPodSandboxMetrics calls metrics.GetMetric()
twice and tries to access container fields inside the else branch when
GetMetric() is nil, which results in zero-values and drops entries; change the
logic to only access container metrics from a non-nil PodSandboxMetrics: in
listPodSandboxMetrics call current := metrics.GetMetric(); if current != nil
append current to responseMetricsList and, if you need to expand per-container
entries, iterate current.GetContainerMetrics() and use current.GetPodSandboxId()
there; remove the else branch that dereferences a nil metric and ensure all
accesses to GetContainerMetrics()/GetPodSandboxId() are performed on the non-nil
current.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aca66e23-32a0-479f-80e4-1ba1f74ed30d
⛔ Files ignored due to path filters (20)
go.sumis excluded by!**/*.sumvendor/google.golang.org/grpc/internal/envconfig/envconfig.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/client_stream.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/http2_client.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/transport.gois excluded by!vendor/**vendor/google.golang.org/grpc/server.gois excluded by!vendor/**vendor/google.golang.org/grpc/stream.gois excluded by!vendor/**vendor/google.golang.org/grpc/version.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!vendor/**vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.protois excluded by!vendor/**vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/cri-api/pkg/apis/services.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (8)
go.modserver/container_list.goserver/container_stats_list.goserver/image_list.goserver/sandbox_list.goserver/sandbox_metrics_list.goserver/sandbox_stats_list.goserver/server.go
✅ Files skipped from review due to trivial changes (1)
- server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- server/image_list.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/sandbox_metrics_list.go (1)
10-18:⚠️ Potential issue | 🟠 MajorKeep
ctxon the shared metrics helper.
ListPodSandboxMetricsandStreamPodSandboxMetricsstill drop the caller'sctxatlistPodSandboxMetrics(), so a canceled request will keep collecting the full metrics slice anyway.As per coding guidelines "Propagate context.Context through function calls in Go code".
Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/sandbox_metrics_list.go` around lines 10 - 18, Both RPC handlers discard the incoming context when calling listPodSandboxMetrics(), so cancellation/timeout signals are lost; update the helper to accept a context and propagate it: change listPodSandboxMetrics to listPodSandboxMetrics(ctx context.Context) and update both ListPodSandboxMetrics (pass ctx) and StreamPodSandboxMetrics (pass the incoming ctx from the stream or request handling function) to call the new signature, and adjust any internal calls within listPodSandboxMetrics to use the provided ctx for cancellable operations.
🧹 Nitpick comments (3)
server/health.go (1)
27-27: Handle deferredClose()errors instead of dropping them.Line 27 ignores the returned error from
rrs.Close(ctx), which can hide connection teardown problems in health diagnostics. Thelogpackage is already imported and used elsewhere in the file, so error logging is straightforward.♻️ Proposed change
- defer rrs.Close(ctx) + defer func() { + if cerr := rrs.Close(ctx); cerr != nil { + log.Warnf(ctx, "close remote runtime service: %v", cerr) + } + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/health.go` at line 27, The deferred call to rrs.Close(ctx) currently discards its returned error; change the defer to wrap the close in a closure that captures and logs any error (e.g., defer func() { if err := rrs.Close(ctx); err != nil { log.Printf("rrs.Close error: %v", err) } }()), referencing the existing rrs and ctx variables so connection teardown errors are reported via the package log.server/image_list.go (1)
52-53: Drop the redundant nil check on the proto getter.
filter.GetImage().GetImage()is already nil-safe for these generated CRI types, sofilterImage != nilis just extra branching.Based on learnings "In the cri-o/cri-o repository, protobuf-generated Get* methods for k8s.io/cri-api types are nil-safe: if the receiver is nil, GetX() returns the zero value instead of panicking. Do not add explicit nil checks before chaining calls on such getters."♻️ Proposed simplification
if filter != nil { - if filterImage := filter.GetImage(); filterImage != nil && filterImage.GetImage() != "" { + if filterImage := filter.GetImage(); filterImage.GetImage() != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/image_list.go` around lines 52 - 53, Remove the redundant nil check on the protobuf getter: replace the conditional that does "if filterImage := filter.GetImage(); filterImage != nil && filterImage.GetImage() != "" {" with a nil-safe check using the generated getters directly such as "if filter.GetImage().GetImage() != "" {" (or capture the string with "if img := filter.GetImage().GetImage(); img != "" {") so you rely on the proto-generated GetImage() being nil-safe and eliminate the unnecessary filterImage != nil branch.server/sandbox_list.go (1)
41-53: Avoid filtering sandboxes twice inlistPodSandboxes.
filterSandboxListalready appliesId,State, andLabelSelector, so the extrafilterSandbox(pod, filter)branch just repeats the same work and leaves two filter paths to keep in sync.♻️ Proposed simplification
func (s *Server) listPodSandboxes(ctx context.Context, filter *types.PodSandboxFilter) []*types.PodSandbox { podList := s.filterSandboxList(ctx, filter, s.ListSandboxes()) respList := make([]*types.PodSandbox, 0, len(podList)) for _, sb := range podList { // Skip sandboxes that aren't created yet if !sb.Created() { continue } - pod := sb.CRISandbox() - // Filter by other criteria such as state and labels. - if filterSandbox(pod, filter) { - respList = append(respList, pod) - } + respList = append(respList, sb.CRISandbox()) } return respList }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/sandbox_list.go` around lines 41 - 53, listPodSandboxes duplicates filtering: remove the inner call to filterSandbox(pod, filter) inside Server.listPodSandboxes since filterSandboxList already applies Id, State and LabelSelector. Keep the Created() check and simply append sb.CRISandbox() to respList for each sb in podList; reference functions: Server.listPodSandboxes, filterSandboxList, and filterSandbox to locate and remove the redundant branch so there aren’t two separate filter paths to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/image_list.go`:
- Around line 30-33: StreamImages currently calls listImages which builds the
full []*types.Image before any stream.Send happens, causing eager buffering;
change the implementation to stream results incrementally by adding a
visitor/callback or channel-based API (e.g., listImagesWalk or listImagesStream)
that accepts a context and a func(*types.Image) error and calls that callback
for each image as it's produced, then update StreamImages (and the other Stream*
handlers with the same pattern) to pass a callback that calls stream.Send for
each image and respects cancellation/errors returned from the callback so items
are emitted as they become available rather than accumulated.
---
Duplicate comments:
In `@server/sandbox_metrics_list.go`:
- Around line 10-18: Both RPC handlers discard the incoming context when calling
listPodSandboxMetrics(), so cancellation/timeout signals are lost; update the
helper to accept a context and propagate it: change listPodSandboxMetrics to
listPodSandboxMetrics(ctx context.Context) and update both ListPodSandboxMetrics
(pass ctx) and StreamPodSandboxMetrics (pass the incoming ctx from the stream or
request handling function) to call the new signature, and adjust any internal
calls within listPodSandboxMetrics to use the provided ctx for cancellable
operations.
---
Nitpick comments:
In `@server/health.go`:
- Line 27: The deferred call to rrs.Close(ctx) currently discards its returned
error; change the defer to wrap the close in a closure that captures and logs
any error (e.g., defer func() { if err := rrs.Close(ctx); err != nil {
log.Printf("rrs.Close error: %v", err) } }()), referencing the existing rrs and
ctx variables so connection teardown errors are reported via the package log.
In `@server/image_list.go`:
- Around line 52-53: Remove the redundant nil check on the protobuf getter:
replace the conditional that does "if filterImage := filter.GetImage();
filterImage != nil && filterImage.GetImage() != "" {" with a nil-safe check
using the generated getters directly such as "if filter.GetImage().GetImage() !=
"" {" (or capture the string with "if img := filter.GetImage().GetImage(); img
!= "" {") so you rely on the proto-generated GetImage() being nil-safe and
eliminate the unnecessary filterImage != nil branch.
In `@server/sandbox_list.go`:
- Around line 41-53: listPodSandboxes duplicates filtering: remove the inner
call to filterSandbox(pod, filter) inside Server.listPodSandboxes since
filterSandboxList already applies Id, State and LabelSelector. Keep the
Created() check and simply append sb.CRISandbox() to respList for each sb in
podList; reference functions: Server.listPodSandboxes, filterSandboxList, and
filterSandbox to locate and remove the redundant branch so there aren’t two
separate filter paths to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16041f60-d64c-4638-819e-e8459af34166
⛔ Files ignored due to path filters (81)
go.sumis excluded by!**/*.sumvendor/github.com/prometheus/client_golang/prometheus/collectors/collectors.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/dbstats_collector.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/expvar_collector.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/go_collector_go116.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/go_collector_latest.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/process_collector.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/envconfig/envconfig.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/client_stream.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/http2_client.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/transport.gois excluded by!vendor/**vendor/google.golang.org/grpc/server.gois excluded by!vendor/**vendor/google.golang.org/grpc/stream.gois excluded by!vendor/**vendor/google.golang.org/grpc/version.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!vendor/**vendor/k8s.io/component-base/featuregate/OWNERSis excluded by!vendor/**vendor/k8s.io/component-base/featuregate/feature_gate.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/OWNERSis excluded by!vendor/**vendor/k8s.io/component-base/metrics/buckets.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/collector.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/counter.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/desc.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/gauge.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/histogram.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/http.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/labels.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/legacyregistry/registry.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/metric.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/options.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/opts.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/processstarttime.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/processstarttime_others.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/processstarttime_windows.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/prometheus/feature/metrics.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/prometheusextension/timing_histogram.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/prometheusextension/timing_histogram_vec.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/prometheusextension/weighted_histogram.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/prometheusextension/weighted_histogram_vec.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/registry.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/summary.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/timing_histogram.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/value.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/version.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/version_parser.gois excluded by!vendor/**vendor/k8s.io/component-base/metrics/wrappers.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/OWNERSis excluded by!vendor/**vendor/k8s.io/component-base/tracing/api/v1/config.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/api/v1/doc.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/api/v1/types.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/api/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/api/v1/zz_generated.model_name.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/tracing.gois excluded by!vendor/**vendor/k8s.io/component-base/tracing/utils.gois excluded by!vendor/**vendor/k8s.io/component-base/version/OWNERSis excluded by!vendor/**vendor/k8s.io/component-base/version/base.gois excluded by!vendor/**vendor/k8s.io/component-base/version/dynamic.gois excluded by!vendor/**vendor/k8s.io/component-base/version/version.gois excluded by!vendor/**vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api.protois excluded by!vendor/**vendor/k8s.io/cri-api/pkg/apis/runtime/v1/api_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/cri-api/pkg/apis/services.gois excluded by!vendor/**vendor/k8s.io/cri-client/pkg/internal/log.gois excluded by!vendor/**vendor/k8s.io/cri-client/pkg/remote_image.gois excluded by!vendor/**vendor/k8s.io/cri-client/pkg/remote_runtime.gois excluded by!vendor/**vendor/k8s.io/cri-client/pkg/util/util_unix.gois excluded by!vendor/**vendor/k8s.io/cri-client/pkg/util/util_unsupported.gois excluded by!vendor/**vendor/k8s.io/cri-client/pkg/util/util_windows.gois excluded by!vendor/**vendor/k8s.io/klog/v2/README.mdis excluded by!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues.gois excluded by!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.gois excluded by!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.gois excluded by!vendor/**vendor/k8s.io/klog/v2/klog.gois excluded by!vendor/**vendor/k8s.io/klog/v2/klogr.gois excluded by!vendor/**vendor/k8s.io/klog/v2/klogr_slog.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (9)
go.modserver/container_list.goserver/container_stats_list.goserver/health.goserver/image_list.goserver/sandbox_list.goserver/sandbox_metrics_list.goserver/sandbox_stats_list.goserver/server.go
✅ Files skipped from review due to trivial changes (1)
- server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/container_stats_list.go
- go.mod
Add server-side streaming alternatives to all List* CRI RPCs, as defined in KEP-5825 (CRIListStreaming). These new RPCs stream results one item at a time instead of returning a single large response. The following streaming RPCs are implemented: - StreamContainers (RuntimeService) - StreamContainerStats (RuntimeService) - StreamPodSandboxes (RuntimeService) - StreamPodSandboxStats (RuntimeService) - StreamPodSandboxMetrics (RuntimeService) - StreamImages (ImageService) Each existing List handler is refactored to extract the core listing logic into an unexported helper (e.g. listContainers), which is then shared by both the unary List RPC and the new streaming RPC. The streaming handlers iterate over the helper's results and send each item individually via stream.Send. Vendor dependencies are updated to pick up the new CRI API proto definitions and regenerated gRPC code from k8s.io/cri-api. Signed-off-by: Ayato Tokubi <atokubi@redhat.com> Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/sandbox_metrics_list.go (1)
38-50:⚠️ Potential issue | 🔴 CriticalCritical: Nil pointer dereference in the else branch.
When
metrics.GetMetric()returnsnil(line 38 condition fails), the else branch at line 42 callsmetrics.GetMetric().GetContainerMetrics()on the same nil value, causing a panic.🐛 Proposed fix
for _, metrics := range metricsList { - if current := metrics.GetMetric(); current != nil { - responseMetricsList = append(responseMetricsList, current) - } else { - // Iterate over container metrics within each PodSandboxMetrics. - containerMetricsList := metrics.GetMetric().GetContainerMetrics() - for _, containerMetrics := range containerMetricsList { - containerPodSandboxMetrics := &types.PodSandboxMetrics{ - PodSandboxId: metrics.GetMetric().GetPodSandboxId(), - ContainerMetrics: []*types.ContainerMetrics{containerMetrics}, - } - responseMetricsList = append(responseMetricsList, containerPodSandboxMetrics) - } + current := metrics.GetMetric() + if current == nil { + continue + } + // If the metric has container metrics, expand them; otherwise add as-is. + containerMetricsList := current.GetContainerMetrics() + if len(containerMetricsList) == 0 { + responseMetricsList = append(responseMetricsList, current) + } else { + for _, containerMetrics := range containerMetricsList { + containerPodSandboxMetrics := &types.PodSandboxMetrics{ + PodSandboxId: current.GetPodSandboxId(), + ContainerMetrics: []*types.ContainerMetrics{containerMetrics}, + } + responseMetricsList = append(responseMetricsList, containerPodSandboxMetrics) + } } }Note: The original logic intent is unclear—please verify whether the else branch should expand container metrics or if the entire block should simply skip nil metrics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/sandbox_metrics_list.go` around lines 38 - 50, The code dereferences metrics.GetMetric() in the else branch causing a nil pointer panic; fix by capturing the result once and guarding it, e.g. assign m := metrics.GetMetric() and then if m == nil { continue } else inspect m.GetContainerMetrics(): if it has 1 or 0 containers append m to responseMetricsList, otherwise iterate m.GetContainerMetrics() and for each containerMetrics build a &types.PodSandboxMetrics{PodSandboxId: m.GetPodSandboxId(), ContainerMetrics: []*types.ContainerMetrics{containerMetrics}} and append; ensure all references use the local m variable (and keep symbols responseMetricsList, metrics.GetMetric(), GetContainerMetrics, GetPodSandboxId, PodSandboxMetrics, ContainerMetrics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/sandbox_metrics_list.go`:
- Around line 38-50: The code dereferences metrics.GetMetric() in the else
branch causing a nil pointer panic; fix by capturing the result once and
guarding it, e.g. assign m := metrics.GetMetric() and then if m == nil {
continue } else inspect m.GetContainerMetrics(): if it has 1 or 0 containers
append m to responseMetricsList, otherwise iterate m.GetContainerMetrics() and
for each containerMetrics build a &types.PodSandboxMetrics{PodSandboxId:
m.GetPodSandboxId(), ContainerMetrics:
[]*types.ContainerMetrics{containerMetrics}} and append; ensure all references
use the local m variable (and keep symbols responseMetricsList,
metrics.GetMetric(), GetContainerMetrics, GetPodSandboxId, PodSandboxMetrics,
ContainerMetrics).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5199c705-e493-4c31-921d-5686aa23c871
⛔ Files ignored due to path filters (236)
go.sumis excluded by!**/*.sumvendor/github.com/emicklei/go-restful/v3/.travis.ymlis excluded by!vendor/**vendor/github.com/emicklei/go-restful/v3/CHANGES.mdis excluded by!vendor/**vendor/github.com/emicklei/go-restful/v3/README.mdis excluded by!vendor/**vendor/github.com/emicklei/go-restful/v3/curly.gois excluded by!vendor/**vendor/github.com/emicklei/go-restful/v3/custom_verb.gois excluded by!vendor/**vendor/github.com/emicklei/go-restful/v3/doc.gois excluded by!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/collectors.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/dbstats_collector.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/expvar_collector.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/go_collector_go116.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/go_collector_latest.gois excluded by!vendor/**vendor/github.com/prometheus/client_golang/prometheus/collectors/process_collector.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/.golangci.ymlis excluded by!vendor/**vendor/github.com/prometheus/procfs/Makefileis excluded by!vendor/**vendor/github.com/prometheus/procfs/Makefile.commonis excluded by!vendor/**vendor/github.com/prometheus/procfs/arp.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/buddyinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cmdline.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_armx.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_loong64.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_mipsx.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_others.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_ppcx.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_riscvx.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_s390x.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/cpuinfo_x86.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/crypto.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/doc.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/fs.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/fs_statfs_notype.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/fs_statfs_type.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/fscache.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/internal/fs/fs.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/internal/util/parse.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/internal/util/readfile.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/internal/util/sysreadfile.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/internal/util/sysreadfile_compat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/internal/util/valueparser.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/ipvs.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/kernel_hung.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/kernel_random.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/loadavg.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/mdstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/meminfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/mountinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/mountstats.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_conntrackstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_dev.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_dev_snmp6.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_ip_socket.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_protocols.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_route.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_sockstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_softnet.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_tcp.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_tls_stat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_udp.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_unix.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_wireless.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/net_xfrm.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/netstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/nfnetlink_queue.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_cgroup.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_cgroups.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_environ.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_fdinfo.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_interrupts.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_io.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_limits.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_maps.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_netstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_ns.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_psi.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_smaps.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_snmp.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_snmp6.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_stat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_statm.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_status.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/proc_sys.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/schedstat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/slab.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/softirqs.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/stat.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/swaps.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/thread.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/vm.gois excluded by!vendor/**vendor/github.com/prometheus/procfs/zoneinfo.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/client.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/common.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/config.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/doc.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/handler.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv/client.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv/env.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv/gen.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv/httpconv.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv/server.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv/util.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/transport.gois excluded by!vendor/**vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/version.gois excluded by!vendor/**vendor/go.opentelemetry.io/otel/semconv/v1.39.0/httpconv/metric.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/envconfig/envconfig.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/client_stream.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/http2_client.gois excluded by!vendor/**vendor/google.golang.org/grpc/internal/transport/transport.gois excluded by!vendor/**vendor/google.golang.org/grpc/server.gois excluded by!vendor/**vendor/google.golang.org/grpc/stream.gois excluded by!vendor/**vendor/google.golang.org/grpc/version.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/generated.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/admissionregistration/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/admissionregistration/v1/register.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/zz_generated.model_name.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1/zz_generated.prerelease-lifecycle.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1alpha1/generated.protois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1alpha1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/admissionregistration/v1alpha1/types.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1alpha1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/admissionregistration/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1beta1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/admissionregistration/v1beta1/zz_generated.prerelease-lifecycle.gois excluded by!vendor/**vendor/k8s.io/api/apidiscovery/v2/generated.protois excluded by!vendor/**vendor/k8s.io/api/apidiscovery/v2/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/apidiscovery/v2/types.gois excluded by!vendor/**vendor/k8s.io/api/apidiscovery/v2beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/apidiscovery/v2beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/apidiscovery/v2beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/apiserverinternal/v1alpha1/generated.protois excluded by!vendor/**vendor/k8s.io/api/apiserverinternal/v1alpha1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/apiserverinternal/v1alpha1/types.gois excluded by!vendor/**vendor/k8s.io/api/apiserverinternal/v1alpha1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/apps/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/apps/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/apps/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/apps/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/apps/v1beta2/generated.protois excluded by!vendor/**vendor/k8s.io/api/apps/v1beta2/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/apps/v1beta2/types.gois excluded by!vendor/**vendor/k8s.io/api/authentication/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/authentication/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/authentication/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/authentication/v1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/authentication/v1alpha1/generated.protois excluded by!vendor/**vendor/k8s.io/api/authentication/v1alpha1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/authentication/v1alpha1/types.gois excluded by!vendor/**vendor/k8s.io/api/authentication/v1alpha1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/authentication/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/authentication/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/authentication/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/authentication/v1beta1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/authorization/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/authorization/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/authorization/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/authorization/v1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/authorization/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/authorization/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/authorization/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/authorization/v1beta1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/autoscaling/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2/generated.protois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/autoscaling/v2/types.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/doc.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/generated.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/register.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/zz_generated.model_name.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta1/zz_generated.prerelease-lifecycle.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/doc.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/generated.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/generated.protois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/register.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/types.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/zz_generated.deepcopy.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/zz_generated.model_name.gois excluded by!vendor/**vendor/k8s.io/api/autoscaling/v2beta2/zz_generated.prerelease-lifecycle.gois excluded by!vendor/**vendor/k8s.io/api/batch/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/batch/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/batch/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/batch/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/batch/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/batch/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/certificates/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/certificates/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/certificates/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/certificates/v1alpha1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/certificates/v1beta1/generated.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/certificates/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/certificates/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/certificates/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/certificates/v1beta1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/certificates/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/k8s.io/api/certificates/v1beta1/zz_generated.prerelease-lifecycle.gois excluded by!vendor/**vendor/k8s.io/api/coordination/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/coordination/v1alpha2/generated.protois excluded by!vendor/**vendor/k8s.io/api/coordination/v1alpha2/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/coordination/v1alpha2/types.gois excluded by!vendor/**vendor/k8s.io/api/coordination/v1beta1/generated.protois excluded by!vendor/**vendor/k8s.io/api/coordination/v1beta1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/coordination/v1beta1/types.gois excluded by!vendor/**vendor/k8s.io/api/core/v1/generated.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/core/v1/generated.protois excluded by!vendor/**vendor/k8s.io/api/core/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/k8s.io/api/core/v1/types.gois excluded by!vendor/**vendor/k8s.io/api/core/v1/types_swagger_doc_generated.gois excluded by!vendor/**vendor/k8s.io/api/core/v1/zz_generated.deepcopy.gois excluded by!vendor/**vendor/k8s.io/api/core/v1/zz_generated.model_name.gois excluded by!vendor/**vendor/k8s.io/api/discovery/v1/generated.protois excluded by!vendor/**
📒 Files selected for processing (9)
go.modserver/container_list.goserver/container_stats_list.goserver/health.goserver/image_list.goserver/sandbox_list.goserver/sandbox_metrics_list.goserver/sandbox_stats_list.goserver/server.go
✅ Files skipped from review due to trivial changes (1)
- server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Contributor
Author
|
@cri-o/cri-o-maintainers PTAL |
Member
|
/approve would be good to add streams to cri-tools as well eventually to test in integration |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, haircommander The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
Author
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add server-side streaming alternatives to all List* CRI RPCs, as defined in KEP-5825 (CRIListStreaming). These new RPCs stream results one item at a time instead of returning a single large response.
The following streaming RPCs are implemented:
Each existing List handler is refactored to extract the core listing logic into an unexported helper (e.g. listContainers), which is then shared by both the unary List RPC and the new streaming RPC. The streaming handlers iterate over the helper's results and send each item individually via stream.Send.
Vendor dependencies are updated to pick up the new CRI API proto definitions and regenerated gRPC code from k8s.io/cri-api.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR implements KEP-5825.
Which issue(s) this PR fixes:
https://kep.k8s.io/5825
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Release Notes
New Features
Chores