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

Kubernetes adapter updates #250

Closed
wants to merge 1 commit into from
Closed

Kubernetes adapter updates #250

wants to merge 1 commit into from

Conversation

treydock
Copy link
Contributor

  • Rework how configmap is defined for several purposes
    • Support changing mountPath for different files in configmap
    • Support mounting subPath files in configmap
    • Support mounting files that might not be defined statically but instead generated in init container
  • Add some helper environment variable to both main and init container

@treydock treydock requested a review from johrstrom March 16, 2021 19:11
* Rework how configmap is defined for several purposes
  * Support mounting at subPath
  * Support mounting files that might not be defined statically but instead generated in init container
* Add some helper environment variable to both main and init container
@@ -1,12 +1,28 @@
module OodCore::Job::Adapters::Kubernetes::Resources
require_relative 'batch'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this require here, seems like it'd be cyclical.

Comment on lines +51 to +57
- name: HOME
value: "<%= home_dir %>"
- name: GROUP
value: "<%= group %>"
- name: GID
value: "<%= run_as_group %>"
<%- unless spec.container.env.empty? -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we should merge with what the app supplies. Like we supply these items by default, but the app can override them.

I'm thinking about use cases where the k8s cluster doesn't mount a real shared NFS so the user's actual $HOME may not exist in the container and cause problems because HOME is a well known environment variable.

@johrstrom
Copy link
Contributor

Can we break this up into 2 PRs? One for the configmap updates and the other for the environment updates.

I think the configmap updates look OK, though I just need to lookup the k8s documentation on those fields for my own edification. But existing test cases didn't need a lot of updates, so that's a good sign.

@treydock
Copy link
Contributor Author

Will need to merge configmap updates first and then the env updates will have to get updated to ensure the new test file has appropriate env values for testing. I opened #251 for confgmap changes, will open another PR for environment variable changes.

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.

2 participants