Katib manifests v3 version

[andreyvelich]

Fixes: #1092.

For Katib manifest v3 version:

1. I Created v3 folder for Katib crds and Katib controller.

2. For Katib crds kustomize manifest includes all base resources and application overlay.

3. Katib crds kustomize has commonLabels for each crd component.

4. For Katib controller kustomize manifests includes all base resources, application and istio overlay.

5. Katib controller kustomize has commonLabels for each controller component.

6. Katib controller kustomize has images to specify images for Katib controller components.

7. I renamed clusterDomain to katib-ui-clusterDomain and namespace to katib-ui-namespace to make these variables unique. @jlewi What do you think about this change?

8. Katib controller kustomize has configurations to identify katib-ui-clusterDomain variable.

9. I didn't add these params to Katib controller kustomize, as we did here. I am not sure that it is needed.

10. I didn't add generatorOptions: disableNameSuffixHash: true to v3 kustomize manifests. Is it necessary?

11. I added Katib v3 manifest to GCP example stacks. @jlewi Do we need to make it in this PR?

I didn't add ibm-storage-config to v3 manifest, since it is related only to IBM kfdef. We can create this patch later.

/assign @jlewi @johnugeorge
/cc @gaocegege @richardsliu

Checklist:

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

[kubeflow-bot]

This change is 

[jlewi]

Thanks.

I added Katib v3 manifest to GCP example stacks. @jlewi Do we need to make it in this PR?

Probably easier to do it a follow on PR. Smaller PRs are easier to deal with and this way if the manifest breaks GCP stacks we can just revert that PR and not the full PR.

I renamed clusterDomain to katib-ui-clusterDomain and namespace to katib-ui-namespace to make these variables unique. @jlewi What do you think about this change?

For clusterDomain I'm not sure we want to make the name application specific. The point of using the same vars across the applications is to allow the values to be defined once and consistently set across all applications.

So I think the pattern you want is to have 2 versions of the package

1. For backwards compatibility you should have a kustomization that defines the existing vars in that kustomize
   • This can't be used in the stacks since vars can only be defined once
2. You want a v3 version which doesn't define the vars in the kustomization file; instead these will be defined at a kustomization at the stack level

These packages should share resources either through a common base package or by referencing the same YAML files.

You can take a look at the jupyter package
https://github.com/kubeflow/manifests/blob/master/jupyter/notebook-controller/base_v3/kustomization.yaml

• Here that the vars aren't defined in this kustomization
• The vars are defined in base/kustomization.yaml for backwards compatibility
• Note that base_v3 is pulling in resources from the base package

katib-ui-namespace

How is this being used? Is this internal to the package; i.e. its not being used to allow the user to set something but just a way to force kusotmize to substitute the namespace into a field where otherwise it currently wouldn't get substituted?

If it is then this is a good pattern.

[jlewi]

Reviewable status: 0 of 73 files reviewed, 3 unresolved discussions (waiting on @andreyvelich, @gaocegege, and @richardsliu)

---

katib/katib-controller/base/kustomization.yaml, line 37 at r1 (raw file):

  newName: mysql
vars:
- name: katib-ui-clusterDomain

This should probably stay with clusterDomain so it can be set once in the stack across all applications

---

katib/katib-controller/base/kustomization.yaml, line 44 at r1 (raw file):

  fieldref:
    fieldpath: data.katib-ui-clusterDomain
- name: katib-ui-namespace

This is largely internal to the project; you are just using

---

katib/katib-controller/v3/kustomization.yaml, line 37 at r1 (raw file):

commonLabels:
  app.kubernetes.io/component: katib
  app.kubernetes.io/instance: katib-controller-0.8.0

Remove version from the instance label and remove version as a common label.
See comment here:
kubeflow/kfctl#304 (comment)

[jlewi]

How does this relate to @discordianfish's PR #986 ?

As discussed in #986 can we split katib into separate kustomize packages for the controller and the DB and then use composition to combine them? So that we can support incluster and external DBs?

[andreyvelich]

@jlewi Thank you for your review.

• I don't update stacks in this PR.
• I renamed katib-ui-clusterDomain to clusterDomain as you said.
• I removed version from common label. Should we also modify application: https://github.com/kubeflow/manifests/blob/master/katib/katib-crds/overlays/application/application.yaml#L57 in current kustomize implementation?

[andreyvelich]

katib/katib-controller/base/kustomization.yaml, line 44 at r1 (raw file):

  fieldref:
    fieldpath: data.katib-ui-clusterDomain
- name: katib-ui-namespace

This is largely internal to the project; you are just using

Yes, and it always must be metadata.namespace from katib-ui service. The same as in jupyter-web-app.

[andreyvelich]

How does this relate to @discordianfish's PR #986 ?

As discussed in #986 can we split katib into separate kustomize packages for the controller and the DB and then use composition to combine them? So that we can support incluster and external DBs?

We can merge #986 PR first.
After this, modify v3 Katib package. What do you think @johnugeorge @gaocegege ?

[jlewi]

If we merge #986 first is that creating more work in the long run? Per the discussion in the review thread
#986 (comment)

The suggestion was to have separate kustomize packages for

• controller
• db-manager
• my-sql

And then use a top level package to compose this into a single package. This way the controller and db-manager packages could be reused in the case of a different backend. #985 is making db-manager an overlay.

My suggestion would be to do something like the following. In this PR define

• katib-system
• katib-db-manager
• katib-mysql

Each have these kustomize packages should pull in resources from base (like your v3) package is doing now.

You can then change the v3 package to compose these three kustomize packages. (Maybe rename it to something like standalone?). This package should also define a patch for the db-manager as in
https://github.com/kubeflow/manifests/blob/3476c3ca6d13966b61a1539748bdeab3187d083d/katib/katib-controller/overlays/db-mysql/katib-db-manager-deployment.yaml
to set the secrets for mysql

This would largely cover @discordianfish's changes to support the external DB case.

The one other change is you need to change
base//katib-db-manager-deployment.yaml to remove the DB environment variables for MYSQL; These should be applied via a batch which should be included in base/ (for backwards compatibility) but not katib-db-manager.

[jlewi]

So per comment: #1124 (comment)

I think this should be something like

resources:

• ../katib-system
• ../katib-db-manager
• ../katib-mysql

The resources listed here would then move into the respective packages.

patchesStrategicMerge

[andreyvelich]

Yes I agree, we can use patch here.
What do you think @discordianfish, can you make these changes in #986 PR?

/cc @johnugeorge @gaocegege

[jlewi]

I think in #986 @discordianfish said he didn't have time to make any additional changes. So maybe @andreyvelich you could just make the changes in this PR? Or a follow on PR?

[discordianfish]

I was waiting for you to settle on one way to do it. But now it's probably better to merge this here first. I can look into a new PR to change this.

[jlewi]

Should we also modify application: https://github.com/kubeflow/manifests/blob/master/katib/katib-crds/overlays/application/application.yaml#L57 in current kustomize implementation?

Yes I think we should go ahead and remove the mutable bits from the commonLabels from the application resource

[andreyvelich]

Should we also modify application: https://github.com/kubeflow/manifests/blob/master/katib/katib-crds/overlays/application/application.yaml#L57 in current kustomize implementation?

Yes I think we should go ahead and remove the mutable bits from the commonLabels from the application resource

I removed versions from Katib controller Application and Katib Crd Application

[andreyvelich]

@jlewi What do you think about katib-ui-namespace variable, should we keep it as a namespace to be consistent with jupyter-web-app or identify it as the Katib variable ?

[jlewi]

katib-ui-namespace

I think it needs to be "katib-ui-namespace". vars need to be unique globally. So if we use a generic name like namespace it will end up conflicting.

[jlewi]

/lgtm
/approve
/hold

@andreyvelich On my end I'm happy with this PR as is. I think we still need to refactor the manifests to support external DB but I'm ok with doing that in a follow on PR.

Ideally someone focused on katib would LGTM this PR as well.

[k8s-ci-robot]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [jlewi]

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

[andreyvelich]

/lgtm
/approve
/hold

@andreyvelich On my end I'm happy with this PR as is. I think we still need to refactor the manifests to support external DB but I'm ok with doing that in a follow on PR.

Ideally someone focused on katib would LGTM this PR as well.

Sure, I will update it in the following PR. Thank you for the review.
/assign @johnugeorge

[johnugeorge]

Based on the comment #1124 (comment) , @discordianfish will do it in a new PR. Thanks @discordianfish

Merging this

/hold cancel