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

[ANSIENG-4200] | fixing race condition in changing file owner #1919

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

rrbadiani
Copy link
Member

@rrbadiani rrbadiani commented Mar 6, 2025

Description

This task tries to change owner and group of files which exist inside data dir.

All those files are created by kafka user in zookeeper setups.

2 files meta.properties and bootstrap.checkpoint are created by ansible_user which is typically root and different from kafka_broker_user.

Running this task for greenfield fresh setup would change the owner/grp of the above files to kafka_broker_user,kafka_broker_group which is correct behaviour.

But changing the user and owner using this ansible task while cluster upgrades doesnt do anything.

When I tried to change owner and group of data dir and kafka server process in a running cluster using kafka_broker_user & kafka_broker_group it wasnt able to start the cluster and also messed up logs. log files owner didnt change it simply changed the owner of directory which contains log files and thus no new logs were getting written unless manually i change the owner, groups of log files. So it seems even for changing user, group this task isn't helping.

Solution

We can check if kraft/kafka service is running or not.
If running we should skip the task causing race condition and it will have no impact
If not running then we should not skip and run this task. But here it wont be getting trapped in race condition as there wont be any .tmp files coming and going while it runs.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

kraft
zk->kr migration

Checklist:

  • Any variable/code changes have been validated to be backwards compatible (doesn't break upgrade)
  • I have added tests that prove my fix is effective or that my feature works
  • If required, I have ensured the changes can be discovered by cp-ansible discovery codebase
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Sorry, something went wrong.

…ng clusters
@rrbadiani rrbadiani requested a review from a team as a code owner March 6, 2025 10:16
@rrbadiani rrbadiani changed the title [ANSIENG-4200] | fixing race condition by skipping the task for runni… [ANSIENG-4200] | fixing race condition in changing file owner Mar 6, 2025
Copy link
Contributor

@mansi-jain-1206 mansi-jain-1206 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

Copy link
Member

@mansisinha mansisinha left a comment

Choose a reason for hiding this comment

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

LGTM, Also this method of identifying whether the setup is greenfield/brownfield can be used in future as well to extend some of our functionalities. Thanks!

@rrbadiani rrbadiani merged commit 79b9b9d into 7.4.x Mar 10, 2025
2 checks passed
@rrbadiani rrbadiani deleted the fix-data-dir-race-condn-74-ANSIENG-4200 branch March 10, 2025 04:44
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.

None yet

3 participants