-
Notifications
You must be signed in to change notification settings - Fork 301
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
DAOS-12751 control: Add a daos filesystem evict command. #12331
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
5fdc6a7
DAOS-12751 tool: Add a daos filesystem evict command.
ashleypittman f051fe0
Revise test.
ashleypittman 5273d95
Merge branch 'master' into amd/dfuse-evict
ashleypittman 400c9f0
Fix test.
ashleypittman 5a5dfe1
Remove dead code.
ashleypittman ad83f4b
Add a test and make it work.
ashleypittman e4a64b5
Add documentation.
ashleypittman dc37309
Remove debugging.
ashleypittman 4bb9c76
Use macro.
ashleypittman 6f08396
Merge branch 'master' into amd/dfuse-evict
ashleypittman f435405
Fix build.
ashleypittman 6530951
Merge branch 'master' into amd/dfuse-evict
ashleypittman 75db207
Fix typo and improve output.
ashleypittman d385d0f
Merge branch 'master' into amd/dfuse-evict
ashleypittman 03a6c97
Take on review feedback.
ashleypittman e8aa287
Merge branch 'master' into amd/dfuse-evict
ashleypittman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is ONLY for dfuse - not DFS - maybe
evict
->dfuse-evict
?Or maybe it's okay if it only makes sense for dfuse anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only thing al these commands have in common is they're for POSIX containers and therefore DFS at some level.
evict
doesn't mean anything in dfs directly that I'm aware of.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing i was thinking about here is that the daos fs options are on the container (server side), but these dfuse commands apply only on the client side (just the node they are running on). so they are just local operations and only executed on the node where this runs.
i don't know if that counts as grounds that we need to split this off into 2 tools or just have an extension to the dfuse command, or just keep it as it is. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this extensively in the working group calls, this was your suggestion.
I'd be fine with making a new subcommand for the
daos
command other thandaos filesystem
but I don't want to do it in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i know, and i was just rethinking this. im fine to keeping it that way for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should just add a comment that this is a local client operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree though, there's a clear distinction between commands that have an effect on the container itself vs commands that change local in-memory state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something else to keep in mind is that we currently don't guarantee backwards compatibility for the command line, but I wonder if that will/should be a requirement in the future, since changing the command line interface later can be disruptive to deployment, job setup/cleanup, etc.