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 some more ctdb related hacks #124

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

phlogistonjohn
Copy link
Collaborator

Add some additional options to the ctdb related sambacc commmands. The --write-node option in particular is a bit hacky but it helps me get on with prototyping samba+ctdb in containers under ceph.

Add the cluster_meta_to_nodes function that will simply take a
clustermeta object as an input and write out a corresponding nodes
file for ctdb.

Signed-off-by: John Mulligan <[email protected]>
Add an archive_tdb function that archives (moves) the tdb files that
converted into ctdb backing files.

Signed-off-by: John Mulligan <[email protected]>
Add a --write-node option to the ctdb-must-have-node command so that
containers without a shared file system can at least join a ctdb cluster.
This a bit hacky but it works to test things out.

Signed-off-by: John Mulligan <[email protected]>
Passing this argument with a path will archive any of the tdb files that
get migrated to ctdb to the specified path. This moves files from the
typical location to aid in debugging (it's more obvious that this
instance is using ctdb) and for comparison in case something needs to
be compared post-conversion.

Signed-off-by: John Mulligan <[email protected]>
@phlogistonjohn phlogistonjohn marked this pull request as ready for review June 30, 2024 14:23
@phlogistonjohn
Copy link
Collaborator Author

cc @avanthakkar

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

Looks fine, minor style comments

Comment on lines +502 to +503
nffh.flush()
os.fsync(nffh)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please remind me why we need this pattern of doing both flush and os.fsync ?
It is repeated in this module; maybe have a local helper-function with minimal doc-string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be fair it is a copy-paste that was tweaked. The goal of the pattern is to call flush to make sure any buffers internal to the process get flushed to the OS. Then fsync tells the os to flush any buffers it has and ensure the entire file is written to the storage device. Normally, flush is not required as it is done also by close, but to use fync we need an fd - so we end up having to manually flush first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I still think it would be nice to have it local helper-function but this can wait to future cleanups PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, if you note I'm not writing very many tests for some of this stuff either. Normally, I'd criticize myself for that but I'm doing a bunch of prototyping simultaneously on the ceph codebase and sambacc. I'm trying to get as many of these into the codebase here in sambacc so I don't end up with massive private branches on both sides. :-)
I'd welcome anyone to jump in and help contribute tests and cleanups on top. :-)

Comment on lines +597 to +599
for tdbfile in _SRC_TDB_FILES:
for parent in _SRC_TDB_DIRS:
tdb_path = os.path.join(parent, tdbfile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of double nested for loop, consider something like:

from itertools import product
from pathlib import Path

for tdb_path in [ Path(p) / f for p, f in product(_SRC_TDB_DIRS, _SRC_TDB_FILES) ]:
    ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how the older, migrate functions is written. I don't want to deviate from the pattern now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough

@synarete synarete self-requested a review July 1, 2024 13:52
@phlogistonjohn
Copy link
Collaborator Author

@synarete these answers good enough? && ping @anoopcs9

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +502 to +503
nffh.flush()
os.fsync(nffh)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I still think it would be nice to have it local helper-function but this can wait to future cleanups PR.

@phlogistonjohn
Copy link
Collaborator Author

@anoopcs9 2nd review please?
I now have #126 that depends on this PR as well.

@mergify mergify bot merged commit 59f896e into samba-in-kubernetes:master Jul 2, 2024
9 checks passed
@phlogistonjohn phlogistonjohn deleted the jjm-ctdb-2 branch July 2, 2024 18:19
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.

3 participants