Skip to content

refactor(vmsnapshot): remove freeze/unfreeze logic from VMSnapshot controller#2357

Draft
hardcoretime wants to merge 2 commits into
mainfrom
refactor/vmsnapshot/remove-freeze-unfreeze-duplication
Draft

refactor(vmsnapshot): remove freeze/unfreeze logic from VMSnapshot controller#2357
hardcoretime wants to merge 2 commits into
mainfrom
refactor/vmsnapshot/remove-freeze-unfreeze-duplication

Conversation

@hardcoretime
Copy link
Copy Markdown
Contributor

@hardcoretime hardcoretime commented May 17, 2026

Description

Removes filesystem freeze/unfreeze logic from VMSnapshot controller. The VMSnapshot controller was incorrectly handling freeze/unfreeze operations, which is the responsibility of VDSnapshot.

Changes made:

  • Removed freezeVirtualMachine(), unfreezeVirtualMachineIfCan(), and needToFreeze() methods from VMSnapshot lifecycle handler
  • Removed all freeze/unfreeze related code from the Handle() function
  • Simplified Snapshotter interface from 13 to 6 methods (removed: Freeze, Unfreeze, IsFrozen, CanFreeze, CanUnfreezeWithVirtualMachineSnapshot, SyncFSFreezeRequest, GetVirtualMachineInstance)
  • Updated unit tests to remove mock fields and tests related to freeze/unfreeze
  • Regenerated mock to match new interface

Why do we need it, and what problem does it solve?

The VMSnapshot controller was duplicating freeze/unfreeze logic that already exists in VDSnapshot controller. This caused:

  1. Race conditions - both controllers could attempt to freeze/unfreeze the same VM
  2. Code duplication - same logic implemented in two places
  3. Violation of Single Responsibility Principle - VMSnapshot should only coordinate VDSnapshot creation

VDSnapshot is the correct place for freeze/unfreeze because:

  • VDSnapshot is created for each disk of the VM
  • VDSnapshot already has proper coordination logic via CanUnfreezeWithVirtualDiskSnapshot
  • VDSnapshot handles cleanup (unfreeze) on deletion via DeletionHandler

What is the expected result?

  • VMSnapshot now only creates VDSnapshot resources and waits for them to become Ready
  • VDSnapshot is fully responsible for filesystem freeze/unfreeze operations
  • No duplicate freeze/unfreeze attempts
  • Cleaner separation of concerns

Checklist

  • The code is covered by unit tests.
  • e2e tests passed.
  • Documentation updated according to the changes.
  • Changes were tested in the Kubernetes cluster manually.

Changelog entries

section: vmsnapshot
type: chore
summary: "Remove freeze/unfreeze duplication from VMSnapshot controller. VDSnapshot is now fully responsible for filesystem freeze/unfreeze operations."
impact_level: low

…ntroller

VMSnapshot controller was incorrectly handling filesystem freeze/unfreeze
operations, which is the responsibility of VDSnapshot. This change:

- Removes freeze/unfreeze methods from VMSnapshot lifecycle handler
- Simplifies Snapshotter interface to only necessary methods
- Removes freeze/unfreeze related code from unit tests
- Updates mock to match new interface

Now VMSnapshot only coordinates VDSnapshot creation, while VDSnapshot
is fully responsible for freeze/unfreeze operations.

Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
@hardcoretime hardcoretime added this to the v1.9.0 milestone May 18, 2026
@hardcoretime hardcoretime changed the title refactor(vmsnapshot): remove freeze/unfreeze logic from VMSnapshot co… refactor(vmsnapshot): remove freeze/unfreeze logic from VMSnapshot controller May 18, 2026
@hardcoretime hardcoretime marked this pull request as draft May 18, 2026 00:40
VDSnapshot controller now checks if other VDSnapshots are still in
progress before attempting to unfreeze. If other snapshots are active,
it will requeue and retry until all snapshots are ready.

This fixes a regression where multiple VDSnapshots created by VMSnapshot
would race each other - the first one to complete would block the others
from unfreezing.

Additionally:
- Added unit test case for waiting when other VDSnapshots are active
- Updated existing test to expect canUnfreeze=true for Ready phase
- Added UnfreezeFunc to mock in vdsnapshot tests

Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
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.

1 participant