Katib move db to overlay disable loadrestrictions

[discordianfish]

Which issue is resolved by this Pull Request:
Resolves #904

Description of your changes:
This is an alternative to #910 but I can't get the tests to pass because some assumptions on how the tests are built. If this is the preferred way over #910, please advise on how to change the test generation etc to make it work.

Checklist:

• Unit tests have been rebuilt:
  1. cd manifests/tests
  2. make generate-changed-only
  3. make test

---

This change is 

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign yanniszark
You can assign the PR to them by writing /assign @yanniszark 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:

• OWNERS

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

[discordianfish]

/assign @yanniszark

Since this disables the load restrictions, it probably makes sense for @jlewi to approve this. Hope doing this here is fine, given that it seems to be the only solution and http://bit.ly/kf_kustomize_v3 suggestes doing this in the future anyways

[jlewi]

@discordianfish Where are you disabling the load restrictor? In the tests? That seems fine to me.

Someone on the Katib team should review overall layout of the katib package.

[discordianfish]

btw, this is based upon #1055 - so we can either merge this as is or that one first and I'll rebase, up to you.

[jlewi]

Thanks. I LGTM'd #1055.
You should also be able to do make generate-changed-only to only regenerate tests based on what changed but its imperfect.

[discordianfish]

@jlewi Yes, that created a mess on this PR that failed tests.
Rebased this, let see what the tests say.

[discordianfish]

No tests? Hrm..

/retest

[discordianfish]

Ok, tests passed. @yanniszark can you review?

[jlewi]

/assign @gaocegege

@gaocegege Does someone from Katib want to take a look at this?

[andreyvelich]

I am not sure that we can move Katib mysql DB to the overlay currently.
Katib db manager will not work without running Katib mysql deployment.

In that case, if user wants to deploy Katib using the base from manifests repository without other Kubeflow components and overlays, how he can do it?

What do you think @gaocegege @johnugeorge @richardsliu ?

[discordianfish]

@andreyvelich I'm following the same pattern here as in #673 - if this isn't a problem there, why is is a problem here?

Yes, the/some db overlay is required but this is required in many other manifests as well. What is the problem with that?

[andreyvelich]

I am no sure how metadata connects with the DB, but Katib DB Manager can't work without mysql DB.
If users want to use their own DB (not mysql), they must create custom DB manager.

@johnugeorge @jlewi Do we have use-case when user deploys Katib from Manifest repository from the base folder, or in that case user will use manifest folder from the Katib repository?

[discordianfish]

@andreyvelich Yes, that's true. A user either needs to use the db overlay here, to get the bundled database, or a custom overlay for their custom database setup.

Maybe it helps to clarify by providing our overlay as example (it uses RDS but that shouldn't matter):

kustomization.yaml:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
bases:
- ../../base
patchesStrategicMerge:
- katib-db-manager-deployment.yaml

katib-db-manager-deployment.yaml:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: katib-db-manager
spec:
  template:
    spec:
      containers:
      - name: katib-db-manager
        env:
          - name: DB_NAME
            value: mysql
          - name: KATIB_MYSQL_DB_DATABASE
            value: katib_production
          - name: KATIB_MYSQL_DB_HOST
            valueFrom:
              secretKeyRef:
                name: db-secrets
                key: MYSQL_HOST
          - name: DB_USER
            valueFrom:
              secretKeyRef:
                name: db-secrets
                key: MYSQL_USER_NAME
          - name: DB_PASSWORD
            valueFrom:
              secretKeyRef:
                name: db-secrets
                key: MYSQL_ROOT_PASSWORD

[andreyvelich]

In that case, should we also move Katib DB Manager to the overlay?
For example, if user wants to use not mysql DB ? How he can do it?

[jlewi]

Would a composition pattern work better here? It looks like we have three sub-applications

• Katib controller (+ associated resources) e.g. RBAC
• DB manager
• A MySQL itself.

What if each of those components was their own Kustomize package and then we used Kustomize to compose those applications into different configurations so we'd have

• One for standalone - Which would combine all three and add an overlays to configure DB Manager to work with MySQL
• One for an external DB which would combine the first two
  • I'm guessing user would have to add in their own overlays to specify custom information

To put this another way, with these changes why is the MySQL DB an overlay on the "base" package? With the changes in this PR the base package is defining the controller and some other core resources. It doesn't look like the MySQL DB is actually modifying any of those resources.

Should we do the composition in KFDefs or should we define a new kustomize package e.g. "standalone" which combines istio, application, db-manager, and db-mysql packages?

[andreyvelich]

@jlewi If we create three sub-application, each of them will have base, right?
Like we did with crd: https://github.com/kubeflow/manifests/tree/master/katib/katib-crds.
In that case, how user can install Katib standalone?

Run kustomize build in each folder:

1. katib-controller
2. db-manager
3. db-mysql
4. katib-crds

Right?

[discordianfish]

Let me know what you decide, until then I'll put this on hold and and we'll change our strategy to maintaining a fork of manifests instead of just adding our overlays.

[jlewi]

@andreyvelich You can use kustomize to compose kustomize packages. A resource can be a path to a folder containing a kustomization.yaml file. You can have multiple such folders listed in resources provided each unique resource is defined only in folder. (You would use patches to modify a resource).

So you can define a standalone package that would incorporate the other three.

The kustomize would look like

resources:
- <path>/<to>/katib-controller
- <path>/<to>/db-manager
- <path>/<to>/db-mysql

[andreyvelich]

@jlewi I am fine with this approach. Then as @jlewi mentioned before, we can create overlays to patch DB Manager deployment with appropriate DB.
What do you think @johnugeorge @richardsliu @gaocegege ?

[discordianfish]

Now the tests fail. Hopefully #1104 will fix this.

Can we avoid broaden the scope of a two month old PR every time? This doesn't seem to be a good strategy, given how much in flux everything else it.

[k8s-ci-robot]

@discordianfish: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
kubeflow-manifests-presubmit	3476c3c	link	/test kubeflow-manifests-presubmit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[discordianfish]

Replaced by #1141