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

Use correct container CLI for docker or cri-o #9829

Merged
merged 1 commit into from Aug 31, 2018
Merged

Use correct container CLI for docker or cri-o #9829

merged 1 commit into from Aug 31, 2018

Conversation

mtnbikenc
Copy link
Member

Image management tasks updated to use openshift_container_cli which will
be defined as either 'crictl' when using cri-o runtime or 'docker' when
using docker.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1618969

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2018
@mtnbikenc mtnbikenc force-pushed the fix-1618969 branch 2 times, most recently from 667da76 to 4397d3e Compare August 30, 2018 12:27
@mtnbikenc mtnbikenc changed the title [WIP] Use correct container CLI for docker or cri-o Use correct container CLI for docker or cri-o Aug 30, 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 Aug 30, 2018
michaelgugino
michaelgugino suggested changes Aug 30, 2018
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Just one item.

roles/openshift_facts/defaults/main.yml Outdated
@@ -43,6 +43,7 @@ repoquery_installed: "{{ (ansible_pkg_mgr == 'dnf') | ternary('dnf repoquery --l
openshift_use_crio: False
openshift_use_crio_only: False
openshift_crio_enable_docker_gc: True
openshift_container_cli: "{{ openshift_use_crio | ternary('crictl', 'docker') }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to cast openshift_use_crio to bool.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I forgot that my other change had not landed.
https://github.com/openshift/openshift-ansible/pull/9732/files#diff-3f7ba27815bc4fa8679d59a6e27e5d5cR136

@mtnbikenc
Copy link
Member Author

@michaelgugino Updated.

michaelgugino
michaelgugino approved these changes Aug 30, 2018
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2018
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

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

Rebased for merge conflicts.

@mtnbikenc
Copy link
Member Author

/test gcp-crio

@mtnbikenc
Use correct container CLI for docker or cri-o
1f87954 
Image management tasks updated to use openshift_container_cli which will
be defined as either 'crictl' when using cri-o runtime or 'docker' when
using docker.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1618969
@mtnbikenc
Copy link
Member Author

Rebased for merge conflict.

@sdodson
Copy link
Member

sdodson commented Aug 31, 2018

/retest

@sdodson
Copy link
Member

sdodson commented Aug 31, 2018

/lgtm

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

sdodson commented Aug 31, 2018

I thought crictl would work for docker too? Anyway, we can figure that out in the future.

michaelgugino
michaelgugino approved these changes Aug 31, 2018
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michaelgugino, mtnbikenc, sdodson

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [michaelgugino,mtnbikenc,sdodson]

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

@mtnbikenc
Copy link
Member Author

@sdodson I think this is more about using what is installed. If we are not using cri-o, then we don't have crictl. If nodes are only using cri-o, they would not have docker. The commands themselves should be (mostly) interchangeable.

@sdodson
Copy link
Member

sdodson commented Aug 31, 2018

@sdodson I think this is more about using what is installed. If we are not using cri-o, then we don't have crictl. If nodes are only using cri-o, they would not have docker. The commands themselves should be (mostly) interchangeable.

ok, I thought we were installing crictl all the time but maybe that didn't make it in.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@mtnbikenc
Copy link
Member Author

gcp-crio test is having a really hard time. This could be related to openshift/origin#20325

@mrunalp, thoughts?

@openshift-ci-robot
Copy link

@mtnbikenc: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/gcp-crio 1f87954 link /test gcp-crio

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@michaelgugino
Copy link
Contributor

@sdodson @mtnbikenc crictl doesn't seem to actually work with docker daemon, even though the commands are similar, they talk to different endpoints.

@sdodson sdodson merged commit 8027b1a into openshift:master Aug 31, 2018
@sdodson
Copy link
Member

sdodson commented Aug 31, 2018

crio tests had previously passed on the same commit, merging

@mtnbikenc mtnbikenc deleted the fix-1618969 branch September 4, 2018 12:44
@mtnbikenc
Copy link
Member Author

/cherrypick release-3.10

@openshift-cherrypick-robot
Copy link

@mtnbikenc: #9829 failed to apply on top of branch "release-3.10":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	playbooks/byo/calico/legacy_upgrade.yml
M	playbooks/common/openshift-cluster/upgrades/pre/verify_upgrade_targets.yml
M	roles/calico/tasks/main.yml
M	roles/etcd/tasks/static.yml
M	roles/openshift_control_plane/tasks/pre_pull.yml
M	roles/openshift_facts/defaults/main.yml
M	roles/openshift_node/tasks/prepull.yml
M	roles/openshift_node/tasks/upgrade_pre.yml
Falling back to patching base and 3-way merge...
Auto-merging roles/openshift_node/tasks/upgrade_pre.yml
Auto-merging roles/openshift_facts/defaults/main.yml
CONFLICT (content): Merge conflict in roles/openshift_facts/defaults/main.yml
Auto-merging roles/openshift_control_plane/tasks/pre_pull.yml
Auto-merging roles/etcd/tasks/static.yml
Auto-merging roles/calico/tasks/main.yml
Auto-merging playbooks/common/openshift-cluster/upgrades/pre/verify_upgrade_targets.yml
Auto-merging playbooks/byo/calico/legacy_upgrade.yml
Patch failed at 0001 Use correct container CLI for docker or cri-o

In response to this:

/cherrypick release-3.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mtnbikenc mtnbikenc mentioned this pull request Nov 21, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelgugino michaelgugino michaelgugino approved these changes

@vrutkovs vrutkovs Awaiting requested review from vrutkovs

Assignees

@sdodson sdodson

@michaelgugino michaelgugino

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mtnbikenc @openshift-bot @sdodson @openshift-ci-robot @michaelgugino @openshift-cherrypick-robot