Skip to content
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

Add hashed configs to pod annotations #469

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hoyhbx
Copy link
Contributor

@hoyhbx hoyhbx commented Apr 28, 2022

Change log description

  • Automatically trigger statefulSet rolling update when config is changed
  • Add hashed zookeeper configuration as the pod annotation in the statefulSet

Purpose of the change

Fixes #454

What the code does

Add hashed zookeeper configuration as the pod annotation in the statefulSet.
The makeZkPodAnnotations function computes the hash for the zookeeper config string and calls mergeAnnotations to merge with the annotations from the CR.

There are several possible improvements regarding the patch:

  1. I was not sure whether to start the function name with capital case or lower case (e.g. makeZkPodAnnotations or MakeZkPodAnnotations), since both code styles exist in the code base.
  2. The mergeAnnotations function encodes the same logic as the mergeLabels function, we could reduce these two into a general mergeMap function to reduce the code redundency.
  3. I am not sure if the changes in the env.sh could reflect to the zookeeper cluster without a restart. If that also requires a restart, then we can add the hash of result of makeZkEnvConfigString to the annotation map too.

How to verify it

Similar to the steps mentioned in #454

  1. Deploy a default zookeeper cluster
  2. Change one of the fields in the config field, and observe that the statefulSet performed a rolling update, and the config used by the zookeeper cluster is updated.

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2c8bfec) 85.91% compared to head (5967165) 86.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   85.91%   86.03%   +0.11%     
==========================================
  Files          12       12              
  Lines        1633     1647      +14     
==========================================
+ Hits         1403     1417      +14     
  Misses        145      145              
  Partials       85       85              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anishakj
Copy link
Contributor

@hoyhbx Could you please add an end-to-end test to validate this?

@anishakj
Copy link
Contributor

Change log description

  • Automatically trigger statefulSet rolling update when config is changed
  • Add hashed zookeeper configuration as the pod annotation in the statefulSet

Purpose of the change

Fixes #454

What the code does

Add hashed zookeeper configuration as the pod annotation in the statefulSet. The makeZkPodAnnotations function computes the hash for the zookeeper config string and calls mergeAnnotations to merge with the annotations from the CR.

There are several possible improvements regarding the patch:

  1. I was not sure whether to start the function name with capital case or lower case (e.g. makeZkPodAnnotations or MakeZkPodAnnotations), since both code styles exist in the code base.
  2. The mergeAnnotations function encodes the same logic as the mergeLabels function, we could reduce these two into a general mergeMap function to reduce the code redundency.
  3. I am not sure if the changes in the env.sh could reflect to the zookeeper cluster without a restart. If that also requires a restart, then we can add the hash of result of makeZkEnvConfigString to the annotation map too.

How to verify it

Similar to the steps mentioned in #454

  1. Deploy a default zookeeper cluster
  2. Change one of the fields in the config field, and observe that the statefulSet performed a rolling update, and the config used by the zookeeper cluster is updated.

For the functions that are local name is starting with small letter
It is better to combine mergeAnootations and mergeLabels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Zookeeper-operator does not restart pods when static configs are changed
2 participants