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

core: Allow ceph.conf to be updated if already exists #11399

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

travisn
Copy link
Member

@travisn travisn commented Dec 7, 2022

Description of your changes:
The ceph.conf was being written with 0x444 permissions, which means the file could not be updated if it already exists. Specifically, when an osd is purged, if the operation is retried multiple times, only the first operation was succeeding. The subsequent requests to purge an osd were failing due to the config already existing. If we make the config writeable with 0x644 then the operator can always write the file.

A small typo is also fixed in the osd purge message.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

The ceph.conf was being written with 0x444 permissions, which means
the file could not be updated if it already exists. Specifically, when
an osd is purged, if the operation is retried multiple times, only
the first operation was succeeding. The subsequent requests to purge
an osd were failing due to the config already existing. If we make
the config writeable with 0x644 then the operator can always write the file.

Signed-off-by: Travis Nielsen <tnielsen@redhat.com>
The OSD purge was printing an incorrect sentence, so this now
fixes the grammar.

Signed-off-by: Travis Nielsen <tnielsen@redhat.com>
@@ -293,7 +293,7 @@ func WriteCephConfig(context *clusterd.Context, clusterInfo *ClusterInfo) error
if err != nil {
return errors.Wrap(err, "failed to copy connection config to /etc/ceph. failed to read the connection config")
}
err = ioutil.WriteFile(DefaultConfigFilePath(), src, 0444)
err = ioutil.WriteFile(DefaultConfigFilePath(), src, 0600)
Copy link
Member

Choose a reason for hiding this comment

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

Just to check to give this update permission to ceph.conf doesn't harm or create any secruity problem

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the operator has access to this inside the pod, so it will be fine. It's really just giving write access to itself. Notice in other places in the operator code we also use 0600 for some other config files.

Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

approving, with one question, what was not updating in ceph.conf or what osd purge was trying to update in ceph.conf?

@travisn
Copy link
Member Author

travisn commented Dec 12, 2022

approving, with one question, what was not updating in ceph.conf or what osd purge was trying to update in ceph.conf?

The command is generating the ceph.conf needed to run the ceph commands during the purge, whether the command is run in its own pod or as part of the operator pod. It's just a different path than the path where the operator usually generates the ceph.conf.

@travisn travisn merged commit d532e27 into rook:master Dec 12, 2022
@travisn travisn deleted the osd-purge-config branch December 13, 2022 21:51
mergify bot added a commit that referenced this pull request Dec 16, 2022
core: Allow ceph.conf to be updated if already exists (backport #11399)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants