-
Notifications
You must be signed in to change notification settings - Fork 573
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
Do not restart cluster when scaling up #4964
Conversation
29d33a5
to
c0fe3fb
Compare
c0fe3fb
to
7b4c5e7
Compare
7b4c5e7
to
1f2496c
Compare
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.
Approach looks good 👍
I think there are some other fields that change with replicas that we need to unhash
@@ -120,6 +120,7 @@ func (c *GlobalConfiguration) GetNodeConfigurationHash() (string, error) { | |||
clone := *c | |||
// clean any cluster property from config before serializing | |||
clone.ClusterConfiguration = nil | |||
removeIgnoredFields(&clone) |
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.
I think probably we don't need to clone the object, but the code above seems to suggest we do a deep copy, while in practice it's doing a shallow one and the object is probably being modified
func removeIgnoredFields(clone *GlobalConfiguration) { | ||
// ignore seeds for hash computation so that changes in this field don't | ||
// trigger cluster restats | ||
clone.NodeConfiguration.Redpanda.SeedServers = []config.SeedServer{} |
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.
I think there are other cases where we iterate over the replicas to create urls, such as for panda proxy and schema registry
existingCluster.Spec.Replicas = &repliasP1 | ||
Expect(k8sClient.Update(context.Background(), &existingCluster)).Should(Succeed()) | ||
newConfigMapHash := sts.Annotations[res.ConfigMapHashAnnotationKey] | ||
Expect(newConfigMapHash).Should(Equal(configMapHash)) |
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.
Better using Consistently(func ...)
in this case, even if it takes more time
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.
Also the sts
is not reloaded
Closing this as it will not be relevant for the v2 operator: https://github.com/redpanda-data/redpanda/tree/operator-v2 |
Cover letter
Currently when scaling up, we always update seed servers and that update triggered restart of the whole cluster. That's unnecessary, it's only important that there's at least one node up from all seeds, there does not have to be all listed.
This PR changes the current behavior so the algorithm computing hashes to not include seeds - so change in seeds will never trigger restart. Considering the way how clusters are scaled up and down right now, this is correct behavior.
We cannot backport this as it triggers restart of the whole cluster because it changes the existing hashes.
Fixes #ISSUE-NUMBER, Fixes #ISSUE-NUMBER, ...
Release notes
Improvements