-
Notifications
You must be signed in to change notification settings - Fork 37
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
Modify fio job files to use ConfigMap #12
base: main
Are you sure you want to change the base?
Conversation
Hello @hookak |
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.
LGTM
d894f7f
to
b9225bf
Compare
@hookak |
@hookak
|
affd4cd
to
04fb941
Compare
Sure, I can do that. @derekbit |
Got it. I'm fine with it if we still want to support one command execution like |
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.
This change will duplicate fio files which we maintain in the fio folder as our default profiling configs.
To prevent duplication, I suggest the below changes:
- make the config map empty by default
- modify run.sh to check whether any customized fio files defined in the config map. If the customized fio file is found, use it, or fallback to use the default one.
f33d1a8
to
72ef9e4
Compare
Signed-off-by: jinhong.kim0 <[email protected]>
72ef9e4
to
f79b820
Compare
I agree with your comment because I also want to avoid duplicate fio files. However, if we make the config maps empty in Therefore, I suggest creating a |
I’ve implemented the idea discussed in the issue. This PR depends on the previous #9, so I will make further changes after that PR is merged.
If we use *.fio files in a ConfigMap:
Cons:
Pros:
Please review and provide feedback on these points. @derekbit
related: longhorn/longhorn#9100