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

Adds profiles component #2

Merged
merged 8 commits into from
Mar 14, 2019
Merged

Conversation

swiftdiaries
Copy link
Member

@swiftdiaries swiftdiaries commented Feb 27, 2019

Link to ksonnet components this PR changes:
Profiles

Mapping ksonnet parameters to kustomize is kind of hard. For installing kubeflow to a custom namespace, with ksonnet we set an app level ks env with the namespace.

Kustomize does support a global namespace but it's unable to set it inside spec fields like in ambassador's deployment.

Opened an issue in kustomize for this: kubernetes-sigs/kustomize#854
I'm currently investigating how to add multiple parameters like we have in ksonnet to kubeflow manifests.

With respect to a ksonnet package having multiple prototypes, I'm currently treating them as separate manifests. See pytorch and pytorch-job in PR(#5)
This works well for pytorch-job because the only parameter is the image. And that can be set in straight-forward way using kustomize edit set image <image name> from inside the manifest folder.

/cc @jlewi @kkasravi


This change is Reviewable

@swiftdiaries
Copy link
Member Author

/assign @kkasravi

@jlewi jlewi changed the title Adds profiles and jupyter components [wip] Adds profiles and jupyter components Feb 28, 2019
@jlewi
Copy link
Contributor

jlewi commented Feb 28, 2019

Marking this WIP because its blocked on other PRs. When the other PRs are merged and this one is updated remove WIP (this way I'll know its ready for another look).

@swiftdiaries
Copy link
Member Author

/assign @jlewi
Ready for review.

@swiftdiaries swiftdiaries changed the title [wip] Adds profiles and jupyter components Adds profiles and jupyter components Mar 1, 2019
@jlewi
Copy link
Contributor

jlewi commented Mar 1, 2019

Can you please update the PR description to include the following

  • Links to the ksonnet packages these are based on
  • How you are mapping whatever customizability exists in the ksonnet package to the Kubeflow package?
    • In particular, does the ksonnet package have multiple parameters? If so how are they mapped to kustomize?
    • If there are multiple ksonnet prototypes how are these mapped to kustomize?

Also going forward it might be good to do a single package per PR.

@swiftdiaries
Copy link
Member Author

Ok, that makes sense. Will update the PR with comments and split out future PRs into individual components.

@jlewi
Copy link
Contributor

jlewi commented Mar 1, 2019

Thanks.

app: profiles
spec:
containers:
- image: metacontroller/jsonnetd@sha256:25c25f217ad030a0f67e37078c33194785b494569b0c088d8df4f00da8fd15a0
Copy link
Contributor

Choose a reason for hiding this comment

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

the image parameter (normally passed in ksonnet) isn't being set. Normally it would be
kustomize edit set image metacontroller/jsonnetd@sha256:25c25f217ad030a0f67e37078c33194785b494569b0c088d8df4f00da8fd15a0

I think it would be a good idea to note the parameters that ksonnet honors somewhere

@@ -5,6 +5,16 @@ A repository for Kustomize manifests

`go get -u github.com/kubernetes-sigs/kustomize`

## List of Kubeflow components available

* Ambassador
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe list parameters that are used from ksonnet here so it's easier to track

README.md Outdated
* Jupyter

* Profiles

## Basic Usage
Copy link
Contributor

Choose a reason for hiding this comment

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

@swiftdiaries
Copy link
Member Author

Updated the PR comments. And added parameters from ksonnet to the README to track

@jlewi
Copy link
Contributor

jlewi commented Mar 6, 2019

Sorry for the slow reply. Since this is one of the first kustomize package conversions I'd like to take a close look. I'm a bit swamped today but I will do my best to make time to look at it.

@swiftdiaries
Copy link
Member Author

swiftdiaries commented Mar 6, 2019 via email

Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 57 files at r1.
Reviewable status: 3 of 23 files reviewed, 10 unresolved discussions (waiting on @jlewi and @swiftdiaries)


README.md, line 14 at r5 (raw file):

kustomize build | kubectl apply -f

Bridging kustomize and ksonnet

Do we need to reference ksonnet in the README?

Eventually we will want to remove all references to ksonnet.

If you want to provide transition instructions maybe put them in a separate doc that we can just delete one we're done.


README.md, line 54 at r5 (raw file):

* Ambassador

### ksonnet parameters

Why are we referencing ksonnet here? Per above if if its information related to migration lets put in a separate doc.


README.md, line 63 at r5 (raw file):

      name: "ambassador",
      platform: "none",
      replicas: 3,

So for now if someone wanted to change the number of replicas would they have to either modify the base or use an overlay?

For the other parameters I don't think there's a strong need to easily expose them.


README.md, line 85 at r5 (raw file):

```json
jupyter: {

This is JupyterHub right? I think we can just remove all JupyterHub components because we won't be using it in 0.5


jupyter/base/jupyter-config-map.yaml, line 14 at r5 (raw file):

    Note that this file is also responsible for importing the UI-specific Spawner
    class from <ui-dir>/spawner.py, and setting the `spawner_class` configuration
    option.

Is this configmap only for JupyterHub?

If so then maybe we don't even need to add it or any other resources for JupyterHub
because with 0.5 we won't be deploying jupyterHub.


jupyter/base/jupyter-statefulset.yaml, line 2 at r5 (raw file):

apiVersion: apps/v1beta1
kind: StatefulSet

Per comment above; lets not add jupyterhub related components.


profiles/base/profiles-composite-controller-rbac.yaml, line 8 at r5 (raw file):

    name: profiles
    namespace: default
    server: https://35.203.186.20

Why is server specified here?

@jlewi
Copy link
Contributor

jlewi commented Mar 7, 2019

We are likely moving away from Ambassador so i don't think its super critical to have a convenient way to set the namespace for Ambassador.

@swiftdiaries
Copy link
Member Author

Ohh cool. I'll modify the README and add a separate doc for migration.
For number of replicas, the current usage is that you have an overlay defined for it.
For common edits/variants, I think it'll be better if we provide manifests and overlays OTB. Docs that point to how you use/set those variants.

Ok noted. Will remove all references to JupyterHub.

@jlewi
Copy link
Contributor

jlewi commented Mar 8, 2019

@swiftdiaries Thanks is this ready for another look or are you still working on it?

@swiftdiaries
Copy link
Member Author

swiftdiaries commented Mar 8, 2019 via email

@swiftdiaries swiftdiaries changed the title Adds profiles and jupyter components Adds profiles component Mar 13, 2019
@swiftdiaries
Copy link
Member Author

@jlewi This is ready for another look.
I'm currently referencing the profiles controller kustomization from within my fork. Once kubeflow/kubeflow#2692 merges, I can replace it in the next commit.

@jlewi
Copy link
Contributor

jlewi commented Mar 14, 2019

/lgtm
/approve
Please file an issue to fix the reference to the kustomize package.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@k8s-ci-robot k8s-ci-robot merged commit 7823858 into kubeflow:master Mar 14, 2019
@swiftdiaries swiftdiaries deleted the kustomize branch April 19, 2019 21:06
Tomcli pushed a commit to Tomcli/manifests that referenced this pull request Sep 23, 2020
[IBM] Split stack to components for customization
Tomcli pushed a commit to Tomcli/manifests that referenced this pull request Dec 11, 2020
Tomcli added a commit to Tomcli/manifests that referenced this pull request Jul 19, 2021
@jbottum jbottum mentioned this pull request Dec 20, 2022
1 task
google-oss-prow bot pushed a commit that referenced this pull request Jun 21, 2024
* Make the kubeflow-m2m-oidc-configurator a CronJob

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

* cronjob.kubeflow-m2m-oidc-configurator: concurrencyPolicy: Forbid

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

* add tests/gh-actions/wait_for_kubeflow_m2m_oidc_configurator.sh

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

* Improve wait routine for m2m oidc configurator (#2)

It was tested with self-hosted runner using custom dockerconfig credentials for debugging.

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

* use docker.io/curlimages/curl for cronjob.kubeflow-m2m-oidc-configurator.yaml

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

* make the m2m oidc configurator idempotent

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

* verify jwks configuration in requestauthentication

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>

---------

Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski94@gmail.com>
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.

None yet

4 participants