-
Notifications
You must be signed in to change notification settings - Fork 200
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
Switch Pulpcore testing to using puppet-pulpcore test suite #1729
Conversation
pipelines/pulpcore/03-tests.yml
Outdated
BEAKER_HYPERVISOR: "docker" | ||
BEAKER_provision: "yes" | ||
BEAKER_setfile: "centos8-64{hostname=centos8-64.example.com}" | ||
BEAKER_destroy: "no" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not destroying them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes debugging easier if it sticks around. We end up destroying the box anyway so no harm in keeping it.
pipelines/vars/forklift_pulpcore.yml
Outdated
@@ -1,9 +1,11 @@ | |||
forklift_name: "pipe-pulp-{{ pipeline_version }}-{{ pipeline_os }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you have to define that? it should come from install_base.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The should did not pan out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Weird. (I mean, you stopped including it, but still)
pipelines/pulpcore/03-tests.yml
Outdated
args: | ||
chdir: /src/puppet-pulpcore | ||
environment: | ||
BEAKER_HYPERVISOR: "docker" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like that we spin up a centos box just to run a centos container inside, especially as our primary way of deployment is not a container.
Shouldn't beaker default to using the machine it's running on when we do not set BEAKER_HYPERVIRSOR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue it's not just to run a container inside -- we also install dependencies needed for the puppet module to perform testing: https://github.com/theforeman/forklift/pull/1729/files#diff-dec5007e2a651ace599f5154ceb5187aa75fb8373133bac84a45fd15840fbe9dR19-R41
Especially for our CI, that means we have a clean environment every time (yes, I know CentOS CI starts with a clean box and thus we could shorten this). Locally, this also means we have a clean environment every time. And, it's consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair.
I was also thinking about BEAKER_HYPERVISOR: vagrant
which would fit nicely if we wouldn't run this all from inside a vagrant box already.
It's hypervisors all the way down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could possibly do container-inception as well, however, I was not trying to re-invent the wheel here. Rather my goal was to switch away from the Ansible based Pulp installer and to using our own test suite that we vet a working Pulp against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today we spin up very expensive bare metal boxes so we can use virtualization. If we don't, it's much cheaper for CentOS CI if we request a VM. So I'd say we either go with BEAKER_HYPERVISOR=vagrant_libvirt
so we can also test SELinux (which we can't inside containers) or enhance our Duffy integration so it can request VMs via a parameter.
Today that's hardcoded here:
https://github.com/theforeman/jenkins-jobs/blob/d78f0f3b828444b109105fa5d27927be3f158c31/centos.org/ansible/provision_duffy.yml#L6
Perhaps also a good excuse to address theforeman/foreman-infra#1828.
Overall I think this will work, and is down the line the more meaningful test (as it uses the same codebase as we use for prod deployments later on). What is the plan wrt older Pulpcore releases? Those do not reside on stagingyum and often can't be tested with |
That is part of the reason this is in draft :) So I appreciate you having thought it through and thrown out an idea. I think keeping the status quo for prior releases is the easiest option. |
BTW, I was intending to vet this model on new ground first by doing it with Candlepin if you want to review that first -- #1712 |
We could also be adventurous. According to https://github.com/theforeman/forklift/blob/master/vagrant/config/versions.yaml we have only to care for 3.22+, so really 3.22, 3.28 and 3.39 (after this whole change). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may end up with a circular dependency here. puppet-pulpcore tests its CI against nightly, but you need puppet-pulpcore to pass to promote nightly.
pipelines/pulpcore/02-install.yml
Outdated
- pipeline_version == 'nightly' or pipeline_version is version('3.28', '>=') | ||
- pipeline_os is defined | ||
- pipeline_os is search("centos8-stream") | ||
- name: Install podman-docker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used beaker-docker a few times with plain podman. How I do it on my Fedora 38 box:
$ systemctl start --user podman.socket
$ export DOCKER_HOST=unix:///run/user/$UID/podman/podman.sock
Does it make sense to introduce a podman role for this?
pipelines/pulpcore/02-install.yml
Outdated
- libyaml-devel | ||
state: installed | ||
|
||
- name: Clone puppet-pulpcore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to add a parameter to select the branch. In git master we may drop support for a version that still has pipelines. If we can then pin it to some stable branch it should still be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could include the role from #1580 and then set the source repository and any number of desired remotes and branches in pipelines/vars/forklift_pulpcore.yml
That handles the bundle install as well if you add install_gems: true
We certainly will (even tho in many cases in the past adding a new version was more something optional, as it enabled new features or something), and I guess we won't get away w/o running releases with an unmerged puppet-pulpcore branch for a moment in the case where it's truly needed (like now with 3.39), but this seems "ok"? (I mean, right now, we point nightly/3.39 at a fork of the old installer pcreech hacked together, so same same) |
I was thinking that even though it may be circular, it would help identify if new Pulp versions / dependencies arriving via the nightly pipeline introduce a breakage we need to be aware of in puppet-pulpcore as early as possible. |
We can work around it, by pointing a PR (temporarily) at staging (theforeman/puppet-pulpcore@fa2bd4f makes that possible). My main issue is that we should acknowledge this by documenting is somewhere. |
4292cc5
to
9c3e413
Compare
Now that theforeman/puppet-pulpcore#333 was merged, we can look at this PR again. |
9c3e413
to
0df4650
Compare
8eedbcf
to
7a876ba
Compare
7a876ba
to
1e46ed7
Compare
Given we install Pulpcore using puppet-pulpcore, the idea here is to vet packaging changes against the puppet module rather than the Pulp Ansible installer.
This is in draft mode because first the puppet module needs to support configuring the baseurl in order to point the testing at
stagingyum.theforeman.org
.