Skip to content
This repository was archived by the owner on Feb 7, 2023. It is now read-only.

tests: install a different package on CentOS #261

Merged

Conversation

miabbott
Copy link
Collaborator

@miabbott miabbott commented Oct 2, 2017

This PR includes two changes:

  • improved support for checking if a package has been installed or uninstalled

The rpm_ostree_install* and rpm_ostree_uninstall* roles have been enhanced so that they can be run multiple times in the same playbook. Additionally, the *verify roles can now check for a binary that does not match the package name. For example, check for ntpd when ntp is installed.

  • conditional install of the ntp package on CentOS

The version of rpm-ostree in CentOS doesn't support layering files on /boot and this is causing the install of httpd to fail because of the centos-logos dependency that gets pulled in. We'll temporarily test package layering using the ntp package on CentOS until CentOS gets a version of rpm-ostree that supports sticking files in /boot.

Closes #254

- role: rpm_ostree_install
packages: ntp
reboot: false
when: ansible_distribution == 'CentOS'
Copy link
Member

Choose a reason for hiding this comment

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

This seems OK, but isn't it more usual Ansible style to derive variables in vars.yml and then reference those?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that sounds right. I'll fix that.

@miabbott
Copy link
Collaborator Author

miabbott commented Oct 3, 2017

@miabbott
Copy link
Collaborator Author

miabbott commented Oct 4, 2017

I found an issue when running the pkg-layering test suite using this PR....investigating the root cause.

@miabbott
Copy link
Collaborator Author

miabbott commented Oct 5, 2017

The fedora/26/atomic issue should hopefully get resolved in the next 2 week release due this week; we can retry that context once the release lands.

@miabbott
Copy link
Collaborator Author

miabbott commented Oct 5, 2017

I think what is happening on centos/7/atomic is that the pkg-layering test has a section where it installs a non-root package (httpd) and uses the reboot flag. Because the reboot flag is used, an asynchronous Ansible command is used and the result is never inspected. Since installing httpd on CentOS AH will fail (the whole reason for this now massive PR), the reboot is never initiated. This causes the playbook to fail because it is waiting for the host under test to go down.

My opinion is that this particular part of the pkg-layering playbook should be left intact and the centos/7/atomic failure is ignored.

@mike-nguyen
Copy link
Collaborator

mike-nguyen commented Oct 5, 2017

I think what is happening on centos/7/atomic is that the pkg-layering test has a section where it installs a non-root package (httpd) and uses the reboot flag. Because the reboot flag is used, an asynchronous Ansible command is used and the result is never inspected. Since installing httpd on CentOS AH will fail (the whole reason for this now massive PR), the reboot is never initiated. This causes the playbook to fail because it is waiting for the host under test to go down.

My opinion is that this particular part of the pkg-layering playbook should be left intact and the centos/7/atomic failure is ignored.

I agree since its a valid failure on the test. The error in the log throws you off but that's the nature of commands that automatically initiate reboots using Ansible.

Overall this LGTM. Great work on cleaning it up and adding in additional functionality.

The `rpm_ostree_install*` and `rpm_ostree_uninstall*` roles had some
limitations when first created.  Namely, only being able to be used
once per playbook and being unable to check for a binary that did not
match the package name.

This changes the roles to use the `allow_duplicates` boolean, so that
they may be used multiple times in the same playbook.  The `*verify`
roles have also been changed to allow for checking for a different
binary name than what is used by the package.  For example, the `ntp`
package installs an `ntpd` binary, whereas the `httpd` package installs
an `httpd` binary.
In projectatomic#254, it is noted that the package layering of `httpd` isn't
working on CentOS because the `centos-logos` dependency is trying to
layer files on `/boot`.  Until CentOS gets a version of `rpm-ostree`
that supports layering on `/boot` (see coreos/rpm-ostree#969),
we need to use a different package to test package layering in CentOS.

This change brings in a conditional installation of `ntp` in the
CentOS case, until we have a newer version of `rpm-ostree` to use.
This changes how the variables used by the `improved-sanity-test` are
loaded in each playbook section.  Commonly used variables are now
found in `vars/common.yml` and are loaded for each playbook section.
Additional variables that may change per OS platform, are broken out
into separate files.

By supplying a list of files to the `vars_files:` statement, Ansible
will try to import each file in the list and stopping when a file is
found.[0]  This allows us to override any variables per OS while
maintaing sensible defaults.

[0] http://docs.ansible.com/ansible/latest/playbooks_conditionals.html#conditional-imports
When running a role multiple times in a playbook, we can run into
problems with variables since they all live in the same namespace.
Notably, when you run a role multiple times, variables that are
optional but have been defined, will carry over to the next execution
of the role.  Or if two roles have the same variable name, you can get
values stomping on each other.

This commit tries to alleviate some of these problems by updating the
variable names in the roles to be unique to the role.  Additionally,
when using roles multiple times in playbooks, both required and optional
variables are explicitly defined to avoid any overwriting.
@miabbott miabbott force-pushed the fix_rpm_ostree_install_centos_254 branch from 8dc6f6c to 329f778 Compare October 5, 2017 19:44
@miabbott
Copy link
Collaborator Author

miabbott commented Oct 5, 2017

Rebased on master and squashed a fixup in the process ⬆️

@mike-nguyen
Copy link
Collaborator

bot, retest this please

@mike-nguyen mike-nguyen merged commit 4558686 into projectatomic:master Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants