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.

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

syscontainers: fix CRI-O and container-engine installation #6316

Merged
merged 7 commits into from Dec 8, 2017

Conversation

Projects
None yet
8 participants
@giuseppe
Contributor

giuseppe commented Nov 29, 2017

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

@giuseppe giuseppe requested a review from michaelgugino Nov 29, 2017

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino

michaelgugino Nov 30, 2017

Contributor

@giuseppe I have a large refactor of this role out that covers some of this type of thing: #6297

Your feedback on that patch would be appreciated.

Contributor

michaelgugino commented Nov 30, 2017

@giuseppe I have a large refactor of this role out that covers some of this type of thing: #6297

Your feedback on that patch would be appreciated.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Nov 30, 2017

Contributor

if you are already addressing this issue in your PR, I am fine to close this one. I don't want to create merge conflicts for such a trivial change :-)

The only reason I've opened this PR is that the installation was failing for me, as openshift.common.is_node_system_container is not defined

Contributor

giuseppe commented Nov 30, 2017

if you are already addressing this issue in your PR, I am fine to close this one. I don't want to create merge conflicts for such a trivial change :-)

The only reason I've opened this PR is that the installation was failing for me, as openshift.common.is_node_system_container is not defined

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino

michaelgugino Nov 30, 2017

Contributor

@giuseppe I'm hoping to merge my PR by tomorrow. If it doesn't ship, I can merge this one and rebase, not a big deal.

Contributor

michaelgugino commented Nov 30, 2017

@giuseppe I'm hoping to merge my PR by tomorrow. If it doesn't ship, I can merge this one and rebase, not a big deal.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Nov 30, 2017

Contributor

/retest

Contributor

giuseppe commented Nov 30, 2017

/retest

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Nov 30, 2017

Contributor

@michaelgugino I've done the change you requested

Contributor

giuseppe commented Nov 30, 2017

@michaelgugino I've done the change you requested

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Dec 5, 2017

Member

/test logging

Member

ashcrow commented Dec 5, 2017

/test logging

@openshift-ci-robot openshift-ci-robot added size/S and removed size/XS labels Dec 6, 2017

@giuseppe giuseppe changed the title from crio: define and use l_is_node_system_container to syscontainers: fix CRI-O and container-engine installation Dec 6, 2017

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Dec 6, 2017

Contributor

I've pushed more patches to this PR to address also some other issues. I've not opened a new PR since the patch already present here is necessary as well.

Contributor

giuseppe commented Dec 6, 2017

I've pushed more patches to this PR to address also some other issues. I've not opened a new PR since the patch already present here is necessary as well.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe
Contributor

giuseppe commented Dec 6, 2017

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow
Member

ashcrow commented Dec 6, 2017

@@ -13,3 +13,5 @@ galaxy_info:
- cloud
dependencies:
- role: lib_utils
- role: container_runtime

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Please checkout my other patch to see the general direction that we're moving.

Container runtime will be setup as a prerequisite before openshift is deployed. It cannot be a meta-dependency of any other role.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Please checkout my other patch to see the general direction that we're moving.

Container runtime will be setup as a prerequisite before openshift is deployed. It cannot be a meta-dependency of any other role.

@@ -13,3 +13,5 @@ galaxy_info:
- cloud
dependencies:
- role: lib_utils
- role: container_runtime
- role: openshift_facts

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Not sure this is necessary here.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Not sure this is necessary here.

This comment has been minimized.

@giuseppe

giuseppe Dec 6, 2017

Contributor

I tried without openshift_facts and it was still failing

@giuseppe

giuseppe Dec 6, 2017

Contributor

I tried without openshift_facts and it was still failing

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

I'm happy to openshift_facts here if you like, that's not really going to change anything in this role, and it's essentially a 0 task import.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

I'm happy to openshift_facts here if you like, that's not really going to change anything in this role, and it's essentially a 0 task import.

@@ -375,6 +375,13 @@ def oo_split(string, separator=','):
return string.split(separator)
def oo_list_to_dict(lst, separator='='):

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Let's not do this. Let's have the user give us a dictionary and create a new variable. Since this isn't actually used by docker, no need to re-use docker's variables.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Let's not do this. Let's have the user give us a dictionary and create a new variable. Since this isn't actually used by docker, no need to re-use docker's variables.

This comment has been minimized.

@giuseppe

giuseppe Dec 6, 2017

Contributor

This is used by docker. it is used by daemon.json is the template for /etc/docker/container-daemon.json which is used by docker when running in a container. So in this case log-opts is exactly the same as openshift_docker_log_options

@giuseppe

giuseppe Dec 6, 2017

Contributor

This is used by docker. it is used by daemon.json is the template for /etc/docker/container-daemon.json which is used by docker when running in a container. So in this case log-opts is exactly the same as openshift_docker_log_options

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Okay, that sounds good then. I think in the future we should do the reverse, we've talked about the current inflexibility of the docker options as impemented. I think it would be better as a list of kv pairs, and we convert a deprecated list k=v.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Okay, that sounds good then. I think in the future we should do the reverse, we've talked about the current inflexibility of the docker options as impemented. I think it would be better as a list of kv pairs, and we convert a deprecated list k=v.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Dec 6, 2017

Contributor

@sdodson @ingvagabund could we enable container-engine in the system-containers CI tests (should be enough to openshift_docker_use_system_container=True? This would help us to catch earlier some of the issues fixed here.

Contributor

giuseppe commented Dec 6, 2017

@sdodson @ingvagabund could we enable container-engine in the system-containers CI tests (should be enough to openshift_docker_use_system_container=True? This would help us to catch earlier some of the issues fixed here.

@giuseppe

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Dec 6, 2017

Contributor

the current status is that CRI-O is completely broken, and that is causing problems to @runcom and @mrunalp. I'd suggest to merge this PR as soon as we can and rework it later

Contributor

giuseppe commented Dec 6, 2017

the current status is that CRI-O is completely broken, and that is causing problems to @runcom and @mrunalp. I'd suggest to merge this PR as soon as we can and rework it later

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Dec 6, 2017

Member

the current status is that CRI-O is completely broken, and that is causing problems to @runcom and @mrunalp. I'd suggest to merge this PR as soon as we can and rework it later

I second this.

Member

ashcrow commented Dec 6, 2017

the current status is that CRI-O is completely broken, and that is causing problems to @runcom and @mrunalp. I'd suggest to merge this PR as soon as we can and rework it later

I second this.

@ashcrow

ashcrow approved these changes Dec 6, 2017

@michaelgugino

We can implement this PR as long as we don't include container_runtime as meta to openshift_version.

@@ -375,6 +375,13 @@ def oo_split(string, separator=','):
return string.split(separator)
def oo_list_to_dict(lst, separator='='):

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Okay, that sounds good then. I think in the future we should do the reverse, we've talked about the current inflexibility of the docker options as impemented. I think it would be better as a list of kv pairs, and we convert a deprecated list k=v.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

Okay, that sounds good then. I think in the future we should do the reverse, we've talked about the current inflexibility of the docker options as impemented. I think it would be better as a list of kv pairs, and we convert a deprecated list k=v.

@@ -13,3 +13,5 @@ galaxy_info:
- cloud
dependencies:
- role: lib_utils
- role: container_runtime
- role: openshift_facts

This comment has been minimized.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

I'm happy to openshift_facts here if you like, that's not really going to change anything in this role, and it's essentially a 0 task import.

@michaelgugino

michaelgugino Dec 6, 2017

Contributor

I'm happy to openshift_facts here if you like, that's not really going to change anything in this role, and it's essentially a 0 task import.

@sdodson

This comment has been minimized.

Show comment
Hide comment
@sdodson

sdodson Dec 6, 2017

Member

We can implement this PR as long as we don't include container_runtime as meta to openshift_version.

We can get away with that if we assume that openshift_container_runtime has been executed prior to anything that includes openshift_version, right?

Member

sdodson commented Dec 6, 2017

We can implement this PR as long as we don't include container_runtime as meta to openshift_version.

We can get away with that if we assume that openshift_container_runtime has been executed prior to anything that includes openshift_version, right?

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino

michaelgugino Dec 6, 2017

Contributor

We can get away with that if we assume that openshift_container_runtime has been executed prior to anything that includes openshift_version, right?

Yes, prerequisites is now part of the CI jobs for everything that had 'install origin' as a step. I added some logic to openshift_version to skip it during prerequisites as those steps shouldn't be version dependent.

My latest patch has container_runtime role going to include_role with an empty main.yml, so the only that would be shared even if we did implement this here are the default role variables.

We have discussed previously reducing the scope of openshift-version, so I'm interested to know what pieces here are blocking for crio and system containers so we can patch those more properly.

Contributor

michaelgugino commented Dec 6, 2017

We can get away with that if we assume that openshift_container_runtime has been executed prior to anything that includes openshift_version, right?

Yes, prerequisites is now part of the CI jobs for everything that had 'install origin' as a step. I added some logic to openshift_version to skip it during prerequisites as those steps shouldn't be version dependent.

My latest patch has container_runtime role going to include_role with an empty main.yml, so the only that would be shared even if we did implement this here are the default role variables.

We have discussed previously reducing the scope of openshift-version, so I'm interested to know what pieces here are blocking for crio and system containers so we can patch those more properly.

@sdodson

This comment has been minimized.

Show comment
Hide comment
@sdodson

sdodson Dec 6, 2017

Member

So specific to this pull request we should be able to remove the openshift_container_runtime dependency from openshift_verrsion and merge this?

Member

sdodson commented Dec 6, 2017

So specific to this pull request we should be able to remove the openshift_container_runtime dependency from openshift_verrsion and merge this?

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino

michaelgugino Dec 7, 2017

Contributor

close and reopen, I think the CI was using the old job definition for crio.

Contributor

michaelgugino commented Dec 7, 2017

close and reopen, I think the CI was using the old job definition for crio.

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino

michaelgugino Dec 7, 2017

Contributor

Close/reopen again. Updated tests weren't pushed yet, so the crio options for ansible playbook were still missing. Should be present now.

Contributor

michaelgugino commented Dec 7, 2017

Close/reopen again. Updated tests weren't pushed yet, so the crio options for ansible playbook were still missing. Should be present now.

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino
Contributor

michaelgugino commented Dec 8, 2017

@michaelgugino

This comment has been minimized.

Show comment
Hide comment
@michaelgugino

michaelgugino Dec 8, 2017

Contributor

This is the last missing piece: #6394

If someone wants to review that and merge it, it should unblock crio.

Contributor

michaelgugino commented Dec 8, 2017

This is the last missing piece: #6394

If someone wants to review that and merge it, it should unblock crio.

@sdodson

This comment has been minimized.

Show comment
Hide comment
@sdodson

sdodson Dec 8, 2017

Member

/retest

Member

sdodson commented Dec 8, 2017

/retest

@ashcrow

This comment has been minimized.

Show comment
Hide comment
@ashcrow

ashcrow Dec 8, 2017

Member
ci/openshift-jenkins/install — Jenkins job failed. 

Oddly enough CI says it's still running. I'll rebase this PR while @giuseppe is AFK.

Member

ashcrow commented Dec 8, 2017

ci/openshift-jenkins/install — Jenkins job failed. 

Oddly enough CI says it's still running. I'll rebase this PR while @giuseppe is AFK.

giuseppe added some commits Nov 29, 2017

crio: define and use l_is_node_system_container
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
openshift_version: add dependency to container_runtime
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
openshift_version: define openshift_use_crio_only
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
openshift_version: add dependency to openshift_facts
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
container-engine: log-opts is a dictionary in the daemon.json file
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
container-engine: skip openshift_docker_log_driver when it is False
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
openshift_node: reintroduce restart of CRI-O.
change introduced with 39cf508

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

This comment has been minimized.

Show comment
Hide comment
@giuseppe

giuseppe Dec 8, 2017

Contributor

@ashcrow thanks. I've just pushed a rebased version ⬆️

Contributor

giuseppe commented Dec 8, 2017

@ashcrow thanks. I've just pushed a rebased version ⬆️

@sdodson

This comment has been minimized.

Show comment
Hide comment
@sdodson

sdodson Dec 8, 2017

Member

Merging based on the previously green tests and the fact that this has only been rebased. Not having this merged is blocking other work.

Member

sdodson commented Dec 8, 2017

Merging based on the previously green tests and the fact that this has only been rebased. Not having this merged is blocking other work.

@sdodson sdodson merged commit 8b48428 into openshift:master Dec 8, 2017

0 of 9 checks passed

Submit Queue Required Github CI test is not green: ci/openshift-jenkins/tox
Details
ci/openshift-jenkins/containerized Jenkins job running.
Details
ci/openshift-jenkins/extended_conformance_install_crio Jenkins job running.
Details
ci/openshift-jenkins/install Jenkins job running.
Details
ci/openshift-jenkins/logging Jenkins job running.
Details
ci/openshift-jenkins/system-containers Jenkins job running.
Details
ci/openshift-jenkins/tox Jenkins job running.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fedora/27/atomic Running tests...
@openshift-ci-robot

This comment has been minimized.

Show comment
Hide comment
@openshift-ci-robot

openshift-ci-robot Dec 8, 2017

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

Test name Commit Details Rerun command
ci/openshift-jenkins/install 40d63b7 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.

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.

openshift-ci-robot commented Dec 8, 2017

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

Test name Commit Details Rerun command
ci/openshift-jenkins/install 40d63b7 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.

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.

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Dec 8, 2017

Member

The CRI-O and Origin CI is still failing, right now with rpmbuild of openshift origin release

Member

runcom commented Dec 8, 2017

The CRI-O and Origin CI is still failing, right now with rpmbuild of openshift origin release

openshift-merge-robot added a commit that referenced this pull request Dec 11, 2017

Merge pull request #6406 from sdodson/remove-cr-dep
Automatic merge from submit-queue.

Remove container_runtime from the openshift_version

We meant to remove this before merging #6316
@adelton

This comment has been minimized.

Show comment
Hide comment
@adelton

adelton Dec 13, 2017

Contributor

Is seems this merge has caused regressions #6464.

Contributor

adelton commented Dec 13, 2017

Is seems this merge has caused regressions #6464.

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