-
Notifications
You must be signed in to change notification settings - Fork 146
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
Make mount and staging dir creation flexible #159
Make mount and staging dir creation flexible #159
Conversation
}, | ||
} | ||
|
||
sanity.Test(t, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually verify that CreateTargetDir
and CreateStagingDir
are getting used as intended, does it?
Admittedly a thorough check is hard, but perhaps record inside createTargetDir
that it was called and then add some asserts against that after sanity.Test
returns? If you invoke _apitest2/api_test.go
with -test.run
such that it executes a well-known set of tests, it might even become possible to have a deterministic set of created directories to compare against.
// If not defined, directories are created in the default way at TargetPath | ||
// and StagingPath. | ||
CreateTargetDir func(path string) (string, error) | ||
CreateStagingDir func(path string) (string, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with not exposing this functionality in the command line tool.
If someone needs it, we could add it by letting the test command invoke an external tool.
pkg/sanity/sanity.go
Outdated
if sc.Config.CreateStagingDir != nil { | ||
newpath, err := sc.Config.CreateStagingDir(sc.Config.StagingPath) | ||
Expect(err).NotTo(HaveOccurred()) | ||
sc.stagingPath = newpath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always explain what the error is about, see kubernetes/kubernetes#34059. I know, the rest of the code doesn't do that either (would be a good enhancement), but it's never too late to do it better.
This code is rather repetitive. I would change the createMountTargetLocation
method and then use it like this:
sc.targetPath = createMountTargetLocation(sc.Config.TargetPath, sc.Config.CreateTargetDir)
sc.stagingPath = createMountTargetLocation(sc.Config.StagingPath, sc.Config.CreateStagingDir)
Speaking of createMountTargetLocation
: there is a call to it in node.go
which causes directories to be created on the host even when sc.Config.StagingPath
is set. I think that call can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that in scenario we use (staging and target directories are created on VMs where pods run, host acting only as test orchestrator) I see /var/tmp/tmp.XXXXXXXXXX directories created on host which are not used and remain undeleted, both with csi-test 1.0.2 and with current PR change.
Positive feedback is that tests pass same way with current PR changes (as with 1.0.2)
92fd1a2
to
c8ac1b0
Compare
Updated the PR addressing the suggestions. Changed Added more context to the test failure errors and changed the signature of |
pkg/sanity/sanity.go
Outdated
func createMountTargetLocation(targetPath string, customCreateDir func(string) (string, error)) (string, error) { | ||
|
||
// Return the target path if empty. | ||
if len(targetPath) < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty string has length 0, so this check is not getting triggered as intended.
Better compare with if targetPath == ""
. It's more obvious what's meant (always most important) and there's no obvious performance benefit from the length comparison (performance doesn't matter here anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that's really a mistake 😅 . Changed it.
Thanks!
pkg/sanity/sanity.go
Outdated
} else { | ||
fileInfo, err := os.Stat(targetPath) | ||
if err != nil && os.IsNotExist(err) { | ||
return "", os.MkdirAll(targetPath, 0755) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you return an empty string also when os.MkdirAll
succeeds. That can't be right? I wonder why it isn't breaking the Travis CI check. Ah, probably because the mock driver doesn't actually use the directories and doesn't check whether they exist (unverified). Might be worth adding such a check. Perhaps add such a check as part of the PR, verify "make test" fails, then fix this code?
Overall I find this version more readable:
fileInfo, err := os.Stat(targetPath)
if err != nil {
if !os.IsNotExist(err) {
return "", err
}
if err := os.MkdirAll(targetPath, 0755); err != nil {
return "", err
}
}
if !fileInfo.IsDir() {
return "", fmt.Errorf("Target location %s is not a directory", targetPath)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! thanks for noticing that.
I've added new checks in NodePublishVolume and NodeStageVolume to fail if the target paths don't exist. And adopted the above code snippet, this is actually better.
I said earlier that adding support for this to the command line can wait until we have a need for it. I now found a use case, so can you add command line flags for this? They should accept a single parameter, the command to run, and then call that command with the path as argument. stdout of the command is the new path, stderr can go to stderr of the caller (i.e. don't redirect). My use case is for adding sanity testing to kubernetes-csi/csi-driver-host-path#16: when building the hostpath driver (or soon, also other drivers), I want to check out a certain revision of csi-test and run the sanity command against the deployed driver, without having to built a custom test suite. The custom command is then the name of a script which contains Access to the |
I've added command line support for passing external commands that can create the target path. An example usage is in the e2e test file. I realized that it would be better if we change the names of a few flags and variables. Maybe we need to maintain backwards compatibility? |
pkg/sanity/sanity.go
Outdated
} | ||
|
||
func (sc *SanityContext) teardown() { | ||
// Delete the created paths if any. | ||
os.RemoveAll(sc.targetPath) | ||
os.RemoveAll(sc.stagingPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this in teardown because without this, some tests were using the directories created by the previous tests and that was resulting in the directory exists check in createMountTargetLocation()
to be true and hence, not returning a target path even if it was created was passing, because that code block was being skipped.
With this, the directories are deleted for every test and not returning target path from createMountTargetLocation()
results in failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's nicer if the test cleans up after itself, we then also need functions that correspond to CreateStagingDir
and CreateTargetDir
. The main use-case for these functions is for testing a driver that was deployed somewhere, and then those functions will not create the dirs locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! I wasn't thinking in that way. Thanks, I'll add the corresponding cleanup options as well.
This is a good point. The proposed new names make more sense, but then the next release will have to be v2.0.0 due to the API change. I'm a bit undecided myself. @lpabon what do you think? |
700baf2
to
6edd078
Compare
Added support for custom target path removal callback function and command line flag. The same is being used in the e2e test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thanks for seeing this through to the end.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: darkowlzz, pohly 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 |
This change adds an option to attach callback functions to CSI sanity for mount target directory creation. This could be used to customize the target and staging mount path creation. Also, adds `targetPath` and `stagingPath` fields to `SanityContext` to store the actual target and staging paths, derived from the sanity config. Adds an e2e test section to test the above setup.
This adds target path exists checks in the mock driver in NodeStageVolume and NodePublishVolume to ensure that the paths actually exist. Also adds path cleanup in the teardown. This ensures that every test creates and deletes their target directories. Without this, the test setup function for Controller only tests created the target directory and the same was being used by subsequent tests.
This adds three new flags to support custom target path creation: - `createmountpathcmd`: Command to create a custom mount path. - `createstagingpathcmd`: Command to create a custom staging path. - `createpathcmdtimeout`: Timeout for the custom path creation commands. Also adds a new test section in e2e tests to test this custom command support.
This adds three new flags to support custom target path removal: - `removemountpathcmd`: Command to remove a custom mount path. - `removestagingpathcmd`: Command to remove a custom staging path. - `removepathcmdtimeout`: Timeout for the custom path removal commands.
6edd078
to
506d128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
kubernetes-csi#159 introduced the ability to use different paths for each tests via external commands or callbacks, but it missed two lines in the cleanup code which also have to use the actual paths instead of the configured paths.
…dirs Make mount and staging dir creation flexible
kubernetes-csi#159 introduced the ability to use different paths for each tests via external commands or callbacks, but it missed two lines in the cleanup code which also have to use the actual paths instead of the configured paths.
This change adds an option to attach callback functions to CSI sanity
for mount target directory creation. This could be used to customize the
target and staging mount path creation.
Also, adds
targetPath
andstagingPath
fields toSanityContext
tostore the actual target and staging paths, derived from the sanity
config.
Adds an e2e test section to test the above setup.
Fixes #144