feat: volume clone from source volume #426

wozniakjan · 2023-03-09T15:46:54Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR introduces a first step towards supporting snapshots - volume cloning from other volumes. It's implemented as outlined in #30 (comment), and it is just an invocation of cp -a [src]/. [dst] to recursively copy the content of the source volume to the destination volume and adequately treat soft and hard links.

I would like to first get a consensus on the design decisions and then I can follow up with snapshots and volume copy from snapshots.

Which issue(s) this PR fixes:

first step towards implementing #31

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

feat: volume clone from source volume

wozniakjan · 2023-03-09T15:56:57Z

pkg/nfs/controllerserver.go

+	var volCap *csi.VolumeCapability
+	if len(req.GetVolumeCapabilities()) > 0 {
+		volCap = req.GetVolumeCapabilities()[0]
+	}


I am not sure if req capabilities apply for both src and dst volume internal mounting. I took this from here:

csi-driver-nfs/pkg/nfs/controllerserver.go

Lines 120 to 123 in 5edf03c

var volCap *csi.VolumeCapability

if len(req.GetVolumeCapabilities()) > 0 {

volCap = req.GetVolumeCapabilities()[0]

}

coveralls · 2023-03-09T16:29:40Z

Pull Request Test Coverage Report for Build 4417866944

3 of 48 (6.25%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-4.05%) to 73.278%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/nfs/controllerserver.go	2	47	4.26%

Totals
Change from base Build 4370941627:	-4.05%
Covered Lines:	617
Relevant Lines:	842

💛 - Coveralls

pkg/nfs/controllerserver.go

andyzhangx

this PR lgtm, could you also write an e2e test for volume cloning, you could refer to https://github.com/kubernetes-sigs/azuredisk-csi-driver/blob/master/test/e2e/dynamic_provisioning_test.go#L539-L570

andyzhangx · 2023-03-12T02:46:33Z

this PR lgtm, could you also write an e2e test for volume cloning, you could refer to https://github.com/kubernetes-sigs/azuredisk-csi-driver/blob/master/test/e2e/dynamic_provisioning_test.go#L539-L570

there is an easier way to add volume cloning test, just add pvcDataSource: true in external e2e test:

csi-driver-nfs/test/external-e2e/testdriver.yaml

Lines 9 to 14 in af29ce4

    
           Capabilities: 
        
             persistence: true 
        
             exec: true 
        
             multipods: true 
        
             RWX: true 
        
             fsGroup: true

wozniakjan · 2023-03-13T10:40:50Z

/retest

maybe someone else should trigger @k8s-ci-robot, all of these tests currently fail with Unauthorized

Pod can not be created: create pod test-pod ... e5ab5e009e in cluster default: Unauthorized BaseSHA:af29ce4d9ff5396cc22cb6c1e18bddcfab23338f

andyzhangx · 2023-03-13T11:19:38Z

/retest

andyzhangx · 2023-03-13T11:54:51Z

/ok-to-test

andyzhangx · 2023-03-13T11:55:05Z

/retest

wozniakjan · 2023-03-14T09:04:46Z

/retest
maybe yesterday was just flaky?

wozniakjan · 2023-03-14T09:22:24Z

the failure in pull-csi-driver-nfs-sanity relates to the changes introduced here:

E0314 09:08:26.406775    7781 utils.go:93] GRPC error: rpc error: code = NotFound desc = could not split fake-vol-id-f600e740-a into server, baseDir and subDir with separator(/)

the volumeID tests are trying to use was fake-vol-id-f600e740-a which doesn't comply with the expected volumeID as {nfs-server-address}#{sub-dir-name}#{share-name}, I will see if there is a way to bend the tests to comply with our expectations (or maybe write custom copy volume test as described in #426 (review))

for pull-csi-driver-nfs-integration, the error is

Package nfs-common is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source

E: Package 'nfs-common' has no installation candidate

seems unrelated, maybe debian package name changed? I will take a look

k8s-ci-robot · 2023-03-14T12:00:03Z

@wozniakjan: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-csi-driver-nfs-e2e
/test pull-csi-driver-nfs-external-e2e
/test pull-csi-driver-nfs-integration
/test pull-csi-driver-nfs-sanity
/test pull-csi-driver-nfs-unit
/test pull-kubernetes-csi-csi-driver-nfs

Use /test all to run all jobs.

In response to this:

/retest pull-csi-driver-nfs-integration

this seems to work just fine, another transient glitch in the matrix?

$ $ docker run registry.k8s.io/build-image/debian-base:bullseye-v1.4.2  sh -c "apt update && apt upgrade -y && apt-mark unhold libcap2 && clean-install ca-certificates mount nfs-common netbase"
...
Updating certificates in /etc/ssl/certs...
0 added, 0 removed; done.
Running hooks in /etc/ca-certificates/update.d...
done.

$ echo $?
0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wozniakjan · 2023-03-14T12:23:50Z

Package nfs-common is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source

E: Package 'nfs-common' has no installation candidate

/test pull-csi-driver-nfs-integration

this seems to work just fine, another transient glitch in the matrix?

$ $ docker run registry.k8s.io/build-image/debian-base:bullseye-v1.4.2  sh -c "apt update && apt upgrade -y && apt-mark unhold libcap2 && clean-install ca-certificates mount nfs-common netbase"
...
Updating certificates in /etc/ssl/certs...
0 added, 0 removed; done.
Running hooks in /etc/ca-certificates/update.d...
done.

$ echo $?
0

andyzhangx · 2023-03-14T13:37:55Z

sanity test is failing in clone volume test:
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-driver-nfs/426/pull-csi-driver-nfs-sanity/1635567645178728448/build-log.txt

I0314 09:08:26.081201    7781 utils.go:88] GRPC call: /csi.v1.Controller/CreateVolume
I0314 09:08:26.081280    7781 utils.go:89] GRPC request: {"capacity_range":{"limit_bytes":10737418240,"required_bytes":10737418240},"name":"sanity-controller-vol-from-vol-1BFA11DF-89190C95","parameters":{"server":"127.0.0.1","share":"/"},"volume_capabilities":[{"AccessType":{"Mount":{}},"access_mode":{"mode":1}}],"volume_content_source":{"Type":{"Volume":{"volume_id":"127.0.0.1##sanity-controller-source-vol-1BFA11DF-89190C95#"}}}}
I0314 09:08:26.081693    7781 controllerserver.go:295] internally mounting 127.0.0.1:/ at /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95
I0314 09:08:26.081879    7781 nodeserver.go:123] NodePublishVolume: volumeID(127.0.0.1##sanity-controller-vol-from-vol-1BFA11DF-89190C95#) source(127.0.0.1:/) targetPath(/tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95) mountflags([])
I0314 09:08:26.081911    7781 mount_linux.go:220] Mounting cmd (mount) with arguments (-t nfs 127.0.0.1:/ /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95)
I0314 09:08:26.107444    7781 nodeserver.go:140] skip chmod on targetPath(/tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95) since mountPermissions is set as 0
I0314 09:08:26.107489    7781 nodeserver.go:142] volume(127.0.0.1##sanity-controller-vol-from-vol-1BFA11DF-89190C95#) mount 127.0.0.1:/ on /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95 succeeded
I0314 09:08:26.108825    7781 controllerserver.go:325] copy volume from volume /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C95 -> /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95/sanity-controller-vol-from-vol-1BFA11DF-89190C95
I0314 09:08:26.108859    7781 controllerserver.go:295] internally mounting 127.0.0.1:/ at /tmp/sanity-controller-source-vol-1BFA11DF-89190C95
I0314 09:08:26.108993    7781 nodeserver.go:123] NodePublishVolume: volumeID(127.0.0.1##sanity-controller-source-vol-1BFA11DF-89190C95#) source(127.0.0.1:/) targetPath(/tmp/sanity-controller-source-vol-1BFA11DF-89190C95) mountflags([])
I0314 09:08:26.109020    7781 mount_linux.go:220] Mounting cmd (mount) with arguments (-t nfs 127.0.0.1:/ /tmp/sanity-controller-source-vol-1BFA11DF-89190C95)
I0314 09:08:26.174764    7781 nodeserver.go:140] skip chmod on targetPath(/tmp/sanity-controller-source-vol-1BFA11DF-89190C95) since mountPermissions is set as 0
I0314 09:08:26.174875    7781 nodeserver.go:142] volume(127.0.0.1##sanity-controller-source-vol-1BFA11DF-89190C95#) mount 127.0.0.1:/ on /tmp/sanity-controller-source-vol-1BFA11DF-89190C95 succeeded
I0314 09:08:26.174944    7781 controllerserver.go:295] internally mounting 127.0.0.1:/ at /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95
I0314 09:08:26.178668    7781 controllerserver.go:351] copied /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C95 -> /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95/sanity-controller-vol-from-vol-1BFA11DF-89190C95 output: cp: cannot stat '/tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C9547.': No such file or directory

