Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Support configuring etcd flags #4033

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Support configuring etcd flags #4033

merged 1 commit into from
Dec 2, 2022

Conversation

DanielXiao
Copy link
Contributor

@DanielXiao DanielXiao commented Nov 29, 2022

Add Cluster variable etcdExtraArgs to set arbitrary etcd flags. Add Cluster variable tlsCipherSuites to store the default cipher suites. When etcdExtraArgs does not contain cipher-suites, the default value is set.

Add variable ETCD_EXTRA_ARGS for UX backward compatibility.

What this PR does / why we need it

We need to support configuring etcd flags since Nimbus testbeds have only low performance storages.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Release note

Use the variable ETCD_EXTRA_ARGS to configure etcd flags. See the following example:
ETCD_EXTRA_ARGS: "heartbeat-interval=300;election-timeout=2000"

Additional information

Special notes for your reviewer

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4033/20221129093640/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #4033 (0b0d8f2) into main (e006486) will decrease coverage by 0.79%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4033      +/-   ##
==========================================
- Coverage   48.15%   47.35%   -0.80%     
==========================================
  Files         433      456      +23     
  Lines       43136    44595    +1459     
==========================================
+ Hits        20772    21118     +346     
- Misses      20373    21470    +1097     
- Partials     1991     2007      +16     
Impacted Files Coverage Δ
...ons/controllers/packageinstallstatus_controller.go 77.99% <0.00%> (-1.16%) ⬇️
cmd/cli/plugin/tkr/v1alpha3/os.go 73.50% <0.00%> (-0.86%) ⬇️
cmd/cli/plugin/cluster/get_machinehealthcheck.go 11.42% <0.00%> (ø)
cmd/cli/plugin/cluster/create.go 42.22% <0.00%> (ø)
cmd/cli/plugin/cluster/credentials_update.go 8.73% <0.00%> (ø)
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
cmd/cli/plugin/cluster/list.go 11.36% <0.00%> (ø)
...in/cluster/get_machinehealthcheck_control_plane.go 11.11% <0.00%> (ø)
cmd/cli/plugin/cluster/available_upgrade.go 16.32% <0.00%> (ø)
cmd/cli/plugin/cluster/delete_node_pool.go 16.66% <0.00%> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@DanielXiao DanielXiao marked this pull request as ready for review November 30, 2022 02:12
@DanielXiao DanielXiao requested review from a team as code owners November 30, 2022 02:12
Add Cluster variable etcdExtraArgs to set arbitrary etcd flags.
Add Cluster variable tlsCipherSuites to store the default cipher suites.
When etcdExtraArgs does not contain cipher-suites, the default value is
set.

Add variable ETCD_EXTRA_ARGS for UX backward compatibility.
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4033/20221130075253/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

Copy link
Contributor

@tenczar tenczar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems reasonable to me. I'm becoming a little concerned with how much customization is being baked directly into every clusterclass. I think podsecuritypolicy added clusterclass customizations in an interesting way, or maybe we even adopt kustomize in the future...

@DanielXiao
Copy link
Contributor Author

@tenczar @wjun As you can see jsonpatch is aweful, other folks can introduce a patch to change the same fields by accident and there is no way to write unit test to ensure the expected behavior. We should try runtime sdk next release so we can easily modify map, array and leverage unit tests to avoid regression.

Copy link
Contributor

@wjun wjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DanielXiao DanielXiao added the ok-to-merge PRs should be labelled with this before merging label Dec 2, 2022
@DanielXiao DanielXiao merged commit ab2b2b7 into main Dec 2, 2022
@DanielXiao DanielXiao deleted the yifengx/etcd_flags branch December 2, 2022 02:39
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants