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

Lay foundations for re-enabling windows tests: Start CSI-Proxy as a background Job. #122

Merged
merged 1 commit into from
Sep 28, 2020

Conversation

boddumanohar
Copy link
Contributor

@boddumanohar boddumanohar commented Sep 25, 2020

What type of PR is this?

/kind test

What this PR does / why we need it:
Currently we are skipping a lot of test cases in windows. This is because using a fakeMounter to test on windows is causing isues. This PR lays a basic foundations to run windows based tests related to node by running CSI-Proxy as a background job and the all the mount related operation will be handled by this proxy. For more in-depth design details for CSI-Proxy, please refer to the design doc: https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190714-windows-csi-support.md

Which issue(s) this PR fixes:

Requirements:

Special notes for your reviewer:

Added a script to .github.com/workflows/windows.yml that downloads and starts CSI-Proxy as windows background job.

Release note:

none

@k8s-ci-robot
Copy link
Contributor

@boddumanohar: The label(s) kind/test cannot be applied, because the repository doesn't have them

In response to this:

What type of PR is this?

/kind test

What this PR does / why we need it:
Currently we are skipping a lot of test cases in windows. This is because using a fakeMounter to test on windows is causing isues. This PR lays a basic foundations to run windows based tests related to node by running CSI-Proxy as a background job and the all the mount related operation will be handled by this proxy. For more in-depth design details for CSI-Proxy, please refer to the design doc: https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190714-windows-csi-support.md

Which issue(s) this PR fixes:

Fixes #121

Requirements:

Special notes for your reviewer:

Added a script to .github.com/workflows/windows.yml that downloads and starts CSI-Proxy as windows background job.

Release note:

none

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.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @boddumanohar. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 25, 2020
@coveralls
Copy link

coveralls commented Sep 25, 2020

Pull Request Test Coverage Report for Build 275289520

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.819%

Totals Coverage Status
Change from base Build 268691318: 0.0%
Covered Lines: 620
Relevant Lines: 706

💛 - Coveralls

# start the CSI Proxy before running tests on windows
# TODO: (boddumanohar) CSI-Proxy team are not building the csi-proxy.exe. Currently I am using a binary that I built. In future we should change to offcial repo.
Start-Job -Name CSIProx -ScriptBlock {
Invoke-WebRequest https://github.com/boddumanohar/csi-proxy/releases/download/v0.2.1/csi-proxy.zip -OutFile csi-proxy.zip;
Copy link
Member

Choose a reason for hiding this comment

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

could you use this binary: https://kubernetesartifacts.azureedge.net/csi-proxy/v0.2.0/binaries/csi-proxy.tar.gz ?

@@ -347,6 +393,7 @@ func TestNodeUnpublishVolume(t *testing.T) {
for _, test := range tests {
_, err := d.NodeUnpublishVolume(context.Background(), &test.req)
if !reflect.DeepEqual(err, test.expectedErr) {
fmt.Println(err)
Copy link
Member

Choose a reason for hiding this comment

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

remove this

platform = "windows"
}

fmt.Println(platform)
Copy link
Member

Choose a reason for hiding this comment

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

remove this

}
smbFile := map[string]string{
"linux": "./smb.go",
"windows": "C:\\var\\lib\\kubelet\\smb.go",
Copy link
Member

Choose a reason for hiding this comment

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

why using this path C:\\var\\lib\\kubelet\\ on windows?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason why I am using that path is because, within the CSI-Proxy logs I get this error:

PS C:\Users\manoh\github\csi-proxy\bin> .\csi-proxy.exe -v 9
I0925 19:31:16.912285   17948 main.go:51] Starting CSI-Proxy Server ...
I0925 19:31:16.913292   17948 main.go:52] Version: v0.2.1-2-g25491d0
I0925 19:31:39.611493   17948 server.go:122] calling PathExists with path "C:\\Users\\manoh\\github\\csi-driver-smb\\pkg\\smb\\smg.go"
E0925 19:31:39.611493   17948 server.go:125] failed validatePathWindows path: C:\Users\manoh\github\csi-driver-smb\pkg\smb\smg.go is not within context path: C:\var\lib\kubelet

It explicitly tells here the path is supposed to be C:\\var\\lib\\kubelet\\,

Copy link
Member

Choose a reason for hiding this comment

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

then what about use func like this, that would be more clear in logic:

func getTestPath(path string) string {
	if runtime.GOOS == "windows" {
		return filepath.Join("C:\\var\\lib\\kubelet\\", path)
	}
	return filepath.Join("./", path)
}

@boddumanohar boddumanohar force-pushed the master branch 3 times, most recently from 4b4984e to b1f7740 Compare September 25, 2020 14:57
@@ -241,7 +261,8 @@ func TestNodePublishVolume(t *testing.T) {
TargetPath: smbFile,
StagingTargetPath: sourceTest,
Readonly: true},
expectedErr: status.Errorf(codes.Internal, "Could not mount target \"./smb.go\": mkdir ./smb.go: not a directory"),
expectedErrWindows: nil,
Copy link
Member

Choose a reason for hiding this comment

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

why it's nil on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CSI-Proxy is not returning any error for the above. Here are the CSI-Proxy logs:

I0925 20:14:06.058327    3480 main.go:51] Starting CSI-Proxy Server ...
I0925 20:14:06.058327    3480 main.go:52] Version: v0.2.0-0-ga37f58f
I0925 20:14:11.417233    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\smb.go"
I0925 20:14:11.418235    3480 server.go:220] calling IsMountPoint with path "C:\\var\\lib\\kubelet\\smb.go"
I0925 20:14:11.420241    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\smb.go"
I0925 20:14:11.427243    3480 server.go:172] calling Rmdir with path "C:\\var\\lib\\kubelet\\smb.go"
I0925 20:14:11.428252    3480 server.go:193] calling LinkPath with targetPath "C:\\var\\lib\\kubelet\\smb.go" sourcePath "C:\\var\\lib\\kubelet\\source_test"
I0925 20:14:11.429237    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.430255    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.432234    3480 server.go:172] calling Rmdir with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.438235    3480 server.go:193] calling LinkPath with targetPath "C:\\var\\lib\\kubelet\\target_test" sourcePath "C:\\var\\lib\\kubelet\\error_mount_source"
I0925 20:14:11.439234    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.439234    3480 server.go:220] calling IsMountPoint with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.440235    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.441235    3480 server.go:172] calling Rmdir with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.441235    3480 server.go:193] calling LinkPath with targetPath "C:\\var\\lib\\kubelet\\target_test" sourcePath "C:\\var\\lib\\kubelet\\source_test"
I0925 20:14:11.442243    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\false_is_likely_exist_target"
I0925 20:14:11.443232    3480 server.go:220] calling IsMountPoint with path "C:\\var\\lib\\kubelet\\false_is_likely_exist_target"
I0925 20:14:11.443232    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\false_is_likely_exist_target"
I0925 20:14:11.444234    3480 server.go:172] calling Rmdir with path "C:\\var\\lib\\kubelet\\false_is_likely_exist_target"
I0925 20:14:11.444234    3480 server.go:193] calling LinkPath with targetPath "C:\\var\\lib\\kubelet\\false_is_likely_exist_target" sourcePath "C:\\var\\lib\\kubelet\\source_test"
I0925 20:14:11.445234    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.445234    3480 server.go:220] calling IsMountPoint with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.446233    3480 server.go:122] calling PathExists with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.454239    3480 server.go:172] calling Rmdir with path "C:\\var\\lib\\kubelet\\target_test"
I0925 20:14:11.454239    3480 server.go:193] calling LinkPath with targetPath "C:\\var\\lib\\kubelet\\target_test" sourcePath "C:\\var\\lib\\kubelet\\source_test"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me find out the exact reason for this behavior and will get back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andyzhangx This is because, on windows during Mount operation is called, under the hood we are actually calling linkPath. https://github.com/kubernetes-csi/csi-driver-smb/blob/master/pkg/mounter/safe_mounter_windows.go#L96. Linking a filepath to a directory which will succeed. Where as in unix we are actually performing an mount from file to a folder which will fail.

Do you have any suggestions on how this can fixed?

Copy link
Member

Choose a reason for hiding this comment

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

mount behavior between Linux and Windows is a little different, that's ok. Could you write code like this:

[]struct {
		desc               string
		req                csi.NodePublishVolumeRequest
		expectedErr   	   error
		expectedErrLinux   error
		expectedErrWindows error
	}
...
expectedErr := test.expectedErr
if test.expectedErrWindows != test.expectedErrLinux {
	if runtime.GOOS == "windows" {
		expectedErr = test.expectedErrWindows
	} else {
		expectedErr = test.expectedErrLinux
	}
}

So in most cases, we only need to define expectedErr, and only need to define expectedErrLinux and expectedErrWindows when it's different. That would save test code.

@@ -241,7 +261,8 @@ func TestNodePublishVolume(t *testing.T) {
TargetPath: smbFile,
StagingTargetPath: sourceTest,
Readonly: true},
expectedErr: status.Errorf(codes.Internal, "Could not mount target \"./smb.go\": mkdir ./smb.go: not a directory"),
expectedErrWindows: nil,
Copy link
Member

Choose a reason for hiding this comment

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

mount behavior between Linux and Windows is a little different, that's ok. Could you write code like this:

[]struct {
		desc               string
		req                csi.NodePublishVolumeRequest
		expectedErr   	   error
		expectedErrLinux   error
		expectedErrWindows error
	}
...
expectedErr := test.expectedErr
if test.expectedErrWindows != test.expectedErrLinux {
	if runtime.GOOS == "windows" {
		expectedErr = test.expectedErrWindows
	} else {
		expectedErr = test.expectedErrLinux
	}
}

So in most cases, we only need to define expectedErr, and only need to define expectedErrLinux and expectedErrWindows when it's different. That would save test code.


// For CSI-Proxy(windows), the path has to be absolute and it should be within the context of value passed for
// csi-proxy argument: --kubelet-pod-path. In our tests we are using `C:\var\lib\kubelet\`
getTestPath := func(path string) string {
Copy link
Member

Choose a reason for hiding this comment

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

pls move this function outside(standalone), other test cases would also needs this function, e.g. TestNodeUnpublishVolume

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 26, 2020
Comment on lines 222 to 226
desc string
req csi.NodePublishVolumeRequest
expectedErr error
expectedErrUnix error
expectedErrWindows error
Copy link
Contributor

@mayankshah1607 mayankshah1607 Sep 26, 2020

Choose a reason for hiding this comment

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

We currently have code to differentiate Windows and Linux errors. Try taking a look at - https://github.com/kubernetes-csi/csi-driver-smb/blob/master/test/utils/testutil/testutil.go#L28

This object holds the error messages for both Windows and Linux as a single entity. Also try looking at the following helper methods for assertion.
Why don't we try reusing that instead of re-introducing the same functionality?

Here's an example of this - https://github.com/kubernetes-csi/csi-driver-smb/blob/master/pkg/smb/nodeserver_test.go#L60

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks will update the PR accordingly.

Copy link
Contributor Author

@boddumanohar boddumanohar Sep 26, 2020

Choose a reason for hiding this comment

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

I think using we might have to make some modification to the existing struct:

type TestError struct {
	WindowsError error
	DefaultError error
}

In this struct, we are taking the DefaultError when the WindowsError is nil. But there are cases where we are explicitly asking to check with nil. In such cases its bit difficult to check for errors. So I propose a small modification to the struct to have a fields like:

type TestError struct {
        expectedError error
	WindowsError error
	DefaultError error
}

So that explictly checking for windows error even if its nil is easier.

what are your thoughts on this?

Copy link
Contributor

@mayankshah1607 mayankshah1607 Sep 26, 2020

Choose a reason for hiding this comment

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

@boddumanohar We're actually following the current approach in the azurefile and azuredisk repos for unit testing - https://github.com/kubernetes-sigs/azurefile-csi-driver , https://github.com/kubernetes-sigs/azuredisk-csi-driver

We didn't really have to make changes to the assertion logic there. It seems like you're suggesting this change because of the unexpected error given by TestNodeUnpublishVolume ([Error] Unmount error mocked by IsLikelyNotMountPoint) and TestNodeUnstageVolume ([Error] CleanupMountPoint error mocked by IsLikelyNotMountPoint). IMO, because these are unexpected errors from the CSI proxy, its best if we temporarily skip only these particular test cases on Windows until we figure out why CSI proxy is giving that issue, rather than accept these outliers as desired results without deeper investigation.

I've opened a PR here with the above thoughts, and keeping the same approach as azuredisk and azurefile repos - #123

@boddumanohar @andyzhangx Please let me know your thoughts and opinions, and we can make more changes accordingly. Thanks! :)

Copy link
Member

Choose a reason for hiding this comment

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

I think that's case by case

  • error mocked by IsLikelyNotMountPoint error is likely caused by no fake of IsLikelyNotMountPoint on Linux, so need to fix it on Linux, that may fix the issue?
  • [Error] Mount error mocked by Mount
    • it's caused by different mount behavior between linux and windows, for this case, let's only run on Linux

In general, I think we should use https://github.com/kubernetes-csi/csi-driver-smb/blob/master/test/utils/testutil/testutil.go#L28, it's more clear.

And if there is one case that only Linux would return error(Windows return nil), we could make it separate, e.g. add OnlyRunOnPlatform in test case struct, that would make logic more clear: positive and negative cases should be separated

Copy link
Member

@andyzhangx andyzhangx Sep 27, 2020

Choose a reason for hiding this comment

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

point is TestError should really contain error, if it's nil, then it's not error. if there is one case that only Linux would return error(Windows return nil), we could make it separate, e.g. add OnlyRunOnPlatform in test case struct, that would make logic more clear: positive and negative cases should be separated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Let me try this out.

- name: Build Test
run: |
make smb-windows
- name: Run Windows Unit Tests
run: |
# start the CSI Proxy before running tests on windows
Start-Job -Name CSIProx -ScriptBlock {
Invoke-WebRequest https://kubernetesartifacts.azureedge.net/csi-proxy/v0.2.0/binaries/csi-proxy.tar.gz -OutFile csi-proxy.tar.gz;
Copy link
Member

Choose a reason for hiding this comment

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

use v0.2.1

Comment on lines 342 to 344
errorTarget := getTestPath("error_is_likely_target")
targetFile := getTestPath("abc.go")
targetTest := getTestPath("target_test")
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have utility for generating test paths - https://github.com/kubernetes-csi/csi-driver-smb/blob/master/test/utils/testutil/testutil.go#L62

Can we reuse that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In windows the path is supposed to be within the context of the value that was passed as this an argument to --kubelet-pod-path of csi-proxy binary The default is C:\\var\\lib\\kubelet\\. Since this is a new function, that required within the scope of the file nodeserver_test.go, I created a new function.

@boddumanohar boddumanohar changed the title fix TestNodePublishVolume test cases in Windows platform fix TestNodePublishVolume, TestNodeUnpublishVolume, TestNodeUnstageVolume, TestIsCorruptedDir test cases in Windows platform Sep 26, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 27, 2020
@boddumanohar boddumanohar changed the title fix TestNodePublishVolume, TestNodeUnpublishVolume, TestNodeUnstageVolume, TestIsCorruptedDir test cases in Windows platform Lay foundations for re-enabling windows tests: Start CSI-Proxy as a background Job. Sep 27, 2020
@boddumanohar
Copy link
Contributor Author

Since most of the comments in this PR have been addressed by @mayankshah1607 in his PR#123. I've removed the changes related to tests and kept the original basis of this PR i.e to start CSI Proxy before running windows tests.

@mayankshah1607
Copy link
Contributor

Thanks @boddumanohar !
I will rebase my PR once this one is merged. :)

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
/approve
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Sep 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0dc7e9f into kubernetes-csi:master Sep 28, 2020
andyzhangx pushed a commit to andyzhangx/csi-driver-smb that referenced this pull request May 1, 2022
…e-spellcheck-boilerplate-tests

Add spelling and boilerplate checks
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants