Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement snapshots as tarballs #430

Merged

Conversation

wozniakjan
Copy link
Member

@wozniakjan wozniakjan commented Mar 18, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:
This PR proposes simple support for VolumeSnapshots as tarballs created with

tar -C /[src_vol_path] -czvf /[snapshot_path]/[snapshot_name]/[src_vol_name].tar.gz .

The reason why [src_vol_uuid] is included in the snapshot path is to be able to efficiently discover AlreadyExists cases

It is the second out of three intended PRs to provide basic snapshot workflow including volume cloning from snapshots. It is preceded by #426 which implemented volume cloning from other volumes.

Which issue(s) this PR fixes:

Moving forward with snapshots as discussed in #31

Special notes for your reviewer:
Some parts of this PR related to helm chart and external-snapshotter are not ideal. As a followup task, I would like to attempt to address kubernetes-csi/external-snapshotter#812. That way all csi-driver helm charts including this one, instead of embedding external-snapshotter CRDs and controller resources and duplicating them, they could just add the official external-snapshotter chart as a dependency. This should also simplify updating to new releases. But not in scope to be addressed here.

Does this PR introduce a user-facing change?:

feat: volume snapshots

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 18, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 18, 2023
deploy/install-driver.sh Show resolved Hide resolved
pkg/nfs/controllerserver.go Show resolved Hide resolved
pkg/nfs/controllerserver.go Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 18, 2023

Pull Request Test Coverage Report for Build 4597662542

  • 173 of 218 (79.36%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.1%) to 78.426%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/nfs/controllerserver.go 172 217 79.26%
Totals Coverage Status
Change from base Build 4594162046: 1.1%
Covered Lines: 847
Relevant Lines: 1080

💛 - Coveralls

test/external-e2e/testdriver.yaml Outdated Show resolved Hide resolved
deploy/example/snapshotclass-nfs.yaml Outdated Show resolved Hide resolved
@andyzhangx
Copy link
Member

when the main work of this feature is almost done, pls also add some unit tests, ut coverage is decreasing a lot

Overall coverage decreased (-9.8%) to 63.45%

@wozniakjan
Copy link
Member Author

wozniakjan commented Mar 19, 2023

I will turn this into draft for now and undraft when it's reviewable again

I will make this as WIP for now and un-WIP when it's reviewable again

it's reviewable again :)

@wozniakjan wozniakjan marked this pull request as draft March 19, 2023 08:17
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2023
@wozniakjan wozniakjan marked this pull request as ready for review March 20, 2023 08:06
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023
@wozniakjan wozniakjan changed the title feat: implement snapshots as tarballs [WIP] feat: implement snapshots as tarballs Mar 20, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023
@wozniakjan wozniakjan force-pushed the snapshots_with_tarballs branch 5 times, most recently from 3b4adc7 to 826d2f3 Compare March 20, 2023 16:10
@wozniakjan
Copy link
Member Author

wozniakjan commented Mar 20, 2023

hey @andyzhangx, any ideas why external-e2e tests continue to fail with

log
        Mar 20 16:37:35.445: Unexpected error:
            <*errors.StatusError | 0xc003ea4320>: {
                ErrStatus: {
                    TypeMeta: {Kind: "", APIVersion: ""},
                    ListMeta: {
                        SelfLink: "",
                        ResourceVersion: "",
                        Continue: "",
                        RemainingItemCount: nil,
                    },
                    Status: "Failure",
                    Message: "the server could not find the requested resource",
                    Reason: "NotFound",
                    Details: {
                        Name: "",
                        Group: "",
                        Kind: "",
                        UID: "",
                        Causes: [
                            {
                                Type: "UnexpectedServerResponse",
                                Message: "404 page not found",
                                Field: "",
                            },
                        ],
                        RetryAfterSeconds: 0,
                    },
                    Code: 404,
                },
            }
            the server could not find the requested resource
        occurred

        test/e2e/storage/framework/snapshot_resource.go:65

I think the failure is here and iiuc my guess could be that SnapshotClass maybe doesn't exist in the kube-apiserver or maybe the create call sends an empty GVK in the request?
https://github.com/kubernetes/kubernetes/blob/v1.24.0/test/e2e/storage/framework/snapshot_resource.go#L64-L65

maybe this bit is not quite right?

SnapshotClass:
FromName: true

EDIT: for future reference, fixed in 5643e5f and fd1f166, the external-e2e-tests install a helm chart of this driver and expect external-snapshotter CRDs and controller to be part of the chart.

@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 3, 2023

after other PRs merged, there was a go.mod conflict, appended go mod tidy to the PR after rebase

@andyzhangx
Copy link
Member

after other PRs merged, there was a go.mod conflict, appended go mod tidy to the PR after rebase

@wozniakjan could you squash the commits to only a few commits?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2023
@wozniakjan
Copy link
Member Author

@wozniakjan could you squash the commits to only a few commits?

Thanks for pinging, rebased and did the best effort to logically squash commits, ptal @andyzhangx

@wozniakjan wozniakjan force-pushed the snapshots_with_tarballs branch 2 times, most recently from ce8c44e to 089eb0c Compare April 3, 2023 10:31
@smuda
Copy link

smuda commented Apr 5, 2023

Thank you for adding this feature! Will there be a status or event I can listen for to be sure the snapshot has succeeded?

The scenario is shutting down a database pod (to flush to disk), requesting a snapshot and then waiting for it to finish before starting the database pod again.

@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 6, 2023

Will there be a status or event I can listen for to be sure the snapshot has succeeded?

the VolumeSnapshot has status.readyToUse field which is set to true by external-snapshotter controller after the snapshot routine in a specific driver has completed
https://github.com/kubernetes-csi/external-snapshotter/blob/129de4cbb3fc5679285d70d191e66976481082a9/client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml#L167

And just like you are used to with other kubernetes resources, you can run

$ kubectl describe volumesnapshot [name]
$ kubectl describe volumesnapshotcontent [name]

to debug for any errors

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, wozniakjan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit 3f5c566 into kubernetes-csi:master Apr 8, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants