Repository navigation
Disable VolumeSnapshot-based replica creation - #66
Conversation
- GetCandidateStorageSourceForReplica now returns nil; replicas bootstrap via streaming base-backup. - A VolumeSnapshot is confined to its parent's storage node, so snapshot-backed replicas can't honor anti-affinity placement. - Replace replica-source unit tests with always-nil cases. - Delete the e2e scale-up test that asserted snapshot-backed PVCs; general scale-up is covered in scaling_test.go and cluster_setup_test.go.
|
I am wondering if it would be better to add a setting to disable creating replicas from snapshots. We can default it to "on", so the end result is the same. But maybe more "fork-friendly"? |
| return getCandidateSourceFromClusterDefinition(cluster) | ||
| _ = backupList | ||
| log.FromContext(ctx).Trace( | ||
| "VolumeSnapshot-based replica creation is disabled; using streaming base-backup", |
There was a problem hiding this comment.
i assume we are thinking to potentially bring this back later? otherwise we could also remove the two function calls getCandidateSourceFromClusterDefinition and getCandidateSourceFromBackupList since they are not called anywhere else
There was a problem hiding this comment.
True.
Yes, we will bring this back at some point in the future. That point is pretty far away though. Let me pull this out properly, we can come back when we get to it.
After gutting GetCandidateStorageSourceForReplica, this helper has no production callers (its only caller was the function we just disabled). Drop the helper and the two test describe blocks that exercised it directly. GetCandidateStorageSourceForPrimary still uses getCandidateSourceFromBackup and getCandidateSourceFromClusterDefinition; those stay.
The function returned nil unconditionally and had a single production
caller whose `if storageSource \!= nil { RestoreReplicaInstance }` branch
was dead. Removing it lets us also drop:
- The BackupList fetch in joinReplicaInstance (one less API List per
replica creation).
- The dead conditional and the storageSource log field.
- RestoreReplicaInstance itself (orphan once the dead branch is gone).
jobRoleSnapshotRecovery and the restoresnapshot manager subcommand stay
— they're still used by the primary path (CreatePrimaryJobViaRestoreSnapshot).
|
We have several red actions: seems like they're downstream from us being behind a major Go version. Will merge... we should upgrade, but these changes are not directly related. |
Replica PVCs were being hydrated from a VolumeSnapshot whenever a recent snapshot-backed backup or a bootstrap snapshot was available. Our storage backend confines a snapshot child to its parent's storage node, so a snapshot-sourced replica can never honor anti-affinity placement. This PR closes that path: replicas always come up via streaming base-backup.
Start here:
pkg/reconciler/persistentvolumeclaim/storagesource.go:59—GetCandidateStorageSourceForReplicanow early-returns nil. The caller atinternal/controller/cluster_create.go:1257already treats nil as "stream from primary," so no caller-side change.GetCandidateStorageSourceForPrimaryis untouched; bootstrap-from-snapshot for a new cluster still works.Compat: clusters whose next scale-up would previously have used a VolumeSnapshot will now stream base-backup instead — slower first-replica catch-up, identical end state. Existing snapshot-restored replica PVCs are reused, not recreated, so they're unaffected.
Scope: first slice of the anti-affinity work. Subsequent slices (PVC affinity-group labels, anti-affinity-group label from the branch-operator side, storage-side placement strategy) land separately.