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 of initContainers and sideCars in poddefault #6749

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

deepk2u
Copy link
Contributor

@deepk2u deepk2u commented Nov 17, 2022

This PR resolve #6745

@deepk2u
Copy link
Contributor Author

deepk2u commented Nov 17, 2022

/assign @kimwnasptd

@kimwnasptd
Copy link
Member

@deepk2u thanks for the PR! I have this in my list of PRs to review and will get to it next week.

From a high level overview, the addition of initContainers and sidecars looks good. One concern I had is that sidecars are only used for appending values. So what about users that might need to use PodDefaults to define an explicit list of containers to enforce in a Pod?

But on a second though would expect a huge minority to be interested in such a feature. So we could look into that in the future once/if we have a more concrete use-case.

@deepk2u
Copy link
Contributor Author

deepk2u commented Dec 14, 2022

So what about users that might need to use PodDefaults to define an explicit list of containers to enforce in a Pod?

I think that might be another specific use case, we can implement that if needed

@deepk2u
Copy link
Contributor Author

deepk2u commented Dec 14, 2022

@kimwnasptd let me know your thoughts after reviewing your PR, we would like to get this feature in the next release, if possible.

@kimwnasptd
Copy link
Member

Picking this up, thank you for the patience @deepk2u. I'm confident that this will make it in the KF 1.7 release.

As a first step, I'd like us to introduce an admission-webhook/examples folder and start placing yaml files there that users could try out. Then we can use this source of truth as context for updating the docs in kubeflow.org with use-cases around PodDefaults.

@deepk2u could you create that folder in this PR and add one/some(?) example PodDefault yamls that will be setting initContainers and extra Containers?

@deepk2u
Copy link
Contributor Author

deepk2u commented Dec 16, 2022

@kimwnasptd really great idea. I added the examples

@kimwnasptd
Copy link
Member

Tested the PR and everything works as expected and the test pass. Nice work @deepk2u!

One thing I realized is that we don't have an automated action for running the unit tests for the PodDefaults webhook. @deepk2u would you have cycles to send a separate PR to have a GH Action similar to the Notebook Controller one to run the tests on each PR?
https://github.com/kubeflow/kubeflow/blob/master/.github/workflows/notebook_controller_unit_test.yaml

@kimwnasptd
Copy link
Member

I'll go forward and merge this PR. @deepk2u the next step will be to update the docs to expose this functionality to end users. There's some other small refactor work in the docs I want to do so I can send the first PR to setup the structure and then I'll ping you, so that we can continue with a docs PR.

Thanks for your time and patience!
/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Jan 13, 2023
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 5a720a0 into kubeflow:master Jan 13, 2023
@kimwnasptd
Copy link
Member

@orfeas-k I think I actually spotted the first bug from this feature. In the notebooks details page we'll need to slightly modify the logic on how to extract the ENV Vars from PodDefaults.

Could you take a look at this? For reference this is the error I see in the dev console

ERROR TypeError: Cannot read properties of undefined (reading 'map')
    at t.envExistsInGroupConfiguration (overview.component.ts:509:10)
    at t.generatePodEnv (overview.component.ts:434:20)
    at set pod [as pod] (overview.component.ts:63:10)
    at lo (shared.ts:2074:16)
    at Fs (shared.ts:998:5)
    at qo (property.ts:42:5)
    at dG (notebook-page.component.html:33:15)
    at Ts (shared.ts:518:5)
    at Es (shared.ts:371:7)
    at shared.ts:1677:9

You'll also need to:

  1. Build the new PodDefaults webhook image and apply it to your cluster
  2. Apply the PodDefault in the example dir of the webhook
  3. Create a notebook that will use this PodDefault

@orfeas-k
Copy link
Contributor

Sent a PR with a fix for this bug @kimwnasptd here #6903. Note that in order to reproduce the bug I needed to edit the example and use kubeflow-user instead of kubeflow as a namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support of initContainers and sideCars in poddefault
3 participants