Skip to content

lxd: don't attach config disk to focal+ LXD VMs#66

Merged
OddBloke merged 3 commits into
canonical:masterfrom
OddBloke:lxd_vm
Dec 8, 2020
Merged

lxd: don't attach config disk to focal+ LXD VMs#66
OddBloke merged 3 commits into
canonical:masterfrom
OddBloke:lxd_vm

Conversation

@OddBloke
Copy link
Copy Markdown
Contributor

@OddBloke OddBloke commented Nov 25, 2020

Per a comment from stgraber[0], the config disk is "only used for legacy images that do not have the agent". As the agent is present in focal and later, instances using these profiles do not need to attach the config disk. (This is pertinent because until https://github.com/lxc/lxd/pull/8182 is released, we cannot use network configuration via the config disk.)

This change also switches us to using the KVM disk images for focal and later, for full agent support.

[0] https://github.com/lxc/lxd/issues/7292#issuecomment-733143079

This reduces our boilerplate a little, and will help with future
changes.
We need the config disk attached so that cloud-init will run within the
VM and install the agent.  If the agent is builtin, we don't need the
config disk because the agent will take care of writing out a seed
directory.
@OddBloke OddBloke changed the title lxd/defaults.py: don't attach config disk to LXD VMs lxd: don't attach config disk to focal+ LXD VMs Dec 7, 2020
Comment thread pycloudlib/lxd/cloud.py
DISK1_HASH_KEY = "combined_disk1-img_sha256"
DISK_KVM_HASH_KEY = "combined_disk-kvm-img_sha256"

def _image_info(self, image_id, image_hash_key=None):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely happy with this; we have a strange circularity in the code currently, where _get_image_hash_key is called twice: the first time with the Ubuntu release specified, and the second time with None (or, rather, without a value).

The fact that we need to use different keys for different releases means that there isn't a sensible default to return for None: if we return DISK1_HASH_KEY, then we will see failures on focal+ (because the fingerprint looked up previously will be the DISK_KVM_HASH_KEY value for the given image, meaning that we won't find an image with DISK1_HASH_KEY == fingerprint), and vice versa if we return DISK_KVM_HASH_KEY.

That means that, in turn, we need this implementation of _image_info to handle trying each of the potential hash keys which it could be. This leads to two extra stream lookups when launching a pre-focal image:

DEBUG:pycloudlib.cloud:finding released Ubuntu image for bionic
DEBUG:pycloudlib.streams:looking for image with the following config:
DEBUG:pycloudlib.streams:{'filters': [datatype = image-downloads [none=], ftype = lxd.tar.xz [none=], arch = amd64 [none=], release = bionic [none=]]}
DEBUG:pycloudlib.cloud:finding image serial for LXD Ubuntu image ubuntu:e0c589c3e37dd839cf45cf4a37210e13a708f5ef914d04439c83f2dc66fe1274
DEBUG:pycloudlib.streams:looking for image with the following config:
DEBUG:pycloudlib.streams:{'filters': [combined_disk-kvm-img_sha256 = e0c589c3e37dd839cf45cf4a37210e13a708f5ef914d04439c83f2dc66fe1274 [none=]]}
DEBUG:pycloudlib.streams:looking for image with the following config:
DEBUG:pycloudlib.streams:{'filters': [combined_disk1-img_sha256 = e0c589c3e37dd839cf45cf4a37210e13a708f5ef914d04439c83f2dc66fe1274 [none=]]}
Image serial: 20201125
DEBUG:pycloudlib.streams:looking for image with the following config:
DEBUG:pycloudlib.streams:{'filters': [combined_disk-kvm-img_sha256 = e0c589c3e37dd839cf45cf4a37210e13a708f5ef914d04439c83f2dc66fe1274 [none=]]}
DEBUG:pycloudlib.streams:looking for image with the following config:
DEBUG:pycloudlib.streams:{'filters': [combined_disk1-img_sha256 = e0c589c3e37dd839cf45cf4a37210e13a708f5ef914d04439c83f2dc66fe1274 [none=]]}
DEBUG:pycloudlib.cloud:The profile named pycloudlib-vm-bionic already exists
The profile named pycloudlib-vm-bionic already exists
DEBUG:pycloudlib.cloud:Full release to launch: 'ubuntu:e0c589c3e37dd839cf45cf4a37210e13a708f5ef914d04439c83f2dc66fe1274'

Without a broader refactor, I expect this is unavoidable one way or another.

(I suspect that we should be carrying around some different identifier: perhaps the fingerprint of the LXD tarball which is used for both, or perhaps a tuple of (hash_key, image_id), and then looking up the specific fingerprint we need only when we need it. That's probably a fair refactor though, so I've done this minimal workaround for now.)

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

I think your concern is valid, and I agree that it's ripe for a refactor, but it looks to be the same thing the lxd container is doing too, so it seems to fit in with what was already happening.

@OddBloke OddBloke merged commit 3247632 into canonical:master Dec 8, 2020
@OddBloke OddBloke deleted the lxd_vm branch December 8, 2020 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants