WIP: Do not set --capacity to tikv-server if resources.limits.storage is absent

[cofyc]

What problem does this PR solve?

fixes #944

This makes it possible for users to set capacity in TiKV configuration.

What is changed and how does it work?

Do not set --capacity to tikv-server if resources.limits.storage is absent.

Check List

Tests

• Unit test
• E2E test
• Manual test (add detailed scripts or steps below)

Code changes

• Has Helm charts change
• Has Go code change
• Has CI related scripts change
• Has documents 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?:

Fix a bug that `raftstore.capacity` is ignored even if  `limits.storage` is absent.

[cofyc]

/run-e2e-in-kind

[cofyc]

/run-e2e-in-kind

[cofyc]

/run-e2e-in-kind

[cofyc]

/run-e2e-in-kind

[tennix]

The 0 capacity means unlimited for TiKV, this is the same meaning that users do not set the limit of the storage. So I don't think it's necessary to do this change. Besides, when users upgrade tidb-operator to this version, the existing clusters maybe rolling upgrade due to pod spec changes which is unexpected behavior.

[cofyc]

The 0 capacity means unlimited for TiKV, this is the same meaning that users do not set the limit of the storage.

The problem is users cannot specify capacity via raftstore.capacity configuration because it will be overridden by the --capacity 0 even if they didn't specify resources.limits.storage.

[cofyc]

/run-e2e-in-kind

[tennix]

The 0 capacity means unlimited for TiKV, this is the same meaning that users do not set the limit of the storage.

The problem is users cannot specify capacity via raftstore.capacity configuration because it will be overridden by the --capacity 0 even if they didn't specify resources.limits.storage.

In this case, if users want to set raftstore.capacity, can we guide users to configure the storage limit? Since semantically this two should have the same meaning.

[cofyc]

You mean we disable raftstore.capacity configuration and ask users to configure resources.limits.storage?

I don't think it's a good idea, it might confuse users that part of configuration does not take effect. It will be convenient to view all TiKV configuration in one place, especially we plan to manage component configurations via CRDs.

[cofyc]

@weekface @aylei @onlymellb
What do you think?

[onlymellb]

I think this modification is reasonable, but introduce this PR will lead to the existing clusters rolling upgrade. So in order to avoid this problem, we should not modify the sts's pod spec but make a judgment in the tikv startup script, If CAPACITY is "-1" we do not specify the --capacity for tikv command-line arguments, otherwise specify --capacity =${CAPACITY} command-line arguments
as before. This is a compatible modification, what do you think?
@cofyc @weekface @tennix @aylei @xiaojingchen

[cofyc]

you're right, it's an incompatible change.

but I don't like using a special -1 because it requires users to act if they want to use raftstore.capacity in TiKV configuration.

A general solution is to fix this kind of bugs in the next version of TiDBCluster CRD. When the user upgrades tidb-operator, it will set CAPACITY environment for the old version of TiDBCluster but don't for the new version of TiDBCluster (the meaning of tikv.resources.limit.storage does not change, except raftstore.capacity will not be overridden).

This can extend to other incompatible changes. We can hold this until we are going to release the next version of TiDBCluster, e.g. v1alpha2.

[onlymellb]

If users want to use raftstore.capacity in TiKV configuration, they just don't set tikv.resources.limits.storage, if users want to specify command-line arguments just need to set
tikv.resources.limits.storage. Set CAPACITY to "-1"(of course, you can use any special tag to distinguish whether the user wants to use TiKV configuration or TiKV command-line.) is automatically by tidb-operator based on the user's behavior and is transparent to the user.

[cofyc]

IIUC, the problem exists when the tidb-operator is upgraded, desired StatefulSet spec from the same CRD may change (if tikv.resources.limits.storage is absent). The original value for CAPACITY is 0, so setting CAPACITY to -1 automatically is the same as skipping it.

[onlymellb]

Yes, this is what I think.

[DanielZhangQD]

LGTM