Able to omit storageClassName to use the default storage class of Kubernetes #1581

cofyc · 2020-01-18T06:22:07Z

Signed-off-by: Yecheng Fu fuyecheng@pingcap.com

What problem does this PR solve?

I've manually tested with the following YAML:

apiVersion: pingcap.com/v1alpha1
kind: TidbCluster
metadata:
  name: basic
spec:
  version: v3.0.8
  timezone: UTC
  pvReclaimPolicy: Delete
  pd:
    baseImage: pingcap/pd
    replicas: 3
    requests:
      storage: "1Gi"
    config: {}
  tikv:
    baseImage: pingcap/tikv
    replicas: 3
    requests:
      storage: "1Gi"
    config: {}
  tidb:
    baseImage: pingcap/tidb
    replicas: 2
    service:
      type: ClusterIP
    config: {}

What is changed and how does it work?

Check List

Tests

Unit test
E2E test
Stability test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has Go code change
Has CI related scripts change
Has Terraform scripts change

Side effects

Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Does this PR introduce a user-facing change?:

ACTION REQUIRED: `--default-storage-class-name` and `--default-backup-storage-class-name `are abandoned, storage class defaults to Kubernetes default storage class right now. If you have set default storage class different than Kubernetes default storage class, please set them explicitly in your tidb cluster helm or YAML files.

cofyc · 2020-01-18T06:25:16Z

pkg/apis/pingcap/v1alpha1/types.go

-	// 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.


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.

cofyc · 2020-01-18T06:25:22Z

pkg/apis/pingcap/v1alpha1/types.go

-	// 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.


cofyc · 2020-01-18T06:25:35Z

pkg/apis/pingcap/v1alpha1/types.go

-	// 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.


cofyc · 2020-01-18T06:26:21Z

pkg/apis/pingcap/v1alpha1/types.go

+	// 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"`


apply the same change from #1409 to backup/restore spec

cofyc · 2020-01-18T06:26:45Z

pkg/apis/pingcap/v1alpha1/types.go

+	// 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"`


cofyc · 2020-01-18T06:26:56Z

pkg/apis/pingcap/v1alpha1/types.go

+	// 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"`


cofyc · 2020-01-18T06:37:13Z

pkg/manager/member/pd_member_manager.go

@@ -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 {


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.

cofyc · 2020-01-18T06:37:26Z

pkg/manager/member/tikv_member_manager.go

@@ -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 {


cofyc · 2020-01-18T06:42:46Z

charts/tidb-operator/values.yaml

+#
+# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/
+# 
+# defaultStorageClassName: standard 


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

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?

Yes, I think so

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

cofyc · 2020-01-18T06:42:52Z

charts/tidb-operator/values.yaml

+#
+# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/
+# 
+# defaultBackupStorageClassName: standard


Yisaer · 2020-01-18T06:55:39Z

I'm not sure is it reasonable to use kubernetes cluster default storageClass instead of throwing the error to mention the users to set the proper storageClass when both Operator and TidbCluster storageClassName are not setted. @cofyc @aylei

cofyc · 2020-01-18T07:21:31Z

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.

cofyc · 2020-01-18T07:24:48Z

actually, we have a hard-coded default storage class standard...
if we want to apply YAML to a cluster in which the default class is not "standard", we need to remove this hard-coded value, see discussion.

aylei

LGTM, thanks!

aylei · 2020-01-18T13:26:19Z

charts/tidb-operator/values.yaml

+#
+# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/
+# 
+# defaultStorageClassName: standard 


Yes, I think so

aylei · 2020-01-18T13:30:42Z

pkg/backup/util/util.go

@@ -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 == "" {


why this is still mandatory?

aylei · 2020-01-18T13:35:58Z

charts/tidb-operator/values.yaml

+#
+# https://kubernetes.io/docs/tasks/administer-cluster/change-default-storage-class/
+# 
+# defaultStorageClassName: standard 


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

aylei · 2020-01-18T13:44:25Z

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:

To be backward compatible, we should at least do not mutate storageClass when update, deleting with orphan dependents is rare and could be documented as an known issue if it happens with storageClass name change at the same time;
For better sematic, the defaultStorageClassName should work like the default storageClass name like kubernetes, i.e., set the storageClass in mutating webhook, of course, the operator one should be prior to the kubernetes one

aylei · 2020-01-18T13:46:30Z

And I think this PR deserves a release note, maybe ACTION REQUIRED

cofyc · 2020-01-19T03:45:23Z

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.

cofyc · 2020-01-19T03:54:40Z

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.
Of course, we should add an ACTION REQUIRED release note to ask people to set storage class names in CRD objects explicitly if they didn't and the default storage class of the Kubernetes cluster is not the storage class they're using.

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.

cofyc · 2020-01-19T03:57:09Z

cc @weekface what do you think?

aylei · 2020-01-19T04:01:04Z

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.

weekface · 2020-01-19T04:09:34Z

cc @weekface what do you think?

Agree.

People who use tidb-cluster helm chart have set storage class names in their value files.

cofyc · 2020-01-19T04:28:15Z

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.

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.

aylei

LGTM

Signed-off-by: Yecheng Fu <fuyecheng@pingcap.com>

flags Signed-off-by: Yecheng Fu <fuyecheng@pingcap.com>

cofyc · 2020-01-19T08:30:58Z

I've commented on the wrong page (the comment have been deleted)

Yisaer

LGTM

cofyc · 2020-01-19T11:46:04Z

@aylei @weekface PR is updated, PTAL again.

aylei

LGTM

sre-bot · 2020-01-19T16:57:27Z

cherry pick to release-1.1 in PR #1589

…ernetes (#1581) (#1589)

cofyc requested a review from aylei January 18, 2020 06:22

cofyc commented Jan 18, 2020

View reviewed changes

cofyc force-pushed the fix1478 branch from 25a5051 to 70859a1 Compare January 18, 2020 06:29

cofyc commented Jan 18, 2020

View reviewed changes

cofyc force-pushed the fix1478 branch 2 times, most recently from 196a5a4 to df335a8 Compare January 18, 2020 06:41

cofyc commented Jan 18, 2020

View reviewed changes

cofyc force-pushed the fix1478 branch from df335a8 to 7232a2f Compare January 18, 2020 06:45

aylei approved these changes Jan 18, 2020

View reviewed changes

aylei suggested changes Jan 18, 2020

View reviewed changes

aylei previously approved these changes Jan 19, 2020

View reviewed changes

cofyc changed the title ~~Able to omit storageClassName~~ WIP: Able to omit storageClassName to use the default storage class of Kubernetes Jan 19, 2020

cofyc dismissed aylei’s stale review via d55eb53 January 19, 2020 06:19

Able to omit storageClassName

8606e55

Signed-off-by: Yecheng Fu <fuyecheng@pingcap.com>

cofyc force-pushed the fix1478 branch from d55eb53 to 2135d17 Compare January 19, 2020 06:22

cofyc changed the title ~~WIP: Able to omit storageClassName to use the default storage class of Kubernetes~~ Able to omit storageClassName to use the default storage class of Kubernetes Jan 19, 2020

remove -default-storage-class-name and -default-backup-storage-class

3911076

flags Signed-off-by: Yecheng Fu <fuyecheng@pingcap.com>

cofyc force-pushed the fix1478 branch from 2135d17 to 3911076 Compare January 19, 2020 06:36

cofyc added the needs-cherry-pick-1.1 label Jan 19, 2020

cofyc closed this Jan 19, 2020

cofyc reopened this Jan 19, 2020

Merge branch 'master' into fix1478

b255dcf

Yisaer approved these changes Jan 19, 2020

View reviewed changes

aylei approved these changes Jan 19, 2020

View reviewed changes

aylei merged commit f29aff0 into pingcap:master Jan 19, 2020

sre-bot mentioned this pull request Jan 19, 2020

Able to omit storageClassName to use the default storage class of Kubernetes (#1581) #1589

Merged

sre-bot added a commit that referenced this pull request Jan 19, 2020

Able to omit storageClassName to use the default storage class of Kub…

9d00647

…ernetes (#1581) (#1589)

cofyc added the release-note-action-required label Feb 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Able to omit storageClassName to use the default storage class of Kubernetes #1581

Able to omit storageClassName to use the default storage class of Kubernetes #1581

cofyc commented Jan 18, 2020 •

edited

Loading

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

cofyc Jan 18, 2020

aylei Jan 18, 2020

aylei Jan 18, 2020

cofyc Jan 18, 2020 •

edited

Loading

Yisaer commented Jan 18, 2020

cofyc commented Jan 18, 2020 •

edited

Loading

cofyc commented Jan 18, 2020

aylei left a comment

aylei Jan 18, 2020

aylei Jan 18, 2020

aylei Jan 18, 2020

aylei commented Jan 18, 2020 •

edited

Loading

aylei commented Jan 18, 2020

cofyc commented Jan 19, 2020

cofyc commented Jan 19, 2020 •

edited

Loading

cofyc commented Jan 19, 2020

aylei commented Jan 19, 2020

weekface commented Jan 19, 2020

cofyc commented Jan 19, 2020

aylei left a comment

cofyc commented Jan 19, 2020

Yisaer left a comment

cofyc commented Jan 19, 2020

aylei left a comment

sre-bot commented Jan 19, 2020

Able to omit storageClassName to use the default storage class of Kubernetes #1581

Able to omit storageClassName to use the default storage class of Kubernetes #1581

Conversation

cofyc commented Jan 18, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Yisaer commented Jan 18, 2020

cofyc commented Jan 18, 2020 • edited Loading

cofyc commented Jan 18, 2020

aylei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei commented Jan 18, 2020 • edited Loading

aylei commented Jan 18, 2020

cofyc commented Jan 19, 2020

cofyc commented Jan 19, 2020 • edited Loading

cofyc commented Jan 19, 2020

aylei commented Jan 19, 2020

weekface commented Jan 19, 2020

cofyc commented Jan 19, 2020

aylei left a comment

Choose a reason for hiding this comment

cofyc commented Jan 19, 2020

Yisaer left a comment

Choose a reason for hiding this comment

cofyc commented Jan 19, 2020

aylei left a comment

Choose a reason for hiding this comment

sre-bot commented Jan 19, 2020

cofyc commented Jan 18, 2020 •

edited

Loading

cofyc Jan 18, 2020 •

edited

Loading

cofyc commented Jan 18, 2020 •

edited

Loading

aylei commented Jan 18, 2020 •

edited

Loading

cofyc commented Jan 19, 2020 •

edited

Loading