Skip to content
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

Add automatic snapshot feature for QEMU VMs #775

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nesitor
Copy link
Member

@nesitor nesitor commented Mar 11, 2025

Create automatic Qemu snapshots every 10 minutes

Related ClickUp, GitHub or Jira tickets : ALEPH-XXX

Self proofreading checklist

  • The new code clear, easy to read and well commented.
  • New code does not duplicate the functions of builtin or popular libraries.
  • An LLM was used to review the new code and look for simplifications.
  • New classes and functions contain docstrings explaining what they provide.
  • All new code is covered by relevant tests.
  • Documentation has been updated regarding these changes.
  • Dependencies update in the project.toml have been mirrored in the Debian package build script packaging/Makefile

Changes

  • Add automatic snapshots for QEMU VMs every 10 minutes by default
  • Create QemuSnapshotManager to handle QEMU VM snapshot scheduling
  • Implement create_snapshot method in AlephQemuInstance using QMP
  • Keep only one active snapshot at a time, deleting previous ones
  • Make QEMU confidential VMs also support snapshots
  • Update pool and execution classes to use appropriate snapshot manager

🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]

How to test

Check if a snapshot is created every 10 minutes

- Add automatic snapshots for QEMU VMs every 10 minutes by default
- Create QemuSnapshotManager to handle QEMU VM snapshot scheduling
- Implement create_snapshot method in AlephQemuInstance using QMP
- Keep only one active snapshot at a time, deleting previous ones
- Make QEMU confidential VMs also support snapshots
- Update pool and execution classes to use appropriate snapshot manager

🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
@nesitor nesitor requested a review from olethanh March 11, 2025 12:16
@nesitor nesitor self-assigned this Mar 11, 2025
Copy link
Collaborator

@Psycojoker Psycojoker left a comment

Choose a reason for hiding this comment

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

LGTM in general

Shouldnt' we add at least a few additional tests?

"""
try:
logger.debug(f"Creating snapshot {snapshot_name} for VM {self.vm.vm_id}")
self.qmp_client.command("savevm", **{"name": snapshot_name})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.qmp_client.command("savevm", **{"name": snapshot_name})
self.qmp_client.command("savevm", name=snapshot_name)

"""
try:
logger.debug(f"Deleting snapshot {snapshot_name} for VM {self.vm.vm_id}")
self.qmp_client.command("delvm", **{"name": snapshot_name})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.qmp_client.command("delvm", **{"name": snapshot_name})
self.qmp_client.command("delvm", name=snapshot_name)


class QemuSnapshotExecution:
vm_hash: ItemHash
execution: any # AlephQemuInstance
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
execution: any # AlephQemuInstance
execution: Any # AlephQemuInstance

This should be Any from typing, I'm surprised this worked


async def start(self) -> None:
logger.debug(f"Starting QEMU snapshots for VM {self.vm_hash} every {self.frequency} minutes")
job = self._scheduler.every(self.frequency).minutes.do(run_threaded_snapshot, self.execution)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
job = self._scheduler.every(self.frequency).minutes.do(run_threaded_snapshot, self.execution)
self._job = self._scheduler.every(self.frequency).minutes.do(run_threaded_snapshot, self.execution)

And remove the next line


async def start(self) -> None:
logger.debug(f"Starting QEMU snapshots for VM {self.vm_hash} every {self.frequency} minutes")
job = self._scheduler.every(self.frequency).minutes.do(run_threaded_snapshot, self.execution)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
job = self._scheduler.every(self.frequency).minutes.do(run_threaded_snapshot, self.execution)
self._job = self._scheduler.every(self.frequency).minutes.do(run_threaded_snapshot, self.execution)

And remove the next line

await snapshot_execution.stop()

async def stop_all(self) -> None:
await asyncio.gather(*(self.stop_for(vm_hash) for vm_hash in list(self.executions.keys())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this function also do a job_thread.stop()? I'm no sure

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