Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

Add dib image build test to hoist #354

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eggshell
Copy link
Contributor

Currently, we have no tests for dib image builds using the elements
defined in hoist. This results in untested image builds ending up
in prod. This will add a job for that.

Implements: BonnyCI/projman#219

Signed-off-by: Cullen Taylor [email protected]

@eggshell eggshell changed the title Add dib image build test to hoist [WIP] Add dib image build test to hoist Apr 18, 2017
@bonnyci
Copy link

bonnyci bot commented Apr 18, 2017

@eggshell
Copy link
Contributor Author

recheck

@bonnyci
Copy link

bonnyci bot commented Apr 18, 2017


git clone https://github.com/BonnyCI/hoist.git $HOME
export ELEMENTS_PATH=$HOME/hoist/roles/nodepool/files/etc/nodepool/elements/nodepool
disk-image-create -a amd64 -o test-ubuntu vm ubuntu
Copy link
Contributor

Choose a reason for hiding this comment

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

So we want to actually test an image build with all the elements we have in-repo. The build command we actually end up running in production is:

disk-image-create -x -t qcow2 --checksum --no-tmpfs --qemu-img-options 'compat=0.10' -o /opt/nodepool/images/ubuntu-xenial-0000000037 ubuntu-minimal vm simple-init growroot openssh-server devuser haveged pip-and-virtualenv nodepool bonnyci-nodepool

Not sure if want to hard-code that into the job definition here, parse the elements-to-test out of a 'nodepool_diskimages' variable somewhere in inventory, or just try to build an image with all elements listed in $HOME/hoist/roles/nodepool/files/etc/nodepool/elements/nodepool ?

Copy link
Contributor

@gandelman-a gandelman-a left a comment

Choose a reason for hiding this comment

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

Added some comments. Would like to get input from others on the team about whether we should be adding an extra job to our pipeline, or to continue to use the vanilla bonnyci job to keep up good dog foodin' and avoid taking advantage of features that we dont provide our users

gate_github:
- bonnyci-run-gate-multinode
- dib-build-gate

- name: BonnyCI/sandbox
check_github:
Copy link
Contributor

Choose a reason for hiding this comment

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

So we should be able to only run the dib-build-pipeline job when files in the roles/nodepool/files/etc/nodepool/elements are changed by the patch under test. See https://docs.openstack.org/infra/zuul/zuul.html#jobs

I think we just need to add an entry to the layouts "jobs" section that applies the "files:" option to the dib job.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should limit this job to only run when the elements are changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bonnyci
Copy link

bonnyci bot commented Apr 18, 2017

@@ -0,0 +1,38 @@
- publisher:
Copy link
Contributor

Choose a reason for hiding this comment

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

This publisher is already defined in bonnyci.yaml, I don't think we need to redefine it here.

keep-hierarchy: true
copy-after-failure: true

- job-template:
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not doing anything different in the two pipelines, so I think we can just create a single job rather than use a job-template to build out multiple jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jesusaurus thanks for the feedback! I updated accordingly.

@eggshell eggshell force-pushed the dib_tests branch 2 times, most recently from c631899 to 048e350 Compare April 24, 2017 00:28
@eggshell eggshell changed the title [WIP] Add dib image build test to hoist Add dib image build test to hoist Apr 24, 2017
@eggshell eggshell force-pushed the dib_tests branch 2 times, most recently from e186d21 to 7a89561 Compare April 24, 2017 18:33
@bonnyci
Copy link

bonnyci bot commented Apr 24, 2017

@bonnyci
Copy link

bonnyci bot commented Apr 24, 2017

Copy link
Contributor

@jesusaurus jesusaurus left a comment

Choose a reason for hiding this comment

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

+1 LGTM

disk-image-create -x -t qcow2 --checksum --no-tmpfs \
--qemu-img-options 'compat=0.10' \
-o /opt/nodepool/images/ubuntu-xenial \
ubuntu-minimal vm
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to specify the elements from the hoist repo that we want to include in the build and test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gandelman-a isn't that what ELEMENTS_PATH specifies above?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eggshell no, the elements path is where to find the listed elements, but this command only lists the two elements ubuntu-minimal and vm. I don't think there's a way to re-use the elements list from the nodepool_diskimages variable since it's scoped to a different host and all of these zuul jobs are files not templates, so for now the best thing might be to copy those elements into this command and add comments about keeping the two lists of elements in sync.

disk-image-create -x -t qcow2 --checksum --no-tmpfs \
--qemu-img-options 'compat=0.10' \
-o /opt/nodepool/images/ubuntu-xenial \
ubuntu-minimal vm
Copy link
Contributor

Choose a reason for hiding this comment

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

@eggshell no, the elements path is where to find the listed elements, but this command only lists the two elements ubuntu-minimal and vm. I don't think there's a way to re-use the elements list from the nodepool_diskimages variable since it's scoped to a different host and all of these zuul jobs are files not templates, so for now the best thing might be to copy those elements into this command and add comments about keeping the two lists of elements in sync.

Copy link
Contributor

@gandelman-a gandelman-a left a comment

Choose a reason for hiding this comment

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

Couple more comments.

Also, we need the job to install some distro package requirements for dib builds to work on the slave:
https://github.com/BonnyCI/hoist/blob/master/roles/nodepool/tasks/main.yml#L4

. $HOME/dib-build/bin/activate
pip install diskimage-builder

git clone https://github.com/BonnyCI/hoist.git /opt/hoist
Copy link
Contributor

Choose a reason for hiding this comment

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

The bonnyci user isn't going to have access to create this directory. Need to sudo this or create it in ~

@eggshell eggshell force-pushed the dib_tests branch 2 times, most recently from cfd6bb9 to a31ccfa Compare May 2, 2017 21:08
Copy link
Contributor

@mlangbehn mlangbehn left a comment

Choose a reason for hiding this comment

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

This looks good to me.

# with the elements specified in the nodepool role.
disk-image-create -x -t qcow2 --checksum --no-tmpfs \
--qemu-img-options 'compat=0.10' \
-o /opt/nodepool/images/ubuntu-xenial \
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there'll also be an issue with the bonnyci user writing the image to /opt/nodepool/images/ubuntu-xenial

Currently, we have no tests for dib image builds using the elements
defined in hoist. This results in untested image builds ending up
in prod. This will add a job for that.

Implements: BonnyCI/projman#219

Signed-off-by: Cullen Taylor <[email protected]>
Copy link
Contributor

@gandelman-a gandelman-a left a comment

Choose a reason for hiding this comment

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

A few more things... FWIW I'm able to test this locally in a fresh ubuntu vm by just copying the shell snippet into a file and running it there. With my comments addressed, the build still quietly fails in elements/nodepool/extra.d/60-copy-nodepool-scripts with a pushd error.

export ELEMENTS_PATH=/opt/hoist/roles/nodepool/files/etc/nodepool/elements/nodepool
# NOTE: be sure the list of nodepool elements here stays consistent
# with the elements specified in the nodepool role.
sudo disk-image-create -x -t qcow2 --checksum --no-tmpfs \
Copy link
Contributor

Choose a reason for hiding this comment

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

You loose the virtualenv environment when you sudo here, so disk-image-create is not in $PATH. Might make more sense to just write the image to /tmp or somewhere in $HOME instead?

sudo apt-get install -y debootstrap qemu-utils curl \
uuid-runtime parted kpartx

sudo git clone https://github.com/BonnyCI/hoist.git /opt/hoist
Copy link
Contributor

Choose a reason for hiding this comment

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

So cloning and using the upstream repository means we wouldn't actually be testing the proposed changes here, and instead using whats already been merged. When the test runs, it'll be running from the root of the hoist repo with the proposed PR already checked out, so you should be able to just point ELEMENTS_PATH to ./roles/nodepoole/files/etc/nodepool/elements/

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.

4 participants