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

Modify fio job files to use ConfigMap #12

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

Conversation

hookak
Copy link

@hookak hookak commented Aug 5, 2024

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:

  • Requires two apply commands to run the workload:
k apply -f fio-config.yaml
k apply -f fio.yaml

Pros:

  • Easier to modify workloads through the ConfigMap.

Please review and provide feedback on these points. @derekbit

related: longhorn/longhorn#9100

@hookak hookak changed the title Modify fio job files to use ConfigMap [WIP] Modify fio job files to use ConfigMap Aug 8, 2024
@derekbit
Copy link
Member

derekbit commented Aug 9, 2024

Hello @hookak
DCO check failed. Please sign off your commit by git commit -s. Thank you.

Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

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

LGTM

@hookak
Copy link
Author

hookak commented Aug 9, 2024

Thanks for the review! @derekbit

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.

As mentioned earlier, there's a dependency on PR #9. Do you have plans to merge that?"

@derekbit
Copy link
Member

@hookak
#9 needs to refine later due to #9 (review). Can you update the PR without the dependency on #9? Then, we can merge this PR first. WDYT?

@hookak
Copy link
Author

hookak commented Aug 21, 2024

Thanks for the reply, @derekbit.

I reviewed PR #9, but it's difficult to remove the dependency.
It might be better to update this after #9 is merged.

Please let me know once #9 is merged. Thanks!

@derekbit
Copy link
Member

@hookak
Can you merge fio-config.yaml into fio.yaml and then delete fio-config.yaml ? After that, we can merge it. Thank you.

content of fio-config.yaml
---
content of fio.yaml

@hookak hookak force-pushed the fio-job-to-configmaps branch 2 times, most recently from affd4cd to 04fb941 Compare August 28, 2024 10:10
@hookak
Copy link
Author

hookak commented Aug 28, 2024

Sure, I can do that. @derekbit
However, if we make the changes, the contents of fio-config.yaml will be duplicated in both fio.yaml and fio-cmp.yaml.
It should be updated as follows. Please review the changes.

@derekbit
Copy link
Member

Sure, I can do that. @derekbit However, if we make the changes, the contents of fio-config.yaml will be duplicated in both fio.yaml and fio-cmp.yaml. It should be updated as follows. Please review the changes.

Got it. I'm fine with it if we still want to support one command execution like kubectl apply -f https://raw.githubusercontent.com/yasker/kbench/main/deploy/fio.yaml.

@hookak hookak changed the title [WIP] Modify fio job files to use ConfigMap Modify fio job files to use ConfigMap Aug 29, 2024
Copy link
Member

@innobead innobead left a 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.

@hookak hookak force-pushed the fio-job-to-configmaps branch 2 times, most recently from f33d1a8 to 72ef9e4 Compare September 9, 2024 10:31
@hookak
Copy link
Author

hookak commented Sep 9, 2024

I agree with your comment because I also want to avoid duplicate fio files. However, if we make the config maps empty in fio.yaml, it will override custom configs when I want to use them.

Therefore, I suggest creating a fio-custom.yaml for those who want to use custom configs, and checking if any customized fio files are defined in run.sh.
Since most users are expected to use the default config, I think this is a reasonable change.

Could you please review this? @innobead @derekbit

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.

3 participants