diff --git a/Dockerfile b/Dockerfile index 4290017ff..a167bc4bb 100644 --- a/Dockerfile +++ b/Dockerfile @@ -70,7 +70,7 @@ RUN mkdir -p /output/usr/bin && \ go clean -modcache -cache # Velero image packing section -FROM paketobuildpacks/run-jammy-tiny:0.2.5 +FROM paketobuildpacks/run-jammy-tiny:0.2.11 LABEL maintainer="Xun Jiang " diff --git a/changelogs/CHANGELOG-1.12.md b/changelogs/CHANGELOG-1.12.md index 8182b3e8a..2cc74d621 100644 --- a/changelogs/CHANGELOG-1.12.md +++ b/changelogs/CHANGELOG-1.12.md @@ -1,3 +1,56 @@ +## v1.12.3 +### 2024-01-09 + +### Download +https://github.com/vmware-tanzu/velero/releases/tag/v1.12.3 + +### Container Image +`velero/velero:v1.12.3` + +### Documentation +https://velero.io/docs/v1.12/ + +### Upgrading +https://velero.io/docs/v1.12/upgrade-to-1.12/ + +### All changes +* Fix issue #7244. By the end of the upload, check the outstanding incomplete snapshots and delete them by calling ApplyRetentionPolicy (#7247, @Lyndon-Li) +* Fix issue #7189, data mover generic restore - don't assume the first volume as the restore volume (#7203, @Lyndon-Li) +* Update CSIVolumeSnapshotsCompleted in backup's status and the metric +during backup finalize stage according to async operations content. (#7202, @blackpiglet) +* Node agent restart enhancement (#7130, @qiuming-best) +* Fix issue #6928, remove snapshot deletion timeout for PVB (#7283, @Lyndon-Li) + +## v1.12.2 +### 2023-11-20 + +### Download +https://github.com/vmware-tanzu/velero/releases/tag/v1.12.2 + +### Container Image +`velero/velero:v1.12.2` + +### Documentation +https://velero.io/docs/v1.12/ + +### Upgrading +https://velero.io/docs/v1.12/upgrade-to-1.12/ + +### All changes +* Fix issue #7068, due to a behavior of CSI external snapshotter, manipulations of VS and VSC may not be handled in the same order inside external snapshotter as the API is called. So add a protection finalizer to ensure the order (#7114, @Lyndon-Li) +* Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize (#7111, @kaovilai) +* Cherry-pick #6917 - Support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers (#7049, @27149chen) +* Bump up Velero base image to latest patch release (#7110, @allenxu404) +* Fix the node-agent missing metrics-address defines. (#7098, @yanggangtony) +* Fix issue #7094, fallback to full backup if previous snapshot is not found (#7097, @Lyndon-Li) +* Add DataUpload Result and CSI VolumeSnapshot check for restore PV. (#7087, @blackpiglet) +* Fix issue #7027, data mover backup exposer should not assume the first volume as the backup volume in backup pod (#7060, @Lyndon-Li) +* Truncate the credential file to avoid the change of secret content messing it up (#7058, @ywk253100) +* restore: Use warning when Create IsAlreadyExist and Get error (#7054, @kaovilai) +* Read information from the credential specified by BSL (#7033, @ywk253100) +* Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted (#7025, @shubham-pampattiwar) +* Fix unified repository (kopia) s3 credentials profile selection (#6997, @kaovilai) + ## v1.12.1 ### 2023-10-20 diff --git a/changelogs/unreleased/6997-kaovilai b/changelogs/unreleased/6997-kaovilai deleted file mode 100644 index ef3ab9406..000000000 --- a/changelogs/unreleased/6997-kaovilai +++ /dev/null @@ -1 +0,0 @@ -Fix unified repository (kopia) s3 credentials profile selection \ No newline at end of file diff --git a/changelogs/unreleased/7025-shubham-pampattiwar b/changelogs/unreleased/7025-shubham-pampattiwar deleted file mode 100644 index c95688b39..000000000 --- a/changelogs/unreleased/7025-shubham-pampattiwar +++ /dev/null @@ -1 +0,0 @@ -Fix issue 6913: Velero Built-in Datamover: Backup stucks in phase WaitingForPluginOperations when Node Agent pod gets restarted \ No newline at end of file diff --git a/changelogs/unreleased/7033-ywk253100 b/changelogs/unreleased/7033-ywk253100 deleted file mode 100644 index b4ce2bbab..000000000 --- a/changelogs/unreleased/7033-ywk253100 +++ /dev/null @@ -1 +0,0 @@ -Read information from the credential specified by BSL \ No newline at end of file diff --git a/changelogs/unreleased/7054-kaovilai b/changelogs/unreleased/7054-kaovilai deleted file mode 100644 index 3b85df196..000000000 --- a/changelogs/unreleased/7054-kaovilai +++ /dev/null @@ -1 +0,0 @@ -restore: Use warning when Create IsAlreadyExist and Get error \ No newline at end of file diff --git a/changelogs/unreleased/7060-Lyndon-Li b/changelogs/unreleased/7060-Lyndon-Li deleted file mode 100644 index f2a3a9ae2..000000000 --- a/changelogs/unreleased/7060-Lyndon-Li +++ /dev/null @@ -1 +0,0 @@ -Fix issue #7027, data mover backup exposer should not assume the first volume as the backup volume in backup pod \ No newline at end of file diff --git a/design/merge-patch-and-strategic-in-resource-modifier.md b/design/merge-patch-and-strategic-in-resource-modifier.md new file mode 100644 index 000000000..a8127bc49 --- /dev/null +++ b/design/merge-patch-and-strategic-in-resource-modifier.md @@ -0,0 +1,193 @@ +# Proposal to Support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers + +- [Proposal to Support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers](#proposal-to-support-json-merge-patch-and-strategic-merge-patch-in-resource-modifiers) + - [Abstract](#abstract) + - [Goals](#goals) + - [Non Goals](#non-goals) + - [User Stories](#user-stories) + - [Scenario 1](#scenario-1) + - [Scenario 2](#scenario-2) + - [Detailed Design](#detailed-design) + - [How to choose the right patch type](#how-to-choose-the-right-patch-type) + - [New Field MergePatches](#new-field-mergepatches) + - [New Field StrategicPatches](#new-field-strategicpatches) + - [Conditional Patches in ALL Patch Types](#conditional-patches-in-all-patch-types) + - [Wildcard Support for GroupResource](#wildcard-support-for-groupresource) + - [Helper Command to Generate Merge Patch and Strategic Merge Patch](#helper-command-to-generate-merge-patch-and-strategic-merge-patch) + - [Security Considerations](#security-considerations) + - [Compatibility](#compatibility) + - [Implementation](#implementation) + - [Future Enhancements](#future-enhancements) + - [Open Issues](#open-issues) + +## Abstract +Velero introduced the concept of Resource Modifiers in v1.12.0. This feature allows the user to specify a configmap with a set of rules to modify the resources during restore. The user can specify the filters to select the resources and then specify the JSON Patch to apply on the resource. This feature is currently limited to the operations supported by JSON Patch RFC. +This proposal is to add support for JSON Merge Patch and Strategic Merge Patch in the Resource Modifiers. This will allow the user to use the same configmap to apply JSON Merge Patch and Strategic Merge Patch on the resources during restore. + +## Goals +- Allow the user to specify a JSON patch, JSON Merge Patch or Strategic Merge Patch for modification. +- Allow the user to specify multiple JSON Patch, JSON Merge Patch or Strategic Merge Patch. +- Allow the user to specify mixed JSON Patch, JSON Merge Patch and Strategic Merge Patch in the same configmap. + +## Non Goals +- Deprecating the existing RestoreItemAction plugins for standard substitutions(like changing the namespace, changing the storage class, etc.) + +## User Stories + +### Scenario 1 +- Alice has some Pods and part of them have an annotation `{"for": "bar"}`. +- Alice wishes to restore these Pods to a different cluster without this annotation. +- Alice can use this feature to remove this annotation during restore. + +### Scenario 2 +- Bob has a Pod with several containers and one container with name nginx has an image `repo1/nginx`. +- Bob wishes to restore this Pod to a different cluster, but new cluster can not access repo1, so he pushes the image to repo2. +- Bob can use this feature to update the image of container nginx to `repo2/nginx` during restore. + +## Detailed Design +- The design and approach is inspired by kubectl patch command and [this doc](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/). +- New fields `MergePatches` and `StrategicPatches` will be added to the `ResourceModifierRule` struct to support all three patch types. +- Only one of the three patch types can be specified in a single `ResourceModifierRule`. +- Add wildcard support for `groupResource` in `conditions` struct. +- The workflow to create Resource Modifier ConfigMap and reference it in RestoreSpec will remain the same as described in document [Resource Modifiers](https://github.com/vmware-tanzu/velero/blob/main/site/content/docs/main/restore-resource-modifiers.md). + +### How to choose the right patch type +- [JSON Merge Patch](https://datatracker.ietf.org/doc/html/rfc7386) is a naively simple format, with limited usability. Probably it is a good choice if you are building something small, with very simple JSON Schema. +- [JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) is a more complex format, but it is applicable to any JSON documents. For a comparison of JSON patch and JSON merge patch, see [JSON Patch and JSON Merge Patch](https://erosb.github.io/post/json-patch-vs-merge-patch/). +- Strategic Merge Patch is a Kubernetes defined patch type, mainly used to process resources of type list. You can replace/merge a list, add/remove items from a list by key, change the order of items in a list, etc. Strategic merge patch is not supported for custom resources. For more details, see [this doc](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/). + +### New Field MergePatches +MergePatches is a list to specify the merge patches to be applied on the resource. The merge patches will be applied in the order specified in the configmap. A subsequent patch is applied in order and if multiple patches are specified for the same path, the last patch will override the previous patches. + +Example of MergePatches in ResourceModifierRule +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: pods + namespaces: + - ns1 + mergePatches: + - patchData: | + { + "metadata": { + "annotations": { + "foo": null + } + } + } +``` +- The above configmap will apply the Merge Patch to all the pods in namespace ns1 and remove the annotation `foo` from the pods. +- Both json and yaml format are supported for the patchData. + +### New Field StrategicPatches +StrategicPatches is a list to specify the strategic merge patches to be applied on the resource. The strategic merge patches will be applied in the order specified in the configmap. A subsequent patch is applied in order and if multiple patches are specified for the same path, the last patch will override the previous patches. + +Example of StrategicPatches in ResourceModifierRule +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: pods + resourceNameRegex: "^my-pod$" + namespaces: + - ns1 + strategicPatches: + - patchData: | + { + "spec": { + "containers": [ + { + "name": "nginx", + "image": "repo2/nginx" + } + ] + } + } +``` +- The above configmap will apply the Strategic Merge Patch to the pod with name my-pod in namespace ns1 and update the image of container nginx to `repo2/nginx`. +- Both json and yaml format are supported for the patchData. + +### Conditional Patches in ALL Patch Types +Since JSON Merge Patch and Strategic Merge Patch do not support conditional patches, we will use the `test` operation of JSON Patch to support conditional patches in all patch types by adding it to `Conditions` struct in `ResourceModifierRule`. + +Example of test in conditions +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: persistentvolumeclaims.storage.k8s.io + matches: + - path: "/spec/storageClassName" + value: "premium" + mergePatches: + - patchData: | + { + "metadata": { + "annotations": { + "foo": null + } + } + } +``` +- The above configmap will apply the Merge Patch to all the PVCs in all namespaces with storageClassName premium and remove the annotation `foo` from the PVCs. +- You can specify multiple rules in the `matches` list. The patch will be applied only if all the matches are satisfied. + +### Wildcard Support for GroupResource +The user can specify a wildcard for groupResource in the conditions' struct. This will allow the user to apply the patches for all the resources of a particular group or all resources in all groups. For example, `*.apps` will apply to all the resources in the `apps` group, `*` will apply to all the resources in all groups. + +### Helper Command to Generate Merge Patch and Strategic Merge Patch +The patchData of Strategic Merge Patch is sometimes a bit complex for user to write. We can provide a helper command to generate the patchData for Strategic Merge Patch. The command will take the original resource and the modified resource as input and generate the patchData. +It can also be used in JSON Merge Patch. + +Here is a sample code snippet to achieve this: +```go +package main + +import ( + "fmt" + + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func main() { + pod := &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + Image: "nginx", + }, + }, + }, + } + newPod := pod.DeepCopy() + patch := client.StrategicMergeFrom(pod) + newPod.Spec.Containers[0].Image = "nginx1" + + data, _ := patch.Data(newPod) + fmt.Println(string(data)) + // Output: + // {"spec":{"$setElementOrder/containers":[{"name":"web"}],"containers":[{"image":"nginx1","name":"web"}]}} +} +``` + +## Security Considerations +No security impact. + +## Compatibility +Compatible with current Resource Modifiers. + +## Implementation +- Use "github.com/evanphx/json-patch" to support JSON Merge Patch. +- Use "k8s.io/apimachinery/pkg/util/strategicpatch" to support Strategic Merge Patch. +- Use glob to support wildcard for `groupResource` in `conditions` struct. +- Use `test` operation of JSON Patch to calculate the `matches` in `conditions` struct. + +## Future enhancements +- add a Velero subcommand to generate/validate the patchData for Strategic Merge Patch and JSON Merge Patch. +- add jq support for more complex conditions or patches, to meet the situations that the current conditions or patches can not handle. like [this issue](https://github.com/vmware-tanzu/velero/issues/6344) + +## Open Issues +N/A diff --git a/go.mod b/go.mod index 42b473e53..3b0792672 100644 --- a/go.mod +++ b/go.mod @@ -54,6 +54,7 @@ require ( k8s.io/metrics v0.25.6 k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed sigs.k8s.io/controller-runtime v0.12.2 + sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd sigs.k8s.io/yaml v1.3.0 ) @@ -155,7 +156,6 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect k8s.io/component-base v0.24.2 // indirect k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect - sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect ) diff --git a/go.sum b/go.sum index f7462c0df..5aaf8b275 100644 --- a/go.sum +++ b/go.sum @@ -1376,8 +1376,8 @@ sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.30/go.mod h1:fEO7lR sigs.k8s.io/controller-runtime v0.12.2 h1:nqV02cvhbAj7tbt21bpPpTByrXGn2INHRsi39lXy9sE= sigs.k8s.io/controller-runtime v0.12.2/go.mod h1:qKsk4WE6zW2Hfj0G4v10EnNB2jMG1C+NTb8h+DwCoU0= sigs.k8s.io/json v0.0.0-20211208200746-9f7c6b3444d2/go.mod h1:B+TnT182UBxE84DiCz4CVE26eOSDAeYCpfDnC2kdKMY= -sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 h1:iXTIw73aPyC+oRdyqqvVJuloN1p0AC/kzH07hu3NE+k= -sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= +sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo= +sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= sigs.k8s.io/kustomize/api v0.8.11/go.mod h1:a77Ls36JdfCWojpUqR6m60pdGY1AYFix4AH83nJtY1g= sigs.k8s.io/kustomize/api v0.11.4/go.mod h1:k+8RsqYbgpkIrJ4p9jcdPqe8DprLxFUUO0yNOq8C+xI= sigs.k8s.io/kustomize/kyaml v0.11.0/go.mod h1:GNMwjim4Ypgp/MueD3zXHLRJEjz7RvtPae0AwlvEMFM= diff --git a/internal/credentials/file_store.go b/internal/credentials/file_store.go index 1332d4f8d..4b5d25664 100644 --- a/internal/credentials/file_store.go +++ b/internal/credentials/file_store.go @@ -71,7 +71,7 @@ func (n *namespacedFileStore) Path(selector *corev1api.SecretKeySelector) (strin keyFilePath := filepath.Join(n.fsRoot, fmt.Sprintf("%s-%s", selector.Name, selector.Key)) - file, err := n.fs.OpenFile(keyFilePath, os.O_RDWR|os.O_CREATE, 0644) + file, err := n.fs.OpenFile(keyFilePath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644) if err != nil { return "", errors.Wrap(err, "unable to open credentials file for writing") } diff --git a/internal/resourcemodifiers/json_merge_patch.go b/internal/resourcemodifiers/json_merge_patch.go new file mode 100644 index 000000000..5082e2cf5 --- /dev/null +++ b/internal/resourcemodifiers/json_merge_patch.go @@ -0,0 +1,45 @@ +package resourcemodifiers + +import ( + "fmt" + + jsonpatch "github.com/evanphx/json-patch" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" +) + +type JSONMergePatch struct { + PatchData string `json:"patchData,omitempty"` +} + +type JSONMergePatcher struct { + patches []JSONMergePatch +} + +func (p *JSONMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLogger) (*unstructured.Unstructured, error) { + objBytes, err := u.MarshalJSON() + if err != nil { + return nil, fmt.Errorf("error in marshaling object %s", err) + } + + for _, patch := range p.patches { + patchBytes, err := yaml.YAMLToJSON([]byte(patch.PatchData)) + if err != nil { + return nil, fmt.Errorf("error in converting YAML to JSON %s", err) + } + + objBytes, err = jsonpatch.MergePatch(objBytes, patchBytes) + if err != nil { + return nil, fmt.Errorf("error in applying JSON Patch: %s", err.Error()) + } + } + + updated := &unstructured.Unstructured{} + err = updated.UnmarshalJSON(objBytes) + if err != nil { + return nil, fmt.Errorf("error in unmarshalling modified object %s", err.Error()) + } + + return updated, nil +} diff --git a/internal/resourcemodifiers/json_merge_patch_test.go b/internal/resourcemodifiers/json_merge_patch_test.go new file mode 100644 index 000000000..b7323fc61 --- /dev/null +++ b/internal/resourcemodifiers/json_merge_patch_test.go @@ -0,0 +1,41 @@ +package resourcemodifiers + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" +) + +func TestJsonMergePatchFailure(t *testing.T) { + tests := []struct { + name string + data string + }{ + { + name: "patch with bad yaml", + data: "a: b:", + }, + { + name: "patch with bad json", + data: `{"a"::1}`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + err := clientgoscheme.AddToScheme(scheme) + assert.NoError(t, err) + pt := &JSONMergePatcher{ + patches: []JSONMergePatch{{PatchData: tt.data}}, + } + + u := &unstructured.Unstructured{} + _, err = pt.Patch(u, logrus.New()) + assert.Error(t, err) + }) + } +} diff --git a/internal/resourcemodifiers/json_patch.go b/internal/resourcemodifiers/json_patch.go new file mode 100644 index 000000000..d137d40be --- /dev/null +++ b/internal/resourcemodifiers/json_patch.go @@ -0,0 +1,103 @@ +package resourcemodifiers + +import ( + "errors" + "fmt" + "strconv" + "strings" + + jsonpatch "github.com/evanphx/json-patch" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +type JSONPatch struct { + Operation string `json:"operation"` + From string `json:"from,omitempty"` + Path string `json:"path"` + Value string `json:"value,omitempty"` +} + +func (p *JSONPatch) ToString() string { + if addQuotes(&p.Value) { + return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": "%s"}`, p.Operation, p.From, p.Path, p.Value) + } + return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": %s}`, p.Operation, p.From, p.Path, p.Value) +} + +func addQuotes(value *string) bool { + if *value == "" { + return true + } + // if value is escaped, remove escape and add quotes + // this is useful for scenarios where boolean, null and numbers are required to be set as string. + if strings.HasPrefix(*value, "\"") && strings.HasSuffix(*value, "\"") { + *value = strings.TrimPrefix(*value, "\"") + *value = strings.TrimSuffix(*value, "\"") + return true + } + // if value is null, then don't add quotes + if *value == "null" { + return false + } + // if value is a boolean, then don't add quotes + if strings.ToLower(*value) == "true" || strings.ToLower(*value) == "false" { + return false + } + // if value is a json object or array, then don't add quotes. + if strings.HasPrefix(*value, "{") || strings.HasPrefix(*value, "[") { + return false + } + // if value is a number, then don't add quotes + if _, err := strconv.ParseFloat(*value, 64); err == nil { + return false + } + return true +} + +type JSONPatcher struct { + patches []JSONPatch `yaml:"patches"` +} + +func (p *JSONPatcher) Patch(u *unstructured.Unstructured, logger logrus.FieldLogger) (*unstructured.Unstructured, error) { + modifiedObjBytes, err := p.applyPatch(u) + if err != nil { + if errors.Is(err, jsonpatch.ErrTestFailed) { + logger.Infof("Test operation failed for JSON Patch %s", err.Error()) + return u.DeepCopy(), nil + } + return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) + } + + updated := &unstructured.Unstructured{} + err = updated.UnmarshalJSON(modifiedObjBytes) + if err != nil { + return nil, fmt.Errorf("error in unmarshalling modified object %s", err.Error()) + } + + return updated, nil +} + +func (p *JSONPatcher) applyPatch(u *unstructured.Unstructured) ([]byte, error) { + patchBytes := p.patchArrayToByteArray() + jsonPatch, err := jsonpatch.DecodePatch(patchBytes) + if err != nil { + return nil, fmt.Errorf("error in decoding json patch %s", err.Error()) + } + + objBytes, err := u.MarshalJSON() + if err != nil { + return nil, fmt.Errorf("error in marshaling object %s", err.Error()) + } + + return jsonPatch.Apply(objBytes) +} + +func (p *JSONPatcher) patchArrayToByteArray() []byte { + var patches []string + for _, patch := range p.patches { + patches = append(patches, patch.ToString()) + } + patchesStr := strings.Join(patches, ",\n\t") + return []byte(fmt.Sprintf(`[%s]`, patchesStr)) +} diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index dbcd8e7ba..8d8d976d2 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/internal/resourcemodifiers/resource_modifiers.go @@ -3,16 +3,16 @@ package resourcemodifiers import ( "fmt" "regexp" - "strconv" - "strings" jsonpatch "github.com/evanphx/json-patch" + "github.com/gobwas/glob" "github.com/pkg/errors" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/yaml" "github.com/vmware-tanzu/velero/pkg/util/collections" @@ -23,11 +23,9 @@ const ( ResourceModifierSupportedVersionV1 = "v1" ) -type JSONPatch struct { - Operation string `json:"operation"` - From string `json:"from,omitempty"` - Path string `json:"path"` - Value string `json:"value,omitempty"` +type MatchRule struct { + Path string `json:"path,omitempty"` + Value string `json:"value,omitempty"` } type Conditions struct { @@ -35,11 +33,14 @@ type Conditions struct { GroupResource string `json:"groupResource"` ResourceNameRegex string `json:"resourceNameRegex,omitempty"` LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty"` + Matches []MatchRule `json:"matches,omitempty"` } type ResourceModifierRule struct { - Conditions Conditions `json:"conditions"` - Patches []JSONPatch `json:"patches"` + Conditions Conditions `json:"conditions"` + Patches []JSONPatch `json:"patches,omitempty"` + MergePatches []JSONMergePatch `json:"mergePatches,omitempty"` + StrategicPatches []StrategicMergePatch `json:"strategicPatches,omitempty"` } type ResourceModifiers struct { @@ -68,10 +69,10 @@ func GetResourceModifiersFromConfig(cm *v1.ConfigMap) (*ResourceModifiers, error return resModifiers, nil } -func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstructured, groupResource string, log logrus.FieldLogger) []error { +func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) []error { var errs []error for _, rule := range p.ResourceModifierRules { - err := rule.Apply(obj, groupResource, log) + err := rule.apply(obj, groupResource, scheme, log) if err != nil { errs = append(errs, err) } @@ -80,13 +81,22 @@ func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstruc return errs } -func (r *ResourceModifierRule) Apply(obj *unstructured.Unstructured, groupResource string, log logrus.FieldLogger) error { - namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...) - if !namespaceInclusion.ShouldInclude(obj.GetNamespace()) { - return nil +func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) error { + ns := obj.GetNamespace() + if ns != "" { + namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...) + if !namespaceInclusion.ShouldInclude(ns) { + return nil + } } - if r.Conditions.GroupResource != groupResource { + g, err := glob.Compile(r.Conditions.GroupResource, '.') + if err != nil { + log.Errorf("Bad glob pattern of groupResource in condition, groupResource: %s, err: %s", r.Conditions.GroupResource, err) + return err + } + + if !g.Match(groupResource) { return nil } @@ -110,87 +120,82 @@ func (r *ResourceModifierRule) Apply(obj *unstructured.Unstructured, groupResour } } - patches, err := r.PatchArrayToByteArray() + match, err := matchConditions(obj, r.Conditions.Matches, log) if err != nil { return err + } else if !match { + log.Info("Conditions do not match, skip it") + return nil } + log.Infof("Applying resource modifier patch on %s/%s", obj.GetNamespace(), obj.GetName()) - err = ApplyPatch(patches, obj, log) + err = r.applyPatch(obj, scheme, log) if err != nil { return err } return nil } -// PatchArrayToByteArray converts all JsonPatch to string array with the format of jsonpatch.Patch and then convert it to byte array -func (r *ResourceModifierRule) PatchArrayToByteArray() ([]byte, error) { - var patches []string - for _, patch := range r.Patches { - patches = append(patches, patch.ToString()) +func matchConditions(u *unstructured.Unstructured, rules []MatchRule, _ logrus.FieldLogger) (bool, error) { + if len(rules) == 0 { + return true, nil } - patchesStr := strings.Join(patches, ",\n\t") - return []byte(fmt.Sprintf(`[%s]`, patchesStr)), nil -} -func (p *JSONPatch) ToString() string { - if addQuotes(p.Value) { - return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": "%s"}`, p.Operation, p.From, p.Path, p.Value) - } - return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": %s}`, p.Operation, p.From, p.Path, p.Value) -} + var fixed []JSONPatch + for _, rule := range rules { + if rule.Path == "" { + return false, fmt.Errorf("path is required for match rule") + } -func ApplyPatch(patch []byte, obj *unstructured.Unstructured, log logrus.FieldLogger) error { - jsonPatch, err := jsonpatch.DecodePatch(patch) - if err != nil { - return fmt.Errorf("error in decoding json patch %s", err.Error()) + fixed = append(fixed, JSONPatch{ + Operation: "test", + Path: rule.Path, + Value: rule.Value, + }) } - objBytes, err := obj.MarshalJSON() - if err != nil { - return fmt.Errorf("error in marshaling object %s", err.Error()) - } - modifiedObjBytes, err := jsonPatch.Apply(objBytes) + + p := &JSONPatcher{patches: fixed} + _, err := p.applyPatch(u) if err != nil { if errors.Is(err, jsonpatch.ErrTestFailed) { - log.Infof("Test operation failed for JSON Patch %s", err.Error()) - return nil + return false, nil } - return fmt.Errorf("error in applying JSON Patch %s", err.Error()) + return false, err } - err = obj.UnmarshalJSON(modifiedObjBytes) - if err != nil { - return fmt.Errorf("error in unmarshalling modified object %s", err.Error()) - } - return nil + + return true, nil } func unmarshalResourceModifiers(yamlData []byte) (*ResourceModifiers, error) { resModifiers := &ResourceModifiers{} err := yaml.UnmarshalStrict(yamlData, resModifiers) if err != nil { - return nil, fmt.Errorf("failed to decode yaml data into resource modifiers %v", err) + return nil, fmt.Errorf("failed to decode yaml data into resource modifiers, err: %s", err) } return resModifiers, nil } -func addQuotes(value string) bool { - if value == "" { - return true +type patcher interface { + Patch(u *unstructured.Unstructured, logger logrus.FieldLogger) (*unstructured.Unstructured, error) +} + +func (r *ResourceModifierRule) applyPatch(u *unstructured.Unstructured, scheme *runtime.Scheme, logger logrus.FieldLogger) error { + var p patcher + if len(r.Patches) > 0 { + p = &JSONPatcher{patches: r.Patches} + } else if len(r.MergePatches) > 0 { + p = &JSONMergePatcher{patches: r.MergePatches} + } else if len(r.StrategicPatches) > 0 { + p = &StrategicMergePatcher{patches: r.StrategicPatches, scheme: scheme} + } else { + return fmt.Errorf("no patch data found") } - // if value is null, then don't add quotes - if value == "null" { - return false + + updated, err := p.Patch(u, logger) + if err != nil { + return fmt.Errorf("error in applying patch %s", err) } - // if value is a boolean, then don't add quotes - if _, err := strconv.ParseBool(value); err == nil { - return false - } - // if value is a json object or array, then don't add quotes. - if strings.HasPrefix(value, "{") || strings.HasPrefix(value, "[") { - return false - } - // if value is a number, then don't add quotes - if _, err := strconv.ParseFloat(value, 64); err == nil { - return false - } - return true + + u.SetUnstructuredContent(updated.Object) + return nil } diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go index c2150bbc8..a473df641 100644 --- a/internal/resourcemodifiers/resource_modifiers_test.go +++ b/internal/resourcemodifiers/resource_modifiers_test.go @@ -9,6 +9,10 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer/yaml" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" ) func TestGetResourceModifiersFromConfig(t *testing.T) { @@ -116,6 +120,186 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { }, } + cm5 := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: pods\n namespaces:\n - ns1\n matches:\n - path: /metadata/annotations/foo\n value: bar\n mergePatches:\n - patchData: |\n metadata:\n annotations:\n foo: null", + }, + } + + rules5 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{ + "ns1", + }, + Matches: []MatchRule{ + { + Path: "/metadata/annotations/foo", + Value: "bar", + }, + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: "metadata:\n annotations:\n foo: null", + }, + }, + }, + }, + } + + cm6 := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: pods\n namespaces:\n - ns1\n strategicPatches:\n - patchData: |\n spec:\n containers:\n - name: nginx\n image: repo2/nginx", + }, + } + + rules6 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{ + "ns1", + }, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: "spec:\n containers:\n - name: nginx\n image: repo2/nginx", + }, + }, + }, + }, + } + + cm7 := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: pods\n namespaces:\n - ns1\n mergePatches:\n - patchData: |\n {\"metadata\":{\"annotations\":{\"foo\":null}}}", + }, + } + + rules7 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{ + "ns1", + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"annotations":{"foo":null}}}`, + }, + }, + }, + }, + } + + cm8 := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: pods\n namespaces:\n - ns1\n strategicPatches:\n - patchData: |\n {\"spec\":{\"containers\":[{\"name\": \"nginx\",\"image\": \"repo2/nginx\"}]}}", + }, + } + + rules8 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{ + "ns1", + }, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: `{"spec":{"containers":[{"name": "nginx","image": "repo2/nginx"}]}}`, + }, + }, + }, + }, + } + cm9 := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: deployments.apps\n resourceNameRegex: \"^test-.*$\"\n namespaces:\n - bar\n - foo\n patches:\n - operation: replace\n path: \"/value/bool\"\n value: \"\\\"true\\\"\"\n\n\n", + }, + } + + rules9 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "deployments.apps", + ResourceNameRegex: "^test-.*$", + Namespaces: []string{"bar", "foo"}, + }, + Patches: []JSONPatch{ + { + Operation: "replace", + Path: "/value/bool", + Value: `"true"`, + }, + }, + }, + }, + } + cm10 := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "test-namespace", + }, + Data: map[string]string{ + "sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n groupResource: deployments.apps\n resourceNameRegex: \"^test-.*$\"\n namespaces:\n - bar\n - foo\n patches:\n - operation: replace\n path: \"/value/bool\"\n value: \"true\"\n\n\n", + }, + } + + rules10 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "deployments.apps", + ResourceNameRegex: "^test-.*$", + Namespaces: []string{"bar", "foo"}, + }, + Patches: []JSONPatch{ + { + Operation: "replace", + Path: "/value/bool", + Value: "true", + }, + }, + }, + }, + } + type args struct { cm *v1.ConfigMap } @@ -165,6 +349,54 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { want: nil, wantErr: true, }, + { + name: "complex yaml data with json merge patch", + args: args{ + cm: cm5, + }, + want: rules5, + wantErr: false, + }, + { + name: "complex yaml data with strategic merge patch", + args: args{ + cm: cm6, + }, + want: rules6, + wantErr: false, + }, + { + name: "complex json data with json merge patch", + args: args{ + cm: cm7, + }, + want: rules7, + wantErr: false, + }, + { + name: "complex json data with strategic merge patch", + args: args{ + cm: cm8, + }, + want: rules8, + wantErr: false, + }, + { + name: "bool value as string", + args: args{ + cm: cm9, + }, + want: rules9, + wantErr: false, + }, + { + name: "bool value as bool", + args: args{ + cm: cm10, + }, + want: rules10, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -307,7 +539,24 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { }, }, } - + cmTrue := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "data": map[string]interface{}{ + "test": "true", + }, + }, + } + cmFalse := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "data": map[string]interface{}{ + "test": "false", + }, + }, + } type fields struct { Version string ResourceModifierRules []ResourceModifierRule @@ -323,6 +572,33 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { wantErr bool wantObj *unstructured.Unstructured }{ + { + name: "configmap true false string", + fields: fields{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + ResourceNameRegex: ".*", + }, + Patches: []JSONPatch{ + { + Operation: "replace", + Path: "/data/test", + Value: `"false"`, + }, + }, + }, + }, + }, + args: args{ + obj: cmTrue.DeepCopy(), + groupResource: "configmaps", + }, + wantErr: false, + wantObj: cmFalse.DeepCopy(), + }, { name: "Invalid Regex throws error", fields: fields{ @@ -487,6 +763,38 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { wantErr: false, wantObj: deployNginxTwoReplica.DeepCopy(), }, + { + name: "nginx deployment: Empty Resource Regex", + fields: fields{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "deployments.apps", + Namespaces: []string{"foo"}, + }, + Patches: []JSONPatch{ + { + Operation: "test", + Path: "/spec/replicas", + Value: "1", + }, + { + Operation: "replace", + Path: "/spec/replicas", + Value: "2", + }, + }, + }, + }, + }, + args: args{ + obj: deployNginxOneReplica.DeepCopy(), + groupResource: "deployments.apps", + }, + wantErr: false, + wantObj: deployNginxTwoReplica.DeepCopy(), + }, { name: "nginx deployment: Empty Resource Regex and namespaces list", fields: fields{ @@ -704,7 +1012,7 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { Version: tt.fields.Version, ResourceModifierRules: tt.fields.ResourceModifierRules, } - got := p.ApplyResourceModifierRules(tt.args.obj, tt.args.groupResource, logrus.New()) + got := p.ApplyResourceModifierRules(tt.args.obj, tt.args.groupResource, nil, logrus.New()) assert.Equal(t, tt.wantErr, len(got) > 0) assert.Equal(t, *tt.wantObj, *tt.args.obj) @@ -712,6 +1020,633 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { } } +var podYAMLWithNginxImage = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: nginx + name: nginx +` + +var podYAMLWithNginx1Image = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: nginx1 + name: nginx +` + +var podYAMLWithNFSVolume = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: fake + name: fake + volumeMounts: + - mountPath: /fake1 + name: vol1 + - mountPath: /fake2 + name: vol2 + volumes: + - name: vol1 + nfs: + path: /fake2 + - name: vol2 + emptyDir: {} +` + +var podYAMLWithPVCVolume = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: fake + name: fake + volumeMounts: + - mountPath: /fake1 + name: vol1 + - mountPath: /fake2 + name: vol2 + volumes: + - name: vol1 + persistentVolumeClaim: + claimName: pvc1 + - name: vol2 + emptyDir: {} +` + +var svcYAMLWithPort8000 = ` +apiVersion: v1 +kind: Service +metadata: + name: svc1 + namespace: fake +spec: + ports: + - name: fake1 + port: 8001 + protocol: TCP + targetPort: 8001 + - name: fake + port: 8000 + protocol: TCP + targetPort: 8000 + - name: fake2 + port: 8002 + protocol: TCP + targetPort: 8002 +` + +var svcYAMLWithPort9000 = ` +apiVersion: v1 +kind: Service +metadata: + name: svc1 + namespace: fake +spec: + ports: + - name: fake1 + port: 8001 + protocol: TCP + targetPort: 8001 + - name: fake + port: 9000 + protocol: TCP + targetPort: 9000 + - name: fake2 + port: 8002 + protocol: TCP + targetPort: 8002 +` + +var cmYAMLWithLabelAToB = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: fake + labels: + a: b + c: d +` + +var cmYAMLWithLabelAToC = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: fake + labels: + a: c + c: d +` + +var cmYAMLWithoutLabelA = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: fake + labels: + c: d +` + +func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *testing.T) { + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithNFSVolume), nil, nil) + assert.NoError(t, err) + podWithNFSVolume := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithPVCVolume), nil, nil) + assert.NoError(t, err) + podWithPVCVolume := o2.(*unstructured.Unstructured) + + o3, _, err := unstructuredSerializer.Decode([]byte(svcYAMLWithPort8000), nil, nil) + assert.NoError(t, err) + svcWithPort8000 := o3.(*unstructured.Unstructured) + + o4, _, err := unstructuredSerializer.Decode([]byte(svcYAMLWithPort9000), nil, nil) + assert.NoError(t, err) + svcWithPort9000 := o4.(*unstructured.Unstructured) + + o5, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithNginxImage), nil, nil) + assert.NoError(t, err) + podWithNginxImage := o5.(*unstructured.Unstructured) + + o6, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithNginx1Image), nil, nil) + assert.NoError(t, err) + podWithNginx1Image := o6.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "update image", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: `{"spec":{"containers":[{"name":"nginx","image":"nginx1"}]}}`, + }, + }, + }, + }, + }, + obj: podWithNginxImage.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithNginx1Image.DeepCopy(), + }, + { + name: "update image with yaml format", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: `spec: + containers: + - name: nginx + image: nginx1`, + }, + }, + }, + }, + }, + obj: podWithNginxImage.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithNginx1Image.DeepCopy(), + }, + { + name: "replace nfs with pvc in volume", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: `{"spec":{"volumes":[{"nfs":null,"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`, + }, + }, + }, + }, + }, + obj: podWithNFSVolume.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithPVCVolume.DeepCopy(), + }, + { + name: "replace any other volume source with pvc in volume", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: `{"spec":{"volumes":[{"$retainKeys":["name","persistentVolumeClaim"],"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`, + }, + }, + }, + }, + }, + obj: podWithNFSVolume.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithPVCVolume.DeepCopy(), + }, + { + name: "update a service port", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "services", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: `{"spec":{"$setElementOrder/ports":[{"port":8001},{"port":9000},{"port":8002}],"ports":[{"name":"fake","port":9000,"protocol":"TCP","targetPort":9000},{"$patch":"delete","port":8000}]}}`, + }, + }, + }, + }, + }, + obj: svcWithPort8000.DeepCopy(), + groupResource: "services", + wantErr: false, + wantObj: svcWithPort9000.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, scheme, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + +func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.T) { + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToB), nil, nil) + assert.NoError(t, err) + cmWithLabelAToB := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToC), nil, nil) + assert.NoError(t, err) + cmWithLabelAToC := o2.(*unstructured.Unstructured) + + o3, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithoutLabelA), nil, nil) + assert.NoError(t, err) + cmWithoutLabelA := o3.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "update labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":"c"}}}`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "update labels in yaml format", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `metadata: + labels: + a: c`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "delete labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":null}}}`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithoutLabelA.DeepCopy(), + }, + { + name: "add labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":"b"}}}`, + }, + }, + }, + }, + }, + obj: cmWithoutLabelA.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToB.DeepCopy(), + }, + { + name: "delete non-existing labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":null}}}`, + }, + }, + }, + }, + }, + obj: cmWithoutLabelA.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithoutLabelA.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, nil, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + +func TestResourceModifiers_wildcard_in_GroupResource(t *testing.T) { + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToB), nil, nil) + assert.NoError(t, err) + cmWithLabelAToB := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToC), nil, nil) + assert.NoError(t, err) + cmWithLabelAToC := o2.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "match all groups and resources", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":"c"}}}`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "match all resources in group apps", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*.apps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":"c"}}}`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "fake.apps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, nil, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + +func TestResourceModifiers_conditional_patches(t *testing.T) { + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToB), nil, nil) + assert.NoError(t, err) + cmWithLabelAToB := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToC), nil, nil) + assert.NoError(t, err) + cmWithLabelAToC := o2.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "match conditions and apply patches", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + Namespaces: []string{"fake"}, + Matches: []MatchRule{ + { + Path: "/metadata/labels/a", + Value: "b", + }, + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":"c"}}}`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "mismatch conditions and skip patches", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + Namespaces: []string{"fake"}, + Matches: []MatchRule{ + { + Path: "/metadata/labels/a", + Value: "c", + }, + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":"c"}}}`, + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToB.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, nil, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + func TestJSONPatch_ToString(t *testing.T) { type fields struct { Operation string diff --git a/internal/resourcemodifiers/resource_modifiers_validator.go b/internal/resourcemodifiers/resource_modifiers_validator.go index 24fe0dbb2..9ad817eca 100644 --- a/internal/resourcemodifiers/resource_modifiers_validator.go +++ b/internal/resourcemodifiers/resource_modifiers_validator.go @@ -9,6 +9,21 @@ func (r *ResourceModifierRule) Validate() error { if err := r.Conditions.Validate(); err != nil { return err } + + count := 0 + for _, size := range []int{ + len(r.Patches), + len(r.MergePatches), + len(r.StrategicPatches), + } { + if size != 0 { + count++ + } + if count >= 2 { + return fmt.Errorf("only one of patches, mergePatches, strategicPatches can be specified") + } + } + for _, patch := range r.Patches { if err := patch.Validate(); err != nil { return err diff --git a/internal/resourcemodifiers/resource_modifiers_validator_test.go b/internal/resourcemodifiers/resource_modifiers_validator_test.go index a46c46530..accd320a0 100644 --- a/internal/resourcemodifiers/resource_modifiers_validator_test.go +++ b/internal/resourcemodifiers/resource_modifiers_validator_test.go @@ -114,6 +114,32 @@ func TestResourceModifiers_Validate(t *testing.T) { }, wantErr: true, }, + { + name: "More than one patch type in a rule", + fields: fields{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + }, + Patches: []JSONPatch{ + { + Operation: "test", + Path: "/spec/storageClassName", + Value: "premium", + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: `{"metadata":{"labels":{"a":null}}}`, + }, + }, + }, + }, + }, + wantErr: true, + }, } for _, tt := range tests { diff --git a/internal/resourcemodifiers/strategic_merge_patch.go b/internal/resourcemodifiers/strategic_merge_patch.go new file mode 100644 index 000000000..8d3fe4c51 --- /dev/null +++ b/internal/resourcemodifiers/strategic_merge_patch.go @@ -0,0 +1,143 @@ +package resourcemodifiers + +import ( + "fmt" + "net/http" + + "github.com/sirupsen/logrus" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/mergepatch" + "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/apimachinery/pkg/util/validation/field" + kubejson "sigs.k8s.io/json" + "sigs.k8s.io/yaml" +) + +type StrategicMergePatch struct { + PatchData string `json:"patchData,omitempty"` +} + +type StrategicMergePatcher struct { + patches []StrategicMergePatch + scheme *runtime.Scheme +} + +func (p *StrategicMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLogger) (*unstructured.Unstructured, error) { + gvk := u.GetObjectKind().GroupVersionKind() + schemaReferenceObj, err := p.scheme.New(gvk) + if err != nil { + return nil, err + } + + origin := u.DeepCopy() + updated := u.DeepCopy() + for _, patch := range p.patches { + patchBytes, err := yaml.YAMLToJSON([]byte(patch.PatchData)) + if err != nil { + return nil, fmt.Errorf("error in converting YAML to JSON %s", err) + } + + err = strategicPatchObject(origin, patchBytes, updated, schemaReferenceObj) + if err != nil { + return nil, fmt.Errorf("error in applying Strategic Patch %s", err.Error()) + } + + origin = updated.DeepCopy() + } + + return updated, nil +} + +// strategicPatchObject applies a strategic merge patch of `patchBytes` to +// `originalObject` and stores the result in `objToUpdate`. +// It additionally returns the map[string]interface{} representation of the +// `originalObject` and `patchBytes`. +// NOTE: Both `originalObject` and `objToUpdate` are supposed to be versioned. +func strategicPatchObject( + originalObject runtime.Object, + patchBytes []byte, + objToUpdate runtime.Object, + schemaReferenceObj runtime.Object, +) error { + originalObjMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(originalObject) + if err != nil { + return err + } + + patchMap := make(map[string]interface{}) + var strictErrs []error + strictErrs, err = kubejson.UnmarshalStrict(patchBytes, &patchMap) + if err != nil { + return apierrors.NewBadRequest(err.Error()) + } + + if err := applyPatchToObject(originalObjMap, patchMap, objToUpdate, schemaReferenceObj, strictErrs); err != nil { + return err + } + return nil +} + +// applyPatchToObject applies a strategic merge patch of to +// and stores the result in . +// NOTE: must be a versioned object. +func applyPatchToObject( + originalMap map[string]interface{}, + patchMap map[string]interface{}, + objToUpdate runtime.Object, + schemaReferenceObj runtime.Object, + strictErrs []error, +) error { + patchedObjMap, err := strategicpatch.StrategicMergeMapPatch(originalMap, patchMap, schemaReferenceObj) + if err != nil { + return interpretStrategicMergePatchError(err) + } + + // Rather than serialize the patched map to JSON, then decode it to an object, we go directly from a map to an object + converter := runtime.DefaultUnstructuredConverter + if err := converter.FromUnstructuredWithValidation(patchedObjMap, objToUpdate, true); err != nil { + strictError, isStrictError := runtime.AsStrictDecodingError(err) + switch { + case !isStrictError: + // disregard any sttrictErrs, because it's an incomplete + // list of strict errors given that we don't know what fields were + // unknown because StrategicMergeMapPatch failed. + // Non-strict errors trump in this case. + return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ + field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), err.Error()), + }) + //case validationDirective == metav1.FieldValidationWarn: + // addStrictDecodingWarnings(requestContext, append(strictErrs, strictError.Errors()...)) + default: + strictDecodingError := runtime.NewStrictDecodingError(append(strictErrs, strictError.Errors()...)) + return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ + field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), strictDecodingError.Error()), + }) + } + } else if len(strictErrs) > 0 { + switch { + //case validationDirective == metav1.FieldValidationWarn: + // addStrictDecodingWarnings(requestContext, strictErrs) + default: + return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ + field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), runtime.NewStrictDecodingError(strictErrs).Error()), + }) + } + } + + return nil +} + +// interpretStrategicMergePatchError interprets the error type and returns an error with appropriate HTTP code. +func interpretStrategicMergePatchError(err error) error { + switch err { + case mergepatch.ErrBadJSONDoc, mergepatch.ErrBadPatchFormatForPrimitiveList, mergepatch.ErrBadPatchFormatForRetainKeys, mergepatch.ErrBadPatchFormatForSetElementOrderList, mergepatch.ErrUnsupportedStrategicMergePatchFormat: + return apierrors.NewBadRequest(err.Error()) + case mergepatch.ErrNoListOfLists, mergepatch.ErrPatchContentNotMatchRetainKeys: + return apierrors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false) + default: + return err + } +} diff --git a/internal/resourcemodifiers/strategic_merge_patch_test.go b/internal/resourcemodifiers/strategic_merge_patch_test.go new file mode 100644 index 000000000..6c3c700f5 --- /dev/null +++ b/internal/resourcemodifiers/strategic_merge_patch_test.go @@ -0,0 +1,52 @@ +package resourcemodifiers + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" +) + +func TestStrategicMergePatchFailure(t *testing.T) { + tests := []struct { + name string + data string + kind string + }{ + { + name: "patch with unknown kind", + data: "{}", + kind: "BadKind", + }, + { + name: "patch with bad yaml", + data: "a: b:", + kind: "Pod", + }, + { + name: "patch with bad json", + data: `{"a"::1}`, + kind: "Pod", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + err := clientgoscheme.AddToScheme(scheme) + assert.NoError(t, err) + pt := &StrategicMergePatcher{ + patches: []StrategicMergePatch{{PatchData: tt.data}}, + scheme: scheme, + } + + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{Version: "v1", Kind: tt.kind}) + _, err = pt.Patch(u, logrus.New()) + assert.Error(t, err) + }) + } +} diff --git a/pkg/backup/request.go b/pkg/backup/request.go index e9da4bdde..44bc5578f 100644 --- a/pkg/backup/request.go +++ b/pkg/backup/request.go @@ -20,8 +20,6 @@ import ( "fmt" "sort" - snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" - "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/resourcepolicies" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -51,7 +49,6 @@ type Request struct { VolumeSnapshots []*volume.Snapshot PodVolumeBackups []*velerov1api.PodVolumeBackup BackedUpItems map[itemKey]struct{} - CSISnapshots []snapshotv1api.VolumeSnapshot itemOperationsList *[]*itemoperation.BackupOperation ResPolicies *resourcepolicies.Policies SkippedPVTracker *skipPVTracker diff --git a/pkg/backup/snapshots.go b/pkg/backup/snapshots.go new file mode 100644 index 000000000..7bdfc585e --- /dev/null +++ b/pkg/backup/snapshots.go @@ -0,0 +1,71 @@ +package backup + +import ( + "context" + + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/util/sets" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/features" + "github.com/vmware-tanzu/velero/pkg/label" + "github.com/vmware-tanzu/velero/pkg/util/boolptr" +) + +// GetBackupCSIResources is used to get CSI snapshot related resources. +// Returns VolumeSnapshot, VolumeSnapshotContent, VolumeSnapshotClasses referenced +func GetBackupCSIResources( + client kbclient.Client, + volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, + backup *velerov1api.Backup, + backupLog logrus.FieldLogger, +) ( + volumeSnapshots []snapshotv1api.VolumeSnapshot, + volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent, + volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass, +) { + if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) { + backupLog.Info("backup SnapshotMoveData is set to true, skip VolumeSnapshot resource persistence.") + } else if features.IsEnabled(velerov1api.CSIFeatureFlag) { + selector := label.NewSelectorForBackup(backup.Name) + vscList := &snapshotv1api.VolumeSnapshotContentList{} + + if volumeSnapshotLister != nil { + tmpVSs, err := volumeSnapshotLister.List(label.NewSelectorForBackup(backup.Name)) + if err != nil { + backupLog.Error(err) + } + for _, vs := range tmpVSs { + volumeSnapshots = append(volumeSnapshots, *vs) + } + } + + err := client.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector}) + if err != nil { + backupLog.Error(err) + } + if len(vscList.Items) >= 0 { + volumeSnapshotContents = vscList.Items + } + + vsClassSet := sets.NewString() + for index := range volumeSnapshotContents { + // persist the volumesnapshotclasses referenced by vsc + if volumeSnapshotContents[index].Spec.VolumeSnapshotClassName != nil && !vsClassSet.Has(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName) { + vsClass := &snapshotv1api.VolumeSnapshotClass{} + if err := client.Get(context.TODO(), kbclient.ObjectKey{Name: *volumeSnapshotContents[index].Spec.VolumeSnapshotClassName}, vsClass); err != nil { + backupLog.Error(err) + } else { + vsClassSet.Insert(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName) + volumeSnapshotClasses = append(volumeSnapshotClasses, *vsClass) + } + } + } + backup.Status.CSIVolumeSnapshotsAttempted = len(volumeSnapshots) + } + + return volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses +} diff --git a/pkg/builder/backup_builder.go b/pkg/builder/backup_builder.go index 275852bd1..b689bbcae 100644 --- a/pkg/builder/backup_builder.go +++ b/pkg/builder/backup_builder.go @@ -299,3 +299,9 @@ func (b *BackupBuilder) DataMover(name string) *BackupBuilder { b.object.Spec.DataMover = name return b } + +// WithStatus sets the Backup's status. +func (b *BackupBuilder) WithStatus(status velerov1api.BackupStatus) *BackupBuilder { + b.object.Status = status + return b +} diff --git a/pkg/builder/volume_snapshot_builder.go b/pkg/builder/volume_snapshot_builder.go index 19815c0f0..bbaedd16e 100644 --- a/pkg/builder/volume_snapshot_builder.go +++ b/pkg/builder/volume_snapshot_builder.go @@ -67,3 +67,8 @@ func (v *VolumeSnapshotBuilder) BoundVolumeSnapshotContentName(vscName string) * v.object.Status.BoundVolumeSnapshotContentName = &vscName return v } + +func (v *VolumeSnapshotBuilder) SourcePVC(name string) *VolumeSnapshotBuilder { + v.object.Spec.Source.PersistentVolumeClaimName = &name + return v +} diff --git a/pkg/cmd/cli/nodeagent/server.go b/pkg/cmd/cli/nodeagent/server.go index 10df06f89..cb4435c6b 100644 --- a/pkg/cmd/cli/nodeagent/server.go +++ b/pkg/cmd/cli/nodeagent/server.go @@ -114,6 +114,7 @@ func NewServerCommand(f client.Factory) *cobra.Command { command.Flags().Var(formatFlag, "log-format", fmt.Sprintf("The format for log output. Valid values are %s.", strings.Join(formatFlag.AllowedValues(), ", "))) command.Flags().DurationVar(&config.resourceTimeout, "resource-timeout", config.resourceTimeout, "How long to wait for resource processes which are not covered by other specific timeout parameters. Default is 10 minutes.") command.Flags().DurationVar(&config.dataMoverPrepareTimeout, "data-mover-prepare-timeout", config.dataMoverPrepareTimeout, "How long to wait for preparing a DataUpload/DataDownload. Default is 30 minutes.") + command.Flags().StringVar(&config.metricsAddress, "metrics-address", config.metricsAddress, "The address to expose prometheus metrics") return command } @@ -193,14 +194,15 @@ func newNodeAgentServer(logger logrus.FieldLogger, factory client.Factory, confi } s := &nodeAgentServer{ - logger: logger, - ctx: ctx, - cancelFunc: cancelFunc, - fileSystem: filesystem.NewFileSystem(), - mgr: mgr, - config: config, - namespace: factory.Namespace(), - nodeName: nodeName, + logger: logger, + ctx: ctx, + cancelFunc: cancelFunc, + fileSystem: filesystem.NewFileSystem(), + mgr: mgr, + config: config, + namespace: factory.Namespace(), + nodeName: nodeName, + metricsAddress: config.metricsAddress, } // the cache isn't initialized yet when "validatePodVolumesHostPath" is called, the client returned by the manager cannot @@ -273,13 +275,13 @@ func (s *nodeAgentServer) run() { } dataUploadReconciler := controller.NewDataUploadReconciler(s.mgr.GetClient(), s.kubeClient, s.csiSnapshotClient.SnapshotV1(), repoEnsurer, clock.RealClock{}, credentialGetter, s.nodeName, s.fileSystem, s.config.dataMoverPrepareTimeout, s.logger, s.metrics) - s.markDataUploadsCancel(dataUploadReconciler) + s.attemptDataUploadResume(dataUploadReconciler) if err = dataUploadReconciler.SetupWithManager(s.mgr); err != nil { s.logger.WithError(err).Fatal("Unable to create the data upload controller") } dataDownloadReconciler := controller.NewDataDownloadReconciler(s.mgr.GetClient(), s.kubeClient, repoEnsurer, credentialGetter, s.nodeName, s.config.dataMoverPrepareTimeout, s.logger, s.metrics) - s.markDataDownloadsCancel(dataDownloadReconciler) + s.attemptDataDownloadResume(dataDownloadReconciler) if err = dataDownloadReconciler.SetupWithManager(s.mgr); err != nil { s.logger.WithError(err).Fatal("Unable to create the data download controller") } @@ -353,65 +355,28 @@ func (s *nodeAgentServer) markInProgressCRsFailed() { s.markInProgressPVRsFailed(client) } -func (s *nodeAgentServer) markDataUploadsCancel(r *controller.DataUploadReconciler) { +func (s *nodeAgentServer) attemptDataUploadResume(r *controller.DataUploadReconciler) { // the function is called before starting the controller manager, the embedded client isn't ready to use, so create a new one here client, err := ctrlclient.New(s.mgr.GetConfig(), ctrlclient.Options{Scheme: s.mgr.GetScheme()}) if err != nil { s.logger.WithError(errors.WithStack(err)).Error("failed to create client") return } - if dataUploads, err := r.FindDataUploads(s.ctx, client, s.namespace); err != nil { - s.logger.WithError(errors.WithStack(err)).Error("failed to find data uploads") - } else { - for i := range dataUploads { - du := dataUploads[i] - if du.Status.Phase == velerov2alpha1api.DataUploadPhaseAccepted || - du.Status.Phase == velerov2alpha1api.DataUploadPhasePrepared || - du.Status.Phase == velerov2alpha1api.DataUploadPhaseInProgress { - err = controller.UpdateDataUploadWithRetry(s.ctx, client, types.NamespacedName{Namespace: du.Namespace, Name: du.Name}, s.logger.WithField("dataupload", du.Name), - func(dataUpload *velerov2alpha1api.DataUpload) { - dataUpload.Spec.Cancel = true - dataUpload.Status.Message = fmt.Sprintf("found a dataupload with status %q during the node-agent starting, mark it as cancel", du.Status.Phase) - }) - - if err != nil { - s.logger.WithError(errors.WithStack(err)).Errorf("failed to mark dataupload %q cancel", du.GetName()) - continue - } - s.logger.WithField("dataupload", du.GetName()).Warn(du.Status.Message) - } - } + if err := r.AttemptDataUploadResume(s.ctx, client, s.logger.WithField("node", s.nodeName), s.namespace); err != nil { + s.logger.WithError(errors.WithStack(err)).Error("failed to attempt data upload resume") } } -func (s *nodeAgentServer) markDataDownloadsCancel(r *controller.DataDownloadReconciler) { +func (s *nodeAgentServer) attemptDataDownloadResume(r *controller.DataDownloadReconciler) { // the function is called before starting the controller manager, the embedded client isn't ready to use, so create a new one here client, err := ctrlclient.New(s.mgr.GetConfig(), ctrlclient.Options{Scheme: s.mgr.GetScheme()}) if err != nil { s.logger.WithError(errors.WithStack(err)).Error("failed to create client") return } - if dataDownloads, err := r.FindDataDownloads(s.ctx, client, s.namespace); err != nil { - s.logger.WithError(errors.WithStack(err)).Error("failed to find data downloads") - } else { - for i := range dataDownloads { - dd := dataDownloads[i] - if dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseAccepted || - dd.Status.Phase == velerov2alpha1api.DataDownloadPhasePrepared || - dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseInProgress { - err = controller.UpdateDataDownloadWithRetry(s.ctx, client, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name}, s.logger.WithField("datadownload", dd.Name), - func(dataDownload *velerov2alpha1api.DataDownload) { - dataDownload.Spec.Cancel = true - dataDownload.Status.Message = fmt.Sprintf("found a datadownload with status %q during the node-agent starting, mark it as cancel", dd.Status.Phase) - }) - if err != nil { - s.logger.WithError(errors.WithStack(err)).Errorf("failed to mark datadownload %q cancel", dd.GetName()) - continue - } - s.logger.WithField("datadownload", dd.GetName()).Warn(dd.Status.Message) - } - } + if err := r.AttemptDataDownloadResume(s.ctx, client, s.logger.WithField("node", s.nodeName), s.namespace); err != nil { + s.logger.WithError(errors.WithStack(err)).Error("failed to attempt data download resume") } } diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 64138ac6d..f0dc08266 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -761,7 +761,6 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string backupStoreGetter, s.config.formatFlag.Parse(), s.csiSnapshotLister, - s.csiSnapshotClient, s.credentialFileStore, s.config.maxConcurrentK8SConnections, s.config.defaultSnapshotMoveData, @@ -1063,13 +1062,13 @@ func markInProgressBackupsFailed(ctx context.Context, client ctrlclient.Client, } for i, backup := range backups.Items { - if backup.Status.Phase != velerov1api.BackupPhaseInProgress && backup.Status.Phase != velerov1api.BackupPhaseWaitingForPluginOperations { + if backup.Status.Phase != velerov1api.BackupPhaseInProgress { log.Debugf("the status of backup %q is %q, skip", backup.GetName(), backup.Status.Phase) continue } updated := backup.DeepCopy() updated.Status.Phase = velerov1api.BackupPhaseFailed - updated.Status.FailureReason = fmt.Sprintf("found a backup with status %q during the server starting, mark it as %q", velerov1api.BackupPhaseInProgress, updated.Status.Phase) + updated.Status.FailureReason = fmt.Sprintf("found a backup with status %q during the server starting, mark it as %q", backup.Status.Phase, updated.Status.Phase) updated.Status.CompletionTimestamp = &metav1.Time{Time: time.Now()} if err := client.Patch(ctx, updated, ctrlclient.MergeFrom(&backups.Items[i])); err != nil { log.WithError(errors.WithStack(err)).Errorf("failed to patch backup %q", backup.GetName()) @@ -1087,13 +1086,13 @@ func markInProgressRestoresFailed(ctx context.Context, client ctrlclient.Client, return } for i, restore := range restores.Items { - if restore.Status.Phase != velerov1api.RestorePhaseInProgress && restore.Status.Phase != velerov1api.RestorePhaseWaitingForPluginOperations { + if restore.Status.Phase != velerov1api.RestorePhaseInProgress { log.Debugf("the status of restore %q is %q, skip", restore.GetName(), restore.Status.Phase) continue } updated := restore.DeepCopy() updated.Status.Phase = velerov1api.RestorePhaseFailed - updated.Status.FailureReason = fmt.Sprintf("found a restore with status %q during the server starting, mark it as %q", velerov1api.RestorePhaseInProgress, updated.Status.Phase) + updated.Status.FailureReason = fmt.Sprintf("found a restore with status %q during the server starting, mark it as %q", restore.Status.Phase, updated.Status.Phase) updated.Status.CompletionTimestamp = &metav1.Time{Time: time.Now()} if err := client.Patch(ctx, updated, ctrlclient.MergeFrom(&restores.Items[i])); err != nil { log.WithError(errors.WithStack(err)).Errorf("failed to patch restore %q", restore.GetName()) @@ -1116,7 +1115,9 @@ func markDataUploadsCancel(ctx context.Context, client ctrlclient.Client, backup du := dataUploads.Items[i] if du.Status.Phase == velerov2alpha1api.DataUploadPhaseAccepted || du.Status.Phase == velerov2alpha1api.DataUploadPhasePrepared || - du.Status.Phase == velerov2alpha1api.DataUploadPhaseInProgress { + du.Status.Phase == velerov2alpha1api.DataUploadPhaseInProgress || + du.Status.Phase == velerov2alpha1api.DataUploadPhaseNew || + du.Status.Phase == "" { err := controller.UpdateDataUploadWithRetry(ctx, client, types.NamespacedName{Namespace: du.Namespace, Name: du.Name}, log.WithField("dataupload", du.Name), func(dataUpload *velerov2alpha1api.DataUpload) { dataUpload.Spec.Cancel = true @@ -1144,7 +1145,9 @@ func markDataDownloadsCancel(ctx context.Context, client ctrlclient.Client, rest dd := dataDownloads.Items[i] if dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseAccepted || dd.Status.Phase == velerov2alpha1api.DataDownloadPhasePrepared || - dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseInProgress { + dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseInProgress || + dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseNew || + dd.Status.Phase == "" { err := controller.UpdateDataDownloadWithRetry(ctx, client, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name}, log.WithField("datadownload", dd.Name), func(dataDownload *velerov2alpha1api.DataDownload) { dataDownload.Spec.Cancel = true diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index f4a4147cb..acc5febac 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -34,7 +34,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" kerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" @@ -112,7 +111,6 @@ func NewBackupReconciler( backupStoreGetter persistence.ObjectBackupStoreGetter, formatFlag logging.Format, volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, - volumeSnapshotClient snapshotterClientSet.Interface, credentialStore credentials.FileStore, maxConcurrentK8SConnections int, defaultSnapshotMoveData bool, @@ -138,7 +136,6 @@ func NewBackupReconciler( backupStoreGetter: backupStoreGetter, formatFlag: formatFlag, volumeSnapshotLister: volumeSnapshotLister, - volumeSnapshotClient: volumeSnapshotClient, credentialFileStore: credentialStore, maxConcurrentK8SConnections: maxConcurrentK8SConnections, defaultSnapshotMoveData: defaultSnapshotMoveData, @@ -656,65 +653,17 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error { fatalErrs = append(fatalErrs, err) } - // Empty slices here so that they can be passed in to the persistBackup call later, regardless of whether or not CSI's enabled. - // This way, we only make the Lister call if the feature flag's on. - var volumeSnapshots []snapshotv1api.VolumeSnapshot - var volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent - var volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass - if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) { - backupLog.Info("backup SnapshotMoveData is set to true, skip VolumeSnapshot resource persistence.") - } else if features.IsEnabled(velerov1api.CSIFeatureFlag) { - selector := label.NewSelectorForBackup(backup.Name) - vscList := &snapshotv1api.VolumeSnapshotContentList{} - - if b.volumeSnapshotLister != nil { - tmpVSs, err := b.volumeSnapshotLister.List(label.NewSelectorForBackup(backup.Name)) - if err != nil { - backupLog.Error(err) - } - for _, vs := range tmpVSs { - volumeSnapshots = append(volumeSnapshots, *vs) - } - } - - backup.CSISnapshots = volumeSnapshots - - err = b.kbClient.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector}) - if err != nil { - backupLog.Error(err) - } - if len(vscList.Items) >= 0 { - volumeSnapshotContents = vscList.Items - } - - vsClassSet := sets.NewString() - for index := range volumeSnapshotContents { - // persist the volumesnapshotclasses referenced by vsc - if volumeSnapshotContents[index].Spec.VolumeSnapshotClassName != nil && !vsClassSet.Has(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName) { - vsClass := &snapshotv1api.VolumeSnapshotClass{} - if err := b.kbClient.Get(context.TODO(), kbclient.ObjectKey{Name: *volumeSnapshotContents[index].Spec.VolumeSnapshotClassName}, vsClass); err != nil { - backupLog.Error(err) - } else { - vsClassSet.Insert(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName) - volumeSnapshotClasses = append(volumeSnapshotClasses, *vsClass) - } - } - } - } - + // native snapshots phase will either be failed or completed right away + // https://github.com/vmware-tanzu/velero/blob/de3ea52f0cc478e99efa7b9524c7f353514261a4/pkg/backup/item_backupper.go#L632-L639 backup.Status.VolumeSnapshotsAttempted = len(backup.VolumeSnapshots) for _, snap := range backup.VolumeSnapshots { if snap.Status.Phase == volume.SnapshotPhaseCompleted { backup.Status.VolumeSnapshotsCompleted++ } } - - backup.Status.CSIVolumeSnapshotsAttempted = len(backup.CSISnapshots) - for _, vs := range backup.CSISnapshots { - if vs.Status != nil && boolptr.IsSetToTrue(vs.Status.ReadyToUse) { - backup.Status.CSIVolumeSnapshotsCompleted++ - } - } + volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses := pkgbackup.GetBackupCSIResources(b.kbClient, b.volumeSnapshotLister, backup.Backup, backupLog) + // Update CSIVolumeSnapshotsAttempted + backup.Status.CSIVolumeSnapshotsAttempted = len(volumeSnapshots) // Iterate over backup item operations and update progress. // Any errors on operations at this point should be added to backup errors. @@ -813,6 +762,7 @@ func recordBackupMetrics(log logrus.FieldLogger, backup *velerov1api.Backup, bac backupDurationSeconds := float64(backupDuration / time.Second) serverMetrics.RegisterBackupDuration(backupScheduleName, backupDurationSeconds) } + if !finalize { serverMetrics.RegisterVolumeSnapshotAttempts(backupScheduleName, backup.Status.VolumeSnapshotsAttempted) serverMetrics.RegisterVolumeSnapshotSuccesses(backupScheduleName, backup.Status.VolumeSnapshotsCompleted) @@ -820,8 +770,6 @@ func recordBackupMetrics(log logrus.FieldLogger, backup *velerov1api.Backup, bac if features.IsEnabled(velerov1api.CSIFeatureFlag) { serverMetrics.RegisterCSISnapshotAttempts(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsAttempted) - serverMetrics.RegisterCSISnapshotSuccesses(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsCompleted) - serverMetrics.RegisterCSISnapshotFailures(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsAttempted-backup.Status.CSIVolumeSnapshotsCompleted) } if backup.Status.Progress != nil { @@ -832,6 +780,9 @@ func recordBackupMetrics(log logrus.FieldLogger, backup *velerov1api.Backup, bac if backup.Status.Warnings > 0 { serverMetrics.RegisterBackupWarning(backupScheduleName) } + } else if features.IsEnabled(velerov1api.CSIFeatureFlag) { + serverMetrics.RegisterCSISnapshotSuccesses(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsCompleted) + serverMetrics.RegisterCSISnapshotFailures(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsAttempted-backup.Status.CSIVolumeSnapshotsCompleted) } } diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index f18787733..df2e22a22 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -21,11 +21,13 @@ import ( "context" "fmt" "io" + "reflect" "sort" "strings" "testing" "time" + "github.com/google/go-cmp/cmp" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" snapshotfake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" snapshotinformers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions" @@ -43,6 +45,10 @@ import ( ctrl "sigs.k8s.io/controller-runtime" kbclient "sigs.k8s.io/controller-runtime/pkg/client" + kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" + + fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" pkgbackup "github.com/vmware-tanzu/velero/pkg/backup" "github.com/vmware-tanzu/velero/pkg/builder" @@ -1665,3 +1671,63 @@ func Test_getLastSuccessBySchedule(t *testing.T) { }) } } + +// Unit tests to make sure that the backup's status is updated correctly during reconcile. +// To clear up confusion whether status can be updated with Patch alone without status writer and not kbClient.Status().Patch() +func TestPatchResourceWorksWithStatus(t *testing.T) { + type args struct { + original *velerov1api.Backup + updated *velerov1api.Backup + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "patch backup status", + args: args{ + original: defaultBackup().SnapshotMoveData(false).Result(), + updated: defaultBackup().SnapshotMoveData(false).WithStatus(velerov1api.BackupStatus{ + CSIVolumeSnapshotsCompleted: 1, + }).Result(), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + error := velerov1api.AddToScheme(scheme) + if error != nil { + t.Errorf("PatchResource() error = %v", error) + } + fakeClient := fakeClient.NewClientBuilder().WithScheme(scheme).WithObjects(tt.args.original).Build() + fromCluster := &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: tt.args.original.Name, + Namespace: tt.args.original.Namespace, + }, + } + // check original exists + if err := fakeClient.Get(context.Background(), kbclient.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil { + t.Errorf("PatchResource() error = %v", err) + } + // ignore resourceVersion + tt.args.updated.ResourceVersion = fromCluster.ResourceVersion + tt.args.original.ResourceVersion = fromCluster.ResourceVersion + if err := kubeutil.PatchResource(tt.args.original, tt.args.updated, fakeClient); (err != nil) != tt.wantErr { + t.Errorf("PatchResource() error = %v, wantErr %v", err, tt.wantErr) + } + // check updated exists + if err := fakeClient.Get(context.Background(), kbclient.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil { + t.Errorf("PatchResource() error = %v", err) + } + + // check fromCluster is equal to updated + if !reflect.DeepEqual(fromCluster, tt.args.updated) { + t.Error(cmp.Diff(fromCluster, tt.args.updated)) + } + }) + + } +} diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index af439b834..30580aa71 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -59,7 +59,6 @@ import ( ) const ( - snapshotDeleteTimeout = time.Minute deleteBackupRequestMaxAge = 24 * time.Hour ) @@ -513,12 +512,9 @@ func (r *backupDeletionReconciler) deletePodVolumeSnapshots(ctx context.Context, return []error{err} } - ctx2, cancelFunc := context.WithTimeout(ctx, snapshotDeleteTimeout) - defer cancelFunc() - var errs []error for _, snapshot := range snapshots { - if err := r.repoMgr.Forget(ctx2, snapshot); err != nil { + if err := r.repoMgr.Forget(ctx, snapshot); err != nil { errs = append(errs, err) } } diff --git a/pkg/controller/backup_finalizer_controller.go b/pkg/controller/backup_finalizer_controller.go index be88908de..a27aa4521 100644 --- a/pkg/controller/backup_finalizer_controller.go +++ b/pkg/controller/backup_finalizer_controller.go @@ -31,6 +31,8 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" pkgbackup "github.com/vmware-tanzu/velero/pkg/backup" + "github.com/vmware-tanzu/velero/pkg/itemoperation" + "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/metrics" "github.com/vmware-tanzu/velero/pkg/persistence" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" @@ -184,7 +186,10 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ r.metrics.RegisterBackupPartialFailure(backupScheduleName) r.metrics.RegisterBackupLastStatus(backupScheduleName, metrics.BackupLastStatusFailure) } + backup.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()} + backup.Status.CSIVolumeSnapshotsCompleted = updateCSIVolumeSnapshotsCompleted(operations) + recordBackupMetrics(log, backup, outBackupFile, r.metrics, true) // update backup metadata in object store @@ -210,3 +215,19 @@ func (r *backupFinalizerReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&velerov1api.Backup{}). Complete(r) } + +// updateCSIVolumeSnapshotsCompleted calculate the completed VS number according to +// the backup's async operation list. +func updateCSIVolumeSnapshotsCompleted( + operations []*itemoperation.BackupOperation) int { + completedNum := 0 + + for index := range operations { + if operations[index].Spec.ResourceIdentifier.String() == kuberesource.VolumeSnapshots.String() && + operations[index].Status.Phase == itemoperation.OperationPhaseCompleted { + completedNum++ + } + } + + return completedNum +} diff --git a/pkg/controller/backup_finalizer_controller_test.go b/pkg/controller/backup_finalizer_controller_test.go index 011a6561b..207f3a3ac 100644 --- a/pkg/controller/backup_finalizer_controller_test.go +++ b/pkg/controller/backup_finalizer_controller_test.go @@ -36,6 +36,7 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" + "github.com/vmware-tanzu/velero/pkg/features" "github.com/vmware-tanzu/velero/pkg/itemoperation" "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/metrics" @@ -65,12 +66,14 @@ func TestBackupFinalizerReconcile(t *testing.T) { defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result() tests := []struct { - name string - backup *velerov1api.Backup - backupOperations []*itemoperation.BackupOperation - backupLocation *velerov1api.BackupStorageLocation - expectError bool - expectPhase velerov1api.BackupPhase + name string + backup *velerov1api.Backup + backupOperations []*itemoperation.BackupOperation + backupLocation *velerov1api.BackupStorageLocation + enableCSI bool + expectError bool + expectPhase velerov1api.BackupPhase + expectedCompletedVS int }{ { name: "Finalizing backup is completed", @@ -144,6 +147,50 @@ func TestBackupFinalizerReconcile(t *testing.T) { }, }, }, + { + name: "Test calculate backup.Status.BackupItemOperationsCompleted", + backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-3"). + StorageLocation("default"). + ObjectMeta(builder.WithUID("foo")). + StartTimestamp(fakeClock.Now()). + WithStatus(velerov1api.BackupStatus{ + StartTimestamp: &metav1Now, + CompletionTimestamp: &metav1Now, + CSIVolumeSnapshotsAttempted: 1, + Phase: velerov1api.BackupPhaseFinalizing, + }). + Result(), + backupLocation: defaultBackupLocation, + enableCSI: true, + expectPhase: velerov1api.BackupPhaseCompleted, + expectedCompletedVS: 1, + backupOperations: []*itemoperation.BackupOperation{ + { + Spec: itemoperation.BackupOperationSpec{ + BackupName: "backup-3", + BackupUID: "foo", + BackupItemAction: "foo", + ResourceIdentifier: velero.ResourceIdentifier{ + GroupResource: kuberesource.VolumeSnapshots, + Namespace: "ns-1", + Name: "vs-1", + }, + PostOperationItems: []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.Secrets, + Namespace: "ns-1", + Name: "secret-1", + }, + }, + OperationID: "operation-3", + }, + Status: itemoperation.OperationStatus{ + Phase: itemoperation.OperationPhaseCompleted, + Created: &metav1Now, + }, + }, + }, + }, } for _, test := range tests { @@ -159,7 +206,13 @@ func TestBackupFinalizerReconcile(t *testing.T) { initObjs = append(initObjs, test.backupLocation) } + if test.enableCSI { + features.Enable(velerov1api.CSIFeatureFlag) + defer features.Enable() + } + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, initObjs...) + reconciler, backupper := mockBackupFinalizerReconciler(fakeClient, fakeClock) pluginManager.On("CleanupClients").Return(nil) backupStore.On("GetBackupItemOperations", test.backup.Name).Return(test.backupOperations, nil) @@ -180,6 +233,7 @@ func TestBackupFinalizerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, test.expectPhase, backupAfter.Status.Phase) + assert.Equal(t, test.expectedCompletedVS, backupAfter.Status.CSIVolumeSnapshotsCompleted) }) } } diff --git a/pkg/controller/backup_operations_controller.go b/pkg/controller/backup_operations_controller.go index f00e9c205..5e9a5cfd3 100644 --- a/pkg/controller/backup_operations_controller.go +++ b/pkg/controller/backup_operations_controller.go @@ -275,6 +275,8 @@ func (c *backupOperationsReconciler) updateBackupAndOperationsJSON( return nil } +// check progress of backupItemOperations +// return: inProgressOperations, changes, completedCount, failedCount, errs func getBackupItemOperationProgress( backup *velerov1api.Backup, pluginManager clientmgmt.Manager, diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index bc362edb4..b07f11929 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -24,6 +24,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/sirupsen/logrus" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -33,7 +34,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" testclocks "k8s.io/utils/clock/testing" - ctrl "sigs.k8s.io/controller-runtime" ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -429,6 +429,9 @@ var _ = Describe("Backup Sync Reconciler", func() { backupNames = append(backupNames, backup.backup.Name) backupStore.On("GetBackupMetadata", backup.backup.Name).Return(backup.backup, nil) backupStore.On("GetPodVolumeBackups", backup.backup.Name).Return(backup.podVolumeBackups, nil) + backupStore.On("BackupExists", "bucket-1", backup.backup.Name).Return(true, nil) + backupStore.On("GetCSIVolumeSnapshotClasses", backup.backup.Name).Return([]*snapshotv1api.VolumeSnapshotClass{}, nil) + backupStore.On("GetCSIVolumeSnapshotContents", backup.backup.Name).Return([]*snapshotv1api.VolumeSnapshotContent{}, nil) } backupStore.On("ListBackups").Return(backupNames, nil) } diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 0212fa00f..7c530b0ae 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -140,7 +140,7 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request // to help clear up resources instead of clear them directly in case of some conflict with Expose action if err := UpdateDataDownloadWithRetry(ctx, r.client, req.NamespacedName, log, func(dataDownload *velerov2alpha1api.DataDownload) { dataDownload.Spec.Cancel = true - dataDownload.Status.Message = fmt.Sprintf("found a dataupload %s/%s is being deleted, mark it as cancel", dd.Namespace, dd.Name) + dataDownload.Status.Message = fmt.Sprintf("found a datadownload %s/%s is being deleted, mark it as cancel", dd.Namespace, dd.Name) }); err != nil { log.Errorf("failed to set cancel flag with error %s for %s/%s", err.Error(), dd.Namespace, dd.Name) return ctrl.Result{}, err @@ -192,7 +192,6 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request return r.errorOut(ctx, dd, err, "error to expose snapshot", log) } } - log.Info("Restore is exposed") // we need to get CR again for it may canceled by datadownload controller on other @@ -205,7 +204,6 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request } return ctrl.Result{}, errors.Wrap(err, "getting datadownload") } - // we need to clean up resources as resources created in Expose it may later than cancel action or prepare time // and need to clean up resources again if isDataDownloadInFinalState(dd) { @@ -267,7 +265,6 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request return r.errorOut(ctx, dd, err, "error to create data path", log) } } - // Update status to InProgress original := dd.DeepCopy() dd.Status.Phase = velerov2alpha1api.DataDownloadPhaseInProgress @@ -576,6 +573,51 @@ func (r *DataDownloadReconciler) FindDataDownloads(ctx context.Context, cli clie return dataDownloads, nil } +func (r *DataDownloadReconciler) findAcceptDataDownloadsByNodeLabel(ctx context.Context, cli client.Client, ns string) ([]velerov2alpha1api.DataDownload, error) { + dataDownloads := &velerov2alpha1api.DataDownloadList{} + if err := cli.List(ctx, dataDownloads, &client.ListOptions{Namespace: ns}); err != nil { + r.logger.WithError(errors.WithStack(err)).Error("failed to list datauploads") + return nil, errors.Wrapf(err, "failed to list datauploads") + } + + var result []velerov2alpha1api.DataDownload + for _, dd := range dataDownloads.Items { + if dd.Status.Phase != velerov2alpha1api.DataDownloadPhaseAccepted { + continue + } + if dd.Labels[acceptNodeLabelKey] == r.nodeName { + result = append(result, dd) + } + } + return result, nil +} + +// CancelAcceptedDataDownload will cancel the accepted data download +func (r *DataDownloadReconciler) CancelAcceptedDataDownload(ctx context.Context, cli client.Client, ns string) { + r.logger.Infof("Canceling accepted data for node %s", r.nodeName) + dataDownloads, err := r.findAcceptDataDownloadsByNodeLabel(ctx, cli, ns) + if err != nil { + r.logger.WithError(err).Error("failed to find data downloads") + return + } + + for _, dd := range dataDownloads { + if dd.Spec.Cancel { + continue + } + err = UpdateDataDownloadWithRetry(ctx, cli, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name}, + r.logger.WithField("dataupload", dd.Name), func(dataDownload *velerov2alpha1api.DataDownload) { + dataDownload.Spec.Cancel = true + dataDownload.Status.Message = fmt.Sprintf("found a datadownload with status %q during the node-agent starting, mark it as cancel", dd.Status.Phase) + }) + + r.logger.Warn(dd.Status.Message) + if err != nil { + r.logger.WithError(err).Errorf("failed to set cancel flag with error %s", err.Error()) + } + } +} + func (r *DataDownloadReconciler) prepareDataDownload(ssb *velerov2alpha1api.DataDownload) { ssb.Status.Phase = velerov2alpha1api.DataDownloadPhasePrepared ssb.Status.Node = r.nodeName @@ -749,3 +791,35 @@ func UpdateDataDownloadWithRetry(ctx context.Context, client client.Client, name return true, nil }) } + +func (r *DataDownloadReconciler) AttemptDataDownloadResume(ctx context.Context, cli client.Client, logger *logrus.Entry, ns string) error { + if dataDownloads, err := r.FindDataDownloads(ctx, cli, ns); err != nil { + return errors.Wrapf(err, "failed to find data downloads") + } else { + for i := range dataDownloads { + dd := dataDownloads[i] + if dd.Status.Phase == velerov2alpha1api.DataDownloadPhasePrepared { + // keep doing nothing let controller re-download the data + // the Prepared CR could be still handled by datadownload controller after node-agent restart + logger.WithField("datadownload", dd.GetName()).Debug("find a datadownload with status prepared") + } else if dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseInProgress { + err = UpdateDataDownloadWithRetry(ctx, cli, types.NamespacedName{Namespace: dd.Namespace, Name: dd.Name}, logger.WithField("datadownload", dd.Name), + func(dataDownload *velerov2alpha1api.DataDownload) { + dataDownload.Spec.Cancel = true + dataDownload.Status.Message = fmt.Sprintf("found a datadownload with status %q during the node-agent starting, mark it as cancel", dd.Status.Phase) + }) + + if err != nil { + logger.WithError(errors.WithStack(err)).Errorf("failed to mark datadownload %q into canceled", dd.GetName()) + continue + } + logger.WithField("datadownload", dd.GetName()).Debug("mark datadownload into canceled") + } + } + } + + //If the data download is in Accepted status, the expoded PVC may be not created + // so we need to mark the data download as canceled for it may not be recoverable + r.CancelAcceptedDataDownload(ctx, cli, ns) + return nil +} diff --git a/pkg/controller/data_download_controller_test.go b/pkg/controller/data_download_controller_test.go index 46162f0cb..d5068158f 100644 --- a/pkg/controller/data_download_controller_test.go +++ b/pkg/controller/data_download_controller_test.go @@ -69,7 +69,7 @@ func dataDownloadBuilder() *builder.DataDownloadBuilder { } func initDataDownloadReconciler(objects []runtime.Object, needError ...bool) (*DataDownloadReconciler, error) { - var errs []error = make([]error, 5) + var errs []error = make([]error, 6) for k, isError := range needError { if k == 0 && isError { errs[0] = fmt.Errorf("Get error") @@ -81,6 +81,8 @@ func initDataDownloadReconciler(objects []runtime.Object, needError ...bool) (*D errs[3] = fmt.Errorf("Patch error") } else if k == 4 && isError { errs[4] = apierrors.NewConflict(velerov2alpha1api.Resource("datadownload"), dataDownloadName, errors.New("conflict")) + } else if k == 5 && isError { + errs[5] = fmt.Errorf("List error") } } return initDataDownloadReconcilerWithError(objects, errs...) @@ -116,6 +118,8 @@ func initDataDownloadReconcilerWithError(objects []runtime.Object, needError ... fakeClient.patchError = needError[3] } else if k == 4 { fakeClient.updateConflict = needError[4] + } else if k == 5 { + fakeClient.listError = needError[5] } } @@ -936,3 +940,111 @@ func TestFindDataDownloads(t *testing.T) { }) } } + +func TestAttemptDataDownloadResume(t *testing.T) { + tests := []struct { + name string + dataUploads []velerov2alpha1api.DataDownload + du *velerov2alpha1api.DataDownload + pod *corev1.Pod + needErrs []bool + acceptedDataDownloads []string + prepareddDataDownloads []string + cancelledDataDownloads []string + expectedError bool + }{ + // Test case 1: Process Accepted DataDownload + { + name: "AcceptedDataDownload", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Volumes(&corev1.Volume{Name: dataDownloadName}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataDownloadLabel: dataDownloadName, + }).Result(), + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Result(), + acceptedDataDownloads: []string{dataDownloadName}, + expectedError: false, + }, + // Test case 2: Cancel an Accepted DataDownload + { + name: "CancelAcceptedDataDownload", + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Result(), + }, + // Test case 3: Process Accepted Prepared DataDownload + { + name: "PreparedDataDownload", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Volumes(&corev1.Volume{Name: dataDownloadName}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataDownloadLabel: dataDownloadName, + }).Result(), + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhasePrepared).Result(), + prepareddDataDownloads: []string{dataDownloadName}, + }, + // Test case 4: Process Accepted InProgress DataDownload + { + name: "InProgressDataDownload", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Volumes(&corev1.Volume{Name: dataDownloadName}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataDownloadLabel: dataDownloadName, + }).Result(), + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhasePrepared).Result(), + prepareddDataDownloads: []string{dataDownloadName}, + }, + // Test case 5: get resume error + { + name: "ResumeError", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Volumes(&corev1.Volume{Name: dataDownloadName}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataDownloadLabel: dataDownloadName, + }).Result(), + needErrs: []bool{false, false, false, false, false, true}, + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhasePrepared).Result(), + expectedError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.TODO() + r, err := initDataDownloadReconciler(nil, test.needErrs...) + r.nodeName = "node-1" + require.NoError(t, err) + defer func() { + r.client.Delete(ctx, test.du, &kbclient.DeleteOptions{}) + if test.pod != nil { + r.client.Delete(ctx, test.pod, &kbclient.DeleteOptions{}) + } + }() + + assert.NoError(t, r.client.Create(ctx, test.du)) + if test.pod != nil { + assert.NoError(t, r.client.Create(ctx, test.pod)) + } + // Run the test + err = r.AttemptDataDownloadResume(ctx, r.client, r.logger.WithField("name", test.name), test.du.Namespace) + + if test.expectedError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + + // Verify DataDownload marked as Cancelled + for _, duName := range test.cancelledDataDownloads { + dataUpload := &velerov2alpha1api.DataDownload{} + err := r.client.Get(context.Background(), types.NamespacedName{Namespace: "velero", Name: duName}, dataUpload) + require.NoError(t, err) + assert.Equal(t, velerov2alpha1api.DataDownloadPhaseCanceled, dataUpload.Status.Phase) + } + // Verify DataDownload marked as Accepted + for _, duName := range test.acceptedDataDownloads { + dataUpload := &velerov2alpha1api.DataDownload{} + err := r.client.Get(context.Background(), types.NamespacedName{Namespace: "velero", Name: duName}, dataUpload) + require.NoError(t, err) + assert.Equal(t, velerov2alpha1api.DataDownloadPhaseAccepted, dataUpload.Status.Phase) + } + // Verify DataDownload marked as Prepared + for _, duName := range test.prepareddDataDownloads { + dataUpload := &velerov2alpha1api.DataDownload{} + err := r.client.Get(context.Background(), types.NamespacedName{Namespace: "velero", Name: duName}, dataUpload) + require.NoError(t, err) + assert.Equal(t, velerov2alpha1api.DataDownloadPhasePrepared, dataUpload.Status.Phase) + } + } + }) + } +} diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index ec3f4d154..04736fcb0 100644 --- a/pkg/controller/data_upload_controller.go +++ b/pkg/controller/data_upload_controller.go @@ -274,7 +274,6 @@ func (r *DataUploadReconciler) Reconcile(ctx context.Context, req ctrl.Request) return r.errorOut(ctx, du, err, "error to create data path", log) } } - // Update status to InProgress original := du.DeepCopy() du.Status.Phase = velerov2alpha1api.DataUploadPhaseInProgress @@ -581,7 +580,7 @@ func (r *DataUploadReconciler) findDataUploadForPod(podObj client.Object) []reco return []reconcile.Request{requests} } -func (r *DataUploadReconciler) FindDataUploads(ctx context.Context, cli client.Client, ns string) ([]velerov2alpha1api.DataUpload, error) { +func (r *DataUploadReconciler) FindDataUploadsByPod(ctx context.Context, cli client.Client, ns string) ([]velerov2alpha1api.DataUpload, error) { pods := &corev1.PodList{} var dataUploads []velerov2alpha1api.DataUpload if err := cli.List(ctx, pods, &client.ListOptions{Namespace: ns}); err != nil { @@ -605,6 +604,51 @@ func (r *DataUploadReconciler) FindDataUploads(ctx context.Context, cli client.C return dataUploads, nil } +func (r *DataUploadReconciler) findAcceptDataUploadsByNodeLabel(ctx context.Context, cli client.Client, ns string) ([]velerov2alpha1api.DataUpload, error) { + dataUploads := &velerov2alpha1api.DataUploadList{} + if err := cli.List(ctx, dataUploads, &client.ListOptions{Namespace: ns}); err != nil { + r.logger.WithError(errors.WithStack(err)).Error("failed to list datauploads") + return nil, errors.Wrapf(err, "failed to list datauploads") + } + + var result []velerov2alpha1api.DataUpload + for _, du := range dataUploads.Items { + if du.Status.Phase != velerov2alpha1api.DataUploadPhaseAccepted { + continue + } + if du.Labels[acceptNodeLabelKey] == r.nodeName { + result = append(result, du) + } + } + return result, nil +} + +func (r *DataUploadReconciler) CancelAcceptedDataupload(ctx context.Context, cli client.Client, ns string) { + r.logger.Infof("Reset accepted dataupload for node %s", r.nodeName) + dataUploads, err := r.findAcceptDataUploadsByNodeLabel(ctx, cli, ns) + if err != nil { + r.logger.WithError(err).Error("failed to find dataupload") + return + } + + for _, du := range dataUploads { + if du.Spec.Cancel { + continue + } + err = UpdateDataUploadWithRetry(ctx, cli, types.NamespacedName{Namespace: du.Namespace, Name: du.Name}, r.logger.WithField("dataupload", du.Name), + func(dataUpload *velerov2alpha1api.DataUpload) { + dataUpload.Spec.Cancel = true + dataUpload.Status.Message = fmt.Sprintf("found a dataupload with status %q during the node-agent starting, mark it as cancel", du.Status.Phase) + }) + + r.logger.WithField("dataupload", du.GetName()).Warn(du.Status.Message) + if err != nil { + r.logger.WithError(errors.WithStack(err)).Errorf("failed to mark dataupload %q cancel", du.GetName()) + continue + } + } +} + func (r *DataUploadReconciler) prepareDataUpload(du *velerov2alpha1api.DataUpload) { du.Status.Phase = velerov2alpha1api.DataUploadPhasePrepared du.Status.Node = r.nodeName @@ -831,3 +875,34 @@ func UpdateDataUploadWithRetry(ctx context.Context, client client.Client, namesp return true, nil }) } + +func (r *DataUploadReconciler) AttemptDataUploadResume(ctx context.Context, cli client.Client, logger *logrus.Entry, ns string) error { + if dataUploads, err := r.FindDataUploadsByPod(ctx, cli, ns); err != nil { + return errors.Wrap(err, "failed to find data uploads") + } else { + for _, du := range dataUploads { + if du.Status.Phase == velerov2alpha1api.DataUploadPhasePrepared { + // keep doing nothing let controller re-download the data + // the Prepared CR could be still handled by dataupload controller after node-agent restart + logger.WithField("dataupload", du.GetName()).Debug("find a dataupload with status prepared") + } else if du.Status.Phase == velerov2alpha1api.DataUploadPhaseInProgress { + err = UpdateDataUploadWithRetry(ctx, cli, types.NamespacedName{Namespace: du.Namespace, Name: du.Name}, logger.WithField("dataupload", du.Name), + func(dataUpload *velerov2alpha1api.DataUpload) { + dataUpload.Spec.Cancel = true + dataUpload.Status.Message = fmt.Sprintf("found a dataupload with status %q during the node-agent starting, mark it as cancel", du.Status.Phase) + }) + + if err != nil { + logger.WithError(errors.WithStack(err)).Errorf("failed to mark dataupload %q into canceled", du.GetName()) + continue + } + logger.WithField("dataupload", du.GetName()).Debug("mark dataupload into canceled") + } + } + } + + //If the data upload is in Accepted status, the volume snapshot may be deleted and the exposed pod may not be created + // so we need to mark the data upload as canceled for it may not be recoverable + r.CancelAcceptedDataupload(ctx, cli, ns) + return nil +} diff --git a/pkg/controller/data_upload_controller_test.go b/pkg/controller/data_upload_controller_test.go index 34bc4a6aa..cc66a830f 100644 --- a/pkg/controller/data_upload_controller_test.go +++ b/pkg/controller/data_upload_controller_test.go @@ -68,6 +68,7 @@ type FakeClient struct { updateError error patchError error updateConflict error + listError error } func (c *FakeClient) Get(ctx context.Context, key kbclient.ObjectKey, obj kbclient.Object) error { @@ -106,8 +107,16 @@ func (c *FakeClient) Patch(ctx context.Context, obj kbclient.Object, patch kbcli return c.Client.Patch(ctx, obj, patch, opts...) } +func (c *FakeClient) List(ctx context.Context, list kbclient.ObjectList, opts ...kbclient.ListOption) error { + if c.listError != nil { + return c.listError + } + + return c.Client.List(ctx, list, opts...) +} + func initDataUploaderReconciler(needError ...bool) (*DataUploadReconciler, error) { - var errs []error = make([]error, 5) + var errs []error = make([]error, 6) for k, isError := range needError { if k == 0 && isError { errs[0] = fmt.Errorf("Get error") @@ -118,7 +127,9 @@ func initDataUploaderReconciler(needError ...bool) (*DataUploadReconciler, error } else if k == 3 && isError { errs[3] = fmt.Errorf("Patch error") } else if k == 4 && isError { - errs[4] = apierrors.NewConflict(velerov2alpha1api.Resource("datadownload"), dataDownloadName, errors.New("conflict")) + errs[4] = apierrors.NewConflict(velerov2alpha1api.Resource("dataupload"), dataUploadName, errors.New("conflict")) + } else if k == 5 && isError { + errs[5] = fmt.Errorf("List error") } } @@ -196,6 +207,8 @@ func initDataUploaderReconcilerWithError(needError ...error) (*DataUploadReconci fakeClient.patchError = needError[3] } else if k == 4 { fakeClient.updateConflict = needError[4] + } else if k == 5 { + fakeClient.listError = needError[5] } } @@ -981,7 +994,7 @@ func TestFindDataUploads(t *testing.T) { require.NoError(t, err) err = r.client.Create(ctx, &test.pod) require.NoError(t, err) - uploads, err := r.FindDataUploads(context.Background(), r.client, "velero") + uploads, err := r.FindDataUploadsByPod(context.Background(), r.client, "velero") if test.expectedError { assert.Error(t, err) @@ -992,3 +1005,110 @@ func TestFindDataUploads(t *testing.T) { }) } } +func TestAttemptDataUploadResume(t *testing.T) { + tests := []struct { + name string + dataUploads []velerov2alpha1api.DataUpload + du *velerov2alpha1api.DataUpload + pod *corev1.Pod + needErrs []bool + acceptedDataUploads []string + prepareddDataUploads []string + cancelledDataUploads []string + expectedError bool + }{ + // Test case 1: Process Accepted DataUpload + { + name: "AcceptedDataUpload", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataUploadName).Volumes(&corev1.Volume{Name: "dataupload-1"}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataUploadLabel: dataUploadName, + }).Result(), + du: dataUploadBuilder().Phase(velerov2alpha1api.DataUploadPhaseAccepted).Result(), + acceptedDataUploads: []string{dataUploadName}, + expectedError: false, + }, + // Test case 2: Cancel an Accepted DataUpload + { + name: "CancelAcceptedDataUpload", + du: dataUploadBuilder().Phase(velerov2alpha1api.DataUploadPhaseAccepted).Result(), + }, + // Test case 3: Process Accepted Prepared DataUpload + { + name: "PreparedDataUpload", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataUploadName).Volumes(&corev1.Volume{Name: "dataupload-1"}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataUploadLabel: dataUploadName, + }).Result(), + du: dataUploadBuilder().Phase(velerov2alpha1api.DataUploadPhasePrepared).Result(), + prepareddDataUploads: []string{dataUploadName}, + }, + // Test case 4: Process Accepted InProgress DataUpload + { + name: "InProgressDataUpload", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataUploadName).Volumes(&corev1.Volume{Name: "dataupload-1"}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataUploadLabel: dataUploadName, + }).Result(), + du: dataUploadBuilder().Phase(velerov2alpha1api.DataUploadPhasePrepared).Result(), + prepareddDataUploads: []string{dataUploadName}, + }, + // Test case 5: get resume error + { + name: "ResumeError", + pod: builder.ForPod(velerov1api.DefaultNamespace, dataUploadName).Volumes(&corev1.Volume{Name: "dataupload-1"}).NodeName("node-1").Labels(map[string]string{ + velerov1api.DataUploadLabel: dataUploadName, + }).Result(), + needErrs: []bool{false, false, false, false, false, true}, + du: dataUploadBuilder().Phase(velerov2alpha1api.DataUploadPhasePrepared).Result(), + expectedError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.TODO() + r, err := initDataUploaderReconciler(test.needErrs...) + r.nodeName = "node-1" + require.NoError(t, err) + defer func() { + r.client.Delete(ctx, test.du, &kbclient.DeleteOptions{}) + if test.pod != nil { + r.client.Delete(ctx, test.pod, &kbclient.DeleteOptions{}) + } + }() + + assert.NoError(t, r.client.Create(ctx, test.du)) + if test.pod != nil { + assert.NoError(t, r.client.Create(ctx, test.pod)) + } + // Run the test + err = r.AttemptDataUploadResume(ctx, r.client, r.logger.WithField("name", test.name), test.du.Namespace) + + if test.expectedError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + + // Verify DataUploads marked as Cancelled + for _, duName := range test.cancelledDataUploads { + dataUpload := &velerov2alpha1api.DataUpload{} + err := r.client.Get(context.Background(), types.NamespacedName{Namespace: "velero", Name: duName}, dataUpload) + require.NoError(t, err) + assert.Equal(t, velerov2alpha1api.DataUploadPhaseCanceled, dataUpload.Status.Phase) + } + // Verify DataUploads marked as Accepted + for _, duName := range test.acceptedDataUploads { + dataUpload := &velerov2alpha1api.DataUpload{} + err := r.client.Get(context.Background(), types.NamespacedName{Namespace: "velero", Name: duName}, dataUpload) + require.NoError(t, err) + assert.Equal(t, velerov2alpha1api.DataUploadPhaseAccepted, dataUpload.Status.Phase) + } + // Verify DataUploads marked as Prepared + for _, duName := range test.prepareddDataUploads { + dataUpload := &velerov2alpha1api.DataUpload{} + err := r.client.Get(context.Background(), types.NamespacedName{Namespace: "velero", Name: duName}, dataUpload) + require.NoError(t, err) + assert.Equal(t, velerov2alpha1api.DataUploadPhasePrepared, dataUpload.Status.Phase) + } + } + }) + } +} diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 5d6ed505e..f6b9b39d9 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -515,6 +515,11 @@ func (r *restoreReconciler) runValidatedRestore(restore *api.Restore, info backu return errors.Wrap(err, "error fetching volume snapshots metadata") } + csiVolumeSnapshots, err := backupStore.GetCSIVolumeSnapshots(restore.Spec.BackupName) + if err != nil { + return errors.Wrap(err, "fail to fetch CSI VolumeSnapshots metadata") + } + restoreLog.Info("starting restore") var podVolumeBackups []*api.PodVolumeBackup @@ -531,6 +536,7 @@ func (r *restoreReconciler) runValidatedRestore(restore *api.Restore, info backu BackupReader: backupFile, ResourceModifiers: resourceModifiers, DisableInformerCache: r.disableInformerCache, + CSIVolumeSnapshots: csiVolumeSnapshots, } restoreWarnings, restoreErrors := r.restorer.RestoreWithResolvers(restoreReq, actionsResolver, pluginManager) diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index b7f6fbd25..9437f1d1c 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -23,6 +23,7 @@ import ( "testing" "time" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -471,6 +472,7 @@ func TestRestoreReconcile(t *testing.T) { } if test.expectedRestorerCall != nil { backupStore.On("GetBackupContents", test.backup.Name).Return(io.NopCloser(bytes.NewReader([]byte("hello world"))), nil) + backupStore.On("GetCSIVolumeSnapshots", test.backup.Name).Return([]*snapshotv1api.VolumeSnapshot{}, nil) restorer.On("RestoreWithResolvers", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(warnings, errors) diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 8b501e648..111bf8a8b 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -128,6 +128,13 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1.Obje curLog.WithField("vs name", volumeSnapshot.Name).Infof("VS is deleted in namespace %s", volumeSnapshot.Namespace) + err = csi.RemoveVSCProtect(ctx, e.csiSnapshotClient, vsc.Name, csiExposeParam.Timeout) + if err != nil { + return errors.Wrap(err, "error to remove protect from volume snapshot content") + } + + curLog.WithField("vsc name", vsc.Name).Infof("Removed protect from VSC") + err = csi.EnsureDeleteVSC(ctx, e.csiSnapshotClient, vsc.Name, csiExposeParam.Timeout) if err != nil { return errors.Wrap(err, "error to delete volume snapshot content") @@ -162,10 +169,9 @@ func (e *csiSnapshotExposer) Expose(ctx context.Context, ownerObject corev1.Obje } curLog.WithField("pvc name", backupPVC.Name).Info("Backup PVC is created") - defer func() { if err != nil { - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVC.Name, backupPVC.Namespace, curLog) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVC.Name, backupPVC.Namespace, curLog) } }() @@ -241,7 +247,8 @@ func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1.Obj backupVSName := ownerObject.Name kube.DeletePodIfAny(ctx, e.kubeClient.CoreV1(), backupPodName, ownerObject.Namespace, e.log) - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, e.log) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), backupPVCName, ownerObject.Namespace, e.log) + csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, backupVSName, ownerObject.Namespace, e.log) csi.DeleteVolumeSnapshotIfAny(ctx, e.csiSnapshotClient, vsName, sourceNamespace, e.log) } @@ -308,8 +315,8 @@ func (e *csiSnapshotExposer) createBackupVSC(ctx context.Context, ownerObject co return e.csiSnapshotClient.VolumeSnapshotContents().Create(ctx, vsc, metav1.CreateOptions{}) } -func (e *csiSnapshotExposer) createBackupPVC(ctx context.Context, ownerObject corev1.ObjectReference, backupVS string, storageClass string, accessMode string, resource resource.Quantity) (*corev1.PersistentVolumeClaim, error) { - backupVCName := ownerObject.Name +func (e *csiSnapshotExposer) createBackupPVC(ctx context.Context, ownerObject corev1.ObjectReference, backupVS, storageClass, accessMode string, resource resource.Quantity) (*corev1.PersistentVolumeClaim, error) { + backupPVCName := ownerObject.Name volumeMode, err := getVolumeModeByAccessMode(accessMode) if err != nil { @@ -325,7 +332,7 @@ func (e *csiSnapshotExposer) createBackupPVC(ctx context.Context, ownerObject co pvc := &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Namespace: ownerObject.Namespace, - Name: backupVCName, + Name: backupPVCName, OwnerReferences: []metav1.OwnerReference{ { APIVersion: ownerObject.APIVersion, diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index 0868aba47..c73e4a4c3 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -104,7 +104,7 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1.O defer func() { if err != nil { - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, curLog) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVC.Name, restorePVC.Namespace, curLog) } }() @@ -114,6 +114,7 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1.O func (e *genericRestoreExposer) GetExposed(ctx context.Context, ownerObject corev1.ObjectReference, nodeClient client.Client, nodeName string, timeout time.Duration) (*ExposeResult, error) { restorePodName := ownerObject.Name restorePVCName := ownerObject.Name + volumeName := string(ownerObject.UID) curLog := e.log.WithFields(logrus.Fields{ "owner": ownerObject.Name, @@ -127,10 +128,10 @@ func (e *genericRestoreExposer) GetExposed(ctx context.Context, ownerObject core }, pod) if err != nil { if apierrors.IsNotFound(err) { - curLog.WithField("backup pod", restorePodName).Debug("Backup pod is not running in the current node") + curLog.WithField("restore pod", restorePodName).Debug("Restore pod is not running in the current node") return nil, nil } else { - return nil, errors.Wrapf(err, "error to get backup pod %s", restorePodName) + return nil, errors.Wrapf(err, "error to get restore pod %s", restorePodName) } } @@ -143,7 +144,20 @@ func (e *genericRestoreExposer) GetExposed(ctx context.Context, ownerObject core curLog.WithField("restore pvc", restorePVCName).Info("Restore PVC is bound") - return &ExposeResult{ByPod: ExposeByPod{HostingPod: pod, VolumeName: pod.Spec.Volumes[0].Name}}, nil + i := 0 + for i = 0; i < len(pod.Spec.Volumes); i++ { + if pod.Spec.Volumes[i].Name == volumeName { + break + } + } + + if i == len(pod.Spec.Volumes) { + return nil, errors.Errorf("restore pod %s doesn't have the expected restore volume", pod.Name) + } + + curLog.WithField("pod", pod.Name).Infof("Restore volume is found in pod at index %v", i) + + return &ExposeResult{ByPod: ExposeByPod{HostingPod: pod, VolumeName: volumeName}}, nil } func (e *genericRestoreExposer) CleanUp(ctx context.Context, ownerObject corev1.ObjectReference) { @@ -151,7 +165,7 @@ func (e *genericRestoreExposer) CleanUp(ctx context.Context, ownerObject corev1. restorePVCName := ownerObject.Name kube.DeletePodIfAny(ctx, e.kubeClient.CoreV1(), restorePodName, ownerObject.Namespace, e.log) - kube.DeletePVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVCName, ownerObject.Namespace, e.log) + kube.DeletePVAndPVCIfAny(ctx, e.kubeClient.CoreV1(), restorePVCName, ownerObject.Namespace, e.log) } func (e *genericRestoreExposer) RebindVolume(ctx context.Context, ownerObject corev1.ObjectReference, targetPVCName string, sourceNamespace string, timeout time.Duration) error { diff --git a/pkg/install/daemonset.go b/pkg/install/daemonset.go index 8e74e16da..17580f05d 100644 --- a/pkg/install/daemonset.go +++ b/pkg/install/daemonset.go @@ -105,6 +105,7 @@ func DaemonSet(namespace string, opts ...podTemplateOption) *appsv1.DaemonSet { { Name: "node-agent", Image: c.image, + Ports: containerPorts(), ImagePullPolicy: pullPolicy, Command: []string{ "/velero", diff --git a/pkg/restore/request.go b/pkg/restore/request.go index dcc2ef3d6..2a267a5ff 100644 --- a/pkg/restore/request.go +++ b/pkg/restore/request.go @@ -21,6 +21,7 @@ import ( "io" "sort" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/runtime" @@ -60,6 +61,7 @@ type Request struct { itemOperationsList *[]*itemoperation.RestoreOperation ResourceModifiers *resourcemodifiers.ResourceModifiers DisableInformerCache bool + CSIVolumeSnapshots []*snapshotv1api.VolumeSnapshot } type restoredItemStatus struct { diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index e18e4d3e3..164d1bd6c 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -30,6 +30,7 @@ import ( "time" "github.com/google/uuid" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" @@ -298,6 +299,7 @@ func (kr *kubernetesRestorer) RestoreWithResolvers( pvsToProvision: sets.NewString(), pvRestorer: pvRestorer, volumeSnapshots: req.VolumeSnapshots, + csiVolumeSnapshots: req.CSIVolumeSnapshots, podVolumeBackups: req.PodVolumeBackups, resourceTerminatingTimeout: kr.resourceTerminatingTimeout, resourceTimeout: kr.resourceTimeout, @@ -347,6 +349,7 @@ type restoreContext struct { pvsToProvision sets.String pvRestorer PVRestorer volumeSnapshots []*volume.Snapshot + csiVolumeSnapshots []*snapshotv1api.VolumeSnapshot podVolumeBackups []*velerov1api.PodVolumeBackup resourceTerminatingTimeout time.Duration resourceTimeout time.Duration @@ -1287,7 +1290,35 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } case hasPodVolumeBackup(obj, ctx): - ctx.log.Infof("Dynamically re-provisioning persistent volume because it has a pod volume backup to be restored.") + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it has a pod volume backup to be restored.") + ctx.pvsToProvision.Insert(name) + + // Return early because we don't want to restore the PV itself, we + // want to dynamically re-provision it. + return warnings, errs, itemExists + + case hasCSIVolumeSnapshot(ctx, obj): + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it has a related CSI VolumeSnapshot.") + ctx.pvsToProvision.Insert(name) + + // Return early because we don't want to restore the PV itself, we + // want to dynamically re-provision it. + return warnings, errs, itemExists + + case hasSnapshotDataUpload(ctx, obj): + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it has a related snapshot DataUpload.") ctx.pvsToProvision.Insert(name) // Return early because we don't want to restore the PV itself, we @@ -1295,7 +1326,11 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs, itemExists case hasDeleteReclaimPolicy(obj.Object): - ctx.log.Infof("Dynamically re-provisioning persistent volume because it doesn't have a snapshot and its reclaim policy is Delete.") + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it doesn't have a snapshot and its reclaim policy is Delete.") ctx.pvsToProvision.Insert(name) // Return early because we don't want to restore the PV itself, we @@ -1303,7 +1338,11 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs, itemExists default: - ctx.log.Infof("Restoring persistent volume as-is because it doesn't have a snapshot and its reclaim policy is not Delete.") + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Restoring persistent volume as-is because it doesn't have a snapshot and its reclaim policy is not Delete.") // Check to see if the claimRef.namespace field needs to be remapped, and do so if necessary. _, err = remapClaimRefNS(ctx, obj) @@ -1466,7 +1505,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } if ctx.resourceModifiers != nil { - if errList := ctx.resourceModifiers.ApplyResourceModifierRules(obj, groupResource.String(), ctx.log); errList != nil { + if errList := ctx.resourceModifiers.ApplyResourceModifierRules(obj, groupResource.String(), ctx.kbClient.Scheme(), ctx.log); errList != nil { for _, err := range errList { errs.Add(namespace, err) } @@ -1929,6 +1968,55 @@ func hasSnapshot(pvName string, snapshots []*volume.Snapshot) bool { return false } +func hasCSIVolumeSnapshot(ctx *restoreContext, unstructuredPV *unstructured.Unstructured) bool { + pv := new(v1.PersistentVolume) + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredPV.Object, pv); err != nil { + ctx.log.WithError(err).Warnf("Unable to convert PV from unstructured to structured") + return false + } + + for _, vs := range ctx.csiVolumeSnapshots { + if pv.Spec.ClaimRef.Name == *vs.Spec.Source.PersistentVolumeClaimName && + pv.Spec.ClaimRef.Namespace == vs.Namespace { + return true + } + } + return false +} + +func hasSnapshotDataUpload(ctx *restoreContext, unstructuredPV *unstructured.Unstructured) bool { + pv := new(v1.PersistentVolume) + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredPV.Object, pv); err != nil { + ctx.log.WithError(err).Warnf("Unable to convert PV from unstructured to structured") + return false + } + + if pv.Spec.ClaimRef == nil { + return false + } + + dataUploadResultList := new(v1.ConfigMapList) + err := ctx.kbClient.List(go_context.TODO(), dataUploadResultList, &crclient.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + velerov1api.RestoreUIDLabel: label.GetValidName(string(ctx.restore.GetUID())), + velerov1api.PVCNamespaceNameLabel: label.GetValidName(pv.Spec.ClaimRef.Namespace + "." + pv.Spec.ClaimRef.Name), + velerov1api.ResourceUsageLabel: label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)), + }), + }) + if err != nil { + ctx.log.WithError(err).Warnf("Fail to list DataUpload result CM.") + return false + } + + if len(dataUploadResultList.Items) != 1 { + ctx.log.WithError(fmt.Errorf("dataupload result number is not expected")). + Warnf("Got %d DataUpload result. Expect one.", len(dataUploadResultList.Items)) + return false + } + + return true +} + func hasPodVolumeBackup(unstructuredPV *unstructured.Unstructured, ctx *restoreContext) bool { if len(ctx.podVolumeBackups) == 0 { return false diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 4ffd76257..643d001af 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -25,6 +25,7 @@ import ( "testing" "time" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -2256,6 +2257,7 @@ func (*volumeSnapshotter) DeleteSnapshot(snapshotID string) error { // Verification is done by looking at the contents of the API and the metadata/spec/status of // the items in the API. func TestRestorePersistentVolumes(t *testing.T) { + testPVCName := "testPVC" tests := []struct { name string restore *velerov1api.Restore @@ -2265,6 +2267,8 @@ func TestRestorePersistentVolumes(t *testing.T) { volumeSnapshots []*volume.Snapshot volumeSnapshotLocations []*velerov1api.VolumeSnapshotLocation volumeSnapshotterGetter volumeSnapshotterGetter + csiVolumeSnapshots []*snapshotv1api.VolumeSnapshot + dataUploadResult *corev1api.ConfigMap want []*test.APIResource wantError bool wantWarning bool @@ -2923,6 +2927,77 @@ func TestRestorePersistentVolumes(t *testing.T) { ), }, }, + { + name: "when a PV with a reclaim policy of retain has a CSI VolumeSnapshot and does not exist in-cluster, the PV is not restored", + restore: defaultRestore().Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain). + ClaimRef("velero", testPVCName). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.PVs(), + test.PVCs(), + }, + csiVolumeSnapshots: []*snapshotv1api.VolumeSnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "velero", + Name: "test", + }, + Spec: snapshotv1api.VolumeSnapshotSpec{ + Source: snapshotv1api.VolumeSnapshotSource{ + PersistentVolumeClaimName: &testPVCName, + }, + }, + }, + }, + volumeSnapshotLocations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "default").Provider("provider-1").Result(), + }, + volumeSnapshotterGetter: map[string]vsv1.VolumeSnapshotter{ + "provider-1": &volumeSnapshotter{ + snapshotVolumes: map[string]string{"snapshot-1": "new-volume"}, + }, + }, + want: []*test.APIResource{}, + }, + { + name: "when a PV with a reclaim policy of retain has a DataUpload result CM and does not exist in-cluster, the PV is not restored", + restore: defaultRestore().ObjectMeta(builder.WithUID("fakeUID")).Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain). + ClaimRef("velero", testPVCName). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.PVs(), + test.PVCs(), + test.ConfigMaps(), + }, + volumeSnapshotLocations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "default").Provider("provider-1").Result(), + }, + volumeSnapshotterGetter: map[string]vsv1.VolumeSnapshotter{ + "provider-1": &volumeSnapshotter{ + snapshotVolumes: map[string]string{"snapshot-1": "new-volume"}, + }, + }, + dataUploadResult: builder.ForConfigMap("velero", "test").ObjectMeta(builder.WithLabelsMap(map[string]string{ + velerov1api.RestoreUIDLabel: "fakeUID", + velerov1api.PVCNamespaceNameLabel: "velero/testPVC", + velerov1api.ResourceUsageLabel: string(velerov1api.VeleroResourceUsageDataUploadResult), + })).Result(), + want: []*test.APIResource{}, + }, } for _, tc := range tests { @@ -2939,6 +3014,10 @@ func TestRestorePersistentVolumes(t *testing.T) { require.NoError(t, h.restorer.kbClient.Create(context.Background(), vsl)) } + if tc.dataUploadResult != nil { + require.NoError(t, h.restorer.kbClient.Create(context.TODO(), tc.dataUploadResult)) + } + for _, r := range tc.apiResources { h.AddItems(t, r) } @@ -2955,11 +3034,12 @@ func TestRestorePersistentVolumes(t *testing.T) { } data := &Request{ - Log: h.log, - Restore: tc.restore, - Backup: tc.backup, - VolumeSnapshots: tc.volumeSnapshots, - BackupReader: tc.tarball, + Log: h.log, + Restore: tc.restore, + Backup: tc.backup, + VolumeSnapshots: tc.volumeSnapshots, + BackupReader: tc.tarball, + CSIVolumeSnapshots: tc.csiVolumeSnapshots, } warnings, errs := h.restorer.Restore( data, @@ -3652,3 +3732,175 @@ func TestIsAlreadyExistsError(t *testing.T) { }) } } + +func TestHasCSIVolumeSnapshot(t *testing.T) { + tests := []struct { + name string + vs *snapshotv1api.VolumeSnapshot + obj *unstructured.Unstructured + expectedResult bool + }{ + { + name: "Invalid PV, expect false.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": 1, + }, + }, + expectedResult: false, + }, + { + name: "Cannot find VS, expect false", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + }, + }, + expectedResult: false, + }, + { + name: "Find VS, expect true.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "velero", + "name": "test", + }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "test", + }, + }, + }, + }, + vs: builder.ForVolumeSnapshot("velero", "test").SourcePVC("test").Result(), + expectedResult: true, + }, + } + + for _, tc := range tests { + h := newHarness(t) + + ctx := &restoreContext{ + log: h.log, + } + + if tc.vs != nil { + ctx.csiVolumeSnapshots = []*snapshotv1api.VolumeSnapshot{tc.vs} + } + + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expectedResult, hasCSIVolumeSnapshot(ctx, tc.obj)) + }) + } +} + +func TestHasSnapshotDataUpload(t *testing.T) { + tests := []struct { + name string + duResult *corev1api.ConfigMap + obj *unstructured.Unstructured + expectedResult bool + restore *velerov1api.Restore + }{ + { + name: "Invalid PV, expect false.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": 1, + }, + }, + expectedResult: false, + }, + { + name: "PV without ClaimRef, expect false", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + }, + }, + duResult: builder.ForConfigMap("velero", "test").Result(), + restore: builder.ForRestore("velero", "test").ObjectMeta(builder.WithUID("fakeUID")).Result(), + expectedResult: false, + }, + { + name: "Cannot find DataUploadResult CM, expect false", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "testPVC", + }, + }, + }, + }, + duResult: builder.ForConfigMap("velero", "test").Result(), + restore: builder.ForRestore("velero", "test").ObjectMeta(builder.WithUID("fakeUID")).Result(), + expectedResult: false, + }, + { + name: "Find DataUploadResult CM, expect true", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "testPVC", + }, + }, + }, + }, + duResult: builder.ForConfigMap("velero", "test").ObjectMeta(builder.WithLabelsMap(map[string]string{ + velerov1api.RestoreUIDLabel: "fakeUID", + velerov1api.PVCNamespaceNameLabel: "velero/testPVC", + velerov1api.ResourceUsageLabel: string(velerov1api.VeleroResourceUsageDataUploadResult), + })).Result(), + restore: builder.ForRestore("velero", "test").ObjectMeta(builder.WithUID("fakeUID")).Result(), + expectedResult: false, + }, + } + + for _, tc := range tests { + h := newHarness(t) + + ctx := &restoreContext{ + log: h.log, + kbClient: h.restorer.kbClient, + restore: tc.restore, + } + + if tc.duResult != nil { + require.NoError(t, ctx.kbClient.Create(context.TODO(), tc.duResult)) + } + + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expectedResult, hasSnapshotDataUpload(ctx, tc.obj)) + }) + } +} diff --git a/pkg/test/mocks.go b/pkg/test/mocks.go new file mode 100644 index 000000000..9a86d2b70 --- /dev/null +++ b/pkg/test/mocks.go @@ -0,0 +1,20 @@ +package test + +import ( + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" + "k8s.io/apimachinery/pkg/labels" +) + +// VolumeSnapshotLister helps list VolumeSnapshots. +// All objects returned here must be treated as read-only. +// +//go:generate mockery --name VolumeSnapshotLister +type VolumeSnapshotLister interface { + // List lists all VolumeSnapshots in the indexer. + // Objects returned here must be treated as read-only. + List(selector labels.Selector) (ret []*snapshotv1.VolumeSnapshot, err error) + // VolumeSnapshots returns an object that can list and get VolumeSnapshots. + VolumeSnapshots(namespace string) snapshotv1listers.VolumeSnapshotNamespaceLister + snapshotv1listers.VolumeSnapshotListerExpansion +} diff --git a/pkg/test/mocks/VolumeSnapshotLister.go b/pkg/test/mocks/VolumeSnapshotLister.go new file mode 100644 index 000000000..21e4bcb66 --- /dev/null +++ b/pkg/test/mocks/VolumeSnapshotLister.go @@ -0,0 +1,73 @@ +// Code generated by mockery v2.35.4. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + labels "k8s.io/apimachinery/pkg/labels" + + v1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + + volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" +) + +// VolumeSnapshotLister is an autogenerated mock type for the VolumeSnapshotLister type +type VolumeSnapshotLister struct { + mock.Mock +} + +// List provides a mock function with given fields: selector +func (_m *VolumeSnapshotLister) List(selector labels.Selector) ([]*v1.VolumeSnapshot, error) { + ret := _m.Called(selector) + + var r0 []*v1.VolumeSnapshot + var r1 error + if rf, ok := ret.Get(0).(func(labels.Selector) ([]*v1.VolumeSnapshot, error)); ok { + return rf(selector) + } + if rf, ok := ret.Get(0).(func(labels.Selector) []*v1.VolumeSnapshot); ok { + r0 = rf(selector) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v1.VolumeSnapshot) + } + } + + if rf, ok := ret.Get(1).(func(labels.Selector) error); ok { + r1 = rf(selector) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// VolumeSnapshots provides a mock function with given fields: namespace +func (_m *VolumeSnapshotLister) VolumeSnapshots(namespace string) volumesnapshotv1.VolumeSnapshotNamespaceLister { + ret := _m.Called(namespace) + + var r0 volumesnapshotv1.VolumeSnapshotNamespaceLister + if rf, ok := ret.Get(0).(func(string) volumesnapshotv1.VolumeSnapshotNamespaceLister); ok { + r0 = rf(namespace) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(volumesnapshotv1.VolumeSnapshotNamespaceLister) + } + } + + return r0 +} + +// NewVolumeSnapshotLister creates a new instance of VolumeSnapshotLister. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewVolumeSnapshotLister(t interface { + mock.TestingT + Cleanup(func()) +}) *VolumeSnapshotLister { + mock := &VolumeSnapshotLister{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/test/resources.go b/pkg/test/resources.go index 7c2fa17f6..709497fca 100644 --- a/pkg/test/resources.go +++ b/pkg/test/resources.go @@ -142,6 +142,17 @@ func ServiceAccounts(items ...metav1.Object) *APIResource { } } +func ConfigMaps(items ...metav1.Object) *APIResource { + return &APIResource{ + Group: "", + Version: "v1", + Name: "configmaps", + ShortName: "cm", + Namespaced: true, + Items: items, + } +} + func CRDs(items ...metav1.Object) *APIResource { return &APIResource{ Group: "apiextensions.k8s.io", diff --git a/pkg/uploader/kopia/snapshot.go b/pkg/uploader/kopia/snapshot.go index 27ac84253..cf6c2b80f 100644 --- a/pkg/uploader/kopia/snapshot.go +++ b/pkg/uploader/kopia/snapshot.go @@ -44,6 +44,7 @@ import ( ) // All function mainly used to make testing more convenient +var applyRetentionPolicyFunc = policy.ApplyRetentionPolicy var treeForSourceFunc = policy.TreeForSource var setPolicyFunc = policy.SetPolicy var saveSnapshotFunc = snapshot.SaveSnapshot @@ -236,10 +237,10 @@ func SnapshotSource( mani, err := loadSnapshotFunc(ctx, rep, manifest.ID(parentSnapshot)) if err != nil { - return "", 0, errors.Wrapf(err, "Failed to load previous snapshot %v from kopia", parentSnapshot) + log.WithError(err).Warnf("Failed to load previous snapshot %v from kopia, fallback to full backup", parentSnapshot) + } else { + previous = append(previous, mani) } - - previous = append(previous, mani) } else { log.Infof("Searching for parent snapshot") @@ -277,6 +278,11 @@ func SnapshotSource( return "", 0, errors.Wrapf(err, "Failed to save kopia manifest %v", manifest.ID) } + _, err = applyRetentionPolicyFunc(ctx, rep, sourceInfo, true) + if err != nil { + return "", 0, errors.Wrapf(err, "Failed to apply kopia retention policy for si %v", sourceInfo) + } + if err = rep.Flush(ctx); err != nil { return "", 0, errors.Wrapf(err, "Failed to flush kopia repository") } diff --git a/pkg/uploader/kopia/snapshot_test.go b/pkg/uploader/kopia/snapshot_test.go index 65ec22136..62c416029 100644 --- a/pkg/uploader/kopia/snapshot_test.go +++ b/pkg/uploader/kopia/snapshot_test.go @@ -60,6 +60,7 @@ func injectSnapshotFuncs() *snapshotMockes { repoWriterMock: &repomocks.RepositoryWriter{}, } + applyRetentionPolicyFunc = s.policyMock.ApplyRetentionPolicy setPolicyFunc = s.policyMock.SetPolicy treeForSourceFunc = s.policyMock.TreeForSource loadSnapshotFunc = s.snapshotMock.LoadSnapshot @@ -112,7 +113,7 @@ func TestSnapshotSource(t *testing.T) { notError: true, }, { - name: "failed to load snapshot", + name: "failed to load snapshot, should fallback to full backup and not error", args: []mockArgs{ {methodName: "LoadSnapshot", returns: []interface{}{manifest, errors.New("failed to load snapshot")}}, {methodName: "SaveSnapshot", returns: []interface{}{manifest.ID, nil}}, @@ -122,7 +123,7 @@ func TestSnapshotSource(t *testing.T) { {methodName: "Upload", returns: []interface{}{manifest, nil}}, {methodName: "Flush", returns: []interface{}{nil}}, }, - notError: false, + notError: true, }, { name: "failed to save snapshot", diff --git a/pkg/util/csi/volume_snapshot.go b/pkg/util/csi/volume_snapshot.go index 99845d78f..a8ade7acd 100644 --- a/pkg/util/csi/volume_snapshot.go +++ b/pkg/util/csi/volume_snapshot.go @@ -31,6 +31,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/stringptr" + "github.com/vmware-tanzu/velero/pkg/util/stringslice" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" snapshotter "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/typed/volumesnapshot/v1" @@ -41,7 +42,8 @@ import ( ) const ( - waitInternal = 2 * time.Second + waitInternal = 2 * time.Second + volumeSnapshotContentProtectFinalizer = "velero.io/volume-snapshot-content-protect-finalizer" ) // WaitVolumeSnapshotReady waits a VS to become ready to use until the timeout reaches @@ -97,36 +99,17 @@ func GetVolumeSnapshotContentForVolumeSnapshot(volSnap *snapshotv1api.VolumeSnap return vsc, nil } -// RetainVSC updates the VSC's deletion policy to Retain and return the update VSC +// RetainVSC updates the VSC's deletion policy to Retain and add a finalier and then return the update VSC func RetainVSC(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface, vsc *snapshotv1api.VolumeSnapshotContent) (*snapshotv1api.VolumeSnapshotContent, error) { if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentRetain { return vsc, nil } - origBytes, err := json.Marshal(vsc) - if err != nil { - return nil, errors.Wrap(err, "error marshaling original VSC") - } - updated := vsc.DeepCopy() - updated.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentRetain - - updatedBytes, err := json.Marshal(updated) - if err != nil { - return nil, errors.Wrap(err, "error marshaling updated VSC") - } - - patchBytes, err := jsonpatch.CreateMergePatch(origBytes, updatedBytes) - if err != nil { - return nil, errors.Wrap(err, "error creating json merge patch for VSC") - } - - retained, err := snapshotClient.VolumeSnapshotContents().Patch(ctx, vsc.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}) - if err != nil { - return nil, errors.Wrap(err, "error patching VSC") - } - - return retained, nil + return patchVSC(ctx, snapshotClient, vsc, func(updated *snapshotv1api.VolumeSnapshotContent) { + updated.Spec.DeletionPolicy = snapshotv1api.VolumeSnapshotContentRetain + updated.Finalizers = append(updated.Finalizers, volumeSnapshotContentProtectFinalizer) + }) } // DeleteVolumeSnapshotContentIfAny deletes a VSC by name if it exists, and log an error when the deletion fails @@ -169,11 +152,35 @@ func EnsureDeleteVS(ctx context.Context, snapshotClient snapshotter.SnapshotV1In return nil } +func RemoveVSCProtect(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface, vscName string, timeout time.Duration) error { + err := wait.PollImmediate(waitInternal, timeout, func() (bool, error) { + vsc, err := snapshotClient.VolumeSnapshotContents().Get(ctx, vscName, metav1.GetOptions{}) + if err != nil { + return false, errors.Wrapf(err, "error to get VolumeSnapshotContent %s", vscName) + } + + vsc.Finalizers = stringslice.Except(vsc.Finalizers, volumeSnapshotContentProtectFinalizer) + + _, err = snapshotClient.VolumeSnapshotContents().Update(ctx, vsc, metav1.UpdateOptions{}) + if err == nil { + return true, nil + } + + if !apierrors.IsConflict(err) { + return false, errors.Wrapf(err, "error to update VolumeSnapshotContent %s", vscName) + } + + return false, nil + }) + + return err +} + // EnsureDeleteVSC asserts the existence of a VSC by name, deletes it and waits for its disappearance and returns errors on any failure func EnsureDeleteVSC(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface, vscName string, timeout time.Duration) error { err := snapshotClient.VolumeSnapshotContents().Delete(ctx, vscName, metav1.DeleteOptions{}) - if err != nil { + if err != nil && !apierrors.IsNotFound(err) { return errors.Wrap(err, "error to delete volume snapshot content") } @@ -208,3 +215,31 @@ func DeleteVolumeSnapshotIfAny(ctx context.Context, snapshotClient snapshotter.S } } } + +func patchVSC(ctx context.Context, snapshotClient snapshotter.SnapshotV1Interface, + vsc *snapshotv1api.VolumeSnapshotContent, updateFunc func(*snapshotv1api.VolumeSnapshotContent)) (*snapshotv1api.VolumeSnapshotContent, error) { + origBytes, err := json.Marshal(vsc) + if err != nil { + return nil, errors.Wrap(err, "error marshaling original VSC") + } + + updated := vsc.DeepCopy() + updateFunc(updated) + + updatedBytes, err := json.Marshal(updated) + if err != nil { + return nil, errors.Wrap(err, "error marshaling updated VSC") + } + + patchBytes, err := jsonpatch.CreateMergePatch(origBytes, updatedBytes) + if err != nil { + return nil, errors.Wrap(err, "error creating json merge patch for VSC") + } + + patched, err := snapshotClient.VolumeSnapshotContents().Patch(ctx, vsc.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}) + if err != nil { + return nil, errors.Wrap(err, "error patching VSC") + } + + return patched, nil +} diff --git a/pkg/util/csi/volume_snapshot_test.go b/pkg/util/csi/volume_snapshot_test.go index 43ed9a316..4fbac34f5 100644 --- a/pkg/util/csi/volume_snapshot_test.go +++ b/pkg/util/csi/volume_snapshot_test.go @@ -34,6 +34,8 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/stringptr" velerotest "github.com/vmware-tanzu/velero/pkg/test" + + apierrors "k8s.io/apimachinery/pkg/api/errors" ) type reactor struct { @@ -364,9 +366,23 @@ func TestEnsureDeleteVSC(t *testing.T) { err string }{ { - name: "delete fail", + name: "delete fail on VSC not found", vscName: "fake-vsc", - err: "error to delete volume snapshot content: volumesnapshotcontents.snapshot.storage.k8s.io \"fake-vsc\" not found", + }, + { + name: "delete fail on others", + vscName: "fake-vsc", + clientObj: []runtime.Object{vscObj}, + reactors: []reactor{ + { + verb: "delete", + resource: "volumesnapshotcontents", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-delete-error") + }, + }, + }, + err: "error to delete volume snapshot content: fake-delete-error", }, { name: "wait fail", @@ -399,7 +415,7 @@ func TestEnsureDeleteVSC(t *testing.T) { } err := EnsureDeleteVSC(context.Background(), fakeSnapshotClient.SnapshotV1(), test.vscName, time.Millisecond) - if err != nil { + if test.err != "" { assert.EqualError(t, err, test.err) } else { assert.NoError(t, err) @@ -601,7 +617,8 @@ func TestRetainVSC(t *testing.T) { clientObj: []runtime.Object{vscObj}, updated: &snapshotv1api.VolumeSnapshotContent{ ObjectMeta: metav1.ObjectMeta{ - Name: "fake-vsc", + Name: "fake-vsc", + Finalizers: []string{volumeSnapshotContentProtectFinalizer}, }, Spec: snapshotv1api.VolumeSnapshotContentSpec{ DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, @@ -634,3 +651,98 @@ func TestRetainVSC(t *testing.T) { }) } } + +func TestRemoveVSCProtect(t *testing.T) { + vscObj := &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-vsc", + Finalizers: []string{volumeSnapshotContentProtectFinalizer}, + }, + } + + tests := []struct { + name string + clientObj []runtime.Object + reactors []reactor + vsc string + updated *snapshotv1api.VolumeSnapshotContent + timeout time.Duration + err string + }{ + { + name: "get vsc error", + vsc: "fake-vsc", + err: "error to get VolumeSnapshotContent fake-vsc: volumesnapshotcontents.snapshot.storage.k8s.io \"fake-vsc\" not found", + }, + { + name: "update vsc fail", + vsc: "fake-vsc", + clientObj: []runtime.Object{vscObj}, + reactors: []reactor{ + { + verb: "update", + resource: "volumesnapshotcontents", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-update-error") + }, + }, + }, + err: "error to update VolumeSnapshotContent fake-vsc: fake-update-error", + }, + { + name: "update vsc timeout", + vsc: "fake-vsc", + clientObj: []runtime.Object{vscObj}, + reactors: []reactor{ + { + verb: "update", + resource: "volumesnapshotcontents", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, &apierrors.StatusError{ErrStatus: metav1.Status{ + Reason: metav1.StatusReasonConflict, + }} + }, + }, + }, + timeout: time.Second, + err: "timed out waiting for the condition", + }, + { + name: "succeed", + vsc: "fake-vsc", + clientObj: []runtime.Object{vscObj}, + timeout: time.Second, + updated: &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-vsc", + Finalizers: []string{}, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeSnapshotClient := snapshotFake.NewSimpleClientset(test.clientObj...) + + for _, reactor := range test.reactors { + fakeSnapshotClient.Fake.PrependReactor(reactor.verb, reactor.resource, reactor.reactorFunc) + } + + err := RemoveVSCProtect(context.Background(), fakeSnapshotClient.SnapshotV1(), test.vsc, test.timeout) + + if len(test.err) == 0 { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, test.err) + } + + if test.updated != nil { + updated, err := fakeSnapshotClient.SnapshotV1().VolumeSnapshotContents().Get(context.Background(), test.vsc, metav1.GetOptions{}) + assert.NoError(t, err) + + assert.Equal(t, test.updated.Finalizers, updated.Finalizers) + } + }) + } +} diff --git a/pkg/util/kube/pvc_pv.go b/pkg/util/kube/pvc_pv.go index 1a393b3ab..6954db769 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/pkg/util/kube/pvc_pv.go @@ -41,16 +41,37 @@ const ( waitInternal = 2 * time.Second ) -// DeletePVCIfAny deletes a PVC by name if it exists, and log an error when the deletion fails -func DeletePVCIfAny(ctx context.Context, pvcGetter corev1client.CoreV1Interface, pvcName string, pvcNamespace string, log logrus.FieldLogger) { - err := pvcGetter.PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{}) +// DeletePVAndPVCIfAny deletes PVC and delete the bound PV if it exists and log an error when the deletion fails +// We first set the reclaim policy of the PV to Delete, then PV will be deleted automatically when PVC is deleted. +func DeletePVAndPVCIfAny(ctx context.Context, client corev1client.CoreV1Interface, pvcName, pvcNamespace string, log logrus.FieldLogger) { + pvcObj, err := client.PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - log.WithError(err).Debugf("Abort deleting PVC, it doesn't exist, %s/%s", pvcNamespace, pvcName) + log.WithError(err).Debugf("Abort deleting PV and PVC, for related PVC doesn't exist, %s/%s", pvcNamespace, pvcName) + return } else { - log.WithError(err).Errorf("Failed to delete pvc %s/%s", pvcNamespace, pvcName) + log.Warnf("failed to get pvc %s/%s with err %v", pvcNamespace, pvcName, err) + return } } + + if pvcObj.Spec.VolumeName == "" { + log.Warnf("failed to delete PV, for related PVC %s/%s has no bind volume name", pvcNamespace, pvcName) + } else { + pvObj, err := client.PersistentVolumes().Get(ctx, pvcObj.Spec.VolumeName, metav1.GetOptions{}) + if err != nil { + log.Warnf("failed to delete PV %s with err %v", pvcObj.Spec.VolumeName, err) + } else { + _, err = SetPVReclaimPolicy(ctx, client, pvObj, corev1api.PersistentVolumeReclaimDelete) + if err != nil { + log.Warnf("failed to set reclaim policy of PV %s to delete with err %v", pvObj.Name, err) + } + } + } + + if err := client.PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil { + log.Warnf("failed to delete pvc %s/%s with err %v", pvcNamespace, pvcName, err) + } } // WaitPVCBound wait for binding of a PVC specified by name and returns the bound PV object diff --git a/pkg/util/kube/pvc_pv_test.go b/pkg/util/kube/pvc_pv_test.go index 67edca6ee..f6950af0a 100644 --- a/pkg/util/kube/pvc_pv_test.go +++ b/pkg/util/kube/pvc_pv_test.go @@ -289,10 +289,44 @@ func TestWaitPVCConsumed(t *testing.T) { } func TestDeletePVCIfAny(t *testing.T) { + pvObject := &corev1api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-pv", + Annotations: map[string]string{ + KubeAnnBoundByController: "true", + }, + }, + Spec: corev1api.PersistentVolumeSpec{ + ClaimRef: &corev1api.ObjectReference{ + Kind: "fake-kind", + Namespace: "fake-ns", + Name: "fake-pvc", + }, + }, + } + + pvcObject := &corev1api.PersistentVolumeClaim{ + TypeMeta: metav1.TypeMeta{ + Kind: "fake-kind-1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-namespace", + Name: "fake-pvc", + Annotations: map[string]string{ + KubeAnnBindCompleted: "true", + KubeAnnBoundByController: "true", + }, + }, + } + + pvcWithVolume := pvcObject.DeepCopy() + pvcWithVolume.Spec.VolumeName = "fake-pv" + tests := []struct { name string pvcName string pvcNamespace string + pvName string kubeClientObj []runtime.Object kubeReactors []reactor logMessage string @@ -300,17 +334,69 @@ func TestDeletePVCIfAny(t *testing.T) { logError string }{ { - name: "get fail", + name: "pvc not found", pvcName: "fake-pvc", pvcNamespace: "fake-namespace", - logMessage: "Abort deleting PVC, it doesn't exist, fake-namespace/fake-pvc", + logMessage: "Abort deleting PV and PVC, for related PVC doesn't exist, fake-namespace/fake-pvc", logLevel: "level=debug", }, { - name: "delete fail", + name: "failed to get pvc", pvcName: "fake-pvc", pvcNamespace: "fake-namespace", kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-get-error") + }, + }, + }, + logMessage: "failed to get pvc fake-namespace/fake-pvc with err fake-get-error", + logLevel: "level=warning", + }, + { + name: "pvc has no volume name", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, nil + }, + }, + }, + kubeClientObj: []runtime.Object{ + pvcObject, + pvObject, + }, + logMessage: "failed to delete PV, for related PVC fake-namespace/fake-pvc has no bind volume name", + logLevel: "level=warning", + }, + { + name: "failed to delete pvc", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, { verb: "delete", resource: "persistentvolumeclaims", @@ -319,9 +405,116 @@ func TestDeletePVCIfAny(t *testing.T) { }, }, }, - logMessage: "Failed to delete pvc fake-namespace/fake-pvc", - logLevel: "level=error", - logError: "error=fake-delete-error", + kubeClientObj: []runtime.Object{ + pvcObject, + pvObject, + }, + logMessage: "failed to delete pvc fake-namespace/fake-pvc with err fake-delete-error", + logLevel: "level=warning", + }, + { + name: "failed to get pv", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake-get-error") + }, + }, + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + }, + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + logMessage: "failed to delete PV fake-pv with err fake-get-error", + logLevel: "level=warning", + }, + { + name: "set reclaim policy fail", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, nil + }, + }, + { + verb: "patch", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, errors.New("fake-patch-error") + }, + }, + }, + logMessage: "failed to set reclaim policy of PV fake-pv to delete with err error patching PV: fake-patch-error", + logLevel: "level=warning", + logError: "fake-patch-error", + }, + { + name: "delete pv pvc success", + pvcName: "fake-pvc", + pvcNamespace: "fake-namespace", + pvName: "fake-pv", + kubeClientObj: []runtime.Object{ + pvcWithVolume, + pvObject, + }, + kubeReactors: []reactor{ + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, nil + }, + }, + { + verb: "patch", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvObject, nil + }, + }, + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcWithVolume, nil + }, + }, + }, }, } @@ -336,7 +529,7 @@ func TestDeletePVCIfAny(t *testing.T) { var kubeClient kubernetes.Interface = fakeKubeClient logMessage := "" - DeletePVCIfAny(context.Background(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, velerotest.NewSingleLogger(&logMessage)) + DeletePVAndPVCIfAny(context.Background(), kubeClient.CoreV1(), test.pvcName, test.pvcNamespace, velerotest.NewSingleLogger(&logMessage)) if len(test.logMessage) > 0 { assert.Contains(t, logMessage, test.logMessage) diff --git a/site/content/docs/main/restore-resource-modifiers.md b/site/content/docs/main/restore-resource-modifiers.md index 0d59fad5a..0c1f2f217 100644 --- a/site/content/docs/main/restore-resource-modifiers.md +++ b/site/content/docs/main/restore-resource-modifiers.md @@ -105,3 +105,83 @@ resourceModifierRules: - Update a container's image using a json patch with positional arrays kubectl patch pod valid-pod -type='json' -p='[{"op": "replace", "path": "/spec/containers/0/image", "value":"new image"}]' - Before creating the resource modifier yaml, you can try it out using kubectl patch command. The same commands should work as it is. + +#### JSON Merge Patch +You can modify a resource using JSON Merge Patch +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: pods + namespaces: + - ns1 + mergePatches: + - patchData: | + { + "metadata": { + "annotations": { + "foo": null + } + } + } +``` +- The above configmap will apply the Merge Patch to all the pods in namespace ns1 and remove the annotation `foo` from the pods. +- Both json and yaml format are supported for the patchData. +- For more details, please refer to [this doc](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/) + +#### Strategic Merge Patch +You can modify a resource using Strategic Merge Patch +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: pods + resourceNameRegex: "^my-pod$" + namespaces: + - ns1 + strategicPatches: + - patchData: | + { + "spec": { + "containers": [ + { + "name": "nginx", + "image": "repo2/nginx" + } + ] + } + } +``` +- The above configmap will apply the Strategic Merge Patch to the pod with name my-pod in namespace ns1 and update the image of container nginx to `repo2/nginx`. +- Both json and yaml format are supported for the patchData. +- For more details, please refer to [this doc](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/) + + +### Conditional Patches in ALL Patch Types +A new field `matches` is added in conditions to support conditional patches. + +Example of matches in conditions +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: persistentvolumeclaims.storage.k8s.io + matches: + - path: "/spec/storageClassName" + value: "premium" + mergePatches: + - patchData: | + { + "metadata": { + "annotations": { + "foo": null + } + } + } +``` +- The above configmap will apply the Merge Patch to all the PVCs in all namespaces with storageClassName premium and remove the annotation `foo` from the PVCs. +- You can specify multiple rules in the `matches` list. The patch will be applied only if all the matches are satisfied. + +### Wildcard Support for GroupResource +The user can specify a wildcard for groupResource in the conditions' struct. This will allow the user to apply the patches for all the resources of a particular group or all resources in all groups. For example, `*.apps` will apply to all the resources in the `apps` group, `*` will apply to all the resources in core group, `*.*` will apply to all the resources in all groups. +- If both `*.groupName` and `namespaces` are specified, the patches will be applied to all the namespaced resources in this group in the specified namespaces and all the cluster resources in this group. \ No newline at end of file diff --git a/test/e2e/backups/deletion.go b/test/e2e/backups/deletion.go index 34e502df0..c75d09cbf 100644 --- a/test/e2e/backups/deletion.go +++ b/test/e2e/backups/deletion.go @@ -178,6 +178,11 @@ func runBackupDeletionTests(client TestClient, veleroCfg VeleroConfig, backupNam return errors.Wrap(err, "exceed waiting for snapshot created in cloud") } } + + // Hit issue: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html#:~:text=SnapshotCreationPerVolumeRateExceeded + // Sleep for more than 15 seconds to avoid this issue. + time.Sleep(1 * time.Minute) + backupName = "backup-1-" + UUIDgen.String() BackupCfg.BackupName = backupName diff --git a/test/util/csi/common.go b/test/util/csi/common.go index e96e865b0..4839b6049 100644 --- a/test/util/csi/common.go +++ b/test/util/csi/common.go @@ -21,14 +21,12 @@ import ( "fmt" "strings" - "github.com/pkg/errors" - snapshotterClientSet "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" + "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/clientcmd" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - . "github.com/vmware-tanzu/velero/test/util/k8s" ) @@ -124,7 +122,7 @@ func GetCsiSnapshotHandleV1(client TestClient, backupName string) ([]string, err } if len(snapshotHandleList) == 0 { - fmt.Printf("No VolumeSnapshotContent from backup %s", backupName) + fmt.Printf("No VolumeSnapshotContent from backup %s\n", backupName) } return snapshotHandleList, nil } @@ -170,11 +168,11 @@ func CheckVolumeSnapshotCR(client TestClient, backupName string, expectedCount i var snapshotContentNameList []string if apiVersion == "v1beta1" { if snapshotContentNameList, err = GetCsiSnapshotHandle(client, backupName); err != nil { - return nil, errors.Wrap(err, "Fail to get Azure CSI snapshot content") + return nil, errors.Wrap(err, "Fail to get Azure CSI snapshot content for v1beta1") } } else if apiVersion == "v1" { if snapshotContentNameList, err = GetCsiSnapshotHandleV1(client, backupName); err != nil { - return nil, errors.Wrap(err, "Fail to get Azure CSI snapshot content") + return nil, errors.Wrap(err, "Fail to get Azure CSI snapshot content for v1") } } else { return nil, errors.New("API version is invalid") diff --git a/test/util/k8s/common.go b/test/util/k8s/common.go index ed579cb77..db323da4a 100644 --- a/test/util/k8s/common.go +++ b/test/util/k8s/common.go @@ -225,7 +225,7 @@ func GetAPIVersions(client *TestClient, name string) ([]string, error) { fmt.Println(group.Name) if group.Name == name { for _, v := range group.Versions { - fmt.Println(v.Version) + fmt.Printf("group: %s version:%s", group.Name, v.Version) version = append(version, v.Version) } return version, nil diff --git a/test/util/kibishii/kibishii_utils.go b/test/util/kibishii/kibishii_utils.go index f9d2b00d8..252e2018c 100644 --- a/test/util/kibishii/kibishii_utils.go +++ b/test/util/kibishii/kibishii_utils.go @@ -117,6 +117,7 @@ func RunKibishiiTests(veleroCfg VeleroConfig, backupName, restoreName, backupLoc } } snapshotCheckPoint, err := GetSnapshotCheckPoint(client, veleroCfg, 2, kibishiiNamespace, backupName, KibishiiPVCNameList) + if err != nil { return errors.Wrap(err, "Fail to get snapshot checkpoint") } diff --git a/test/util/providers/aws_utils.go b/test/util/providers/aws_utils.go index f9dd597ac..342acba1b 100644 --- a/test/util/providers/aws_utils.go +++ b/test/util/providers/aws_utils.go @@ -345,41 +345,61 @@ func (s AWSStorage) IsSnapshotExisted(cloudCredentialsFile, bslConfig, backupObj fmt.Printf("Fail to create session with profile %s and config %s", cloudCredentialsFile, bslConfig) return errors.Wrapf(err, "Fail to create session with profile %s and config %s", cloudCredentialsFile, bslConfig) } + svc := ec2.New(sess) params := &ec2.DescribeSnapshotsInput{ OwnerIds: []*string{aws.String("self")}, - Filters: []*ec2.Filter{ - { - Name: aws.String("tag:velero.io/backup"), - Values: []*string{ - aws.String(backupObject), + } + + if !snapshotCheck.EnableCSI { + params = &ec2.DescribeSnapshotsInput{ + OwnerIds: []*string{aws.String("self")}, + Filters: []*ec2.Filter{ + { + Name: aws.String("tag:velero.io/backup"), + Values: []*string{ + aws.String(backupObject), + }, }, }, - }, + } } result, err := svc.DescribeSnapshots(params) if err != nil { fmt.Println(err) } - - for _, n := range result.Snapshots { - fmt.Println(n.SnapshotId) - if n.SnapshotId != nil { - fmt.Println(*n.SnapshotId) + var actualCount int + if snapshotCheck.EnableCSI { + for _, snapshotId := range snapshotCheck.SnapshotIDList { + for _, n := range result.Snapshots { + if n.SnapshotId != nil && (*n.SnapshotId == snapshotId) { + actualCount++ + fmt.Printf("SnapshotId: %v, Tags: %v \n", *n.SnapshotId, n.Tags) + if n.VolumeId != nil { + fmt.Printf("VolumeId: %v \n", *n.VolumeId) + } + } + } } - fmt.Println(n.Tags) - fmt.Println(n.VolumeId) - if n.VolumeId != nil { - fmt.Println(*n.VolumeId) - } - } - if len(result.Snapshots) != snapshotCheck.ExpectCount { - return errors.New(fmt.Sprintf("Snapshot count is not as expected %d", snapshotCheck.ExpectCount)) } else { - fmt.Printf("Snapshot count %d is as expected %d\n", len(result.Snapshots), snapshotCheck.ExpectCount) + for _, n := range result.Snapshots { + if n.SnapshotId != nil { + fmt.Printf("SnapshotId: %v, Tags: %v \n", *n.SnapshotId, n.Tags) + if n.VolumeId != nil { + fmt.Printf("VolumeId: %v \n", *n.VolumeId) + } + } + } + actualCount = len(result.Snapshots) + } + if actualCount != snapshotCheck.ExpectCount { + return errors.New(fmt.Sprintf("Snapshot count %d is not as expected %d", actualCount, snapshotCheck.ExpectCount)) + } else { + fmt.Printf("Snapshot count %d is as expected %d\n", actualCount, snapshotCheck.ExpectCount) return nil } + } func (s AWSStorage) GetMinioBucketSize(cloudCredentialsFile, bslBucket, bslPrefix, bslConfig string) (int64, error) { diff --git a/test/util/velero/velero_utils.go b/test/util/velero/velero_utils.go index f256aa6f1..dba18f217 100644 --- a/test/util/velero/velero_utils.go +++ b/test/util/velero/velero_utils.go @@ -35,11 +35,9 @@ import ( "github.com/pkg/errors" "golang.org/x/exp/slices" - "k8s.io/apimachinery/pkg/util/wait" - - kbclient "sigs.k8s.io/controller-runtime/pkg/client" - ver "k8s.io/apimachinery/pkg/util/version" + "k8s.io/apimachinery/pkg/util/wait" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" cliinstall "github.com/vmware-tanzu/velero/pkg/cmd/cli/install" @@ -1201,7 +1199,8 @@ func GetSnapshotCheckPoint(client TestClient, VeleroCfg VeleroConfig, expectCoun snapshotCheckPoint.ExpectCount = expectCount snapshotCheckPoint.NamespaceBackedUp = namespaceBackedUp snapshotCheckPoint.PodName = KibishiiPVCNameList - if VeleroCfg.CloudProvider == "azure" && strings.EqualFold(VeleroCfg.Features, "EnableCSI") { + + if (VeleroCfg.CloudProvider == "azure" || VeleroCfg.CloudProvider == "aws") && strings.EqualFold(VeleroCfg.Features, "EnableCSI") { snapshotCheckPoint.EnableCSI = true resourceName := "snapshot.storage.k8s.io"