KEP-3673: Promote Parallel Image Pull Limit to Beta #4036

ruiwen-zhao · 2023-05-25T19:55:18Z

One-line PR description: Promote Parallel Image Pull Limit to Beta

Issue link: Kubelet limit of Parallel Image Pulls #3673

Other comments:

ruiwen-zhao · 2023-05-25T19:55:28Z

cc @pacoxu

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

SergeyKanzhelev

/lgtm

pacoxu · 2023-05-26T02:07:56Z

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

@@ -597,13 +608,19 @@ rollout. Similarly, consider large clusters and how enablement/disablement
 will rollout across nodes.
 -->

+This is an opt-in feature, and it does not change any default behavior. If there is any bug in this feature, image pulls might fail.
+No running workloads will be imapcted.
+


If I understand correctly, the user may bypass the limit when the node manager is constantly restarting kubelet.

The cluster/Node manager may restart the kubelet when changing the kubelet configuration to do rollout/rollback.

Not sure if we should mention it.

Thanks! I added a paragraph here mentioning that restarting kubelet might allow more image pulls than the limit

pacoxu · 2023-05-26T02:08:25Z

Overall LGTM
/approve

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

ruiwen-zhao · 2023-05-26T17:13:29Z

/assign wojtek-t

for PRR approval

bart0sh · 2023-05-29T06:14:55Z

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

@@ -345,9 +346,15 @@ This can inform certain test coverage improvements that we want to do before
 extending the production code to implement this enhancement.
 -->

-New unit test will be added to image_manager_test.go.
+New unit test is be added to image_manager_test.go.


good catch. I was meant to say that new unit test is added already. Updated.

pacoxu

/lgtm

pacoxu · 2023-05-30T02:01:23Z

/test pull-enhancements-verify
/test pull-enhancements-test

bart0sh · 2023-05-30T07:33:54Z

@ruiwen-zhao please fix CI test failures.

wojtek-t

Couple smaller comments, but overall it LGTM

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

wojtek-t · 2023-06-01T07:38:46Z

@ruiwen-zhao - please also remember to have the issue opted-in to the release

ruiwen-zhao · 2023-06-01T18:00:27Z

@wojtek-t thanks for the review!

@ruiwen-zhao - please also remember to have the issue opted-in to the release

It doesn't seem that I have the permission to add milestone labels to issues: #3673 (comment) Is there anything I can do to opt-in the issue?

wojtek-t · 2023-06-01T18:07:25Z

It doesn't seem that I have the permission to add milestone labels to issues: #3673 (comment) Is there anything I can do to opt-in the issue?

You need to ask SIG-lead to opt-in the feature for milestone.
@SergeyKanzhelev - can you help with it?

wojtek-t

@ruiwen-zhao - just two super minor comments, but PRR already looks good now.

/approve PRR

wojtek-t · 2023-06-02T10:54:19Z

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

@@ -345,8 +346,14 @@ This can inform certain test coverage improvements that we want to do before
 extending the production code to implement this enhancement.
 -->

-New unit test will be added to image_manager_test.go.
- `k8s.io/kubernetes/pkg/kubelet/images/puller.go`: `01/05/2023` - `100.0`
+New unit test is added to image_manager_test.go along with Alpha implementation.


Plesae mark the box in L315.

wojtek-t · 2023-06-02T10:55:00Z

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

@@ -374,7 +381,9 @@ https://storage.googleapis.com/k8s-triage/index.html
 We expect no non-infra related flakes in the last month as a GA graduation criteria.
 -->

-A new node_e2e test with  `serialize-image-pulls==false` will be added to make sure that when maxParallelImagePulls is reached, all further image pulls will be blocked.
+A new node_e2e test with  `serialize-image-pulls==false` will be added test parallel image pull limits. 


L368 - please just say that we don't need integration tests - e2e tests will cover all we need

SergeyKanzhelev

/lgtm

pacoxu · 2023-06-03T00:42:32Z

/assign @dchen1107 @mrunalp

SergeyKanzhelev · 2023-06-09T19:00:42Z

@mrunalp you are listed as an approver on this KEP. Please take a look

mrunalp · 2023-06-09T22:38:38Z

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

-A new node_e2e test with  `serialize-image-pulls==false` will be added to make sure that when maxParallelImagePulls is reached, all further image pulls will be blocked.
+A new node_e2e test with  `serialize-image-pulls==false` will be added test parallel image pull limits. 
+1. When maxParallelImagePulls is reached, all further image pulls will be blocked. 
+2. Verify the behavior when the same image is pulled in parallel, which will happen when image pull policy is `Always`.


Is there logic to wait requests for an image pull of a pull already in progress?

Not sure if I understand the question, but my plan here is to verify the behavior based on the timestamp of image pulling events.

So for the first case, we will check if there are N pulling events started roughly at the same time.

for the second case, we will check if there are two pulling events for the same image.

mrunalp · 2023-06-09T22:40:59Z

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md

+  - Event Reason: Pulling
+
+Assuming `MaxParallelImagePulls` is set to _X_, an operator can look at the container runtime log, and see _X_ PullImageRequests sent to container runtime at the same time.
+If the container images are of similar sizes, an operator can see k8s event and see _X_ images finish pulling at roughly the same time.


Not sure if this will be true if the images are being pulled off different registries or from different servers with different loads.

yeah this will only be true when the pulling takes roughly same time for all the images. If different pulls take different amount of time, then an operator will not be able to deduce the parallel behavior.

Updated the KEP to reflect this.

Signed-off-by: ruiwen-zhao <ruiwen@google.com>

mrunalp · 2023-06-10T02:56:45Z

/approve
/lgtm

k8s-ci-robot · 2023-06-10T02:56:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, pacoxu, ruiwen-zhao, SergeyKanzhelev, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-node/OWNERS~~ [mrunalp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels May 25, 2023

k8s-ci-robot requested review from dchen1107 and derekwaynecarr May 25, 2023 19:55

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 25, 2023

ruiwen-zhao force-pushed the parallel-beta branch from 96cc146 to 646038c Compare May 25, 2023 20:42

SergeyKanzhelev reviewed May 25, 2023

View reviewed changes

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md Show resolved Hide resolved

ruiwen-zhao force-pushed the parallel-beta branch from 646038c to 571b307 Compare May 25, 2023 21:42

SergeyKanzhelev approved these changes May 25, 2023

View reviewed changes

k8s-ci-robot assigned SergeyKanzhelev May 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2023

pacoxu reviewed May 26, 2023

View reviewed changes

wojtek-t reviewed May 26, 2023

View reviewed changes

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md Show resolved Hide resolved

ruiwen-zhao force-pushed the parallel-beta branch from 571b307 to c1b2d53 Compare May 26, 2023 17:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2023

k8s-ci-robot assigned wojtek-t May 26, 2023

bart0sh reviewed May 29, 2023

View reviewed changes

ruiwen-zhao force-pushed the parallel-beta branch from c1b2d53 to 9094218 Compare May 29, 2023 16:28

pacoxu approved these changes May 30, 2023

View reviewed changes

k8s-ci-robot assigned pacoxu May 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023

ruiwen-zhao force-pushed the parallel-beta branch from 9094218 to 28ef18c Compare May 30, 2023 16:05

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023

ruiwen-zhao force-pushed the parallel-beta branch from 28ef18c to 3017cfa Compare May 30, 2023 18:48

wojtek-t reviewed Jun 1, 2023

View reviewed changes

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md Show resolved Hide resolved

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md Outdated Show resolved Hide resolved

keps/sig-node/3673-kubelet-parallel-image-pull-limit/README.md Show resolved Hide resolved

ruiwen-zhao force-pushed the parallel-beta branch from 3017cfa to 0fa21fe Compare June 1, 2023 17:58

wojtek-t reviewed Jun 2, 2023

View reviewed changes

SergeyKanzhelev approved these changes Jun 2, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2023

k8s-ci-robot assigned dchen1107 and mrunalp Jun 3, 2023

mrunalp reviewed Jun 9, 2023

View reviewed changes

Promote Parallel Image Pull Limit to Beta

d1a39f1

Signed-off-by: ruiwen-zhao <ruiwen@google.com>

ruiwen-zhao force-pushed the parallel-beta branch from 0fa21fe to d1a39f1 Compare June 9, 2023 23:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2023

k8s-ci-robot merged commit 0295de2 into kubernetes:master Jun 10, 2023

k8s-ci-robot added this to the v1.28 milestone Jun 10, 2023

ruiwen-zhao mentioned this pull request Jun 13, 2023

Kubelet limit of Parallel Image Pulls #3673

Open

16 tasks

pacoxu mentioned this pull request Sep 14, 2023

update kubelet parallel image pull limit for v1.29 #4173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3673: Promote Parallel Image Pull Limit to Beta #4036

KEP-3673: Promote Parallel Image Pull Limit to Beta #4036

ruiwen-zhao commented May 25, 2023

ruiwen-zhao commented May 25, 2023

SergeyKanzhelev left a comment

pacoxu May 26, 2023

ruiwen-zhao May 26, 2023 •

edited

Loading

pacoxu commented May 26, 2023

ruiwen-zhao commented May 26, 2023

bart0sh May 29, 2023

ruiwen-zhao May 29, 2023

pacoxu left a comment

pacoxu commented May 30, 2023

bart0sh commented May 30, 2023

wojtek-t left a comment

wojtek-t commented Jun 1, 2023

ruiwen-zhao commented Jun 1, 2023

wojtek-t commented Jun 1, 2023

wojtek-t left a comment

wojtek-t Jun 2, 2023

wojtek-t Jun 2, 2023

SergeyKanzhelev left a comment

pacoxu commented Jun 3, 2023

SergeyKanzhelev commented Jun 9, 2023

mrunalp Jun 9, 2023

ruiwen-zhao Jun 9, 2023

mrunalp Jun 9, 2023

ruiwen-zhao Jun 9, 2023 •

edited

Loading

mrunalp commented Jun 10, 2023

k8s-ci-robot commented Jun 10, 2023

KEP-3673: Promote Parallel Image Pull Limit to Beta #4036

KEP-3673: Promote Parallel Image Pull Limit to Beta #4036

Conversation

ruiwen-zhao commented May 25, 2023

ruiwen-zhao commented May 25, 2023

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruiwen-zhao May 26, 2023 • edited Loading

Choose a reason for hiding this comment

pacoxu commented May 26, 2023

ruiwen-zhao commented May 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacoxu left a comment

Choose a reason for hiding this comment

pacoxu commented May 30, 2023

bart0sh commented May 30, 2023

wojtek-t left a comment

Choose a reason for hiding this comment

wojtek-t commented Jun 1, 2023

ruiwen-zhao commented Jun 1, 2023

wojtek-t commented Jun 1, 2023

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

pacoxu commented Jun 3, 2023

SergeyKanzhelev commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruiwen-zhao Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

mrunalp commented Jun 10, 2023

k8s-ci-robot commented Jun 10, 2023

ruiwen-zhao May 26, 2023 •

edited

Loading

ruiwen-zhao Jun 9, 2023 •

edited

Loading