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

teuthology: Add support for seek and sync in write_file #2010

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chrisphoffman
Copy link

@chrisphoffman chrisphoffman commented Oct 25, 2024

Add support for seek and sync in write_file.

This patch will allow for partial writes and sync on completion of write.

This is to support strided writes for cephfs on a multi-mount system.

teuthology/misc.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kshtsk kshtsk left a comment

Choose a reason for hiding this comment

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

Interesting idea, I would suggest to address the comments and maybe add unit tests.

@chrisphoffman
Copy link
Author

Interesting idea, I would suggest to address the comments and maybe add unit tests.

Thanks for the review. Comments addressed and tests added. PTAL

@chrisphoffman chrisphoffman requested a review from kshtsk November 18, 2024 15:43
Comment on lines +233 to +242

@patch("teuthology.orchestra.remote.RemoteShell.write_file")
def test_write_file_offset(self, m_write_file):
self.c.write_file("filename", "content", bs=1, offset=1024)
m_write_file.assert_called_with("filename", "content", bs=1, offset=1024)

@patch("teuthology.orchestra.remote.RemoteShell.write_file")
def test_write_file_sync(self, m_write_file):
self.c.write_file("filename", "content", sync=True)
m_write_file.assert_called_with("filename", "content", sync=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are test not for the remote.write_file but for cluster.remote_write, I guess there should be new tests added to test/test_remote.py module instead.

Comment on lines +105 to +106
def write_file(self, file_name, content, sudo=False, perms=None, owner=None,
**kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is unrelated directly to write_file. To keep PR simple if you really need seek and sync in cluster wide call, maybe create another PR.

@@ -276,11 +277,20 @@ def write_file(self, path, data, sudo=False, mode=None, owner=None,
:param owner: set file owner if provided
:param mkdir: preliminary create the file directory, defaults False
:param append: append data to the file, defaults False
:param bs: write up to N bytes at a time, defaults 512
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess default is None, maybe just give a reference to dd, just add another line right after Write data to remote file above, like:

The data written in 512-byte blocks, provide `bs` to use bigger blocks.

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.

2 participants