Support additional containers and volumes in TiDB cluster #2229
Conversation
|
Thanks for your contribution. If your PR get merged, you will be rewarded 100 points. |
pkg/apis/pingcap/v1alpha1/types.go
Outdated
|
|
||
| // Additional containers of the component. | ||
| // +optional | ||
| Containers []corev1.Container `json:"containers,omitempty"` |
There was a problem hiding this comment.
I think Containers here would be misleading for user. What about Sidecars? As kubernetes support sidecar lifecycle since 1.18, this would be more clear for user to understand.
There was a problem hiding this comment.
Umm, what about additionalContainers? Although this name is longer, but Sidecars is not clear enough. Because something like slow log is already part of the tidb pod
There was a problem hiding this comment.
additionalContainers is ok to me.
pkg/apis/pingcap/v1alpha1/types.go
Outdated
|
|
||
| // Additional volumes of the component. | ||
| // +optional | ||
| Volumes []corev1.Volume `json:"volumes,omitempty"` |
There was a problem hiding this comment.
ditto, if Volumes here is only used for sidecars, we should declare it in name.
There was a problem hiding this comment.
additionalVolumes is ok to me.
yeya24
force-pushed
the
add-sidecar
branch
4 times, most recently
from
f5c9046
to
e7a5e97
Compare
|
/run-e2e-test |
1 similar comment
|
/run-e2e-test |
|
@Yisaer Could you please take a look? The e2e failure is not related to this PR. |
|
@yeya24 please add the release note and fulfill the request description. |
I am sorry I don't know what should I update. Is it okay now? |
it's ok for me |
|
|
||
| // Additional volumes of the component. | ||
| // +optional | ||
| AdditionalVolumes []corev1.Volume `json:"additionalVolumes,omitempty"` |
There was a problem hiding this comment.
I have a question about how to consume additional volumes？
There was a problem hiding this comment.
That's a good question. @yeya24 WDYT?
There was a problem hiding this comment.
Good question! In my first thought, the AdditionalVolumes is only designed for the additional sidecar containers since we have the AdditionalContainers field so we can define the volumeMount there.
So it doesn't support to add additional volumeMount for the component itself (TiDB, TiKV, PD, etc). Maybe we can consider adding it in the future, but currently not needed.
There was a problem hiding this comment.
agreed, If you want to use sidecar to collect log or tracing, shared volume is required.
There was a problem hiding this comment.
"So it doesn't support to add additional volumeMount for the component itself (TiDB, TiKV, PD, etc).", make this clear in the comments?
There was a problem hiding this comment.
@DanielZhangQD Done.
|
/run-e2e-test |
|
@Yisaer Sorry for the ping, is it good to merge now? |
|
/run-e2e-test |
|
@yeya24 the codes LGTM, plz solve the conflicts. |
Signed-off-by: yeya24 <yb532204897@gmail.com>
|
/run-e2e-test |
|
Updated. |
|
@DanielZhangQD Could you please do a quick review? I guess it is ready to go |
|
/merge |
|
Your auto merge job has been accepted, waiting for:
|
@yeya24 Sorry for the late review. Thanks for your contribution! |
|
/run-e2e-in-kind |
|
/run-all-tests |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
|
cherry pick to release-1.1 in PR #2777 |
Signed-off-by: yeya24 yb532204897@gmail.com
UCP: #2186
What problem does this PR solve?
fixes: #2186
What is changed and how does it work?
Add
additionalContainersandadditionalVolumesfields so that Tidb-operator can support adding sidecars toTiDB,TiKV,PD, etc.Check List
Tests
Code changes
Side effects
Related changes
Does this PR introduce a user-facing change?: