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.
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
DAOS-12751 control: Add a daos filesystem evict command. #12331
Changes from 14 commits
5fdc6a7
f051fe0
5273d95
400c9f0
5a5dfe1
ad83f4b
e4a64b5
dc37309
4bb9c76
6f08396
f435405
6530951
75db207
d385d0f
03a6c97
e8aa287
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.