CRI-O: match version against openshift #901

runcom · 2017-12-02T11:58:16Z

@mrunalp @Kargakis @stevekuznetsov PTAL this will fix openshift/origin#17546 by pulling the correct image for the CRI-O system container

Signed-off-by: Antonio Murdaca [email protected]

runcom · 2017-12-03T08:35:50Z

Tags live here for now: https://hub.docker.com/r/runcom/cri-o-system-container/tags/ - we'll configure docker.io/crio/cri-o to rebuild those tags when needed and make the required changes here as well (cri-o/cri-o#1201)

stevekuznetsov · 2017-12-04T16:47:03Z

Historically logic like this has a habit of breaking whenever something crazy happens with versioning. I think it may be better to have a switch-case and very explicit requirements?

runcom · 2017-12-04T16:56:24Z

You mean to not concatenate strings to build up the image string for the installer? We can avoid that, but that requires us to update this repo every time a new release come up no? I think I'm missing something

stevekuznetsov · 2017-12-04T19:40:05Z

Yes, it would require it to be updated. If you think releases are very frequent then maybe it's a bad idea. If it's once per release of Origin that's usually once every couple of months and totally OK. I would rather heave a static list/mapping we have to update than logic that fails in some weird edge cases or ends up installing something unexpected. Switch/case can have a well defined default

ashcrow · 2017-12-04T21:22:37Z

sjb/config/test_cases/test_branch_origin_extended_conformance_crio.yml

@@ -118,7 +119,7 @@ extensions:
                         --inventory sjb/inventory/ \
                         -e deployment_type=origin  \
                         -e openshift_use_crio=True   \
-                         -e openshift_crio_systemcontainer_image_override=docker.io/gscrivano/cri-o-centos \
+                         -e openshift_crio_systemcontainer_image_override="docker.io/runcom/cri-o-system-container:v${crio_tag}" \


Is crio_tag being set? In output that @mrunalp passed over to @sdodson and I the end result looked like:

docker.io/runcom/cri-o-system-container:v

true. If this is the version currently used by the CI then crio_tag looks empty

This seems to be merged .... but not "merged". 😕

I just talked with @runcom He deployed it to test to see if it works. We need to debug why the tag isn't getting set.

yup, I'll debug it asap

@runcom were you able to find the issue?

I'm debugging here just right now, I'll update this once I find something

should be fixed now, I'll work on Steve's switch case for images

I've deployed this to the origin CI already so we can test if it fixes the empty crio_tag issue

runcom · 2017-12-05T15:32:02Z

@stevekuznetsov added the case statement for the crio image PTAL (deployed to the CI as well for the crio jobs)

stevekuznetsov · 2017-12-05T16:16:27Z

sjb/config/test_cases/test_branch_origin_extended_conformance_crio.yml

@@ -105,6 +105,21 @@ extensions:
      timeout: 7200
      repository: "aos-cd-jobs"
      script: |-
+        crio_tag="$( cat ./ORIGIN_PKG_VERSION | cut -d'-' -f2 | cut -d'.' -f1,2 )"


I was also interested in this being fragile. Can you use regex in the case statements?

You mean extracting something like "3.9" with a regexp from that file? I could do that yeah

fixed, now using grep

runcom · 2017-12-05T19:24:42Z

@ashcrow so I've fixed the image tag but now we have another issue https://ci.openshift.redhat.com/jenkins/job/test_branch_origin_extended_conformance_crio/185/consoleFull#54402166758b6e51eb7608a5981914356

which is not related to CRI-O

Failure summary:


  1. Hosts:    localhost
     Play:     OpenShift Health Checks
     Task:     Run health checks (install) - EL
     Message:  One or more checks failed
     Details:  check "docker_image_availability":
               One or more required container images are not available:
                   openshift/origin-docker-registry:a5c8037
               Checked with: skopeo inspect [--tls-verify=false] [--creds=<user>:<pass>] docker://<registry>/<image>
               Default registries searched: docker.io

stevekuznetsov · 2017-12-05T19:35:13Z

Are you starting the job manually? Use /test. The job expects many parameters to be set that were not provided in that run.

runcom · 2017-12-05T19:37:54Z

Are you starting the job manually? Use /test. The job expects many parameters to be set that were not provided in that run.

oh, I see, I'll just /test crio then

runcom · 2017-12-05T21:14:28Z

So it's failing again https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_crio/427/consoleFull#-190934373858b6e51eb7608a5981914356

but this time it tries to restart CRI-O but it hasn't been installed, has something changed in openshift-ansible that now forgets to "atomic install" CRI-O?

@giuseppe @ashcrow

ashcrow · 2017-12-05T21:37:57Z

@runcom not that I'm aware of ... but looking at the code it doesn't look correct. It literally installed node items but didn't install the system container 😕.

I see that the docker role has been renamed container_runtime (which makes sense) ... I wonder if something slipped through the cracks.

ashcrow · 2017-12-05T21:44:50Z

I think this may be it but I'm not sure. The role used to require the docker role, but it no longer has a dependency.

@michaelgugino / @sdodson ^^ Thoughts?

ashcrow · 2017-12-05T21:59:40Z

Looks like it merged yesterday openshift/openshift-ansible#6297

