Add SMP attributes to KfDef #4181

jbrette · 2019-09-24T07:24:10Z

This simple PR opens the door two three important things for kubeflow

It creates the KfDef.yaml CustomResource definition, which let you load the KfDef K8s namespace as follow. This helps by the way for syntax checks of the KfDef
This allows SMP to work properly. See the second comment. Basically you can use kustomize to patch, comment out some of the KfDef.yaml (like removing istio if already have it).
This starts to open the door using automatic kustomize variables and drastically reduce the complexity of the manifest. We should be able to have variables in the manifest that looks like:
$(KfDef.istio.parameters.namespace)... without the need for intermediate text file, configmaps.

This is not part of the current PR:
Note 1: This is mainly to leverage SMP on the client side.
Note 2: We only instrumented the KfDef. We should be able to instrument the plugins if necessary
Note 3: kfctl is linking with kustomize code and has plugins like kustomize. It is also linking with the KfDef code itself. There is no reason why we could move/convert the "kustomize plugin" into kfctl. and do something like kubectl is doing. kubectl is offering kubectl kustomize. Kubeflow code do a kfctl kustomize .... This is something we are seriously considering on our project

This change is

k8s-ci-robot · 2019-09-24T07:24:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kunmingg
You can assign the PR to them by writing /assign @kunmingg in a comment when ready.

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

Needs approval from an approver in each of these files:

bootstrap/OWNERS

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 · 2019-09-24T07:24:24Z

Hi @jbrette. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jbrette · 2019-09-24T07:25:25Z

kubectl potential usage:

../manifests/kfdef$ kubectl apply -k .
customresourcedefinition.apiextensions.k8s.io/kfdefs.kfdef.apps.kubeflow.org created
kfdef.kfdef.apps.kubeflow.org/existing-arrikto created
kfdef.kfdef.apps.kubeflow.org/gcp-basic-auth created
kfdef.kfdef.apps.kubeflow.org/gcp-iap created
kfdef.kfdef.apps.kubeflow.org/k8s-istio created
kfdef.kfdef.apps.kubeflow.org/kubeflow-aws-cognito created
kfdef.kfdef.apps.kubeflow.org/kubeflow-aws created

kubectl get kfdef -n kubeflow
NAME                   AGE
existing-arrikto       11s
gcp-basic-auth         11s
gcp-iap                11s
k8s-istio              11s
kubeflow-aws           10s
kubeflow-aws-cognito   10s

kubectl describe kfdef k8s-istio -n kubeflow

Name:         k8s-istio
Namespace:    kubeflow
Labels:       <none>
Annotations:  kubectl.kubernetes.io/last-applied-configuration:
                {"apiVersion":"kfdef.apps.kubeflow.org/v1beta1","kind":"KfDef","metadata":{"annotations":{},"name":"k8s-istio","namespace":"kubeflow"},"sp...
API Version:  kfdef.apps.kubeflow.org/v1beta1
Kind:         KfDef
Metadata:
  Creation Timestamp:  2019-09-24T07:15:12Z
  Generation:          1
  Resource Version:    70099
  Self Link:           /apis/kfdef.apps.kubeflow.org/v1beta1/namespaces/kubeflow/kfdefs/k8s-istio
  UID:                 f9dcc282-a658-4124-aca9-0af2d6acd6a9
Spec:
  Applications:
    Kustomize Config:
      Parameters:
        Name:   namespace
        Value:  istio-system
      Repo Ref:
        Name:  manifests
        Path:  istio/istio-crds
    Name:      istio-crds
    Kustomize Config:
      Parameters:
        Name:   namespace
        Value:  istio-system
      Repo Ref:
        Name:  manifests
        Path:  istio/istio-install
    Name:      istio-install
    Kustomize Config:

jbrette · 2019-09-24T07:31:38Z

We have an example of SMP merge in action using our branch of Kustomize:

# Clone the following branch of Kustomize.
# This branch imports the branch referenced in this PR
git clone --single-branch --branch armadacrd git@github.com:keleustes/kustomize.git

# Build and install kustomize and its plugins.
# Note that this will overwrite the current copy of kustomize,
# as well as any previously compiled plugins stored in $HOME/.config/kustomize
make install build-plugins

# Run through the example for kubeflow. Further instructions are found in the README there.
cd examples/crds/kubeflow-kubedef
cat README.md

This feature will be much more important as Kubernetes moves forward. It will eventually have an effect on how a kubectl patch works on KfDef resources, as Kubernetes continues to improve support for CRDs. See this PR adding CRD validation field in server-side apply for example.

Extract from the example

patch.yaml for kustomize

apiVersion: kfdef.apps.kubeflow.org/v1beta1
kind: KfDef
metadata:
  name: k8s-istio
  namespace: kubeflow
spec:
  applications:
  - kustomizeConfig:
      parameters:
      - name: namespace
        value: patchednamespace
    name: istio-install
  - kustomizeConfig:
      repoRef:
        name: patchedname
        path: patchedpath
    name: metacontroller
  - kustomizeConfig:
      overlays:
      - addbeforeistio
      - istio
    name: centraldashboard
  - kustomizeConfig:
      overlays:
      - patchedoverlay1
      - patchedoverlay2
    name: webhook

The result of kustomize build overlay is:

 kustomize build . --enable_alpha_plugins
apiVersion: kfdef.apps.kubeflow.org/v1beta1
kind: KfDef
metadata:
  name: k8s-istio
  namespace: kubeflow
spec:
  applications:
  - kustomizeConfig:
      parameters:
      - name: namespace
        value: istio-system
      repoRef:
        name: manifests
        path: istio/istio-crds
    name: istio-crds
  - kustomizeConfig:
      parameters:
      - name: namespace
        value: patchednamespace
      repoRef:
        name: manifests
        path: istio/istio-install
    name: istio-install
  - kustomizeConfig:
      parameters:
      - name: clusterRbacConfig
        value: "OFF"
      repoRef:
        name: manifests
        path: istio/istio
    name: istio
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: application/application-crds
    name: application-crds
  - kustomizeConfig:
      overlays:
      - application
      repoRef:
        name: manifests
        path: application/application
    name: application
  - kustomizeConfig:
      repoRef:
        name: patchedname
        path: patchedpath
    name: metacontroller
  - kustomizeConfig:
      overlays:
      - istio
      repoRef:
        name: manifests
        path: argo
    name: argo
  - kustomizeConfig:
      overlays:
      - addbeforeistio
      - istio
      repoRef:
        name: manifests
        path: common/centraldashboard
    name: centraldashboard
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: admission-webhook/bootstrap
    name: bootstrap
  - kustomizeConfig:
      overlays:
      - patchedoverlay1
      - patchedoverlay2
      repoRef:
        name: manifests
        path: admission-webhook/webhook
    name: webhook
  - kustomizeConfig:
      overlays:
      - istio
      - application
      repoRef:
        name: manifests
        path: jupyter/jupyter-web-app
    name: jupyter-web-app
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: katib-v1alpha2/katib-db
    name: katib-db
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: katib-v1alpha2/katib-manager
    name: katib-manager
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: katib-v1alpha2/katib-controller
    name: katib-controller
  - kustomizeConfig:
      overlays:
      - istio
      repoRef:
        name: manifests
        path: katib-v1alpha2/katib-ui
    name: katib-ui
  - kustomizeConfig:
      overlays:
      - istio
      repoRef:
        name: manifests
        path: metadata
    name: metadata
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: katib-v1alpha2/metrics-collector
    name: metrics-collector
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: katib-v1alpha2/suggestion
    name: suggestion
  - kustomizeConfig:
      overlays:
      - istio
      - application
      repoRef:
        name: manifests
        path: jupyter/notebook-controller
    name: notebook-controller
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pytorch-job/pytorch-job-crds
    name: pytorch-job-crds
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pytorch-job/pytorch-operator
    name: pytorch-operator
  - kustomizeConfig:
      parameters:
      - name: namespace
        value: knative-serving
      repoRef:
        name: manifests
        path: knative/knative-serving-crds
    name: knative-crds
  - kustomizeConfig:
      parameters:
      - name: namespace
        value: knative-serving
      repoRef:
        name: manifests
        path: knative/knative-serving-install
    name: knative-install
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: kfserving/kfserving-crds
    name: kfserving-crds
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: kfserving/kfserving-install
    name: kfserving-install
  - kustomizeConfig:
      parameters:
      - initRequired: true
        name: usageId
        value: <randomly-generated-id>
      - initRequired: true
        name: reportUsage
        value: "true"
      repoRef:
        name: manifests
        path: common/spartakus
    name: spartakus
  - kustomizeConfig:
      overlays:
      - istio
      repoRef:
        name: manifests
        path: tensorboard
    name: tensorboard
  - kustomizeConfig:
      overlays:
      - istio
      repoRef:
        name: manifests
        path: tf-training/tf-job-operator
    name: tf-job-operator
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pipeline/api-service
    name: api-service
  - kustomizeConfig:
      parameters:
      - name: minioPvcName
        value: minio-pv-claim
      repoRef:
        name: manifests
        path: pipeline/minio
    name: minio
  - kustomizeConfig:
      parameters:
      - name: mysqlPvcName
        value: mysql-pv-claim
      repoRef:
        name: manifests
        path: pipeline/mysql
    name: mysql
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pipeline/persistent-agent
    name: persistent-agent
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pipeline/pipelines-runner
    name: pipelines-runner
  - kustomizeConfig:
      overlays:
      - istio
      repoRef:
        name: manifests
        path: pipeline/pipelines-ui
    name: pipelines-ui
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pipeline/pipelines-viewer
    name: pipelines-viewer
  - kustomizeConfig:
      repoRef:
        name: manifests
        path: pipeline/scheduledworkflow
    name: scheduledworkflow
  - kustomizeConfig:
      overlays:
      - istio
      parameters:
      - initRequired: true
        name: admin
        value: johnDoe@acme.com
      repoRef:
        name: manifests
        path: profiles
    name: profiles
  - kustomizeConfig:
      overlays:
      - application
      repoRef:
        name: manifests
        path: seldon/seldon-core-operator
    name: seldon-core-operator
  enableApplications: true
  packageManager: kustomize
  repos:
  - name: manifests
    root: manifests-master
    uri: https://github.com/kubeflow/manifests/archive/master.tar.gz
  - name: kubeflow
    root: kubeflow-master
    uri: https://github.com/kubeflow/kubeflow/archive/master.tar.gz
  skipInitProject: true
  useBasicAuth: false
  useIstio: true
  version: master

jbrette · 2019-09-24T07:35:49Z

/assign @kkasravi @jlewi .

One more thing we have the same kind of updates merged in the argo workflows. SMP does not seeems to be very usefull for Istio. It works well for OpenShift and Armada. We will try to see TektonCD tommorrow.

SMP is so powerfull and simplify so many operations.

krishnadurai · 2019-09-24T09:43:35Z

This is great @jbrette !
This is a good start to address committing configs server side.
/cc @krishnadurai

krishnadurai · 2019-09-24T10:40:34Z

@jbrette It, unfortunately, looks like support for SMP in Custom Resouces is not present according to the issue here: kubernetes/kubernetes#58414. You seem to have already added the required patch merge keys to the CRD definition, though it may come with limitations of whether we can support this accross versions of K8s and this CRD.
So should we include support for SMP with kustomize on KFDef CR?

jbrette · 2019-09-24T13:27:16Z

@kkasravi @yanniszark @jlewi Can you add the ok-to-test thing. My acl PR is still pending.

kkasravi · 2019-09-24T13:44:32Z

/ok-to-test

kkasravi · 2019-09-25T04:13:22Z

@jbrette I had added similar patchStrategy keys under kubeflow/bootstrap/config/types.go.
The original set of overlays

config
├── base
└── overlays
    ├── application
    ├── aws
    ├── basic_auth
    ├── gcp
    ├── istio
    ├── ksonnet
    └── kustomize

would do SMP as each overlay kfctl_default.yaml was merged into base/kfctl_default.yaml.
That code remains today - https://github.com/kubeflow/kfctl/blob/master/pkg/kfapp/kustomize/kustomize.go#L1039

googlebot added the cla: yes label Sep 24, 2019

k8s-ci-robot requested review from kkasravi and yanniszark September 24, 2019 07:24

k8s-ci-robot added needs-ok-to-test size/XXL labels Sep 24, 2019

k8s-ci-robot requested a review from krishnadurai September 24, 2019 09:43

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 24, 2019

jbrette force-pushed the smp branch 3 times, most recently from 6a04d20 to 0dbadea Compare September 24, 2019 15:57

Add SMP attributes to KfDef

f2b60bb

jbrette force-pushed the smp branch from 0dbadea to f2b60bb Compare September 24, 2019 16:56

jbrette closed this Sep 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SMP attributes to KfDef #4181

Add SMP attributes to KfDef #4181

jbrette commented Sep 24, 2019 •

edited

Loading

k8s-ci-robot commented Sep 24, 2019

k8s-ci-robot commented Sep 24, 2019

jbrette commented Sep 24, 2019

jbrette commented Sep 24, 2019 •

edited

Loading

jbrette commented Sep 24, 2019

krishnadurai commented Sep 24, 2019

krishnadurai commented Sep 24, 2019

jbrette commented Sep 24, 2019

kkasravi commented Sep 24, 2019

kkasravi commented Sep 25, 2019

Add SMP attributes to KfDef #4181

Add SMP attributes to KfDef #4181

Conversation

jbrette commented Sep 24, 2019 • edited Loading

k8s-ci-robot commented Sep 24, 2019

k8s-ci-robot commented Sep 24, 2019

jbrette commented Sep 24, 2019

jbrette commented Sep 24, 2019 • edited Loading

jbrette commented Sep 24, 2019

krishnadurai commented Sep 24, 2019

krishnadurai commented Sep 24, 2019

jbrette commented Sep 24, 2019

kkasravi commented Sep 24, 2019

kkasravi commented Sep 25, 2019

jbrette commented Sep 24, 2019 •

edited

Loading

jbrette commented Sep 24, 2019 •

edited

Loading