fix(vd): move metadata sync to its own step #1776
Conversation
hardcoretime
requested review from
Isteb4k,
fl64 and
yaroslavborbat
as code owners
Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
hardcoretime
force-pushed
the
fix/vd/move-metadata-sync-to-its-own-step
branch
from
565b2ce to
706035d
Compare
| originalLabelsMap map[string]string | ||
| ) | ||
|
|
||
| if vs.Annotations != nil { |
There was a problem hiding this comment.
Why are you checking for nil?
|
|
||
| var ( | ||
| isMetadataAdded bool | ||
| originalAnnotationsMap map[string]string |
There was a problem hiding this comment.
Place maps next to the spot where you use them
| } | ||
| } | ||
|
|
||
| if isMetadataAdded { |
There was a problem hiding this comment.
Add a detailed comment explaining why we’re making return like this
| ) | ||
|
|
||
| if vs.Annotations != nil { | ||
| if vs.Annotations[annotations.AnnVirtualDiskOriginalLabels] != "" { |
There was a problem hiding this comment.
Move it into two small independent functions:
setOriginalAnnotations(annotations map[string]string, vs *vsv1.VolumeSnapshot) (bool, error)
setOriginalLabels(labels map[string]string, vs *vsv1.VolumeSnapshot) (bool, error)
|
|
||
| vs, err := object.FetchObject(ctx, types.NamespacedName{Name: vdSnapshot.Status.VolumeSnapshotName, Namespace: vdSnapshot.Namespace}, s.client, &vsv1.VolumeSnapshot{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("fetch volume snapshot: %w", err) |
There was a problem hiding this comment.
Set the phase to Failed for all errors in this step, and set the corresponding condition as well
| } | ||
|
|
||
| func (s AddOriginalMetadataStep) Take(ctx context.Context, vd *v1alpha2.VirtualDisk) (*reconcile.Result, error) { | ||
| s.recorder.Event( |
There was a problem hiding this comment.
Let’s have only one event when isMetadataAdded turns out to be true.
So as not to spam events on every reconciliation.
| ClusterImageNotFound DatasourceReadyReason = "ClusterImageNotFound" | ||
|
|
||
| // AddingOriginalMetadataNotStarted indicates that the adding original metadata process has not started yet. | ||
| AddingOriginalMetadataNotStarted ReadyReason = "AddingOriginalMetadataNotStarted" |
There was a problem hiding this comment.
There’s no need for a reason in the condition. You can use ProvisioningNotStarted. For the event, the reason can be placed in api/core/v1alpha2/events.go
Why do we need it, and what problem does it solve?
Some issues, such as a revision error, can occur when virtual disk metadata is updated in the
CreatePVCFromVDSnapshotStep, and the original metadata can be lost. To prevent this, the updating block has been moved to its own step.What is the expected result?
The original virtual disk metadata syncs correctly when restored from a virtual disk snapshot.
Checklist
Changelog entries