fix: ImagePull should close http connection if there is no available data to read. by fuweid · Pull Request #9369 · containerd/containerd

fuweid · 2023-11-15T12:44:22Z

Close connection if no more data. It's to fix false alert filed by image
pull progress.

dst = OpenWriter (--> Content Store)

src = Fetch
        Open (--> Registry)
        Mark it as active request

Copy(dst, src) (--> Keep updating total received bytes)

   ^
   |  (Active Request > 0, but total received bytes won't be updated)
   v

defer src.Close()
content.Commit(dst)

Before migrating to transfer service, CRI plugin doesn't limit global
concurrent downloads for ImagePulls. Each ImagePull requests have 3 concurrent
goroutines to download blob and 1 goroutine to unpack blob. Like ext4
filesystem 1, the fsync from content.Commit may sync unrelated dirty pages
into disk. The host is running under IO pressure, and then the content.Commit
will take long time and block other goroutines. If httpreadseeker
doesn't close the connection after io.EOF, this connection will be
considered as active. The pull progress reporter reports there is no
bytes transfered and cancels the ImagePull.

The original 1-minute timeout2 is from kubelet settting. Since CRI-plugin
can't limit the total concurrent downloads, this patch is to update 1-minute
to 5-minutes to prevent from unexpected cancel.

Fixes: #8024
Alternative: #9347

Signed-off-by: Wei Fu [email protected]

k8s-ci-robot · 2023-11-15T12:44:24Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

fuweid · 2023-11-15T13:31:01Z

ping @samuelkarp @ruiwen-zhao @thaJeztah please help to review

5-minutes is from https://github.com/kubernetes/kubernetes/blob/1635c380b26a1d8cc25d36e9feace9797f4bae3c/cluster/gce/util.sh#L882 and the information provided by andyzhangx

cc @dmcgowan @ktock since I change the remote/dockers code path. Please take a look. thanks

thaJeztah · 2023-11-15T13:51:24Z

I'll try to have a look at this one, but not deeply familiar, so don't let that keep others from beating me to it 😂

Alternative: #9347

It's fine to make that Alternative, closes https://github.com/containerd/containerd/pull/9347. That way the other PR gets automatically closed if we decide for this one 👍

thaJeztah · 2023-11-15T13:53:20Z

 		EnableCDI:                false,
 		CDISpecDirs:              []string{"/etc/cdi", "/var/run/cdi"},
-		ImagePullProgressTimeout: time.Minute.String(),
+		ImagePullProgressTimeout: (5 * time.Minute).String(),


Just a quick blurb, per my comment on the other PR, it may be good to document where this timeout came from (why we chose this value); which could be a const somewhere that has the documentation on it.

Sorry, I forgot the original comment. Updated. Please take a look.

ktock · 2023-11-15T15:17:53Z

 			return n, nil
 		}
 	}
+	// close the connection if there is no more available data


I think we should have a note about the reason why we need to immediately release the resource (i.e. caller can rely on the status of the reader for monitoring purpose, etc.).

I'm also a bit curious about whether it's possible to run src.Close() before content.Commit(dst) to make sure releasing resources before the long task.

defer src.Close() content.Commit(dst)

I think we should have a note about the reason why we need to immediately release the resource (i.e. caller can rely on the status of the reader for monitoring purpose, etc.).

Thanks! Updated.

I'm also a bit curious about whether it's possible to run src.Close() before content.Commit(dst) to make sure releasing resources before the long task.

I was thinking about this. But the httpReadSeeker can reconnect if there is unexpected EOF. And the pusher has ErrReset case and the content.Copy helper supports the retry. So, we can't just close the src here.

containerd/remotes/docker/pusher_test.go

Line 164 in 7deb68f

assert.Equal(t, content.ErrReset, err)

containerd/content/helpers.go

Line 183 in 7deb68f

