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.

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

openshift_checks: Fix incorrect list cast #5619

Merged
merged 1 commit into from
Oct 3, 2017

Conversation

ashcrow
Copy link
Member

@ashcrow ashcrow commented Oct 2, 2017

docker_image_availability casted openshift_docker_additional_registries
to a list using the list() function. If a string was returned (IE: only
a single registry added) the result would be the string split up by
component characters. This change forces a string result from get_var to
be placed inside a list. If the result is anything BUT a string the
original list() function is called on the result.

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

Resolves #5610

@ashcrow ashcrow added affects_3.7 kind/bug Categorizes issue or PR as related to a bug. labels Oct 2, 2017
@ashcrow ashcrow requested a review from sosiouxme October 2, 2017 16:12
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 2, 2017
@ashcrow
Copy link
Member Author

ashcrow commented Oct 2, 2017

bot, retest this please

@sosiouxme
Copy link
Member

@ashcrow I believe this is the change that introduced the problem: #5559

I'm guessing our CI doesn't test with additional registries; or maybe it does, and the check just passes because the images are available locally or from the default registry.

@ashcrow
Copy link
Member Author

ashcrow commented Oct 2, 2017

Atomic failure is not related to this change.

  1. Hosts:    ocp-master
     Play:     Configure containerized nodes
     Task:     restart node
     Message:  Unable to start service origin-node: Job for origin-node.service failed because the control process exited with error code.
               See "systemctl  status origin-node.service" and "journalctl  -xe" for details.

Issue is:

Oct 02 16:43:46 ocp-master oci-systemd-hook[12534]: systemdhook <debug>: Skipping as container command is /usr/local/bin/origin-node-run.sh, not init or systemd
...
Oct 02 16:46:39 ocp-master origin-node[13856]: Error response from daemon: No such container: origin-node

regs = self.get_var("openshift_docker_additional_registries", default=[])
# https://bugzilla.redhat.com/show_bug.cgi?id=1497274
# if the result was a string type, place it into a list. We must do this
# as using list() on a string will split the string into it's characters.
Copy link
Member

Choose a reason for hiding this comment

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

"its" :)

Not worth holding up the PR over.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix it anyway 😆

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 check for openshift_deployment_type == 'openshift-enterprise' and addend the enterprise image registry.

docker_image_availability casted openshift_docker_additional_registries
to a list using the list() function. If a string was returned (IE: only
a single registry added) the result would be the string split up by
component characters. This change forces a string result from get_var to
be placed inside a list. If the result is anything BUT a string the
original list() function is called on the result.

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

Signed-off-by: Steve Milner <smilner@redhat.com>
@sosiouxme
Copy link
Member

/lgtm
I don't really like that this duplicates the logic elsewhere for determining this value. It's pretty simple logic though so I think we can live with it until it's time to refactor all the role variables properly that feed into checks.

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

ashcrow commented Oct 2, 2017

@sosiouxme agreed

@ashcrow
Copy link
Member Author

ashcrow commented Oct 2, 2017

flake openshift/origin#16246

@ashcrow
Copy link
Member Author

ashcrow commented Oct 2, 2017

/test upgrade

@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 233c548 into openshift:master Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_3.7 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
Development

Successfully merging this pull request may close these issues.

5 participants