From bfbefee0f5ebe552eb84e32ce0fb69dbdf55bdc0 Mon Sep 17 00:00:00 2001 From: divolgin Date: Thu, 25 Aug 2022 01:06:59 +0000 Subject: [PATCH 1/4] Don't panic when storageClassName is not set in stateful sets Signed-off-by: divolgin --- changelogs/unreleased/5301-divolgin | 1 + pkg/restore/change_storageclass_action.go | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/5301-divolgin diff --git a/changelogs/unreleased/5301-divolgin b/changelogs/unreleased/5301-divolgin new file mode 100644 index 000000000..7a6e24cfe --- /dev/null +++ b/changelogs/unreleased/5301-divolgin @@ -0,0 +1 @@ +Fix nil pointer panic when restoring StatefulSets \ No newline at end of file diff --git a/pkg/restore/change_storageclass_action.go b/pkg/restore/change_storageclass_action.go index 5714a1a7f..577ab28ec 100644 --- a/pkg/restore/change_storageclass_action.go +++ b/pkg/restore/change_storageclass_action.go @@ -99,7 +99,7 @@ func (a *ChangeStorageClassAction) Execute(input *velero.RestoreItemActionExecut if len(sts.Spec.VolumeClaimTemplates) > 0 { for index, pvc := range sts.Spec.VolumeClaimTemplates { - exists, newStorageClass, err := a.isStorageClassExist(log, *pvc.Spec.StorageClassName, config) + exists, newStorageClass, err := a.isStorageClassExist(log, pvc.Spec.StorageClassName, config) if err != nil { return nil, err } else if !exists { @@ -124,7 +124,7 @@ func (a *ChangeStorageClassAction) Execute(input *velero.RestoreItemActionExecut return nil, errors.Wrap(err, "error getting item's spec.storageClassName") } - exists, newStorageClass, err := a.isStorageClassExist(log, storageClass, config) + exists, newStorageClass, err := a.isStorageClassExist(log, &storageClass, config) if err != nil { return nil, err } else if !exists { @@ -140,15 +140,15 @@ func (a *ChangeStorageClassAction) Execute(input *velero.RestoreItemActionExecut return velero.NewRestoreItemActionExecuteOutput(obj), nil } -func (a *ChangeStorageClassAction) isStorageClassExist(log *logrus.Entry, storageClass string, cm *corev1.ConfigMap) (exists bool, newStorageClass string, err error) { - if storageClass == "" { +func (a *ChangeStorageClassAction) isStorageClassExist(log *logrus.Entry, storageClass *string, cm *corev1.ConfigMap) (exists bool, newStorageClass string, err error) { + if storageClass == nil || *storageClass == "" { log.Debug("Item has no storage class specified") return false, "", nil } - newStorageClass, ok := cm.Data[storageClass] + newStorageClass, ok := cm.Data[*storageClass] if !ok { - log.Debugf("No mapping found for storage class %s", storageClass) + log.Debugf("No mapping found for storage class %s", *storageClass) return false, "", nil } From 9937607e725f467857ddea68d945cb8fd5891aa0 Mon Sep 17 00:00:00 2001 From: Scott Seago Date: Fri, 19 Aug 2022 14:50:16 -0400 Subject: [PATCH 2/4] Check for empty ns list before checking nslist[0] In determining whether a backup includes all namespaces, item_collector checks for an empty string in the first element of the ns list. If processing includes+excludes results in an empty list, treat this as another case of a not-all-namespaces backup rather than crashing velero. Signed-off-by: Scott Seago --- changelogs/unreleased/5302-sseago | 1 + pkg/backup/item_collector.go | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/5302-sseago diff --git a/changelogs/unreleased/5302-sseago b/changelogs/unreleased/5302-sseago new file mode 100644 index 000000000..4d295cce8 --- /dev/null +++ b/changelogs/unreleased/5302-sseago @@ -0,0 +1 @@ +Check for empty ns list before checking nslist[0] diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index 5cbc17836..474fbabee 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -225,8 +225,11 @@ func (r *itemCollector) getResourceItems(log logrus.FieldLogger, gv schema.Group namespacesToList := getNamespacesToList(r.backupRequest.NamespaceIncludesExcludes) - // Check if we're backing up namespaces, and only certain ones - if gr == kuberesource.Namespaces && namespacesToList[0] != "" { + // Check if we're backing up namespaces for a less-than-full backup. + // We enter this block if resource is Namespaces and the namespae list is either empty or contains + // an explicit namespace list. (We skip this block if the list contains "" since that indicates + // a full-cluster backup + if gr == kuberesource.Namespaces && (len(namespacesToList) == 0 || namespacesToList[0] != "") { resourceClient, err := r.dynamicFactory.ClientForGroupVersionResource(gv, resource, "") if err != nil { log.WithError(err).Error("Error getting dynamic client") From 3de8be83f4e2f0bc1d97e84d6fe2668593924714 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E9=BE=99=E5=B3=B0?= Date: Tue, 16 Aug 2022 09:28:57 +0800 Subject: [PATCH 3/4] check vsc null pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 李龙峰 --- changelogs/unreleased/5303-lilongfeng0902 | 1 + pkg/controller/backup_controller.go | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 changelogs/unreleased/5303-lilongfeng0902 diff --git a/changelogs/unreleased/5303-lilongfeng0902 b/changelogs/unreleased/5303-lilongfeng0902 new file mode 100644 index 000000000..819069ac3 --- /dev/null +++ b/changelogs/unreleased/5303-lilongfeng0902 @@ -0,0 +1 @@ +check vsc null pointer \ No newline at end of file diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index eff0861eb..f33820b1b 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -940,6 +940,10 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []*snapshotv1api if vs.Status.BoundVolumeSnapshotContentName != nil && len(*vs.Status.BoundVolumeSnapshotContentName) > 0 { vsc = vscMap[*vs.Status.BoundVolumeSnapshotContentName] + if nil == vsc { + logger.Errorf("Not find %s from the vscMap", vs.Status.BoundVolumeSnapshotContentName) + return + } if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete { modifyVSCFlag = true } From 313f836d23a43cd0b483b70781f7034f6ffddd56 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Mon, 22 Aug 2022 09:50:30 -0400 Subject: [PATCH 4/4] fix edge cases for already exists resources Signed-off-by: Shubham Pampattiwar --- .../unreleased/5304-shubham-pampattiwar | 1 + pkg/restore/restore.go | 33 +++++++++++++++---- 2 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/5304-shubham-pampattiwar diff --git a/changelogs/unreleased/5304-shubham-pampattiwar b/changelogs/unreleased/5304-shubham-pampattiwar new file mode 100644 index 000000000..693e1c4fb --- /dev/null +++ b/changelogs/unreleased/5304-shubham-pampattiwar @@ -0,0 +1 @@ +Fix edge cases for already exists resources \ No newline at end of file diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 691027c7c..2b252c24d 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1249,12 +1249,31 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso errs.Add(namespace, err) return warnings, errs } - if isAlreadyExistsError { - fromCluster, err := resourceClient.Get(name, metav1.GetOptions{}) - if err != nil { - ctx.log.Infof("Error retrieving cluster version of %s: %v", kube.NamespaceAndName(obj), err) - warnings.Add(namespace, err) - return warnings, errs + + // check if we want to treat the error as a warning, in some cases the creation call might not get executed due to object API validations + // and Velero might not get the already exists error type but in reality the object already exists + objectExists := false + var fromCluster *unstructured.Unstructured + + if restoreErr != nil && !isAlreadyExistsError { + // check for the existence of the object in cluster, if no error then it implies that object exists + // and if err then we want to fallthrough and do another get call later + fromCluster, err = resourceClient.Get(name, metav1.GetOptions{}) + if err == nil { + objectExists = true + } + } + + if isAlreadyExistsError || objectExists { + // do a get call if we did not run this previously i.e. + // we've only run this for errors other than isAlreadyExistError + if fromCluster == nil { + fromCluster, err = resourceClient.Get(name, metav1.GetOptions{}) + if err != nil { + ctx.log.Errorf("Error retrieving cluster version of %s: %v", kube.NamespaceAndName(obj), err) + errs.Add(namespace, err) + return warnings, errs + } } // Remove insubstantial metadata. fromCluster, err = resetMetadataAndStatus(fromCluster) @@ -2024,7 +2043,7 @@ func (ctx *restoreContext) processUpdateResourcePolicy(fromCluster, fromClusterW // try patching the in-cluster resource (resource diff plus latest backup/restore labels) _, err = resourceClient.Patch(obj.GetName(), patchBytes) if err != nil { - ctx.log.Errorf("patch attempt failed for %s %s: %v", fromCluster.GroupVersionKind(), kube.NamespaceAndName(fromCluster), err) + ctx.log.Warnf("patch attempt failed for %s %s: %v", fromCluster.GroupVersionKind(), kube.NamespaceAndName(fromCluster), err) warnings.Add(namespace, err) // try just patching the labels warningsFromUpdate, errsFromUpdate := ctx.updateBackupRestoreLabels(fromCluster, fromClusterWithLabels, namespace, resourceClient)