storage capacity GA #710
storage capacity GA #710
Conversation
aef196e to
3a790ee
Compare
3a790ee to
8a3cad4
Compare
|
@pohly: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
8a3cad4 to
818d15b
Compare
|
/hold Let's test with pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master once it is available: kubernetes/test-infra#25840 |
|
We also need (?) to wait for Kubernetes 1.24.0, unless we are fine with client-go 1.24.0-beta.1. |
|
/test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master |
|
Works as intended: |
|
We discussed this today and concluded that this can be merged with client-go 1.24-beta.0. We can update to 1.24 once it is available. |
818d15b to
f62d56b
Compare
The new API is needed because it includes CSIStorageCapacity v1.
This ensures that we stay up-to-date. In particular this includes CSI 1.6.
We only need the interface for CSIStorageCapacity. This enables writing a shim between v1beta1 and v1 of that API.
f62d56b to
7e61498
Compare
|
@xing-yang: this PR is ready for merging again after I rebased. Can you review? /hold cancel Testing with pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master was successful, but we can also test once more: /test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-master |
| clientFactory := capacity.NewV1ClientFactory(clientset) | ||
| cInformer := factoryForNamespace.Storage().V1().CSIStorageCapacities() | ||
|
|
||
| invalidCapacity := &storagev1.CSIStorageCapacity{ |
There was a problem hiding this comment.
Can you add some comments before line 486 explaining why you are trying to create an object with an invalid name? Although people will get it when reading the code below, it is better to clarify earlier.
pkg/capacity/apibridge.go
Outdated
| @@ -0,0 +1,177 @@ | |||
| /* | |||
| Copyright 2020 The Kubernetes Authors. | |||
When are you planning to remove this shim layer? 3 releases after 1.24 when we can remove v1beta1? |
As soon as the oldest supported release is guaranteed to have the v1 API. |
The code itself uses the v1 API and directly uses the normal client-go API when v1 is supported by the server. If the server doesn't support that API, wrappers around the v1beta1 API convert CSIStorageCapacity objects back and forth as needed. The reason for this more complex solution is that it avoids a breaking change in the external-provisioner and thus provides a smoother transition path: CSI driver developers can update to the next external-provisioner release and use it both with Kubernetes < 1.24 and >= 1.24.
7e61498 to
3937fd7
Compare
|
Can you also added this line "Kubernetes 1.24 will mark the v1beta1 CSIStorageCapacity API as deprecated and introduces it as v1." to the release note? |
|
Done. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, xing-yang 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 PR assumes that the pod will have access to the "default" storage namespace in order to create the CSIStorageCapacity , which is not always the case. Our pods run in a different namespace and the "NAMESPACE" env variable contains the namespace to which the pod have permissions. |
hmmmm https://github.com/kubernetes-csi/external-provisioner/pull/710/files#diff-963c2f5e2076b500e70c47c13f135cdbb29b81f270bc76d1c99f823fecd1b510R493 create the capacity object in |
|
That particular call is meant to fail. If it failed for "default" namespace with a permission error, then the check would have been successful... if the permission error had been accepted as one of the allowed error outcomes. That's not currently the case. I assume you get the Either using the configured namespace or adding the error code should solve this. Using the namespace is simpler. @nbalacha: Can you try that fix and prepare a PR? |
That is correct. I get The call is expected to fail but it should not with a permission issue. I would expect the fix to be to use the configured ns instead of default. createdCapacity, err := clientset.StorageV1().CSIStorageCapacities(namespace).Create(ctx, invalidCapacity, metav1.CreateOptions{}) If this is fine, I can send a fix. |
|
imo, |
|
I agree on the fix. It just would be good to know that it has been tested, because our CI obviously doesn't cover that case. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Kubernetes 1.24 will mark the v1beta1 CSIStorageCapacity API as deprecated and introduces it as v1. To avoid warnings from client-go, external-provisioner must use that v1 API when running on Kubernetes >= 1.24.
Special notes for your reviewer:
To avoid making Kubernetes 1.24 a hard requirement for the next external-provisioner release, a small(is) shim layer converts objects back and forth. external-provisioner detects automatically whether the v1 API is available.
This PR has to be updated once client-go 1.24(pre?) is available.
Does this PR introduce a user-facing change?: