Skip to content

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

Merged
estesp merged 1 commit intocontainerd:release/1.4from
thaJeztah:1.4_backport_docker_push_close_body
Jul 12, 2021
Merged

[release/1.4 backport] Fix missing Body.Close() calls on push to docker remote#5717
estesp merged 1 commit intocontainerd:release/1.4from
thaJeztah:1.4_backport_docker_push_close_body

Conversation

@thaJeztah
Copy link
Copy Markdown
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
Copy Markdown

theopenlab-ci bot commented Jul 12, 2021

Build succeeded.

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Jul 12, 2021

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
Copy Markdown

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
Copy Markdown
Member Author

thaJeztah commented Jul 12, 2021

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
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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