Skip to content
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

cli: do not pull again the image when using Docker #5453

Merged
merged 2 commits into from Oct 23, 2017
Merged

cli: do not pull again the image when using Docker #5453

merged 2 commits into from Oct 23, 2017

Conversation

giuseppe
Copy link
Member

When CRI-O is used and the CLI image is already pulled into Docker
then use it also for copying the CLI files to the host instead of
pulling it once again in the ostree storage.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 19, 2017
@giuseppe giuseppe force-pushed the use-docker-cli-image-if-already-available branch from d783bb1 to 5cc54e0 Compare September 19, 2017 13:26
@giuseppe
Copy link
Member Author

/retest

@giuseppe giuseppe force-pushed the use-docker-cli-image-if-already-available branch 2 times, most recently from b19fdce to 5a3d2b9 Compare September 19, 2017 13:45
@ashcrow
Copy link
Member

ashcrow commented Sep 19, 2017

I believe #5461 will need to be merged before the atomic tests can pass.

@ingvagabund
Copy link
Member

/retest

1 similar comment
@giuseppe
Copy link
Member Author

/retest

@giuseppe giuseppe force-pushed the use-docker-cli-image-if-already-available branch 2 times, most recently from 6a7da08 to 63aca90 Compare September 22, 2017 09:23
@sdodson
Copy link
Member

sdodson commented Sep 22, 2017

/retest

@ashcrow
Copy link
Member

ashcrow commented Sep 25, 2017

bot, retest this please

@ashcrow
Copy link
Member

ashcrow commented Sep 25, 2017

Infra (configuration or image availability) flake

        "Failed: Unable to find docker/openshift/origin:v3.7.0-alpha.1"

@ashcrow
Copy link
Member

ashcrow commented Sep 25, 2017

/test system-containers

@0xmichalis
Copy link
Contributor

/retest

1 similar comment
@ingvagabund
Copy link
Member

/retest

@ashcrow
Copy link
Member

ashcrow commented Sep 29, 2017

/LGTM

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

ashcrow commented Sep 29, 2017

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 29, 2017
@ashcrow
Copy link
Member

ashcrow commented Sep 29, 2017

/test system-containers

@ashcrow
Copy link
Member

ashcrow commented Sep 29, 2017 •
edited

Hrm, it looks like the openshift.common.system_images_registry is being set to "docker" when it should be an actual registry.

docker/openshift/origin:v3.7.0-alpha.1

TASK [openshift_cli : Pull CLI Image] ******************************************
task path: /usr/share/ansible/openshift-ansible/roles/openshift_cli/tasks/main.yml:29
fatal: [localhost]: FAILED! => {
    "changed": false, 
    "cmd": [
        "atomic", 
        "pull", 
        "--storage", 
        "ostree", 
        "docker/openshift/origin:v3.7.0-alpha.1"
    ], 
    "delta": "0:00:00.713754", 
    "end": "2017-09-29 11:46:19.481601", 
    "failed": true, 
    "generated_timestamp": "2017-09-29 11:46:19.502050", 
    "rc": 1, 
    "start": "2017-09-29 11:46:18.767847", 
    "stderr": [
        "Failed: Unable to find docker/openshift/origin:v3.7.0-alpha.1"
    ], 
    "stdout": []
}

@giuseppe
Copy link
Member Author

/retest

@ashcrow
Copy link
Member

ashcrow commented Oct 19, 2017

/test system-containers

@ashcrow
Copy link
Member

ashcrow commented Oct 19, 2017 

TASK [openshift_cli : Pull CLI Image] ******************************************
task path: /usr/share/ansible/openshift-ansible/roles/openshift_cli/tasks/main.yml:29
fatal: [localhost]: FAILED! => {
    "changed": false, 
    "cmd": [
        "atomic", 
        "pull", 
        "--storage", 
        "ostree", 
        "docker/openshift/origin:v3.7.0-alpha.1"
    ], 
    "delta": "0:00:00.638997", 
    "end": "2017-10-19 14:22:04.918710", 
    "failed": true, 
    "generated_timestamp": "2017-10-19 14:22:04.939619", 
    "rc": 1, 
    "start": "2017-10-19 14:22:04.279713", 
    "stderr": [
        "Failed: Unable to find docker/openshift/origin:v3.7.0-alpha.1"
    ], 
    "stdout": []
}

docker/openshift/origin:v3.7.0-alpha.1 ?? 😕

/cc @ingvagabund

@giuseppe giuseppe force-pushed the use-docker-cli-image-if-already-available branch from 63aca90 to bd53e06 Compare October 20, 2017 17:31
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@giuseppe
Copy link
Member Author

@ashcrow just rebased on top of master, if it can make any difference

@ashcrow
Copy link
Member

ashcrow commented Oct 20, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@ashcrow
Copy link
Member

ashcrow commented Oct 20, 2017

/retest

@ashcrow
Copy link
Member

ashcrow commented Oct 20, 2017

/test system-containers

@giuseppe giuseppe force-pushed the use-docker-cli-image-if-already-available branch from bd53e06 to d3da4f9 Compare October 20, 2017 22:41
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 20, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@ashcrow
Copy link
Member

ashcrow commented Oct 21, 2017

/retest

2 similar comments
@giuseppe
Copy link
Member Author

/retest

@ingvagabund
Copy link
Member

/retest

@giuseppe
cli: do not pull again the image when using Docker
26a0c90 
When CRI-O is used and the CLI image is already pulled into Docker
then use it also for copying the CLI files to the host instead of
pulling it once again in the ostree storage.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
cli: use the correct name for the master system container
f7b290d 
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the use-docker-cli-image-if-already-available branch from d3da4f9 to f7b290d Compare October 23, 2017 09:26
ingvagabund
ingvagabund approved these changes Oct 23, 2017
View reviewed changes
roles/openshift_cli/tasks/main.yml

- block:
- name: Pull CLI Image
command: >
atomic pull --storage ostree {{ openshift.common.system_images_registry }}/{{ openshift.common.cli_image }}:{{ openshift_image_tag }}
atomic pull --storage ostree {{ 'docker:' if openshift.common.system_images_registry == 'docker' else openshift.common.system_images_registry + '/' }}{{ openshift.common.cli_image }}:{{ openshift_image_tag }}
Copy link
Member

Choose a reason for hiding this comment

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

+1

@ingvagabund
Copy link
Member

/test logging

1 similar comment
@ashcrow
Copy link
Member

ashcrow commented Oct 23, 2017

/test logging

@ingvagabund
Copy link
Member

/lgtm
based on @ashcrow previous lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2017
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit be7d536 into openshift:master Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashcrow ashcrow ashcrow approved these changes

@ingvagabund ingvagabund ingvagabund approved these changes

Assignees

@ashcrow ashcrow

@ingvagabund ingvagabund

Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@giuseppe @ashcrow @ingvagabund @sdodson @0xmichalis @openshift-merge-robot @openshift-ci-robot