Skip to content

Commit

Permalink
Merge pull request #4297 from mhenriks/restore-corruption
Browse files Browse the repository at this point in the history 
Restore controller in memory corrution
  • Loading branch information
kubevirt-bot committed Oct 4, 2020
2 parents 0c0c443 + c1cca8e commit 5503318
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 50 deletions.
53 changes: 32 additions & 21 deletions pkg/virt-controller/watch/snapshot/restore.go
View file
Expand Up @@ -90,55 +90,58 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual
return 0, nil
}

complete := false
vmRestoreOut := vmRestoreIn.DeepCopy()

if vmRestoreOut.Status == nil {
vmRestoreOut.Status = &snapshotv1.VirtualMachineRestoreStatus{}
f := false
vmRestoreOut.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
}
}

vmRestoreOut.Status.Complete = &complete
vmRestoreOut.Status.RestoreTime = nil

target, err := ctrl.getTarget(vmRestoreOut)
if err != nil {
logger.Reason(err).Error("Error getting restore target")
return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err)
return 0, ctrl.doUpdateError(vmRestoreOut, err)
}

if len(vmRestoreOut.OwnerReferences) == 0 {
target.Own(vmRestoreOut)
updateRestoreCondition(vmRestoreOut, newProgressingCondition(corev1.ConditionTrue, "Initializing VirtualMachineRestore"))
updateRestoreCondition(vmRestoreOut, newReadyCondition(corev1.ConditionFalse, "Initializing VirtualMachineRestore"))
}

// let's make sure everything is initialized properly before continuing
if !reflect.DeepEqual(vmRestoreIn, vmRestoreOut) {
return 0, ctrl.doUpdate(vmRestoreIn, vmRestoreOut)
}

var updated bool
updated, err = ctrl.reconcileVolumeRestores(vmRestoreOut, target)
if err != nil {
logger.Reason(err).Error("Error reconciling VolumeRestores")
return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err)
return 0, ctrl.doUpdateError(vmRestoreIn, err)
}

if !updated {
var ready bool
ready, err = target.Ready()
if err != nil {
logger.Reason(err).Error("Error checking target ready")
return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err)
return 0, ctrl.doUpdateError(vmRestoreIn, err)
}

if ready {
updated, err = target.Reconcile()
if err != nil {
logger.Reason(err).Error("Error reconciling target")
return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err)
return 0, ctrl.doUpdateError(vmRestoreIn, err)
}

if !updated {
if err = target.Cleanup(); err != nil {
logger.Reason(err).Error("Error cleaning up")
return 0, ctrl.doUpdateError(vmRestoreIn, vmRestoreOut, err)
return 0, ctrl.doUpdateError(vmRestoreIn, err)
}

ctrl.Recorder.Eventf(
Expand All @@ -149,7 +152,8 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual
vmRestoreOut.Name,
)

complete = true
t := true
vmRestoreOut.Status.Complete = &t
vmRestoreOut.Status.RestoreTime = currentTime()
updateRestoreCondition(vmRestoreOut, newProgressingCondition(corev1.ConditionFalse, "Operation complete"))
updateRestoreCondition(vmRestoreOut, newReadyCondition(corev1.ConditionTrue, "Operation complete"))
Expand All @@ -172,18 +176,20 @@ func (ctrl *VMRestoreController) updateVMRestore(vmRestoreIn *snapshotv1.Virtual
return 0, ctrl.doUpdate(vmRestoreIn, vmRestoreOut)
}

func (ctrl *VMRestoreController) doUpdateError(original, updated *snapshotv1.VirtualMachineRestore, err error) error {
func (ctrl *VMRestoreController) doUpdateError(restore *snapshotv1.VirtualMachineRestore, err error) error {
ctrl.Recorder.Eventf(
updated,
restore,
corev1.EventTypeWarning,
restoreErrorEvent,
"VirtualMachineRestore encountered error %s",
err.Error(),
)

updated := restore.DeepCopy()

updateRestoreCondition(updated, newProgressingCondition(corev1.ConditionFalse, err.Error()))
updateRestoreCondition(updated, newReadyCondition(corev1.ConditionFalse, err.Error()))
if err2 := ctrl.doUpdate(original, updated); err2 != nil {
if err2 := ctrl.doUpdate(restore, updated); err2 != nil {
return err2
}

Expand Down Expand Up @@ -285,7 +291,7 @@ func (ctrl *VMRestoreController) getBindingMode(pvc *corev1.PersistentVolumeClai
return nil, fmt.Errorf("StorageClass %s does not exist", *pvc.Spec.StorageClassName)
}

sc := obj.(*storagev1.StorageClass)
sc := obj.(*storagev1.StorageClass).DeepCopy()

return sc.VolumeBindingMode, nil
}
Expand Down Expand Up @@ -343,8 +349,13 @@ func (t *vmRestoreTarget) Reconcile() (bool, error) {
var deletedDataVolumes []string
updatedStatus := false

copy(newTemplates, snapshotVM.Spec.DataVolumeTemplates)
copy(newVolumes, snapshotVM.Spec.Template.Spec.Volumes)
for i, t := range snapshotVM.Spec.DataVolumeTemplates {
t.DeepCopyInto(&newTemplates[i])
}

for i, v := range snapshotVM.Spec.Template.Spec.Volumes {
v.DeepCopyInto(&newVolumes[i])
}

for j, v := range snapshotVM.Spec.Template.Spec.Volumes {
if v.DataVolume != nil || v.PersistentVolumeClaim != nil {
Expand Down Expand Up @@ -503,7 +514,7 @@ func (ctrl *VMRestoreController) getSnapshotContent(vmRestore *snapshotv1.Virtua
return nil, fmt.Errorf("VMSnapshot %s does not exist", objKey)
}

vms := obj.(*snapshotv1.VirtualMachineSnapshot)
vms := obj.(*snapshotv1.VirtualMachineSnapshot).DeepCopy()
if !vmSnapshotReady(vms) {
return nil, fmt.Errorf("VMSnapshot %s not ready", objKey)
}
Expand All @@ -526,7 +537,7 @@ func (ctrl *VMRestoreController) getSnapshotContent(vmRestore *snapshotv1.Virtua
return nil, fmt.Errorf("VMSnapshotContent %s does not exist", objKey)
}

vmss := obj.(*snapshotv1.VirtualMachineSnapshotContent)
vmss := obj.(*snapshotv1.VirtualMachineSnapshotContent).DeepCopy()
if !vmSnapshotContentReady(vmss) {
return nil, fmt.Errorf("VMSnapshotContent %s not ready", objKey)
}
Expand All @@ -545,7 +556,7 @@ func (ctrl *VMRestoreController) getVM(namespace, name string) (*kubevirtv1.Virt
return nil, fmt.Errorf("VirtualMachine %s/%s does not exist", namespace, name)
}

return obj.(*kubevirtv1.VirtualMachine), nil
return obj.(*kubevirtv1.VirtualMachine).DeepCopy(), nil
}

func (ctrl *VMRestoreController) getPVC(namespace, name string) (*corev1.PersistentVolumeClaim, error) {
Expand All @@ -559,7 +570,7 @@ func (ctrl *VMRestoreController) getPVC(namespace, name string) (*corev1.Persist
return nil, nil
}

return obj.(*corev1.PersistentVolumeClaim), nil
return obj.(*corev1.PersistentVolumeClaim).DeepCopy(), nil
}

func (ctrl *VMRestoreController) getTarget(vmRestore *snapshotv1.VirtualMachineRestore) (restoreTarget, error) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/virt-controller/watch/snapshot/restore_base.go
View file
Expand Up @@ -129,7 +129,7 @@ func (ctrl *VMRestoreController) processVMRestoreWorkItem() bool {
return 0, fmt.Errorf("unexpected resource %+v", storeObj)
}

return ctrl.updateVMRestore(vmRestore)
return ctrl.updateVMRestore(vmRestore.DeepCopy())
})
}

Expand Down
48 changes: 28 additions & 20 deletions pkg/virt-controller/watch/snapshot/restore_test.go
View file
Expand Up @@ -51,16 +51,6 @@ var _ = Describe("Restore controlleer", func() {
Name: "restore",
Namespace: testNamespace,
UID: uid,
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: kubevirtv1.GroupVersion.String(),
Kind: "VirtualMachine",
Name: vmName,
UID: vmUID,
Controller: &t,
BlockOwnerDeletion: &t,
},
},
},
Spec: snapshotv1.VirtualMachineRestoreSpec{
Target: corev1.TypedLocalObjectReference{
Expand All @@ -73,6 +63,24 @@ var _ = Describe("Restore controlleer", func() {
}
}

createRestoreWithOwner := func() *snapshotv1.VirtualMachineRestore {
r := createRestore()
r.OwnerReferences = []metav1.OwnerReference{
{
APIVersion: kubevirtv1.GroupVersion.String(),
Kind: "VirtualMachine",
Name: vmName,
UID: vmUID,
Controller: &t,
BlockOwnerDeletion: &t,
},
}
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
}
return r
}

addVolumeRestores := func(r *snapshotv1.VirtualMachineRestore) {
r.Status.Restores = []snapshotv1.VolumeRestore{
{
Expand Down Expand Up @@ -323,7 +331,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should error if snapshot does not exist", func() {
r := createRestore()
r := createRestoreWithOwner()
vm := createModifiedVM()
rc := r.DeepCopy()
rc.ResourceVersion = "1"
Expand All @@ -343,7 +351,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should error if different source UID", func() {
r := createRestore()
r := createRestoreWithOwner()
vm := createModifiedVM()
vm.UID = types.UID("foobar")
rc := r.DeepCopy()
Expand All @@ -363,7 +371,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should update restore status, initializing conditions and add owner", func() {
r := createRestore()
r := createRestoreWithOwner()
refs := r.OwnerReferences
r.OwnerReferences = nil
vm := createModifiedVM()
Expand All @@ -384,7 +392,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should update restore status with condition and VolumeRestores", func() {
r := createRestore()
r := createRestoreWithOwner()
vm := createModifiedVM()
rc := r.DeepCopy()
rc.ResourceVersion = "1"
Expand All @@ -403,7 +411,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should create restore PVCs", func() {
r := createRestore()
r := createRestoreWithOwner()
vm := createModifiedVM()
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
Expand All @@ -420,7 +428,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should wait for bound", func() {
r := createRestore()
r := createRestoreWithOwner()
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
Conditions: []snapshotv1.Condition{
Expand All @@ -443,7 +451,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should update restore status with datavolume", func() {
r := createRestore()
r := createRestoreWithOwner()
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
Conditions: []snapshotv1.Condition{
Expand Down Expand Up @@ -473,7 +481,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should update PVCs and restores to have datavolumename", func() {
r := createRestore()
r := createRestoreWithOwner()
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
Conditions: []snapshotv1.Condition{
Expand Down Expand Up @@ -506,7 +514,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should update VM spec", func() {
r := createRestore()
r := createRestoreWithOwner()
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
DeletedDataVolumes: getDeletedDataVolumes(createModifiedVM()),
Expand Down Expand Up @@ -537,7 +545,7 @@ var _ = Describe("Restore controlleer", func() {
})

It("should cleanup and complete", func() {
r := createRestore()
r := createRestoreWithOwner()
r.Status = &snapshotv1.VirtualMachineRestoreStatus{
Complete: &f,
DeletedDataVolumes: getDeletedDataVolumes(createModifiedVM()),
Expand Down
8 changes: 4 additions & 4 deletions pkg/virt-controller/watch/snapshot/snapshot.go
View file
Expand Up @@ -503,7 +503,7 @@ func (ctrl *VMSnapshotController) getSnapshotPVC(namespace, volumeName string) (
return nil, nil
}

pvc := obj.(*corev1.PersistentVolumeClaim)
pvc := obj.(*corev1.PersistentVolumeClaim).DeepCopy()

if pvc.Spec.VolumeName == "" {
log.Log.Warningf("Unbound PVC %s/%s", pvc.Namespace, pvc.Name)
Expand Down Expand Up @@ -533,7 +533,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshotClass(storageClassName string
return "", err
}

storageClass := obj.(*storagev1.StorageClass)
storageClass := obj.(*storagev1.StorageClass).DeepCopy()

var matches []vsv1beta1.VolumeSnapshotClass
volumeSnapshotClasses := ctrl.getVolumeSnapshotClasses()
Expand Down Expand Up @@ -648,7 +648,7 @@ func (ctrl *VMSnapshotController) getVM(vmSnapshot *snapshotv1.VirtualMachineSna
return nil, nil
}

return obj.(*kubevirtv1.VirtualMachine), nil
return obj.(*kubevirtv1.VirtualMachine).DeepCopy(), nil
}

func (ctrl *VMSnapshotController) getContent(vmSnapshot *snapshotv1.VirtualMachineSnapshot) (*snapshotv1.VirtualMachineSnapshotContent, error) {
Expand All @@ -662,7 +662,7 @@ func (ctrl *VMSnapshotController) getContent(vmSnapshot *snapshotv1.VirtualMachi
return nil, nil
}

return obj.(*snapshotv1.VirtualMachineSnapshotContent), nil
return obj.(*snapshotv1.VirtualMachineSnapshotContent).DeepCopy(), nil
}

func (s *vmSnapshotSource) UID() types.UID {
Expand Down
8 changes: 4 additions & 4 deletions pkg/virt-controller/watch/snapshot/snapshot_base.go
View file
Expand Up @@ -208,7 +208,7 @@ func (ctrl *VMSnapshotController) processVMSnapshotWorkItem() bool {
return 0, fmt.Errorf("unexpected resource %+v", storeObj)
}

return ctrl.updateVMSnapshot(vmSnapshot)
return ctrl.updateVMSnapshot(vmSnapshot.DeepCopy())
})
}

Expand All @@ -226,7 +226,7 @@ func (ctrl *VMSnapshotController) processVMSnapshotContentWorkItem() bool {
return 0, fmt.Errorf("unexpected resource %+v", storeObj)
}

return ctrl.updateVMSnapshotContent(vmSnapshotContent)
return ctrl.updateVMSnapshotContent(vmSnapshotContent.DeepCopy())
})
}

Expand Down Expand Up @@ -384,7 +384,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshot(namespace, name string) (*vs
return nil, err
}

return obj.(*vsv1beta1.VolumeSnapshot), nil
return obj.(*vsv1beta1.VolumeSnapshot).DeepCopy(), nil
}

func (ctrl *VMSnapshotController) getVolumeSnapshotClasses() []vsv1beta1.VolumeSnapshotClass {
Expand All @@ -399,7 +399,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshotClasses() []vsv1beta1.VolumeS
var vscs []vsv1beta1.VolumeSnapshotClass
objs := di.informer.GetStore().List()
for _, obj := range objs {
vsc := obj.(*vsv1beta1.VolumeSnapshotClass)
vsc := obj.(*vsv1beta1.VolumeSnapshotClass).DeepCopy()
vscs = append(vscs, *vsc)
}

Expand Down

0 comments on commit 5503318

Please sign in to comment.