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.

Sign up for GitHub

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

Jump to bottom

CSIStorageCapacity beta API #99641

Merged
merged 11 commits into from Mar 8, 2021
Merged

CSIStorageCapacity beta API #99641

merged 11 commits into from Mar 8, 2021

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Mar 2, 2021 •
edited

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

We want to promote CSIStorageCapacity to beta in Kubernetes 1.21.

Special notes for your reviewer:

The new v1beta1 API might get extended before Kubernetes 1.21 gets released if the new CSI spec 1.4.0 gets released quickly enough. Right now, this PR does not include that change (yet).

Does this PR introduce a user-facing change?

storage capacity tracking (= the CSIStorageCapacity feature) is beta, storage.k8s.io/v1alpha1/VolumeAttachment and storage.k8s.io/v1alpha1/CSIStorageCapacity objects are deprecated

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1472-storage-capacity-tracking
- [Usage]: https://kubernetes.io/docs/concepts/storage/storage-capacity/

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 2, 2021
This was referenced Mar 2, 2021
Closed
Closed
Merged
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2021
@pohly pohly force-pushed the storage-capacity-beta branch 2 times, most recently from 7ab8d1e to 177ba61 Compare March 3, 2021 20:06
@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Mar 3, 2021
@pohly pohly force-pushed the storage-capacity-beta branch 3 times, most recently from c8a2aa9 to b52d558 Compare March 4, 2021 09:40
@pohly
CSIStorageCapacity: prepare tests for enabling the feature by default
504f105 
Defaults and validation are such that the field has to be set when
the feature is enabled, just as for the other boolean fields. This
was missing in some tests, which was okay as long as they ran
with the feature disabled. Once it gets enabled, validation will
flag the missing field as error.

Other tests didn't run at all.
@pohly
CSIStorageCapacity: promote API to beta
a94c141 
The v1alpha1 API is left in place for now to ease the migration.
@pohly
CSIStorageCapacity: register types unconditionally
e565951 
That the object was registered depending on the feature gate was
called out as unusual during the 1.21 review. Previously, all beta
storage APIs were unders such feature gate checks, but its better to
drop that to be consistent with the rest of Kubernetes.
@pohly
CSIStorageCapacity: use beta API
5ca0814
@pohly
CSIStorageCapacity: update rbac test data
85bfd26 
This is the result of
  UPDATE_BOOTSTRAP_POLICY_FIXTURE_DATA=true go test k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy
after enabling the CSIStorageCapacity feature. This enables
additional RBAC entries for reading CSIDriver and
CSIStorageCapacity.
@pohly
CSIStorageCapacity: mark v1alpha1 as deprecated
9e7748c 
We still need the alpha API for testing with current
external-provisioner, but then should remove it shortly after the 1.21
release once the external-provisioner is updated to use the beta API.

That VolumeAttachment is still defined in the alpha API looks like an
oversight. To minimize changes during the beta graduation of
CSIStorageCapacity it is left in place with a deprecation notification
in 1.21.
@pohly
CSIStorageCapacity: add MaximumVolumeSize to v1beta1 API
a2972eb
@pohly
CSIStorageCapacity: check MaximumVolumeSize during scheduling
3fa4354 
If available, then the MaximumVolumeSize is a better indicator whether
creating a volume has a chance to succeed than the total (?) Capacity,
which is potentially larger and less well-defined.
@pohly
Copy link
Contributor Author

pohly commented Mar 8, 2021

pull-kubernetes-verify failed.

I need to rebase and regenerate one file... working on it.

@pohly
CSIStorageCapacity: generated files for beta
2b062a6
@pohly
CSIStorageCapacity: add +featureGate=CSIStorageCapacity
39103c1 
The tag is currently mostly ignored by the tooling, but its usage is
encouraged (kubernetes#99641 (comment)).
@pohly
CSIStorageCapacity: enable E2E tests by default
4c7e4c6 
With the feature now in beta, tests should run in the normal jobs.
@msau42
Copy link
Member

msau42 commented Mar 8, 2021

/lgtm

The feature-gate tagging discussion can continue offline and we can update the tags once the KEP is merged.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2021
@msau42
Copy link
Member

msau42 commented Mar 8, 2021

Oh @pohly can you also update the release note saying the alpha capacity objects are also deprecated?

@pohly
Copy link
Contributor Author

pohly commented Mar 8, 2021

Oh @pohly can you also update the release note saying the alpha capacity objects are also deprecated?

Done.

@pohly
Copy link
Contributor Author

pohly commented Mar 8, 2021

/retest

@k8s-ci-robot
Copy link
Contributor

@pohly: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-storage-slow 4c7e4c6 link /test pull-kubernetes-e2e-gce-storage-slow

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.

@k8s-ci-robot k8s-ci-robot merged commit 14c25ee into kubernetes:master Mar 8, 2021
@liggitt liggitt moved this from Assigned to API review completed, 1.21 in API Reviews Mar 9, 2021
k8s-publishing-bot pushed a commit to kubernetes/api that referenced this pull request Mar 9, 2021
@pohly @k8s-publishing-bot
CSIStorageCapacity: add +featureGate=CSIStorageCapacity
c8b7374 
The tag is currently mostly ignored by the tooling, but its usage is
encouraged (kubernetes/kubernetes#99641 (comment)).

Kubernetes-commit: 39103c188f5d17a0baffb90c122f4751e5b33b93
@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 9, 2021
@ialidzhikov ialidzhikov mentioned this pull request Apr 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@thockin thockin thockin left review comments

@msau42 msau42 msau42 left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@thockin thockin

@msau42 msau42

Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
API Reviews
Status: API review completed, 1.21
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pohly @caesarxuchao @k8s-ci-robot @msau42 @fedebongio @liggitt @thockin