-
Notifications
You must be signed in to change notification settings - Fork 498
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
Able to omit storageClassName to use the default storage class of Kubernetes #1581
Conversation
pkg/apis/pingcap/v1alpha1/types.go
Outdated
// and use the cluster default set by admission controller. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator | ||
// The storageClassName of the persistent volume for PD data storage. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator or Kubernetes default storage classs. |
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.
We shouldn't set default value for storageClassName
in defaulting phase.
nil
storageClassName in PVC is a valid value that indicates the default storage class of the cluster to be used.
pkg/apis/pingcap/v1alpha1/types.go
Outdated
// and use the cluster default set by admission controller. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator | ||
// The storageClassName of the persistent volume for TiKV data storage. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator or Kubernetes default storage classs. |
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.
ditto
pkg/apis/pingcap/v1alpha1/types.go
Outdated
// and use the cluster default set by admission controller. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator | ||
// The storageClassName of the persistent volume for Pump data storage. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator or Kubernetes default storage classs. |
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.
ditto
// The storageClassName of the persistent volume for Backup data storage. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator or Kubernetes default storage classs. | ||
// +optional | ||
StorageClassName *string `json:"storageClassName,omitempty"` |
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.
apply the same change from #1409 to backup/restore spec
// The storageClassName of the persistent volume for Backup data storage. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator or Kubernetes default storage classs. | ||
// +optional | ||
StorageClassName *string `json:"storageClassName,omitempty"` |
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.
ditto
// The storageClassName of the persistent volume for Restore data storage. | ||
// Optionals: Defaults to the default-storage-class-name set in the tidb-operator or Kubernetes default storage classs. | ||
// +optional | ||
StorageClassName *string `json:"storageClassName,omitempty"` |
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.
ditto
@@ -588,7 +588,7 @@ func getNewPDSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap) ( | |||
podAnnotations := CombineAnnotations(controller.AnnProm(2379), basePDSpec.Annotations()) | |||
stsAnnotations := getStsAnnotations(tc, label.PDLabelVal) | |||
storageClassName := tc.Spec.PD.StorageClassName | |||
if storageClassName == nil { | |||
if storageClassName == nil && len(controller.DefaultStorageClassName) > 0 { |
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.
if tc.Spec.PD.StorageClassName
is not set, try to use the default storage class of tidb-operator. If the default value of tidb-operator is not set, try to use the default storage class of Kubernetes cluster.
@@ -409,7 +409,7 @@ func getNewTiKVSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap) | |||
capacity := controller.TiKVCapacity(tc.Spec.TiKV.Limits) | |||
headlessSvcName := controller.TiKVPeerMemberName(tcName) | |||
storageClassName := tc.Spec.TiKV.StorageClassName | |||
if storageClassName == nil { | |||
if storageClassName == nil && len(controller.DefaultStorageClassName) > 0 { |
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.
ditto
196a5a4
to
df335a8
Compare
charts/tidb-operator/values.yaml
Outdated
# | ||
# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ | ||
# | ||
# defaultStorageClassName: standard |
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.
- use user-supplied class if provided
- use tidb-operator default class if not-empty (defaults to "standard" for backward compatibility)
- use Kubernetes cluster default class
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.
if we want to use the default storage class of Kubernetes cluster by default, we need to empty the default values of --default-storage-class-name
and --default-backup-storage-class-name
.
cc @aylei what do you think?
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.
Yes, I think so
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.
Looks like that this change is not backward compatible.
Thinks a user upgrade operator with defaultStorageClassName empty, the storageClass of current PVC and PVCTemplate will be synced to the cluster default, which breaks reconciliation because storageClassName is immutable IIRC
charts/tidb-operator/values.yaml
Outdated
# | ||
# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ | ||
# | ||
# defaultBackupStorageClassName: standard |
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.
ditto
the same YAML can be applied to different clusters that may have different default storage implementation. it helps the user to try our example YAML in any Kubernetes provider without modification. If users have no special requirements, they don't need to care about what storage implementation the default storage class is. It's a system administrator's duty to maintain it. Unlike the network, the cluster can have more than one storage providers. |
actually, we have a hard-coded default storage class |
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.
LGTM, thanks!
charts/tidb-operator/values.yaml
Outdated
# | ||
# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ | ||
# | ||
# defaultStorageClassName: standard |
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.
Yes, I think so
pkg/backup/util/util.go
Outdated
@@ -294,7 +294,7 @@ func ValidateBackup(backup *v1alpha1.Backup) error { | |||
if backup.Spec.From.SecretName == "" { | |||
return fmt.Errorf("missing tidbSecretName config in spec of %s/%s", ns, name) | |||
} | |||
if backup.Spec.StorageClassName == "" { | |||
if backup.Spec.StorageClassName == nil || *backup.Spec.StorageClassName == "" { |
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.
why this is still mandatory?
charts/tidb-operator/values.yaml
Outdated
# | ||
# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/ | ||
# | ||
# defaultStorageClassName: standard |
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.
Looks like that this change is not backward compatible.
Thinks a user upgrade operator with defaultStorageClassName empty, the storageClass of current PVC and PVCTemplate will be synced to the cluster default, which breaks reconciliation because storageClassName is immutable IIRC
Having a default storage class name in controller is problematic, even if we avoid mutating the storage-class when update resources, there is still a problem when we delete the StatefulSet with orphan dependents and the controller eventually re-create the StatefulSet with a different storageClass. IMHO:
|
And I think this PR deserves a release note, maybe ACTION REQUIRED |
It's better to abandon --default-storage-class-name and --default-backup-storage-class-name flags... what do you think? The desired PVC spec should not depend on something which is not in CRDs and can be changed. |
In tidb-operator 1.0, I guess most people who use tidb-cluster helm chart should have set storage class names in their value files. If we really want to allow people to configure an operator-level default value for storage class names, we should update the user object in the defaulting phase before storing it in the API server. |
cc @weekface what do you think? |
I agree, users (or their organization) should make sure the stored TidbCluster CR has storageClass full-filled, either through kubernetes defaults, operator webhook defaults or other mechanisms. And I agree we should deprecate the default-storage-class-name flag of tidb-controller-manager. It is an orthogonal problem for tidb-operator chart, if we want to keep offering the operator level defaults in v1.1.0, we need a webhook based implementation. |
Agree. People who use |
After the change I suggested, TidbCluster CR can have no storage class filled if they want to use Kubernetes default storage class and this will not trigger the upgrade of tidb cluster in tidb-operator because the desired volume claim templates in statefulset do not have storage class filled too. If the system administrator changed the default storage class in the cluster, the storage class of the PVC created by statefulset controller or our backup/restore controller will not be changed until the PVC is deleted by the user. The newly created PVC will use the new default storage class. I think it's reasonable and safe. |
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.
LGTM
Signed-off-by: Yecheng Fu <fuyecheng@pingcap.com>
flags Signed-off-by: Yecheng Fu <fuyecheng@pingcap.com>
I've commented on the wrong page (the comment have been deleted) |
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.
LGTM
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.
LGTM
cherry pick to release-1.1 in PR #1589 |
Signed-off-by: Yecheng Fu fuyecheng@pingcap.com
What problem does this PR solve?
fixes #1478
I've manually tested with the following YAML:
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
Does this PR introduce a user-facing change?: