-
Notifications
You must be signed in to change notification settings - Fork 218
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
Copy features from Mock CSI driver #260
Copy features from Mock CSI driver #260
Conversation
Hi @avalluri. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
go.sum
Outdated
@@ -415,11 +414,9 @@ github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5/go.mod h1:v | |||
github.com/onsi/ginkgo v0.0.0-20170829012221-11459a886d9c/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= | |||
github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= | |||
github.com/onsi/ginkgo v1.8.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= | |||
github.com/onsi/ginkgo v1.11.0 h1:JAKSXpt1YjtLA7YpPiqO9ss6sNXEsPfSGdwN0UHqzrw= |
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.
Do the go.sum changes really belong into this commit?
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 result of Go version update, which is not valid anymore.
cmd/hostpathplugin/main.go
Outdated
@@ -41,6 +41,7 @@ var ( | |||
flag.Var(c, "capacity", "Simulate storage capacity. The parameter is <kind>=<quantity> where <kind> is the value of a 'kind' storage class parameter and <quantity> is the total amount of bytes for that kind. The flag may be used multiple times to configure different kinds.") | |||
return c | |||
}() | |||
disableAttach = flag.Bool("disable-attach", false, "Disables RPC_PUBLISH_UNPUBLISH_VOLUME capability.") |
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.
-disable-attach=false
is a double negation, which is harder to understand. Also considering that attach is not really doing anything useful (i.e. it's just for testing), I prefer enable-attach
with false as default.
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.
Agreed, and changed as suggested.
pkg/hostpath/controllerserver.go
Outdated
@@ -99,7 +99,7 @@ func (hp *hostPath) CreateVolume(ctx context.Context, req *csi.CreateVolumeReque | |||
defer hp.mutex.Unlock() | |||
|
|||
capacity := int64(req.GetCapacityRange().GetRequiredBytes()) | |||
topologies := []*csi.Topology{&csi.Topology{ | |||
topologies := []*csi.Topology{{ |
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.
A useful enhancement, but better put it into a separate commit.
Nit: I prefer
topologies := []*csi.Topology{
{Segments: map[string]string{TopologyKeyNode: hp.nodeID}},
}
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.
Moved to a separate commit.
6636544
to
96bf25e
Compare
|
||
var strippedReq fmt.Stringer | ||
v5 := glog.V(5) | ||
if v5 { |
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.
Oh, "glog" literally is github.com/golang/glog. Then the check has to be like this.
Another, separate enhancement would be to replace that with klog/v2.
// convert back to the object, os that logGRPCJson | ||
json.Unmarshal([]byte(strippedReq.String()), req) | ||
json.Unmarshal([]byte(strippedResp.String()), resp) | ||
logGRPCJson(info.FullMethod, req, resp, err) |
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.
If this worked, it would modify "resp", the value returned to the caller. We can't do that.
But I also don't think it actually works as intended: json.Umarshal
just adds values to the object. It does not clear unset ones. That means that secrets remain set.
I think we shouldn't strip secrets in the JSON output. The normal output demonstrates how to do it properly (thus satisfying the goal of showing how to write CSI drivers) and the JSON output is for debugging where we actually may want to see the secrets.
To be on the safe side, we can add a warning to the main README, next to the warning that this driver is just a demo.
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.
If this worked, it would modify "resp", the value returned to the caller. We can't do that.
But I also don't think it actually works as intended:
json.Umarshal
just adds values to the object. It does not clear unset ones. That means that secrets remain set.
Yes, you are right. It's my bad I ignored this completely. I should have written the unmarshalled into a new copy.
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 think we shouldn't strip secrets in the JSON output. The normal output demonstrates how to do it properly (thus satisfying the goal of showing how to write CSI drivers) and the JSON output is for debugging where we actually may want to see the secrets.
To be on the safe side, we can add a warning to the main README, next to the warning that this driver is just a demo.
Changed as suggested to log secret fields in JSON format. And also added a README warning.
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.
For my understanding, how come we can't strip secrets here? This is to handle the mock e2e tests that are verifying logs?
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.
Two reason:
- It's technically complicated because protosanitizer.StripSecrets does not construct new objects, it just wraps the existing ones with a custom String implementation. Therefore a simple
json.Marshal(protosanitizer.StripSecrets(resp))
will still include secrets because it reads fields directly and more complicated code would be needed. - This is indeed for verification in mock e2e tests. I don't know whether we have any which look at secrets, but we might, so conceptually it seems better to me to include secrets.
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.
Thanks, can we document this in the code?
It is also a major change from previous behavior. Would it make sense that we make this a new major release of the hostpath driver with some big warnings that this driver should no longer be used as an example for writing a driver or running in production (however unlikely we think it may be).
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.
A major version bump indeed seems appropriate. Let's see whether I can add the label as a reminder:
/kind api-change
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 comment to the source code:
- It's technically complicated because protosanitizer.StripSecrets does not construct new objects, it just wraps the existing ones with a custom String implementation. Therefore a simple
json.Marshal(protosanitizer.StripSecrets(resp))
will still include secrets because it reads fields directly and more complicated code would be needed.- This is indeed for verification in mock e2e tests. I don't know whether we have any which look at secrets, but we might, so conceptually it seems better to me to include secrets.
a237d13
to
9839b8d
Compare
/retest |
9839b8d
to
529266a
Compare
/lgtm /assign @msau42 For approval. |
// convert back to the object, os that logGRPCJson | ||
json.Unmarshal([]byte(strippedReq.String()), req) | ||
json.Unmarshal([]byte(strippedResp.String()), resp) | ||
logGRPCJson(info.FullMethod, req, resp, err) |
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.
For my understanding, how come we can't strip secrets here? This is to handle the mock e2e tests that are verifying logs?
@@ -41,6 +46,8 @@ var ( | |||
flag.Var(c, "capacity", "Simulate storage capacity. The parameter is <kind>=<quantity> where <kind> is the value of a 'kind' storage class parameter and <quantity> is the total amount of bytes for that kind. The flag may be used multiple times to configure different kinds.") | |||
return c | |||
}() | |||
enableAttach = flag.Bool("enable-attach", false, "Enables RPC_PUBLISH_UNPUBLISH_VOLUME capability.") | |||
proxyEndpoint = flag.String("proxy-endpoint", "", "Instead of running the CSI driver code, just proxy connections from csiEndpoint to the given listening socket.") |
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.
Do we need some documentation on how to use this proxyEndpoint?
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't imagine who besides us in k/k e2e test would want to use this, so I don't think that we need end-user documentation in the README for this. For those who want to know more, there's always the source code...
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 will disagree that just because something is for internal testing we shouldn't document it (and note it as such). It doesn't have to be elaborate, and even just a pointer to the tests using it could be enough.
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.
Where would you put that?
The main README.md has no documentation whatsoever about driver command line parameters. I'm really not keen on having to document all existing parameters and adding just this one there would be inconsistent. Documenting all parameters is fine and appropriate for the sidecars, but for this driver I still think it is overkill.
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.
A comment above this flag is fine with me.
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.
@avalluri can you add this section after "Pre-requisite"?
Features
The driver can provide empty directories that are backed by the same filesystem as EmptyDir volumes. In addition, it can provide raw block volumes that are backed by a single file in that same filesystem and bound to a loop device.
Various command line parameters influence the behavior of the driver. This is relevant in particular for the end-to-end testing that this driver is used for in Kubernetes.
Usually, the driver implements all CSI operations itself. When deployed with the -proxy-endpoint
parameter, it instead proxies all incoming connections for a CSI driver that is embedded inside the Kubernetes E2E test suite and used for mocking a CSI driver with callbacks provided by certain tests.
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.
@avalluri can you add this section after "Pre-requisite"?
Done. Added the 'Features' section.
README.md
Outdated
@@ -2,6 +2,10 @@ | |||
|
|||
This repository hosts the CSI Hostpath driver and all of its build and dependent configuration files to deploy the driver. | |||
|
|||
--- | |||
*WARNING: This driver is just a demo implementation. |
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 would add that this driver is used for CI testing and has many fake implementations and other non-standard best practices, and should not be used as an example of how to write a real driver.
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.
Agreed.
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 rephrased the warning in the ReadME as @msau42 suggested:
*WARNING: This driver is just a demo implementation and is used for CI testing. This has many fake implementations and other non-standard best practices, and should not be used as an example of how to write a real driver.
@@ -41,6 +46,8 @@ var ( | |||
flag.Var(c, "capacity", "Simulate storage capacity. The parameter is <kind>=<quantity> where <kind> is the value of a 'kind' storage class parameter and <quantity> is the total amount of bytes for that kind. The flag may be used multiple times to configure different kinds.") | |||
return c | |||
}() | |||
enableAttach = flag.Bool("enable-attach", false, "Enables RPC_PUBLISH_UNPUBLISH_VOLUME capability.") | |||
proxyEndpoint = flag.String("proxy-endpoint", "", "Instead of running the CSI driver code, just proxy connections from csiEndpoint to the given listening socket.") |
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 will disagree that just because something is for internal testing we shouldn't document it (and note it as such). It doesn't have to be elaborate, and even just a pointer to the tests using it could be enough.
// convert back to the object, os that logGRPCJson | ||
json.Unmarshal([]byte(strippedReq.String()), req) | ||
json.Unmarshal([]byte(strippedResp.String()), resp) | ||
logGRPCJson(info.FullMethod, req, resp, err) |
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.
Thanks, can we document this in the code?
It is also a major change from previous behavior. Would it make sense that we make this a new major release of the hostpath driver with some big warnings that this driver should no longer be used as an example for writing a driver or running in production (however unlikely we think it may be).
Type name is not required when initializing an array elements.
529266a
to
116d82b
Compare
db8b0a5
to
ce8f09f
Compare
Added new commandline argument `--enable-attach` that defauls to `false`. When set to `true`, the host-path plugin implements Controller{Publish,Unpublish}Volume calls.
Some of the new comments could be improved, but I am okay with letting that stand as it is, so /lgtm from me. |
/retest |
@@ -41,6 +46,8 @@ var ( | |||
flag.Var(c, "capacity", "Simulate storage capacity. The parameter is <kind>=<quantity> where <kind> is the value of a 'kind' storage class parameter and <quantity> is the total amount of bytes for that kind. The flag may be used multiple times to configure different kinds.") | |||
return c | |||
}() | |||
enableAttach = flag.Bool("enable-attach", false, "Enables RPC_PUBLISH_UNPUBLISH_VOLUME capability.") | |||
proxyEndpoint = flag.String("proxy-endpoint", "", "Instead of running the CSI driver code, just proxy connections from csiEndpoint to the given listening socket.") |
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.
A comment above this flag is fine with me.
@@ -58,7 +65,30 @@ func main() { | |||
fmt.Fprintln(os.Stderr, "Deprecation warning: The ephemeral flag is deprecated and should only be used when deploying on Kubernetes 1.15. It will be removed in the future.") | |||
} | |||
|
|||
driver, err := hostpath.NewHostPathDriver(*driverName, *nodeID, *endpoint, *ephemeral, *maxVolumesPerNode, version, capacity) | |||
if *proxyEndpoint != "" { |
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.
General question, it seems like this proxy is standalone and doesn't use any methods of the hostpath driver. Would it make sense to have it be a separate binary?
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.
We discussed various options when adding this functionality to the mock driver binary; the situation was the same. We considered the whole range (separate repo, separate binary in its own image, separate binary in the same image, new feature in an existing binary and image) and in the end agreed that the last option was the simplest approach.
Everything else comes with additional overhead, both for managing the releases and also regarding image sizes needed during testing (Go runtime only linked onced, same image used for multiple tests).
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.
A comment above this flag is fine with me.
@msau42 Added the comment explaining the usage of this nuew flag.
ce8f09f
to
8a3b3c5
Compare
This is a code lift from mock driver in favour of replacing mock driver with hostpath driver in Kubernetes e2e testing. kubernetes-csi/csi-test@ef07baf
This section briefly documents the features provided by the hostpath driver.
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! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avalluri, msau42, 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 |
98f23071 Merge pull request kubernetes-csi#260 from TerryHowe/update-csi-driver-version e9d8712d Merge pull request kubernetes-csi#259 from stmcginnis/deprecated-kind-kube-root faf79ff6 Remove --kube-root deprecated kind argument 734c2b95 Merge pull request kubernetes-csi#265 from Rakshith-R/consider-main-branch f95c855b Merge pull request kubernetes-csi#262 from huww98/golang-toolchain 3c8d966f Treat main branch as equivalent to master branch e31de525 Merge pull request kubernetes-csi#261 from huww98/golang fd153a9e Bump golang to 1.23.1 a8b3d050 pull-test.sh: fix "git subtree pull" errors 6b05f0fc use new GOTOOLCHAIN env to manage go version 18b6ac6d chore: update CSI driver version to 1.15 227577e Merge pull request kubernetes-csi#258 from gnufied/enable-race-detection e1ceee2 Always enable race detection while running tests 988496a Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c6 chore: bump to Go 1.22.5 69bd71e chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process git-subtree-dir: release-tools git-subtree-split: 98f23071d946dd3de3188a7e1f84679067003162
406a79ac Merge pull request kubernetes-csi#267 from huww98/gomodcache 9cec273d Set GOMODCACHE to avoid re-download toolchain 98f23071 Merge pull request kubernetes-csi#260 from TerryHowe/update-csi-driver-version e9d8712d Merge pull request kubernetes-csi#259 from stmcginnis/deprecated-kind-kube-root faf79ff6 Remove --kube-root deprecated kind argument 734c2b95 Merge pull request kubernetes-csi#265 from Rakshith-R/consider-main-branch f95c855b Merge pull request kubernetes-csi#262 from huww98/golang-toolchain 3c8d966f Treat main branch as equivalent to master branch e31de525 Merge pull request kubernetes-csi#261 from huww98/golang fd153a9e Bump golang to 1.23.1 a8b3d050 pull-test.sh: fix "git subtree pull" errors 6b05f0fc use new GOTOOLCHAIN env to manage go version 18b6ac6d chore: update CSI driver version to 1.15 227577e Merge pull request kubernetes-csi#258 from gnufied/enable-race-detection e1ceee2 Always enable race detection while running tests 988496a Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c6 chore: bump to Go 1.22.5 69bd71e chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process git-subtree-dir: release-tools git-subtree-split: 406a79acf021b5564108afebeea7d0ed44648d3f
04965932 Merge pull request kubernetes-csi#268 from huww98/cloudbuild 119aee1f Merge pull request kubernetes-csi#266 from jsafrane/bump-sanity-5.3.1 0ae5e52d Update cloudbuild image with go 1.21+ 406a79ac Merge pull request kubernetes-csi#267 from huww98/gomodcache 9cec273d Set GOMODCACHE to avoid re-download toolchain 98f23071 Merge pull request kubernetes-csi#260 from TerryHowe/update-csi-driver-version e9d8712d Merge pull request kubernetes-csi#259 from stmcginnis/deprecated-kind-kube-root faf79ff6 Remove --kube-root deprecated kind argument 734c2b95 Merge pull request kubernetes-csi#265 from Rakshith-R/consider-main-branch 43bde065 Bump csi-sanity to 5.3.1 f95c855b Merge pull request kubernetes-csi#262 from huww98/golang-toolchain 3c8d966f Treat main branch as equivalent to master branch e31de525 Merge pull request kubernetes-csi#261 from huww98/golang fd153a9e Bump golang to 1.23.1 a8b3d050 pull-test.sh: fix "git subtree pull" errors 6b05f0fc use new GOTOOLCHAIN env to manage go version 18b6ac6d chore: update CSI driver version to 1.15 227577e Merge pull request kubernetes-csi#258 from gnufied/enable-race-detection e1ceee2 Always enable race detection while running tests 988496a Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c6 chore: bump to Go 1.22.5 69bd71e chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process git-subtree-dir: release-tools git-subtree-split: 04965932661b6e62709dcdbb9c25da528bac2605
04965932 Merge pull request kubernetes-csi#268 from huww98/cloudbuild 119aee1f Merge pull request kubernetes-csi#266 from jsafrane/bump-sanity-5.3.1 0ae5e52d Update cloudbuild image with go 1.21+ 406a79ac Merge pull request kubernetes-csi#267 from huww98/gomodcache 9cec273d Set GOMODCACHE to avoid re-download toolchain 98f23071 Merge pull request kubernetes-csi#260 from TerryHowe/update-csi-driver-version e9d8712d Merge pull request kubernetes-csi#259 from stmcginnis/deprecated-kind-kube-root faf79ff6 Remove --kube-root deprecated kind argument 734c2b95 Merge pull request kubernetes-csi#265 from Rakshith-R/consider-main-branch 43bde065 Bump csi-sanity to 5.3.1 f95c855b Merge pull request kubernetes-csi#262 from huww98/golang-toolchain 3c8d966f Treat main branch as equivalent to master branch e31de525 Merge pull request kubernetes-csi#261 from huww98/golang fd153a9e Bump golang to 1.23.1 a8b3d050 pull-test.sh: fix "git subtree pull" errors 6b05f0fc use new GOTOOLCHAIN env to manage go version 18b6ac6d chore: update CSI driver version to 1.15 227577e Merge pull request kubernetes-csi#258 from gnufied/enable-race-detection e1ceee2 Always enable race detection while running tests 988496a Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c6 chore: bump to Go 1.22.5 69bd71e chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process git-subtree-dir: release-tools git-subtree-split: 04965932661b6e62709dcdbb9c25da528bac2605
What type of PR is this?
/kind feature
What this PR does / why we need it:
This the first step towards the goal of replacing the Mock CSI driver with host-path driver in Kubernetes E2E testing as described in #247
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: