Bump v1beta3 APIs to v1 #155
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/hold
mauriciopoppe
force-pushed
the
bump-to-v1
branch
22 times, most recently
from
708371a to
dbd3518
Compare
mauriciopoppe
force-pushed
the
bump-to-v1
branch
3 times, most recently
from
9c56721 to
b7d9bc8
Compare
mauriciopoppe
force-pushed
the
bump-to-v1
branch
from
b7d9bc8 to
52601c9
Compare
mauriciopoppe
force-pushed
the
bump-to-v1
branch
from
52601c9 to
3bdc732
Compare
mauriciopoppe
force-pushed
the
bump-to-v1
branch
from
3bdc732 to
faf7450
Compare
|
/approve |
k8s-ci-robot
added
the
approved
jingxu97
reviewed
Jun 17, 2021
| go get -u github.com/golang/protobuf/{proto,protoc-gen-go} | ||
| protoc --version | ||
| cd /home/runner/work/csi-proxy/csi-proxy/go/src/github.com/kubernetes-csi/csi-proxy | ||
| API_GROUP=filesystem OLD_API_VERSION=v1beta2 NEW_API_VERSION=v99 scripts/bump-version.sh |
There was a problem hiding this comment.
not very familiar this part, just want to make sure the version number is the right one,
There was a problem hiding this comment.
yes, this is actually new, so to bump the APIs I created the script scripts/bump-version.sh that automated the process which I used for all of the API versions that we needed to bump. I also added a CI script for it, the nice part is that this is also making sure that we don't break the generator code in the future too :)
The version number v99 is the arbitrary but it's ok for CI, it can't be v1 because we already have v1
|
@jingxu97 @ddebroy all the test passed for v1 too! I think this is ready to be merged, could we merge #154 first please? |
|
/unhold #154 is merged too |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jingxu97, mauriciopoppe 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 |
ddebroy
reviewed
Jun 18, 2021
| // If not an IP address, share name has to be a valid DNS name. | ||
| // UNC specifications to local paths or prefix: \\?\ is not allowed. | ||
| // Characters: + [ ] " / : ; | < > , ? * = $ are not allowed. | ||
| string remote_path = 1; |
There was a problem hiding this comment.
nit: how about a space here to stay consistent?
| service Volume { | ||
| // ListVolumesOnDisk returns the volume IDs (in \\.\Volume{GUID} format) for all volumes from a | ||
| // given disk number and partition number (optional) | ||
| rpc ListVolumesOnDisk(ListVolumesOnDiskRequest) returns (ListVolumesOnDiskResponse) {} |
There was a problem hiding this comment.
nit: let's add a single line space between the rpcs here and the message members below to stay consistent with the rest of the APIs.
| return autoConvert_impl_ListDiskLocationsRequest_To_v1_ListDiskLocationsRequest(in, out) | ||
| } | ||
|
|
||
| func autoConvert_v1_ListDiskLocationsResponse_To_impl_ListDiskLocationsResponse(in *v1.ListDiskLocationsResponse, out *impl.ListDiskLocationsResponse) error { |
There was a problem hiding this comment.
Should this (and autoConvert_v1_ListDiskIDsResponse_To_impl_ListDiskIDsResponse above) go in the conversion.go file? i.e. along with Convert_impl_ListDiskLocationsResponse_To_v1_ListDiskLocationsResponse? Or are these generated correctly?
There was a problem hiding this comment.
I saw that for each request there are 4 conversion functions generated that start with Convert_:
- versioned client request -> internal request
- internal request -> versioned client request
- internal response -> versioned client response
- versioned client response -> internal response
However if you see we only use 2 of the 4 functions generated above:
- versioned client request -> internal request
- internal response -> versioned client response
The other two which are generated by the code generation are the inverse versions however we don't use them, we have unused functions but they never gave problems during testing.
On top of this if we don't provide an override in conversion.go for a version then the code generator will generate additional auto_ functions.
If there's an override to one of the above then both the Convert_ and the auto_ functions for the function overridden won't generated.
There was a problem hiding this comment.
ah indeed. versioned client response -> internal response and internal request -> versioned client request are never used.
|
Thanks @mauriciopoppe /lgtm |
k8s-ci-robot
added
the
lgtm
andyzhangx
reviewed
Jul 4, 2021
| return w.client.Mkdir(context, request, opts...) | ||
| } | ||
|
|
||
| func (w *Client) PathExists(context context.Context, request *v1.PathExistsRequest, opts ...grpc.CallOption) (*v1.PathExistsResponse, error) { |
There was a problem hiding this comment.
@mauriciopoppe do you know what's the correct PathExistsRequest value if I want to invoke v1 PathExists?
I think there is a breaking change migrate from v1beat3 to v1 for this function. original code is here:
isExistsResponse, err := mounter.FsClient.PathExists(context.Background(),
&fs.PathExistsRequest{
Path: normalizeWindowsPath(path),
})
obviously it's now panic:
I hit following error:
[pod/csi-smb-node-win-r4rxn/smb] panic: protobuf tag not enough fields in PathExistsResponse.state:
[pod/csi-smb-node-win-r4rxn/smb]
[pod/csi-smb-node-win-r4rxn/smb] goroutine 53 [running]:
[pod/csi-smb-node-win-r4rxn/smb] github.com/golang/protobuf/proto.(*unmarshalInfo).computeUnmarshalInfo(0xc0003b0460)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/github.com/golang/protobuf/proto/table_unmarshal.go:332 +0x1777
[pod/csi-smb-node-win-r4rxn/smb] github.com/golang/protobuf/proto.(*unmarshalInfo).unmarshal(0xc0003b0460, 0xc00003f860, 0xc0001b0e58, 0x2, 0x2, 0xc8e8ff, 0x20)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/github.com/golang/protobuf/proto/table_unmarshal.go:136 +0xf65
[pod/csi-smb-node-win-r4rxn/smb] github.com/golang/protobuf/proto.(*InternalMessageInfo).Unmarshal(0xc00005cea0, 0x1acf9e56dd0, 0xc00003f860, 0xc0001b0e58, 0x2, 0x2, 0xc00003f801, 0xc00035e780)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/github.com/golang/protobuf/proto/table_unmarshal.go:63 +0x6d
[pod/csi-smb-node-win-r4rxn/smb] github.com/golang/protobuf/proto.(*Buffer).Unmarshal(0xc00035e788, 0x1acf9e56dd0, 0xc00003f860, 0x0, 0x0)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/github.com/golang/protobuf/proto/decode.go:424 +0x1f5
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc/encoding/proto.codec.Unmarshal(0xc0001b0e58, 0x2, 0x2, 0x1d9edc0, 0xc00003f860, 0x2, 0x2)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/encoding/proto/proto.go:93 +0x13d
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.recv(0xc0000041b0, 0x1acf9d4a758, 0x2a4ed80, 0xc000388400, 0x0, 0x0, 0x1d9edc0, 0xc00003f860, 0x400000, 0x0, ...)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/rpc_util.go:711 +0x11c
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.(*csAttempt).recvMsg(0xc0002ff000, 0x1d9edc0, 0xc00003f860, 0x0, 0x0, 0x0)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/stream.go:885 +0xf4
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.(*clientStream).RecvMsg.func1(0xc0002ff000, 0xc0001201c0, 0x66)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/stream.go:736 +0x4d
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.(*clientStream).withRetry(0xc0003b4000, 0xc000497138, 0xc000497108, 0xc0001b0da0, 0xc00003f890)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/stream.go:594 +0xa6
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.(*clientStream).RecvMsg(0xc0003b4000, 0x1d9edc0, 0xc00003f860, 0x0, 0x0)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/stream.go:735 +0x10d
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.invoke(0x20c2508, 0xc00009a010, 0x1ef035e, 0x19, 0x1d9ed00, 0xc0003e34c0, 0x1d9edc0, 0xc00003f860, 0xc000386700, 0x0, ...)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/call.go:73 +0x14f
[pod/csi-smb-node-win-r4rxn/smb] google.golang.org/grpc.(*ClientConn).Invoke(0xc000386700, 0x20c2508, 0xc00009a010, 0x1ef035e, 0x19, 0x1d9ed00, 0xc0003e34c0, 0x1d9edc0, 0xc00003f860, 0x0, ...)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/google.golang.org/grpc/call.go:37 +0x1c5
[pod/csi-smb-node-win-r4rxn/smb] github.com/kubernetes-csi/csi-proxy/client/api/filesystem/v1.(*filesystemClient).PathExists(0xc0000427b0, 0x20c2508, 0xc00009a010, 0xc0003e34c0, 0x0, 0x0, 0x0, 0xf, 0xc00007b4c0, 0x1)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/github.com/kubernetes-csi/csi-proxy/client/api/filesystem/v1/api.pb.go:840 +0xdb
[pod/csi-smb-node-win-r4rxn/smb] github.com/kubernetes-csi/csi-proxy/client/groups/filesystem/v1.(*Client).PathExists(...)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/vendor/github.com/kubernetes-csi/csi-proxy/client/groups/filesystem/v1/client_generated.go:81
[pod/csi-smb-node-win-r4rxn/smb] github.com/kubernetes-csi/csi-driver-smb/pkg/mounter.(*CSIProxyMounter).ExistsPath(0xc0000428b0, 0xc000120070, 0x62, 0x0, 0x0, 0x0)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/pkg/mounter/safe_mounter_windows.go:221 +0x1c2
[pod/csi-smb-node-win-r4rxn/smb] github.com/kubernetes-csi/csi-driver-smb/pkg/mounter.(*CSIProxyMounter).IsLikelyNotMountPoint(0xc0000428b0, 0xc000120070, 0x62, 0x1f7, 0x1c5, 0x240)
[pod/csi-smb-node-win-r4rxn/smb] /home/prow/go/src/sigs.k8s.io/csi-driver-smb/pkg/mounter/safe_mounter_windows.go:164 +0x131
There was a problem hiding this comment.
one change in the proto response was to remove error https://github.com/kubernetes-csi/csi-proxy/pull/143/files
from:
message PathExistsResponse {
// Error message if any. Empty string indicates success
string error = 1;
// Indicates whether the path in PathExistsRequest exists in the host's filesystem
bool exists = 2;
}
to:
message PathExistsResponse {
// Indicates whether the path in PathExistsRequest exists in the host's filesystem
bool exists = 1;
}
There was a problem hiding this comment.
what version of the csi-proxy server are you running? if the csi-proxy server is the same (i.e. if you didn't do any changes to the server component and only made changes to the client module) then it's still incompatible with the v1 client i.e. the v1 client should only connect to the v1 csi-proxy server.
could you list the named pipes that are opened in the vm and confirm that smb-driver is connecting to the v1 named pipes?
There was a problem hiding this comment.
I tested that this call works in both v1 and v1beta2 with GCE PD CSI Driver, you can read the description of this PR and look for PathExists in the changes https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/738/files
There was a problem hiding this comment.
it's using v1.0.0-rc.1 version, unit test also failed with same panic: https://github.com/kubernetes-csi/csi-driver-smb/pull/319/checks?check_run_id=2983427560
\\.\\pipe\\csi-proxy-filesystem-v1alpha1
\\.\\pipe\\csi-proxy-filesystem-v1beta1
\\.\\pipe\\csi-proxy-filesystem-v1beta2
\\.\\pipe\\csi-proxy-filesystem-v1
\\.\\pipe\\csi-proxy-disk-v1alpha1
\\.\\pipe\\csi-proxy-disk-v1beta1
\\.\\pipe\\csi-proxy-disk-v1beta2
\\.\\pipe\\csi-proxy-disk-v1beta3
\\.\\pipe\\csi-proxy-disk-v1
original PR is here: kubernetes-csi/csi-driver-smb#319
There was a problem hiding this comment.
thanks for checking, I'm going to check the integration tests in csi-proxy, I skimmed through kubernetes-csi/csi-driver-smb#319 and it looks good, do you have any chance to deploy csi-proxy in a VM that you can connect to so that we can see the server logs too?
There was a problem hiding this comment.
I reproed on a vm, check issue here: #159
There was a problem hiding this comment.
I made these changes in my copy https://github.com/kubernetes-csi/csi-proxy/compare/master...mauriciopoppe:smb-test?expand=1 and tried run this test on a VM, I saw no errors in the server logs:
I0705 01:38:24.932931 3036 server.go:105] Request: PathExists with path="C:\\var\\lib\\kubelet\\testdir\\test-pod-id\\volumes\\kubernetes.io~csi\\pvc-test47\\rootvol"
And also no errors in the test results:
PS C:\Users\mauriciopoppe\go\src\github.com\kubernetes-csi\csi-proxy> go test -v .\integrationtests\. -run TestFilesystemAPIGroup/v1FilesystemTests
=== RUN TestFilesystemAPIGroup
=== RUN TestFilesystemAPIGroup/v1FilesystemTests
=== RUN TestFilesystemAPIGroup/v1FilesystemTests/PathExists_positive
=== RUN TestFilesystemAPIGroup/v1FilesystemTests/IsMount
--- PASS: TestFilesystemAPIGroup (0.21s)
--- PASS: TestFilesystemAPIGroup/v1FilesystemTests (0.21s)
--- PASS: TestFilesystemAPIGroup/v1FilesystemTests/PathExists_positive (0.15s)
--- PASS: TestFilesystemAPIGroup/v1FilesystemTests/IsMount (0.06s)
PASS
ok github.com/kubernetes-csi/csi-proxy/integrationtests 0.605s
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind api-change
/kind feature
What this PR does / why we need it:
Bumps the following APIs:
Also adds a new CI test for the bump script
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?:
/cc @jingxu97 @ddebroy
/hold
Waiting for #152 to be merged first