-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: Add QEMU datasource #323
Conversation
@@ -56,7 +56,7 @@ def test_wait_execute_failure( | |||
): | |||
"""Test wait calls when execute command fails.""" | |||
instance = concrete_instance_cls(key_pair=None) | |||
m_time.side_effect = [1, 2, 40 * 60, 40 * 60 + 1] | |||
m_time.side_effect = [1, 1, 2, 40 * 60, 40 * 60 + 1] |
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.
Changes in this file are due to additional logging calls calling a time.time()
d015baa
to
9a21b5e
Compare
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.
LGTM, thanks for the great work here!
I left some inline concerns that shouldn't probably block this PR. The most important is that the utility function, Qemu.released_image
, does not operate with released images.
pycloudlib/qemu/cloud.py
Outdated
"Using '%s' as parent directory for all QEMU artifacts created", | ||
self.parent_dir, | ||
) | ||
self.qemu_binary = self.config.get("qemu_binary", "qemu-system-x86_64") |
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.
Can we pre-flight check the package dependencies with shutil.which
for qemu-system-x86_64, genisoimage, and qemu-img and raising a MissingPrerequisiteError on missing dependency and suggested resolution during Qemu class init so we only do this once?
pycloudlib/qemu/cloud.py
Outdated
resp = requests.get(base_url, timeout=5) | ||
resp.raise_for_status() | ||
match = re.search( | ||
r"<title>Ubuntu.*daily \[(?P<date>\d+).*</title>", resp.text |
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.
Good idea matching the date string from the page, then you don't have to look at the .manifest file to guess about changes. Historically, if multiple releases were made during a single day, a .\d
suffix was provided for the date so we may want to just match until next square bracket to handle future formatting changes ]
r"<title>Ubuntu.*daily \[(?P<date>\d+).*</title>", resp.text | |
r"<title>Ubuntu.*daily \[(?P<date>[^]]+).*</title>", resp.text |
QEMU instances require local directories to store images and instances. | ||
These directories can be configured in the `pycloudlib.toml` file: | ||
|
||
`image_dir` - Directory to look for images when launching instances. |
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.
Can we provide a mechanism to configure the base_url or base_url_template somehow? I have a use-case where I'd like to use pycloudlib to install daily live desktop installer images instead of daily server images, I also expect that other distros would like the ability to specify or override this default base_url somehow to a different external image source.
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.
Can that be a separate refactoring PR when needed? I think each page would need its own parsing as the url, title, filename, kernel-name (if even included...it's not for desktop) are all different.
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.
Yes separate PR and iteration on that should be fine and it'd be preparatory work anyway for potential support of other distros anyway so it could be folded into that type of new feature. In the desktop case, we can still curl/wget the image ourselves outside of pycloudlib and feed it into the test framework to exercise the instance lifecycle.
to let cloud-init configure the instance. If `no_seed_iso` is set to | ||
`True`, pycloudlib will not create a seed iso. This may be useful when | ||
using the `kernel_cmdline` behavior or when using an image that already | ||
has keys and/or passwords configured. |
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.
Should we continue adding code samples for various cloud features in ./examples/lifecycle_<cloud>.py
or would you prefer we stick solely to better documentation or features and config options and rely on unittest/integration tests to establish example patterns for setup, launch, teardown etc.
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 think we should only have separate code samples if the cloud provides additional API features on top of the core API. It doesn't here. I think it would be good to have an overarching example that can work on multiple clouds though.
docs/clouds/qemu.md
Outdated
|
||
Under the hood, Pycloudlib uses the QEMU Machine Protocol (QMP) to interact | ||
with running VMs. While this is generally meant to be an implementation | ||
detail, if needed,the socket file may be found in the `working_dir` with the |
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.
detail, if needed,the socket file may be found in the `working_dir` with the | |
detail, if needed, the socket file may be found in the `working_dir` with the |
pycloudlib/qemu/cloud.py
Outdated
image_id, | ||
) | ||
|
||
# We make a COW image from the base image so we can make destructive |
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 make a COW image from the base image so we can make destructive | |
# We make a QCOW image from the base image so we can make destructive |
|
||
* qemu-system-x86 | ||
* qemu-utils # for qemu-img | ||
* genisoimage # to create the cloud-init NoCloud seed image |
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.
Not for this PR:
I like that we are building in supplemental utilities necessary to setup and launch vm, i wonder longer-term if we want to support use-cases where folks want to customize these images using mount-image-callback
or virt-customize
prior to initial launch of those images. Surely, these preparatory steps can be performed outside pycloudlib and fed into the Qemu.launch(image_id="/my/customized/image")
, so probably doesn't make sense at this point. But, seeing our helper methods creating iso images, customizing base images before boot is a frequently used feature when dealing with qemu that we may want to offer in the future if possible.
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.
Yes, thanks for mentioning these tools. I tried myself with qemu-nbd and guestmount, but ran into some brick walls.
self, kernel_path, kernel_cmdline: str, image_id, base_image | ||
) -> Optional[Path]: | ||
kernel = None | ||
if kernel_cmdline and not kernel_path: |
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 are already performing this check outside this scope at the call-site. Do you want to leave this check in here and remove the one at the call-site and we can just set or return the original kernel_path
from this method?
def launch( | ||
self, | ||
image_id: str, | ||
instance_type="c1m1500", |
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.
Encoding cpu and memory size is reasonable for this first pass. But, we may end up painting ourselves into a corner as far as customization of -smp as other parameters overrides may be desired which would modify sockets/cores/threads/maxcpus and I don't know off-hand how we'd support that.
Do we need logic to replace existing -smp
qemu_args if launch_params
specifies competing -smp
config?
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.
Agreed. I had a hard time figuring out the best way to do this. There are tons of options we could support, and I didn't want to go about including all of them in the launch api. I figure more can be added or we can further encode the "instance_type" string as needed. I did include a launch_args
parameter that just appends its arguments to the end of the qemu call, though I consider it kind of a hack.
@blackboxsw , I'm not sure if I understand exactly what you're asking for wrt to |
# use NoCloud and create a seed iso, otherwise we have no | ||
# way of accessing our instance | ||
seed_path = self._create_seed_iso( | ||
instance_dir=instance_dir, |
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.
Do we want the opportunity to provide network_config param at launch and inject it into the seed ISO as well for NoCloud datasource? I can think of more complex cases where we may want to add mutiple nics and or setup bridges/bonds etc and validate this configuration within a qemu instance. We can see something like this historically done in certain SRU tests https://github.com/cloud-init/ubuntu-sru/blob/main/bugs/1682871/btest-launch.sh
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.
Yes, good idea
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.
Added in most recent push
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.
LGTM. One nit on param name network_config
instead of network_data
+1 either way.
pycloudlib/qemu/cloud.py
Outdated
user_data: Optional[str], | ||
meta_data: Optional[str], | ||
vendor_data: Optional[str] = None, | ||
network_data: Optional[str] = None, |
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.
Shouldn't we call this network_config because that is the name of the configuration that cloud-init is consuming?
No description provided.