Skip to content

Comments

Implement container_runtime playbooks and changes#6362

Merged
sdodson merged 1 commit intoopenshift:masterfrom
mgugino-upstream-stage:crt-plays
Dec 7, 2017
Merged

Implement container_runtime playbooks and changes#6362
sdodson merged 1 commit intoopenshift:masterfrom
mgugino-upstream-stage:crt-plays

Conversation

@michaelgugino
Copy link
Contributor

This commit refactors some duplicate code, removes
usage of set_fact where not needed, and reorganizes
container_runtime role to use include_role.

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 5, 2017
@michaelgugino
Copy link
Contributor Author

/retest

crio_image_tag: "latest"
RedHat:
crio_image_name: "cri-o"
crio_image_tag: "{{ openshift_crio_image_tag | default(l_crt_crio_image_tag_dict[openshift_deployment_type]) }}"
Copy link
Member

Choose a reason for hiding this comment

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

could we simply provide the full image name here (except for RedHat where there is also the tag that changes)? The docker.io images are temporary here until we have a better place for them.

In any case, this is not blocking for me as it doesn't depend on this PR, just pointing it out that later we might revisit this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's a fine idea. I wasn't sure what exactly is required for these images, seems like the variables and how we set them could use a redo.

@michaelgugino michaelgugino force-pushed the crt-plays branch 3 times, most recently from e46e644 to 7e4f522 Compare December 6, 2017 17:44
@michaelgugino
Copy link
Contributor Author

Witnessing some weirdness around the variable openshift_docker_use_system_container in this patch. Ansible seems to be arbitrarily assigning boolean values when this variable is evaluated.

I inserted some debug statements to see what's happening and also force-casting to bool.

Copy link
Member

@mtnbikenc mtnbikenc left a comment

Choose a reason for hiding this comment

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

Changes look good. Withholding the lgtm tag until the debugs have been removed.

@mtnbikenc
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2017
@mtnbikenc
Copy link
Member

As long as we have passing tests,
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2017
This commit refactors some duplicate code, removes
usage of set_fact where not needed, and reorganizes
container_runtime role to use include_role.
@openshift-merge-robot
Copy link
Contributor

/lgtm cancel //PR changed after LGTM, removing LGTM. @michaelgugino @mtnbikenc

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2017
@openshift-ci-robot
Copy link

@michaelgugino: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install_crio 2249ba3 link /test crio
ci/openshift-jenkins/install 2249ba3 link /test install

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.

Details

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.

@sdodson sdodson merged commit 33b1271 into openshift:master Dec 7, 2017
enabled: yes
state: restarted
when: openshift_use_crio

Copy link
Member

Choose a reason for hiding this comment

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

this was introduced by 39cf508 from @ashcrow. Is it still needed? Until we can assure it is not needed I think we should leave this part when CRI-O is installed

Copy link
Member

Choose a reason for hiding this comment

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

I have to admit I don't know if the SDN code changed. As @giuseppe noted, it was added because it needed to be restarted after the sdn-ovs was installed. It's possible that sdn-ovs is installed in a different area now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants