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 support for flux option flags #49

Merged
merged 19 commits into from
Dec 22, 2022
Merged

add support for flux option flags #49

merged 19 commits into from
Dec 22, 2022

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Dec 17, 2022

this adds support for fluxOptionFlags to be defined on the level of the cluster. We check if the variable is defined, and if not, we do not attempt to export in the environment, as the base container might already specify a preference. To test this fully I am waiting for the osu benchmarks container to build, and then I can test it here with an example workflow.

Signed-off-by: vsoch [email protected]

@vsoch
Copy link
Member Author

vsoch commented Dec 18, 2022

Okay here is what is needed for merge to be acceptable:

  • a new working workflow that uses option flags running successfully
  • verifying that previous workflows still work
  • it's time to figure out testing. it's not acceptable that I haven't, going to look at https://github.com/kubeflow/mpi-operator/blob/5be2a42bf5af1fc9e5d1d96059f1f2dac489061d/Makefile#L58-L67. We need to be able to programmatically test the workflows we expect to work and verify that. Note that this is checked here and will be further handled in Testing Meta Issue #51 - we have basic tests added here for simple workflows (validating output and return codes) but need more substantial testing.
  • As a result of adding this flag, I've refactored the entrypoint for a flux runner (a wait.sh) to now be a wait<N>.sh that corresponds to the container index, where theoretically we could have more than one flux runner with slight variance of logic. I did this because with the flux + spack container I found I couldn't get anything working without a source of the spack environment in the entrypoint, and some tweaks to the command to run flux. Thus I need to be able to define these in the CRD for the workflow associated with this container.

this adds support for fluxOptionFlags to be defined on the level
of the cluster. We check if the variable is defined, and if not,
we do not attempt to export in the environment, as the base container
might already specify a preference. To test this fully I am waiting
for the osu benchmarks container to build, and then I can test it
here with an example workflow.

Signed-off-by: vsoch <[email protected]>
making a custom container (not from flux-sched
) is more challenging than I anticipated. I seem
to have flux starting but I do not see the server
output so something might be up. Tired today will
pick up tomorrow. Err it is tomorrow so later today
I guess :p

Signed-off-by: vsoch <[email protected]>
@vsoch vsoch force-pushed the add/flux-option-flags branch 2 times, most recently from 535388c to 14dd6eb Compare December 20, 2022 21:16
…ntainer

this has been tested with and without option args and seems
to work for both! We are also using more proper templating
in go instead of what I was doing before. Next I need to be
able to write tests to ensure these workflos do not break.

Signed-off-by: vsoch <[email protected]>
@vsoch vsoch force-pushed the add/flux-option-flags branch from 83584b6 to 8fb265c Compare December 22, 2022 00:47
@vsoch
Copy link
Member Author

vsoch commented Dec 22, 2022

Lots of pushing (sorry for the noise!) but we're getting somewhere! I have a setup that seems to be able to run a named test, get the output after a known time, and then compare it to what is expected.
image

I'll pick up on writing more of these after dinner - I got a weird GitHub ratelimiting error but hopefully that was ephemeral.

@vsoch vsoch mentioned this pull request Dec 22, 2022
5 tasks
@vsoch vsoch merged commit 073b4ef into main Dec 22, 2022
@vsoch vsoch deleted the add/flux-option-flags branch December 22, 2022 06:06
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.

1 participant