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.

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

Add documentation of PV/PVC.storageClassName #2686

Merged
merged 1 commit into from
Mar 14, 2017

Conversation

jsafrane
Copy link
Member

@jsafrane jsafrane commented Mar 3, 2017

In 1.6, we move from storage.k8s.io/v1beta1 to storage.k8s.io/v1 and from annotation volume.beta.kubernetes.io/storage-class to storageClassName attribute. Both v1beta1 API and the old annotation are still working for some transition period and it will be removed in future (Kubernetes 1.8 at the earliest).

@kubernetes/sig-storage-misc


This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 3, 2017
PVs of that default. Specifying a default `StorageClass` is done by setting the
annotation `storageclass.beta.kubernetes.io/is-default-class` equal to "true" in
annotation `storageclass.kubernetes.io/is-default-class` equal to "true" in
a `StorageClass` object. If the administrator does not specify a default, the
cluster responds to PVC creation as if the admission plugin were turned off. If
more than one default is specified, the admission plugin forbids the creation of
all PVCs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the PVC specifices an existing storage class? What if the PVC does not define storage class at all? Will the claims still fail if >1 default has been configured?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the PVC specifices an existing storage class?

".. the administrator may specify a default StorageClass. All PVCs that have no storageClassName can be bound only to PVs of that default. " - i.e., PVCs that require a specific class are not affected by this admission plugin.

What if the PVC does not define storage class at all?
"All PVCs that have no storageClassName can be bound only to PVs of that default. " - i.e., PVCs that do not define a class are can be bound only to PVs of the default storage class. And PVC will fail here if there are two or more default storage classes.

Copy link

@jeffvance jeffvance Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsafrane my comment above was in reference to: if some-condition then "the admission plugin forbids the creation of all PVCs". I was listing a couple of scenarios where I wanted to know if the statment remains true.

@jeffvance
Copy link

Look good.

@chenopis
Copy link
Contributor

chenopis commented Mar 3, 2017

@jsafrane I'm assuming this needs to go out w/ the 1.6 docs release, right? If so, I will tag it as such and move it to the release-1.6 branch.

Copy link
Contributor

@chenopis chenopis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo and grammar suggestions.


In the future after beta, the `volume.beta.kubernetes.io/storage-class`
annotation will become an attribute.
In the past, annotation `volume.beta.kubernetes.io/storage-class` was used instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar -- add 'the' before 'annotation': "In the past, the annotation volume.beta.kubernetes.io/storage-class"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

In the future after beta, the `volume.beta.kubernetes.io/storage-class`
annotation will become an attribute.
In the past, annotation `volume.beta.kubernetes.io/storage-class` was used instead
of `storageClassName` attribute. This annotation is still working, however
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar -- add 'the' after 'of': "...of the storageClassName attribute."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

annotation will become an attribute.
In the past, annotation `volume.beta.kubernetes.io/storage-class` was used instead
of `storageClassName` attribute. This annotation is still working, however
it won't be supported in a future Kubernetes release.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "...it will become fully deprecated in a future Kubernetes release."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -266,38 +265,39 @@ All of the requirements, from both `matchLabels` and `matchExpressions` are ANDe
### Class

A claim can request a particular class by specifying the name of a
`StorageClass`using the annotation `volume.beta.kubernetes.io/storage-class`.
Only PVs of the requested class, ones with the same annotation as the PVC, can
`StorageClass`using the attribute `storageClassName`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: add a space before 'using'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

have no class. In this case the PVCs that have no annotation are treated the
same way as PVCs that have their annotation set to `""`.
`StorageClass`. All PVCs that have no `storageClassName` can be bound only to PVs that
have no class. In this case the PVCs that have no `storageClassName` are treated the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar: add comma after "In this case"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


When a PVC specifies a `selector` in addition to requesting a `StorageClass`,
the requirements are ANDed together: only a PV of the requested class and with
the requested labels may be bound to the PVC. Note that currently, a PVC with a
non-empty `selector` can't have a PV dynamically provisioned for it.

In the future after beta, the `volume.beta.kubernetes.io/storage-class`
annotation will become an attribute.
In the past, annotation `volume.beta.kubernetes.io/storage-class` was used instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar -- add 'the' before 'annotation': "In the past, the annotation volume.beta.kubernetes.io/storage-class"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

In the future after beta, the `volume.beta.kubernetes.io/storage-class`
annotation will become an attribute.
In the past, annotation `volume.beta.kubernetes.io/storage-class` was used instead
of `storageClassName` attribute. This annotation is still working, however
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar -- add 'the' after 'of': "...of the storageClassName attribute."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@jsafrane
Copy link
Member Author

jsafrane commented Mar 6, 2017

Thanks a lot for grammar remarks! + rebased on top of release-1.6

@jsafrane jsafrane changed the base branch from master to release-1.6 March 6, 2017 08:38
@chenopis
Copy link
Contributor

chenopis commented Mar 6, 2017

@jsafrane Great, thanks! Everything looks good. If we're not waiting on anything else, I can merge this.

@jsafrane
Copy link
Member Author

jsafrane commented Mar 7, 2017

cc @kubernetes/feature-reviewers to follow the process. This is documentation update for kubernetes/enhancements#36

@chenopis
Copy link
Contributor

chenopis commented Mar 7, 2017

@jsafrane So, is it okay if I merge this PR now? The kubernetes:release-1.6 branch won't be published until k8s v1.6 is released.

@jsafrane
Copy link
Member Author

jsafrane commented Mar 8, 2017

@chenopis, yes, please merge this. Thanks!

@chenopis chenopis merged commit 146af29 into kubernetes:release-1.6 Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants