Skip to content

Increase PCIe port limit in workshop VMs - #1073

Open
jonathan-conder wants to merge 3 commits into
feature/autoconnect-vm-mountsfrom
fix/vm-pci-port-limit
Open

jonathan-conder wants to merge 3 commits into
feature/autoconnect-vm-mountsfrom
fix/vm-pci-port-limit

Conversation

@jonathan-conder

@jonathan-conder jonathan-conder commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Description

Increases the default PCIe port limit for workshops with several SDKs. This is needed to accomodate the SDK mounts themselves, and any connected mount plugs.

With a lot of SDKs (i.e. a lot of PCIe ports), LXD's patched OVMF firmware likes to reboot the instance to reallocate enough memory for them in a contiguous block. We probably should have already supported mid-boot reboots since it's not as uncommon as you might think, e.g.: https://github.com/canonical/lxd/blob/85f897013ccdb0f8f44d434a3b655d2edcefcfae/lxd-agent/main_agent.go#L98-L113

Unfortunately LXD doesn't tell us if the instance is rebooting or just powering down; in any case we probably shouldn't wait for it to reboot indefinitely. 10s seems to be enough time, in a nested VM it only takes about 3s to go from no QEMU to "QEMU running," we don't need to wait for systemd or anything.

Self-review quick check

  • Make decisions that cost a lot to reverse explicit in the PR description.
  • Avoid nested conditions.
  • Delete dead code and redundant comments.
  • Normalise symmetries by sticking to doing identical things identically.
// one way to handle errors
if err := f(); err != nil {
   ...
}

// one way to handle multiple returns
val, err := f()
if err != nil {
   ...
}
...
  • Check that coupled code elements, files, and directories are adjacent. For example, test data is stored as close as possible to a test.
  • Put variable declaration and initialisation together.
  • Divide large expressions into digestable and self-explanatory ones. Use multiple variables if required.
  • Put a blank line between two logically different chunks of code.
  • Follow the style guide for new error messages.

Docs

Procedure:

  • I have checked and added or updated relevant documentation.
  • I have checked and added or updated relevant release notes.
  • I have included the technical author in the review.

Content:

  • Headings and titles accurately describe the content.
  • New and updated pages include correct metadata.
  • Documentation tests are added or updated where applicable (for tutorial/ and how-to/ sections).
  • Documentation follows the style guide.
  • If needed, docs/.coverage.yaml updated, coverage tags added (.. artefact).

Or:

  • I confirm the PR has no implications for documentation.

@jonathan-conder jonathan-conder self-assigned this Oct 5, 2026
@jonathan-conder
jonathan-conder changed the base branch from main to feature/autoconnect-vm-mounts October 5, 2026 05:08
@jonathan-conder
jonathan-conder added this pull request to stack #1071 October 5, 2026 05:08
@jonathan-conder
jonathan-conder force-pushed the fix/vm-pci-port-limit branch 3 times, most recently from a48176b to 54f1e99 Compare October 11, 2026 23:05
My original motivation for disabling secure boot was performance, but it
turned out not to make much difference. Was still worth doing anyway.
Originally it seemed like a no-brainer to disable this, no one is going
to attach a PS/2 keyboard to a workshop. However, it turns out this
controller is responsible for some unrelated functionality, including a
CPU reset command. This is used when the UEFI firmware in a LXD VM
detects changes in the efivar MemoryTypeInformation. The default memory
reservation is not always enough if limits.max_bus_ports is high.

The upshot is that, with lots of mount plugs, we need to enable i8042;
otherwise the CPU reset does nothing and the VM boot times out. On top
of that, we can no longer abort awaitReadyEvent when LXD reports an
instance shutdown event. Instead, we wait a few seconds to see if the
instance is restarting or just stopping for good. It should be possible
for LXD to distinguish between a reboot event and a shutdown, but it
doesn't provide that information currently (outside debug logs).
@jonathan-conder
jonathan-conder force-pushed the fix/vm-pci-port-limit branch 2 times, most recently from 3c4eede to 69da66a Compare October 13, 2026 04:09
@jonathan-conder
jonathan-conder marked this pull request as ready for review October 13, 2026 04:26
@jonathan-conder
jonathan-conder requested review from akcano and dmitry-lyfar and a lite review from Copilot October 13, 2026 04:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The VM snapshot fixture will fail, and the reboot test does not verify that startup survives a completed reboot.

2 open findings
What changed in this PR

This PR raises PCIe port and CPU limits for LXD virtual-machine workshops with many SDK mounts and allows startup to wait through an initial reboot.

Changes:

  • Scale VM port and CPU limits with the number of SDKs.
  • Add reboot and limit tests.
  • Document VM connection limits and add PCIe to the documentation wordlist.
File Description
internal/​workshop/​lxd/​tests/​integration/​workshop_test.go Adds reboot and default-port integration tests.
internal/​workshop/​lxd/​lxd_backend.go Sets VM limits and waits for startup after a reboot.
internal/​workshop/​lxd/​lxd_backend_test.go Tests VM limit calculations.
docs/​reference/​workshops.rst Describes VM port and processor limits.
docs/​how-to/​customize-workshops/​use-virtual-machines.rst Explains virtual-hardware constraints.
docs/​.custom_wordlist.txt Adds “PCIe” to the wordlist.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

}
}
maxBusPorts := min(minPorts+portsPerSdk*regularSdks, maxPorts)
cfg["limits.max_bus_ports"] = fmt.Sprint(maxBusPorts)
Comment on lines +892 to +893
_, err = fs.Stat("/var/lib/workshop-test-rebooted")
c.Assert(err, check.IsNil)

This branch has not been deployed

No deployments
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