andyzhangx · 2023-03-14T13:41:54Z

pkg/nfs/controllerserver.go

+
+	// recursive 'cp' with '-a' to handle symlinks. Note that the source path must include trailing '/.',
+	// which is the reason why 'filepath.Join()' is not used as it would perform path cleaning
+	out, err := exec.Command("cp", "-a", fmt.Sprintf("%v%v.", srcPath, filepath.Separator), dstPath).CombinedOutput()


per the sanity test failure,

controllerserver.go:351] copied /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C95 -> /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95/sanity-controller-vol-from-vol-1BFA11DF-89190C95 output: cp: cannot stat '/tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C9547.': No such file or directory

it should be cp -a /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/* /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95/ ?

I think it is expected given the implementation of getInternalVolumePath() because volume has a subdir

I0314 09:08:26.078270 7781 utils.go:95] GRPC response: {"volume":{"volume_context":{"server":"127.0.0.1","share":"/","subdir":"sanity-controller-source-vol-1BFA11DF-89190C95"},"volume_id":"127.0.0.1##sanity-controller-source-vol-1BFA11DF-89190C95#"}}

csi-driver-nfs/pkg/nfs/controllerserver.go

Lines 367 to 374 in af29ce4

// Get internal path where the volume is created

// The reason why the internal path is "workingDir/subDir/subDir" is because:

// - the semantic is actually "workingDir/volId/subDir" and volId == subDir.

// - we need a mount directory per volId because you can have multiple

// CreateVolume calls in parallel and they may use the same underlying share.

// Instead of refcounting how many CreateVolume calls are using the same

// share, it's simpler to just do a mount per request.

func getInternalVolumePath(workingMountDir string, vol *nfsVolume) string {

actually, I think I see it now

I0314 09:08:26.178668 7781 controllerserver.go:351] copied /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C95 -> /tmp/sanity-controller-vol-from-vol-1BFA11DF-89190C95/sanity-controller-vol-from-vol-1BFA11DF-89190C95 output: cp: cannot stat '/tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C9547.': No such file or directory

there should not be any path terminated with just ., only /.,

FROM TEST: /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C9547. EXP SRC: /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C9547/. EXP DST: /tmp/sanity-controller-source-vol-1BFA11DF-89190C95/sanity-controller-source-vol-1BFA11DF-89190C9547

looks like filepath.Separator was an empty string?

trying hardcoded trailing /. for src path and slightly improved the logging in 784da6f

tests are passing now, looks like using / instead of filepath.Separator did the trick.

@andyzhangx I am not sure if that is a big issue for portability. Technically using cp imho already restricts this to run with the expectation of POSIX file paths. Supporting POSIX and windows separator may not have much of an impact from the OS portability point of view.

andyzhangx

/lgtm
thanks for the contribution!

k8s-ci-robot · 2023-03-15T16:04:48Z

[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:

~~OWNERS~~ [andyzhangx]

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

pkg/nfs/controllerserver.go

followup to kubernetes-csi#426

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 9, 2023

wozniakjan mentioned this pull request Mar 9, 2023

Is it possible to implement snapshot functionality for nfs csi driver #31

Closed

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 9, 2023

k8s-ci-robot requested review from gnufied and pohly March 9, 2023 15:54

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 9, 2023

wozniakjan commented Mar 9, 2023

View reviewed changes

wozniakjan force-pushed the volume_to_volume_copy branch 2 times, most recently from 75ab0ff to 52620bf Compare March 9, 2023 16:13

wozniakjan marked this pull request as draft March 9, 2023 16:34

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 9, 2023

wozniakjan commented Mar 10, 2023

View reviewed changes

pkg/nfs/controllerserver.go Outdated Show resolved Hide resolved

wozniakjan force-pushed the volume_to_volume_copy branch from 52620bf to 96c3078 Compare March 10, 2023 15:28

andyzhangx reviewed Mar 11, 2023

View reviewed changes

wozniakjan force-pushed the volume_to_volume_copy branch from 96c3078 to 88b7cda Compare March 13, 2023 10:18

wozniakjan added 2 commits March 13, 2023 11:19

feat: volume clone from source volume

bfe7fe4

test: execute datasource e2e tests

61a6301

wozniakjan force-pushed the volume_to_volume_copy branch from 88b7cda to 61a6301 Compare March 13, 2023 10:20

wozniakjan marked this pull request as ready for review March 13, 2023 10:22

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 13, 2023

k8s-ci-robot requested review from mauriciopoppe and xing-yang March 13, 2023 10:22

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 13, 2023

andyzhangx reviewed Mar 14, 2023

View reviewed changes

feat: volume copy src dir fix

784da6f

wozniakjan force-pushed the volume_to_volume_copy branch from 910ee3c to 784da6f Compare March 14, 2023 16:14

andyzhangx approved these changes Mar 15, 2023

View reviewed changes

k8s-ci-robot assigned andyzhangx Mar 15, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2023

k8s-ci-robot merged commit cd50d48 into kubernetes-csi:master Mar 15, 2023

wozniakjan mentioned this pull request Mar 18, 2023

feat: implement snapshots as tarballs #430

Merged

andyzhangx reviewed Mar 19, 2023

View reviewed changes

pkg/nfs/controllerserver.go Show resolved Hide resolved

pkg/nfs/controllerserver.go Show resolved Hide resolved

wozniakjan added a commit to wozniakjan/csi-driver-nfs that referenced this pull request Mar 21, 2023

test: copy volume from volume unit tests

705819f

followup to kubernetes-csi#426

wozniakjan mentioned this pull request Mar 21, 2023

test: copy volume from volume unit tests #433

Merged

wozniakjan added a commit to wozniakjan/csi-driver-nfs that referenced this pull request Mar 21, 2023

test: copy volume from volume unit tests

1507f51

followup to kubernetes-csi#426

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: volume clone from source volume #426

feat: volume clone from source volume #426

wozniakjan commented Mar 9, 2023 •

edited

Loading

wozniakjan Mar 9, 2023

coveralls commented Mar 9, 2023 •

edited

Loading

andyzhangx left a comment

andyzhangx commented Mar 12, 2023

wozniakjan commented Mar 13, 2023 •

edited

Loading

andyzhangx commented Mar 13, 2023

andyzhangx commented Mar 13, 2023

andyzhangx commented Mar 13, 2023

wozniakjan commented Mar 14, 2023

wozniakjan commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

wozniakjan commented Mar 14, 2023 •

edited

Loading

andyzhangx commented Mar 14, 2023

andyzhangx Mar 14, 2023

wozniakjan Mar 14, 2023 •

edited

Loading

wozniakjan Mar 14, 2023 •

edited

Loading

wozniakjan Mar 14, 2023 •

edited

Loading

andyzhangx left a comment

k8s-ci-robot commented Mar 15, 2023

	var volCap *csi.VolumeCapability
	if len(req.GetVolumeCapabilities()) > 0 {
	volCap = req.GetVolumeCapabilities()[0]
	}

	// Get internal path where the volume is created
	// The reason why the internal path is "workingDir/subDir/subDir" is because:
	// - the semantic is actually "workingDir/volId/subDir" and volId == subDir.
	// - we need a mount directory per volId because you can have multiple
	// CreateVolume calls in parallel and they may use the same underlying share.
	// Instead of refcounting how many CreateVolume calls are using the same
	// share, it's simpler to just do a mount per request.
	func getInternalVolumePath(workingMountDir string, vol *nfsVolume) string {

feat: volume clone from source volume #426

feat: volume clone from source volume #426

Conversation

wozniakjan commented Mar 9, 2023 • edited Loading

wozniakjan Mar 9, 2023

Choose a reason for hiding this comment

coveralls commented Mar 9, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4417866944

💛 - Coveralls

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx commented Mar 12, 2023

wozniakjan commented Mar 13, 2023 • edited Loading

andyzhangx commented Mar 13, 2023

andyzhangx commented Mar 13, 2023

andyzhangx commented Mar 13, 2023

wozniakjan commented Mar 14, 2023

wozniakjan commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

wozniakjan commented Mar 14, 2023 • edited Loading

andyzhangx commented Mar 14, 2023

andyzhangx Mar 14, 2023

Choose a reason for hiding this comment

wozniakjan Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

wozniakjan Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

wozniakjan Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 15, 2023

wozniakjan commented Mar 9, 2023 •

edited

Loading

coveralls commented Mar 9, 2023 •

edited

Loading

wozniakjan commented Mar 13, 2023 •

edited

Loading

wozniakjan commented Mar 14, 2023 •

edited

Loading

wozniakjan Mar 14, 2023 •

edited

Loading

wozniakjan Mar 14, 2023 •

edited

Loading

wozniakjan Mar 14, 2023 •

edited

Loading