-
Notifications
You must be signed in to change notification settings - Fork 16.8k
prometheus: switch to Kubernetes 1.6 storage class specification #1332
prometheus: switch to Kubernetes 1.6 storage class specification #1332
Conversation
Hi @bryanlarsen. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. I understand the commands that are listed here. |
/ok-to-test |
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 LGTM, but I'd like a couple other maintainers to weigh in re: if this is how we want to address StorageClass repo-wide.
I'm thinking this also merits a major version bump, since it could break existing installations.
{{- else }} | ||
storageClassName: "{{ .Values.alertmanager.persistentVolume.storageClass }}" | ||
{{- end }} | ||
{{- end }} |
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 seems like a reasonable approach. Provides the necessary options for 1.6+, and users can still use on <1.6 by leaving storageClass
unset and providing the annotation.
Ping @unguiculus @viglesiasce @lachie83 @prydonius @seanknox @linki @sameersbn @foxish: is this a/the pattern we want to adopt repo-wide?
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.
OK with me. Plus, now that v1.7 is out, we should formally deprecate v1.5 support and focus on v1.6 and v1.7.
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.
OK with me too.
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.
+1
## If set to "-", storageClassName: "", which disables dynamic provisioning | ||
## If undefined (the default) or set to null, no storageClassName spec is | ||
## set, choosing the default provisioner. (gp2 on AWS, standard on | ||
## GKE, AWS & OpenStack) |
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.
Perhaps also a note about providing the old annotations for clusters <1.6?
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.
@seanknox's comment indicated a deprecation of v1.5 support.
/lgtm |
Ditching the alpha storage class annotation and using the 1.6 style specification instead brings the following benefits: - a standard way to specify the default provisioner - a mechanism to disble the dynamic provisioner Unfortunately, go template makes the second one hacky. See helm/helm#2600 for more details. So instead of using an empty string to disable the dynamic provisioner, this PR uses "-".
567089e
to
5d520ee
Compare
Bumped to v4.0.0 since we've changed StorageClass behavior. |
Ditching the alpha storage class annotation and using the 1.6 style specification instead brings the following benefits:
Unfortunately, go template makes the second one hacky. See helm/helm#2600 for more details. So instead of using an empty string to disable the dynamic provisioner, this PR uses "-".