Skip to content

Enable parallel image pulls in kubelet #266

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
openshift-merge-robot merged 1 commit into from
Sep 18, 2018
Merged

Enable parallel image pulls in kubelet #266

openshift-merge-robot merged 1 commit into from
Sep 18, 2018

Conversation

@derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented Sep 17, 2018 •
edited
Loading

Upstream disables parallel image pulls by default for backwards compatibility with storage drivers that have problems when this is enabled (devicemapper and aufs), but since we default to overlay, we should be able to run this without a problem.

@derekwaynecarr
Copy link
Member Author

derekwaynecarr commented Sep 17, 2018

/cc @sjenning @smarterclayton @eparis

@rphillips dynamic kubelet config should always ensure we allow parallel image pulls.

@derekwaynecarr derekwaynecarr changed the title Enable serial image pulls in kubelet Enable parallel image pulls in kubelet Sep 17, 2018
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 17, 2018
@derekwaynecarr derekwaynecarr changed the title Enable parallel image pulls in kubelet WIP: Enable parallel image pulls in kubelet Sep 17, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2018
@sjenning sjenning mentioned this pull request Sep 17, 2018
Merged
@derekwaynecarr derekwaynecarr changed the title WIP: Enable parallel image pulls in kubelet Enable parallel image pulls in kubelet Sep 17, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2018
@eparis
Copy link
Member

eparis commented Sep 17, 2018

/lgtm
seth knows if we need to do more...

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2018
@eparis
Copy link
Member

eparis commented Sep 17, 2018 

2018/09/17 19:30:15 Build installer failed, printing logs:
Pulling image "docker-registry.default.svc:5000/ci-op-0cftfs91/pipeline@sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a" ...
error: error pulling image docker-registry.default.svc:5000/ci-op-0cftfs91/pipeline@sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a: Cannot overwrite digest sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a

@eparis
Copy link
Member

eparis commented Sep 17, 2018

/lgtm cancel
odd the failure was about pulls

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2018
@derekwaynecarr
Copy link
Member Author

derekwaynecarr commented Sep 17, 2018

/retest

@wking
Copy link
Member

wking commented Sep 17, 2018 

Pulling image "docker-registry.default.svc:5000/ci-op-0cftfs91/pipeline@sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a" ...
error: error pulling image docker-registry.default.svc:5000/ci-op-0cftfs91/pipeline@sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a: Cannot overwrite digest sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a

This sounds like moby/moby#37781. Maybe we're hitting that same issue via CRI-O?

@derekwaynecarr
Copy link
Member Author

derekwaynecarr commented Sep 17, 2018

retest got things passing.

@mrunalp can you see if we are prone to what @wking linked?

@mrunalp
Copy link
Member

mrunalp commented Sep 17, 2018

@wking Yes, it looks similar to what we fixed in our docker. projectatomic/docker#321.

@mtrmac @runcom Can you take a look and see if we have the same issue in containers/image?

@runcom
Copy link
Member

runcom commented Sep 18, 2018

@mtrmac @runcom Can you take a look and see if we have the same issue in containers/image?

this may be the first layer of c/storage as well @nalind ptal as well

@mtrmac
Copy link
Contributor

mtrmac commented Sep 18, 2018 

error: error pulling image docker-registry.default.svc:5000/ci-op-0cftfs91/pipeline@sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a: Cannot overwrite digest sha256:eb2e7aeb67bd686929d2c328f664a4292f6aaa814189df2f32a02863e6e5f54a

This sounds like moby/moby#37781. Maybe we're hitting that same issue via CRI-O?

Seems very unlikely to me; I can’t find the message in the CRI-O / c/{image,storage} repos at all.

Also, the affected code in Moby is in the graph driver / reference store, which I don’t think was reused in the other projects; c/storage deals with names and references very differently.

Is it possible that this error comes from a Docker process?

wking
wking approved these changes Sep 18, 2018
View reviewed changes
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Is it possible that this error comes from a Docker process?

Maybe? I'd thought we'd been pure-CRI-O since #251 (landed 2018-09-04), but maybe something slipped through? Anyhow, we're green now and Docker is back out of RHCOS since openshift/os#290 (although we haven't bumped our AMI to pick that up). Anyone want to take a last look before I /lgtm?

@wking
Copy link
Member

wking commented Sep 18, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, wking

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2018
@openshift-merge-robot openshift-merge-robot merged commit 3bbf22b into openshift:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wking wking wking approved these changes

@eparis eparis Awaiting requested review from eparis

@sjenning sjenning Awaiting requested review from sjenning

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@crawford crawford Awaiting requested review from crawford

Assignees

@wking wking

@eparis eparis

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@derekwaynecarr @eparis @wking @mrunalp @runcom @mtrmac @openshift-ci-robot @openshift-merge-robot