CRI List Streaming Operations#2099
Conversation
k8s-ci-robot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bitoku The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
bitoku
force-pushed
the
cri-list
branch
2 times, most recently
from
aa96bf3 to
ad01ffb
Compare
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
Add an enable-streaming config option and --stream CLI flag to crictl. When enabled, the streaming API is used for runtime and image service list operations instead of the default unary RPCs.
Adds 17 test cases covering all 6 streaming list APIs: ListPodSandbox, ListContainers, ListContainerStats, ListPodSandboxStats, ListPodSandboxMetrics, and ListImages. Tests are skipped on containerd until it supports the streaming list APIs.
| disable-pull-on-run: Disable pulling image on run requests (default: false) | ||
| max-retries: Max retries for connecting to an explicitly set endpoint (default: 3, 0 to disable, negative for infinite)`, | ||
| max-retries: Max retries for connecting to an explicitly set endpoint (default: 3, 0 to disable, negative for infinite) | ||
| enable-streaming: Enable streaming API for list operations (default: false)`, |
There was a problem hiding this comment.
this will be a temporary setting, right? We will switch to streaming everywhere. Do you see this as "try streaming first" setting or like a "force-streaming"?
In general I see no use for this configuration parameter as crictl users should not care
|
|
||
| version, err := rc.Version(ctx, "") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| // TODO: remove once containerd supports streaming list APIs |
There was a problem hiding this comment.
let's not skip based on runtime name and will skip based on whether it works or not
| ) | ||
|
|
||
| BeforeEach(func(ctx SpecContext) { | ||
| client, err := framework.LoadCRIClientWithStreaming() |
There was a problem hiding this comment.
as far as I remember setting streaming to true will still allow to fallback to non-streaming. Is it right? If this is true, is only option for this test is to go to gRPC level?
SergeyKanzhelev
left a comment
SergeyKanzhelev
left a comment
There was a problem hiding this comment.
when I am thinking about tests for Streaming, I am thinking of a conformance tests that will exceed the size of a regular List calls. Introducing the conformance test that ensures that a certain number of pods/images/etc. is supported will be a good way to validate that List is working. It may be hard though and requires some experimentation
|
@SergeyKanzhelev That makes sense. Thank you for the review. |
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Two minor items on top of the existing review feedback.
| !.github/ | ||
| !.skills/ | ||
|
|
||
| crictl |
There was a problem hiding this comment.
/crictl at line 37 already ignores the root binary. This entry (without the leading slash) is either redundant or unintentionally broader in scope. Should be dropped.
|
|
||
| // LoadCRIClientWithStreaming creates an InternalAPIClient with streaming | ||
| // list operations explicitly enabled, regardless of the --enable-streaming flag. | ||
| func LoadCRIClientWithStreaming() (*InternalAPIClient, error) { |
There was a problem hiding this comment.
This is a near-exact duplicate of LoadCRIClient() above, differing only in the final boolean argument. Consider extracting a shared helper:
func loadCRIClient(streaming bool) (*InternalAPIClient, error) { ... }
func LoadCRIClient() (*InternalAPIClient, error) { return loadCRIClient(false) }
func LoadCRIClientWithStreaming() (*InternalAPIClient, error) { return loadCRIClient(true) }If streaming becomes the default per the redesign, this helper may not be needed at all.
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds
--streamoption to crictl, and test cases using CRIListStreaming.Skips if containerd (PR is not yet merged containerd/containerd#13187)
Which issue(s) this PR fixes:
Special notes for your reviewer:
Assisted-by: Claude
Does this PR introduce a user-facing change?