runcom · 2017-12-05T22:17:48Z

So that's another reason to test cri-o as well in openshift ansible ha

michaelgugino · 2017-12-06T14:33:45Z

Docker has been renamed to container_runtime to more accurately reflect its purpose.

This must now be called from prerequisites.yml before calling byo/config.yml

ashcrow · 2017-12-06T14:37:04Z

@michaelgugino thanks. Was there a reason this wasn't called already or is it a bug from refactoring?

giuseppe · 2017-12-06T15:02:56Z

I have some patches here, I will push them shortly.

Another thing, we should enable container-engine in the system-containers tests.

sdodson · 2017-12-12T21:35:06Z

@michaelgugino thanks. Was there a reason this wasn't called already or is it a bug from refactoring?

It's intentional. So we need to ensure those vars are set when calling prerequisites too before this PR will produce the desired outcome.

runcom · 2017-12-13T15:52:30Z

It's intentional. So we need to ensure those vars are set when calling prerequisites too before this PR will produce the desired outcome.

isn't it all fixed now? we have a bunch of PR in both origin and openshift-ansible that are blocked on this cause they're using the wrong cri-o system container image (maybe I'm missing something)

michaelgugino · 2017-12-13T18:36:04Z

Any and all variables used with any and all of the plays must be used with prerequisites.yml.

In the case of container_runtime, that is all configured during prerequisites.yml. prerequisites.yml configures all of the host-level settings for each container runtime. In the case of system containers, this is when the runtime images are pulled and configured.

I highly recommend refactoring some of the tests to use a extra_vars file instead of passing most things via cli in the script. This will help alleviate some of the issues in the future.

mrunalp · 2017-12-13T18:40:03Z

@michaelgugino Is that something which can be done in a follow-on PR?

ashcrow · 2017-12-13T19:21:56Z

@sdodson++

mrunalp · 2017-12-13T19:22:57Z

@sdodson Thanks!

mrunalp · 2017-12-13T20:16:13Z

@michaelgugino Does it look okay now with @sdodson commit?

sdodson · 2017-12-13T20:38:28Z

Yeah, the job ran with latest on master so I think this change is good to go.

Signed-off-by: Antonio Murdaca <[email protected]>

ashcrow · 2017-12-13T20:45:10Z

I opened up runcom/cri-o-system-container#1 to ensure that this is the image that is in @runcom's namespace. Pulling and looking at it I'm pretty sure it is.

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 2, 2017

runcom force-pushed the match-crio-versions branch 2 times, most recently from d3063a1 to ef1553b Compare December 3, 2017 10:57

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 Dec 3, 2017

ashcrow reviewed Dec 4, 2017

View reviewed changes

runcom force-pushed the match-crio-versions branch 2 times, most recently from 21ff45b to 8923ed9 Compare December 5, 2017 15:31

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2017

stevekuznetsov reviewed Dec 5, 2017

View reviewed changes

runcom force-pushed the match-crio-versions branch from 8923ed9 to e3fa4be Compare December 5, 2017 19:26

runcom mentioned this pull request Dec 6, 2017

Configure nodes [localhost] openshift_node : Restart cri-o openshift/origin#17622

Closed

runcom force-pushed the match-crio-versions branch from e3fa4be to bb41f5f Compare December 11, 2017 09:48

runcom mentioned this pull request Dec 12, 2017

crio: change socket path to /var/run/crio/crio.sock openshift/openshift-ansible#6449

Merged

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 13, 2017

CRI-O: match version against openshift

b8e0743

Signed-off-by: Antonio Murdaca <[email protected]>

sdodson force-pushed the match-crio-versions branch from 38f2408 to b8e0743 Compare December 13, 2017 20:39

sdodson merged commit 241b81f into openshift-eng:master Dec 13, 2017

CRI-O: match version against openshift #901

CRI-O: match version against openshift #901

Conversation

runcom commented Dec 2, 2017

runcom commented Dec 3, 2017 • edited Loading

stevekuznetsov commented Dec 4, 2017

runcom commented Dec 4, 2017

stevekuznetsov commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Dec 5, 2017 • edited Loading

stevekuznetsov commented Dec 5, 2017

runcom commented Dec 5, 2017

runcom commented Dec 5, 2017

ashcrow commented Dec 5, 2017

ashcrow commented Dec 5, 2017 • edited Loading

ashcrow commented Dec 5, 2017

runcom commented Dec 5, 2017

michaelgugino commented Dec 6, 2017

ashcrow commented Dec 6, 2017

giuseppe commented Dec 6, 2017

sdodson commented Dec 12, 2017

runcom commented Dec 13, 2017 • edited Loading

michaelgugino commented Dec 13, 2017

mrunalp commented Dec 13, 2017

ashcrow commented Dec 13, 2017

mrunalp commented Dec 13, 2017

mrunalp commented Dec 13, 2017

sdodson commented Dec 13, 2017

ashcrow commented Dec 13, 2017

runcom commented Dec 3, 2017 •

edited

Loading

runcom commented Dec 5, 2017 •

edited

Loading

ashcrow commented Dec 5, 2017 •

edited

Loading

runcom commented Dec 13, 2017 •

edited

Loading