-
Notifications
You must be signed in to change notification settings - Fork 451
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
Fix json tags for etcd worker config #5151
Conversation
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 for the tag name change, but I have comments for discrepancies in the defaults.
@@ -515,23 +515,23 @@ type ETCDController struct { | |||
// Workers specify number of worker threads in ETCD controller | |||
// Defaults to 3 |
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.
Not strictly related to this change but I think there is something wrong with the defaults:
gardener/pkg/operation/botanist/component/etcd/bootstrap.go
Lines 330 to 332 in c52af1b
if config.ETCDController != nil { | |
command = append(command, "--workers="+strconv.FormatInt(pointer.Int64Deref(config.ETCDController.Workers, 50), 10)) | |
} |
I don't see any explicit defaulting in g/g for this field. There is only defaulting to 50
when ETCDController is non-nil and Workers
is nil. Does the default 3
actually comes from etcd-druid? If yes, doesn't it make sense to update the above handling to use 3
instead of 50
?
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.
Good point.
There is only defaulting to 50 when ETCDController is non-nil and Workers is nil.
This is actually the default that is meant here. We should add it to the defaulting logic in pkg/gardenlet/apis/config/v1alpha1/defaults.go
which was missed in the previous PR. I propose to do this in a future PR, @abdasgupta can take over when he returned.
If yes, doesn't it make sense to update the above handling to use 3 instead of 50?
50
was the hard-coded default earlier and if we now set it to 3
it will break operators who rely on the higher value. Therefore, 50
was chosen here. I will update the comment.
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.
This is actually the default that is meant here. We should add it to the defaulting logic in pkg/gardenlet/apis/config/v1alpha1/defaults.go which was missed in the previous PR. I propose to do this in a future PR, @abdasgupta can take over when he returned.
Can you open an issue about it? There are still some discrepancies - it you don't specify any default config, then for the custodian controller 3
will be used (default in etcd-druid). But if you specify empty custodianController
config, then 10
will be used (default in the etcd component in gardener). Let's better follow-up with separate issue - PR.
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
/squash
…d worker config (#5155) * Fix json tags for etcd worker config * Rectify ETCDConfig comments
* Fix json tags for etcd worker config * Rectify ETCDConfig comments
* Fix json tags for etcd worker config * Rectify ETCDConfig comments
How to categorize this PR?
/area control-plane
/kind api-change
/needs cherry-pick
What this PR does / why we need it:
This PR amends the struct tags for
workers
configuration in theetcdConfig
.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This is an incompatible API change. However, the example as well as chart values assume that the fields were called
workers
before, so the likelihood to break someone is rather small. From an end-user perspective setting the values "just" didn't work.Release note: