Skip to content

Conversation

@rabugopsl
Copy link
Contributor

@rabugopsl rabugopsl commented Nov 19, 2018

This branch adds a disposable, general purpose containerized environment for libvirt tests.
This containerized environment is designed to free developers and new comers from having to set up the libvirt environment on their own and just focus their attention on the installation process.
It uses docker and just to create a containerized cluster in which tests and code changes may be tested.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rabugopsl
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: abhinavdahiya

If they are not already assigned, you can assign the PR to them by writing /assign @abhinavdahiya in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 19, 2018
@wking
Copy link
Member

wking commented Nov 19, 2018

This seems like it's living in a very similar space to ironcladlou/openshift4-libvirt-gcp, which is being consumed by openshift/release#2143. I think it's probably a good idea to have the image-creation tooling for the libvirt e2e tests living in this repository, but I'm not sure how far apart your code here is from @ironcladlou's. Is the plan to port the libvirt e2e tests over to this Dockerfile if/when this PR lands? CC @sallyom, who's probably also familiar with both approaches.

@rabugopsl rabugopsl force-pushed the add_libvirt_environment branch 3 times, most recently from b465174 to 89ae602 Compare November 20, 2018 13:42
@rabugopsl
Copy link
Contributor Author

This seems like it's living in a very similar space to ironcladlou/openshift4-libvirt-gcp, which is being consumed by openshift/release#2143. I think it's probably a good idea to have the image-creation tooling for the libvirt e2e tests living in this repository, but I'm not sure how far apart your code here is from @ironcladlou's. Is the plan to port the libvirt e2e tests over to this Dockerfile if/when this PR lands? CC @sallyom, who's probably also familiar with both approaches.

This environment has a different purpose. It's designed for local use instead of remote and CI integration. It's a tool to generate disposable libvirt environments for local testing, demoing and development.

@ironcladlou
Copy link
Contributor

This environment has a different purpose. It's designed for local use instead of remote and CI integration. It's a tool to generate disposable libvirt environments for local testing, demoing and development.

I don't really have a horse in this race, but want to be clear in terms of the project goals. openshift4-libvirt-gcp is absolutely for "local" development/testing. It just happens that my implementation is conducive to CI applications in that it uses Packer to construct the host image.

@rabugopsl
Copy link
Contributor Author

/test e2e-aws

Copy link
Member

Choose a reason for hiding this comment

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

Already out of date. Also, as a general rule I use registry.fedoraproject.org/fedora to bypass the Docker hub which can lag (and there's trust issues).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FROM clause has been updated accordingly

Copy link
Member

Choose a reason for hiding this comment

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

This bit shouldn't be necessary anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if clause removed

Copy link
Member

Choose a reason for hiding this comment

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

At some point the installer itself is going to pin for releases.

For development, see the code here https://github.com/ironcladlou/openshift4-libvirt-gcp/blob/master/tools/update-rhcos-image#L5

(Which is also what the installer itself is doing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind downloading the image is to have it cached locally to speed the installation by removing the need to download it each time the installer is run. The code has been updated to incorporate Dan's update process.

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to allow the user the choice of podman too.

I don't think --privileged is really necessary, just --device /dev/kvm right? Or, hmm, maybe libvirt wants to control networking.

It would be interesting to see if the installer works with a session libvirt i.e. qemu:///session - but I doubt it since it wants to set up a bridge.

Copy link
Member

Choose a reason for hiding this comment

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

(reads more) oh I see you're running libvirtd inside the container; interesting. And with the container doing a bridged network by default? Then libvirt is setting up a bridge to the bridge? Impressive that works...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --privileged flag is what allows docker to offer the full nested virtualization capabilities to the containerized environment. I tried running the container by passing all available devices and offering all possible capabilities but it turns out that docker's service enables additional hardware layers only if the said flag is enabled.
Podman was also tested as an alternative to run the container. However, the --privileged flag in podman does not offer the same hardware capabilities as docker's and the nested virtualization is not properly provided.
The containerized environment was also tested using command just exec virsh --connect qemu:///session ... and the virsh commands were able to list and retrieve all domain related information.
The reason to use this subnet is to separate the cluster's network interfaces from the host's to make every successive container run as independent as possible from the underlying host.

@rabugopsl
Copy link
Contributor Author

/test e2e-libvirt

@rabugopsl
Copy link
Contributor Author

/test e2e-aws

2 similar comments
@rabugopsl
Copy link
Contributor Author

/test e2e-aws

@rabugopsl
Copy link
Contributor Author

/test e2e-aws

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add ignore to the .gitignore for this directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use the long flags in all of these commands? I think they are more descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to mention that jq is also required.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/desgined/designed/

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to commit these credentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should tell the docker daemon to ignore the ignore directory, since the client sends the entire image over to the daemon even though it isn't used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing the following error:

Error: Failed to synchronize cache for repo 'updates'

Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing space

Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing space

@rabugopsl rabugopsl force-pushed the add_libvirt_environment branch from 2671311 to b6b9001 Compare January 4, 2019 18:40
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 4, 2019

@rabugopsl: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-libvirt 3ec5091158f11953f9010d8518c9c9d92bcca29f link /test e2e-libvirt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 4, 2019
@abhinavdahiya
Copy link
Contributor

This looks stale, also code-ready team already has a solution to test libvirt on gcp instance.

/close

@openshift-ci-robot
Copy link
Contributor

@abhinavdahiya: Closed this PR.

Details

In response to this:

This looks stale, also code-ready team already has a solution to test libvirt on gcp instance.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform/libvirt size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants