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

cephfs_mirror: correctly set top level dir permissions #50049

Conversation

mchangir
Copy link
Contributor

@mchangir mchangir commented Feb 9, 2023

Fixes: https://tracker.ceph.com/issues/58678
Signed-off-by: Milind Changire mchangir@redhat.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@github-actions github-actions bot added cephfs Ceph File System tests labels Feb 9, 2023
@mchangir mchangir self-assigned this Feb 9, 2023
@mchangir mchangir force-pushed the cephfs_mirror-correctly-set-top-level-dir-permissions branch 3 times, most recently from 881096d to 0f6d6b1 Compare February 10, 2023 06:20
@mchangir
Copy link
Contributor Author

Teuthology job with the new integration test.

@mchangir mchangir marked this pull request as ready for review February 10, 2023 08:56
<< cpp_strerror(r) << dendl;
return r;
}
r = ceph_fchmod(m_remote_mount, fh.r_fd_dir_root, tstx.stx_mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do this when creating the directory structure in PeerReplayer::try_lock_directory()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mostly because this has nothing to do with directory locking

Copy link
Contributor

Choose a reason for hiding this comment

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

True, but that's when the remote directory is created, and it makes sense to fix the perms at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mchangir ping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the top-level dir permissions change over time ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. What if the mirror daemon could just adjust the permissions after it acquired the remote lock on the directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revised the code

@mchangir mchangir force-pushed the cephfs_mirror-correctly-set-top-level-dir-permissions branch 5 times, most recently from c80a56e to 8e2baf3 Compare March 4, 2023 04:52
@mchangir
Copy link
Contributor Author

mchangir commented Mar 4, 2023

Teuthology Job: successful

@vshankar vshankar requested a review from a team March 6, 2023 13:38
@mchangir mchangir force-pushed the cephfs_mirror-correctly-set-top-level-dir-permissions branch from 8e2baf3 to 9f0c444 Compare March 7, 2023 02:10
@mchangir
Copy link
Contributor Author

mchangir commented Mar 7, 2023

jenkins test make check arm64

Fixes: https://tracker.ceph.com/issues/58678
Signed-off-by: Milind Changire <mchangir@redhat.com>
Signed-off-by: Milind Changire <mchangir@redhat.com>
@mchangir mchangir force-pushed the cephfs_mirror-correctly-set-top-level-dir-permissions branch from 9f0c444 to 7849939 Compare March 7, 2023 13:34
Copy link
Contributor

@dparmar18 dparmar18 left a comment

Choose a reason for hiding this comment

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

LGTM

vshankar added a commit to vshankar/ceph that referenced this pull request Mar 13, 2023
* refs/pull/50049/head:
	qa: add cephfs_mirror test case to check root dir modes
	cephfs_mirror: sync snap dir root mode with remote dir

Reviewed-by: Dhairya Parmar <dparmar@redhat.com>
Reviewed-by: Venky Shankar <vshankar@redhat.com>
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

@vshankar
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cephfs Ceph File System tests
Projects
None yet
3 participants