if errors.Is(err, ErrReset) {

henry118 · 2023-11-16T22:40:29Z

+	//
+	// NOTE:
+	//
+	// This function is ported from kubelet/dockershim's --image-pull-progress-deadline.


confused about this line of comment. which function is it referring?

Updated. Please take a look.

henry118 · 2023-11-16T22:42:51Z

+	// The CRI's imagePullProgressTimeout relies on responseBody.Close to
+	// update the process monitor's status. If the err is io.EOF, close
+	// the connection since there is no more available data.
+	if err == io.EOF {


feels like it's better to be a else if here?

Updated. PTAL

fuweid · 2023-11-17T15:32:26Z

CI fails and it needs #9392

henry118

LGTM

The new active request is filed and there is no bytes read yet when the progress reporter just wakes up. If the timeout / 2 is less than the minPullProgressReportInternal, it's easy to file false alert. We should remove the minPullProgressReportInternal limit. Fixes: containerd#8024 Signed-off-by: Wei Fu <[email protected]>

Signed-off-by: Wei Fu <[email protected]>

Close connection if no more data. It's to fix false alert filed by image pull progress. ``` dst = OpenWriter (--> Content Store) src = Fetch Open (--> Registry) Mark it as active request Copy(dst, src) (--> Keep updating total received bytes) ^ | (Active Request > 0, but total received bytes won't be updated) v defer src.Close() content.Commit(dst) ``` Before migrating to transfer service, CRI plugin doesn't limit global concurrent downloads for ImagePulls. Each ImagePull requests have 3 concurrent goroutines to download blob and 1 goroutine to unpack blob. Like ext4 filesystem [1][1], the fsync from content.Commit may sync unrelated dirty pages into disk. The host is running under IO pressure, and then the content.Commit will take long time and block other goroutines. If httpreadseeker doesn't close the connection after io.EOF, this connection will be considered as active. The pull progress reporter reports there is no bytes transfered and cancels the ImagePull. The original 1-minute timeout[2][2] is from kubelet settting. Since CRI-plugin can't limit the total concurrent downloads, this patch is to update 1-minute to 5-minutes to prevent from unexpected cancel. [1]: https://lwn.net/Articles/842385/ [2]: https://github.com/kubernetes/kubernetes/blob/release-1.23/pkg/kubelet/config/flags.go#L45-L48 Signed-off-by: Wei Fu <[email protected]>

fuweid marked this pull request as ready for review November 15, 2023 13:25

fuweid added the ok-to-test label Nov 15, 2023

fuweid requested review from dmcgowan, ktock, ruiwen-zhao, samuelkarp and thaJeztah November 15, 2023 13:27

thaJeztah reviewed Nov 15, 2023

View reviewed changes

ktock reviewed Nov 15, 2023

View reviewed changes

fuweid force-pushed the fix-pull-progress branch 2 times, most recently from eea52b3 to 2f33a9a Compare November 15, 2023 15:49

henry118 reviewed Nov 16, 2023

View reviewed changes

fuweid force-pushed the fix-pull-progress branch from 2f33a9a to 584dc70 Compare November 17, 2023 15:31

henry118 approved these changes Nov 17, 2023

View reviewed changes

samuelkarp approved these changes Nov 17, 2023

View reviewed changes

samuelkarp added this pull request to the merge queue Nov 17, 2023

samuelkarp added area/cri Container Runtime Interface (CRI) cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Nov 17, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 17, 2023

samuelkarp added this pull request to the merge queue Nov 17, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 17, 2023

fuweid added 3 commits November 18, 2023 10:23

integration: reproduce containerd#9347

7f410ae

Signed-off-by: Wei Fu <[email protected]>

fuweid force-pushed the fix-pull-progress branch from 584dc70 to 80dd779 Compare November 18, 2023 02:25

samuelkarp added this pull request to the merge queue Nov 20, 2023

Merged via the queue into containerd:main with commit f12e84b Nov 20, 2023

fuweid mentioned this pull request Nov 21, 2023

fix: increase ImagePullProgressTimeout as 10min #9347

Closed

ruiwen-zhao mentioned this pull request Nov 21, 2023

[release/1.7] fix: ImagePull should close http connection if there is no available data to read. #9409

Merged

thaJeztah added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Nov 27, 2023

fuweid deleted the fix-pull-progress branch November 28, 2023 11:39

fuweid mentioned this pull request Dec 6, 2023

Update CRI to use transfer service for image pull by default #8515

Merged

Conversation

fuweid commented Nov 15, 2023

Uh oh!

k8s-ci-robot commented Nov 15, 2023

Uh oh!

fuweid commented Nov 15, 2023

Uh oh!

thaJeztah commented Nov 15, 2023

Uh oh!

thaJeztah Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

ktock Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuweid Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

henry118 Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

henry118 Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid commented Nov 17, 2023

Uh oh!

henry118 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ktock Nov 15, 2023 •

edited

Loading