8000 [release/1.4 backport] Fix missing Body.Close() calls on push to docker remote by thaJeztah · Pull Request #5717 · containerd/containerd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[release/1.4 backport] Fix missing Body.Close() calls on push to docker remote #5717

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

Jump to bottom
Merged
merged 1 commit into from
Jul 12, 2021
Merged

[release/1.4 backport] Fix missing Body.Close() calls on push to docker remote #5717

merged 1 commit into from
Jul 12, 2021

Conversation

thaJeztah
Copy link
Member
@thaJeztah thaJeztah commented Jul 12, 2021 •
edited
Loading

note: This is a slightly modified version of the commit in main, because
pusher.Commit() uses a regular http.Response, not a wrapped one.

backport of #5712

Discovered this while using HTTP tracing via OpenTelemetry inside of
buildkitd, where the trace spans were not being reported for the
registry PUT http requests. The spans are only reported on the Close
for the Body, after adding these Close calls, the spans are reported as
expected.

Signed-off-by: coryb cbennett@netflix.com
(cherry picked from commit 894b6ae)
Signed-off-by: Sebastiaan van Stijn github@gone.nl

@theopenlab-ci
Copy link
theopenlab-ci bot commented Jul 12, 2021

Build succeeded.

@thaJeztah
Copy link
Member Author

Ah, hmmm.. perhaps not needed in 1.4.x, or in a slightly different fashion;

level=warning msg="[runner] Can't run linter goanalysis_metalinter: vrp: failed prerequisites: buildssa@github.com/containerd/containerd/runtime/restart"
level=warning msg="[runner] Can't run linter unused: buildssa: analysis skipped: errors in package: [/home/runner/work/containerd/containerd/src/github.com/containerd/containerd/remotes/docker/pusher.go:342:13: resp.Response undefined (type *http.Response has no field or method Response) -: could not load export data: no export data for \"github.com/containerd/containerd/remotes/docker\"]"
level=error msg="Running error: buildssa: analysis skipped: errors in package: [/home/runner/work/containerd/containerd/src/github.com/containerd/containerd/remotes/docker/pusher.go:342:13: resp.Response undefined (type *http.Response has no field or method Response) -: could not load export data: no export data for \"github.com/containerd/containerd/remotes/docker\"]"
Makefile:129: recipe for target 'check' failed
make: *** [check] Error 3
Error: Process completed with exit code 2.

@coryb @thaJeztah
[release/1.4] Fix missing Body.Close() calls on push to docker remote
591744a 
> note: This is a slightly modified version of the commit in main, because
> pusher.Commit() uses a regular http.Response, not a wrapped one.

Discovered this while using HTTP tracing via OpenTelemetry inside of
buildkitd, where the trace spans were not being reported for the
registry PUT http requests.  The spans are only reported on the Close
for the Body, after adding these Close calls, the spans are reported as
expected.

Signed-off-by: coryb <cbennett@netflix.com>
(cherry picked from commit 894b6ae)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the 1.4_backport_docker_push_close_body branch from f219b40 to 591744a Compare July 12, 2021 19:09
@theopenlab-ci
Copy link
theopenlab-ci bot commented Jul 12, 2021

Build succeeded.

@thaJeztah thaJeztah marked this pull request as draft July 12, 2021 19:09
thaJeztah
thaJeztah commented Jul 12, 2021
View reviewed changes
@dmcgowan dmcgowan modified the milestones: 1.5.3, 1.4.7 Jul 12, 2021
@thaJeztah thaJeztah marked this pull request as ready for review July 12, 2021 20:29
@thaJeztah
Copy link
Member Author

could use some extra eyes on this one to verify this is all correct for the 1.4 implementation (as some things changed; see above), and this code is not trivial

mikebrow
mikebrow approved these changes Jul 12, 2021
Copy link
Member
@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

estesp
estesp approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member
@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit e6d4da0 into containerd:release/1.4 Jul 12, 2021
@thaJeztah thaJeztah deleted the 1.4_backport_docker_push_close_body branch July 12, 2021 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@estesp estesp estesp approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.7
Development

Successfully merging this pull request may close these issues.
5 participants
@thaJeztah @dmcgowan @estesp @mikebrow @coryb
0