Managed instance names don't take the base into account

[tigarmo]

Bug Description

The instance name currently reflects the app, project, build-on, build-for and inode of the directory, but not the project's base. What this means is that if a project's base changes (say, a Rockcraft's project build-base goes from [email protected] to [email protected] then the instance will be incorrectly reused with no warnings.

Now granted this doesn't happen too often, mostly to us when developing, but it's still a simple issue to fix.

To Reproduce

Create a project and change its effective base.

part yaml

No response

Relevant log output

.

[syncronize-issues-to-jira]

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/CRAFT-2524.

This message was autogenerated

[mr-cal]

I haven't looked into the code but I can reproduce the problem:

> lxc --project rockcraft list
+------+-------+------+------+------+-----------+
| NAME | STATE | IPV4 | IPV6 | TYPE | SNAPSHOTS |
+------+-------+------+------+------+-----------+
> sudo snap refresh rockcraft --channel=latest/edge
snap "rockcraft" has no updates available
> rockcraft init
> rockcraft pack
Packed my-rock-name_0.1_amd64.rock
> lxc --project rockcraft list
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
|                             NAME                              |  STATE  | IPV4 | IPV6 |   TYPE    | SNAPSHOTS |
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
| base-instance-rockcraft-buildd-base-v7-c-a38d05774a6de0cf6ab1 | STOPPED |      |      | CONTAINER | 0         |
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
| rockcraft-my-rock-name-on-amd64-for-amd64-18486507            | STOPPED |      |      | CONTAINER | 0         |
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
> sed -i 's/22.04/24.04/g; $a\build-base: devel' rockcraft.yaml
> rockcraft pack
The development build-base should only be used for testing purposes, as its contents are bound to change with the opening of new Ubuntu releases, suddenly and without warning.
The development build-base should only be used for testing purposes, as its contents are bound to change with the opening of new Ubuntu releases, suddenly and without warning.
Packed my-rock-name_0.1_amd64.rock
> lxc --project rockcraft list
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
|                             NAME                              |  STATE  | IPV4 | IPV6 |   TYPE    | SNAPSHOTS |
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
| base-instance-rockcraft-buildd-base-v7-c-a38d05774a6de0cf6ab1 | STOPPED |      |      | CONTAINER | 0         |
+---------------------------------------------------------------+---------+------+------+-----------+-----------+
| rockcraft-my-rock-name-on-amd64-for-amd64-18486507            | STOPPED |      |      | CONTAINER | 0         |
+---------------------------------------------------------------+---------+------+------+-----------+-----------+

[mr-cal]

I think @tigarmo is spot on. We can quickly address this in craft-application by including the base in the instance_name.

An alternative is to validate inside of craft-providers. After craft-providers creates a LXD instance, subsequent runs of craft-providers only look for an instance with that name and assume it is the correct base. They don't check the OS distro and version.

[mr-cal]

As discussed as a team, we are going to fix this in craft-providers. This code should be catching this but is not.

[syncronize-issues-to-jira]

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/CRAFT-2652.

This message was autogenerated

[mattculler]

After investigation, it was determined that the issue only occurs when the base is changed and the build-base is devel. This happens because this function doesn't perform the base version comparison when build-base is devel. In fact, the Base class hierarchy doesn't even store the OS version number for the container when using devel.

It's also less desirable to be performing this check after the instance has started, though changing the base in devel mode may be rare enough to make the larger change (storing OS version as instance metadata) not worth it.

Storing the base version in the instance name is infeasible due to name length issues. Always recreating the base instance when in devel mode would be expensive at runtime.

See also discussion in this thread.

[cmatsuoka]

We don't need to (and shouldn't) always recreate the base when it's devel, however migrating from devel to a production base is a common scenario after a release. Encoding the base in the hash solves this case, even if the base name is not human readable in the image name.

[mattculler]

@cmatsuoka, @mr-cal and I discussed the issues and came up with three possible solutions:

1. Add the base OS version to the instance name. The simplest solution. This was originally rejected as somewhat of a cop-out, but the other fields that comprise the instance name are all included for the same reason that we would be adding the base OS release version - to ensure base instances are only used when appropriate. Instance names exceeding LXD's 63-character limit are already truncated and appended with a hash of the full name, so even if and when the OS version information is truncated, it would still be performing the function of ensuring instance compatibility from inside the hash.
2. Insert the OS version into the instance config. Inside instances, there is a file at /etc/craft-instance.conf, which is abstracted as InstanceConfiguration, in craft-providers/craft_providers/instance_config.py. Base OS version information could be added to this existing facility and checked after base instance startup to verify the instance's base is correct. This solution should be nearly as simple as option # 1, but is less desirable because instances must be started before the check can be performed.
3. New instance metadata facility. In option # 2, instances must be started in order to check the InstanceConfiguration. Instead of using this, a new "instance metadata" abstraction could be created that could store and provide instance information without starting the instance. For LXC, the config facility can be used (lxc --project ... config show ...). We could add a field, user.craft_providers.osversion, which can contain the overloaded OS version field (which may be devel). (The lack of the possibility to contain devel is why the existing field image.version cannot be used.) For multipass, the closest analogue is settings, (accessed via get/set). The only one we can make use of is comment, into which we could de/serialize a YAML datastructure representing our metadata fields. This solution would require the most effort, exceeding the original scope of this issue.

Our recommendation is to go with option # 1, and to reassess around the time of release for core26.

[mattculler]

After discussion with the team, we realized that if we went with option # 1, craft-providers would end up abandoning old instances, leaving them around stale for 14 days, which is unacceptable. Instead we settled on option # 2, with a few modifications/clarifications:

• Currently, the (insufficient) base information for the existing instance is pulled from /etc/os-version in _ensure_os_compatible. (This is insufficient because it will always be "22.04", "24.04" etc and never "devel", which is the crux of the whole issue.) Instead, we should store the build-base ("22.04", "24.04", "devel", etc) on the InstanceConfig, and compare build-base from the *craft.yaml against that.
• When build-base is "devel", it doesn't matter what the actual instance OS version is. It only matters that the instance is "devel".
• This shouldn't require moving much around, since the class hierarchy in question already deals with InstanceConfiguration.
• Preliminary story point estimation at 8 (or perhaps 5).
• The InstanceConfiguration's compatibility tag will need to be updated, so new versions are recognized.
• This task is now low priority.

[mr-cal]

In addition to the base, we also need to consider the platform. For example, these should build in different containers:

base: [email protected]
platforms:
  amd64:
    build-on: amd64
    build-for: amd64
  amd64-debug:
    build-on: amd64
    build-for: amd64