fix kustomize manifests for kubeflow #1498

davidspek · 2021-03-30T15:34:32Z

When trying to build the manifests for the Kubeflow deployment the following error occurs:

Error: accumulating resources: accumulation err='accumulating resources from '../../components/controller/controller.yaml': security; file '/home/david/Documents/GitHub/katib/manifests/v1beta1/components/controller/controller.yaml' is not in or below '/home/david/Documents/GitHub/katib/manifests/v1beta1/installs/katib-with-kubeflow'': got file 'controller.yaml', but '/home/david/Documents/GitHub/katib/manifests/v1beta1/components/controller/controller.yaml' must be a directory to be a root

This PR creates kustomization.yaml files for each of the folders so that the kustomization.yaml in katib-with-kubeflow can reference the component folders properly. Similarly, the katib-config-path.yaml that was being referenced from katib-standalone has been copied into the katib-with-kubeflow so that the manifests can be properly built with kustomize.

/cc @andreyvelich

andreyvelich

I left few comments @davidspek.
For the CI: we should change the path for the katib-config with image tags here: https://github.com/kubeflow/katib/blob/master/test/scripts/v1beta1/setup-katib.sh#L42.

For the release scripts: we should also modify the path for the katib-config: https://github.com/kubeflow/katib/blob/master/scripts/v1beta1/release.sh#L74.

andreyvelich · 2021-04-07T13:37:56Z

manifests/v1beta1/installs/katib-external-db/katib-config-patch.yaml

@@ -0,0 +1,61 @@
+apiVersion: v1


As we discussed on the meeting, we should move this file to /manifests/v1beta1/components/controller and remove current katib-config.yaml file.

I've removed the katib-config-patch.yaml files in the installation directory and move the image tags to /manifests/v1beta1/components/controller/katib-config.yaml.

andreyvelich · 2021-04-07T13:40:25Z

manifests/v1beta1/installs/katib-external-db/kustomization.yaml

@@ -3,28 +3,17 @@ kind: Kustomization
 namespace: kubeflow
 resources:
  # Namespace.
-  - ../../components/namespace.yaml
+  - namespace.yaml


Any ideas how we can keep namespace under manifests/v1beta1/components ?
Maybe create folder components/namespace and add namespace.yaml and kustomization.yaml there.
WDYT @davidspek ? cc @yanniszark

I think this is the easiest way so I have implemented this.

andreyvelich · 2021-04-07T13:42:56Z

manifests/v1beta1/installs/katib-with-kubeflow-cert-manager/patches/mysql-pvc.yaml

@@ -0,0 +1,6 @@
+[


Do we have any options to combine this patch and https://github.com/kubeflow/katib/blob/master/manifests/v1beta1/installs/katib-with-kubeflow/patches/mysql-pvc.yaml in the one file ? cc @yanniszark

I've implemented katib-with-kubeflow-cert-manager to use katib-with-kubeflow as a base, which solves needing to duplicate this file.

andreyvelich · 2021-04-07T13:43:24Z

manifests/v1beta1/installs/katib-with-kubeflow-cert-manager/ui-virtual-service.yaml

@@ -0,0 +1,20 @@
+apiVersion: networking.istio.io/v1alpha3


The same comment as for the mysql-pvc.yaml patch.

I've implemented katib-with-kubeflow-cert-manager to use katib-with-kubeflow as a base, which solves needing to duplicate this file.

manifests/v1beta1/installs/katib-with-kubeflow/kustomization.yaml

andreyvelich · 2021-04-07T13:47:06Z

It would be great if @yanniszark could also take a look at these changes.

yanniszark · 2021-04-07T14:52:08Z

@davidspek thanks for this effort. I will take a look shortly but I should comment that I am not treating this PR as release blocking (I think we've discussed in the past, but also mentioning it now for clarity). The first thing I would check for a PR like this would be that the result of kustomize build before and after the PR is the same for every entry under installs. Could you confirm that this is the case? And of course, CI should be passing and @andreyvelich confirm that these changes won't break something we don't expect in Katib docs / examples.

davidspek · 2021-04-07T14:56:54Z

@yanniszark That is indeed how I am testing things. I just got caught up with creating a PR for a fix to the JWA as something seemed to be broken but I am now continuing with working on these manifests. It isn't release blocking, but as the RC period is partly meant for fixing manifests if this PR is done in time for the release cut it should be included I think.

andreyvelich · 2021-04-07T15:26:23Z

@yanniszark Since we are using katib-with-kubeflow-cert-manager install for the kubeflow installation maybe we can remove the overlay which is using the Katib cert generator for now ?
I think if any distribution owners will require this install for Katib, we can this overlay later.

andreyvelich · 2021-04-07T23:49:33Z

@davidspek @yanniszark I was actually thinking about the Katib installs more. What do you think about keeping these installs:

/installs/katib-external-db
/installs/katib-ibm
/installs/katib-standalone
/installs/katib-cert-manager
/installs/katib-with-kubeflow

Since Kubeflow installation uses cert manager instead of custom cert generator we don't need to have 2 installs for Katib with Kubeflow.
We can create katib-cert-manager with all Katib Standalone components without Cert generator.
This is useful in cases when our custom Cert Generator doesn't work, e.g. #1512

WDYT @davidspek @yanniszark ?

davidspek · 2021-04-09T09:50:03Z

@andreyvelich Sorry I completely missed your comment yesterday as I was very busy. Hopefully I will have time to continue wit this today (otherwise it'll be in the weekend).

andreyvelich · 2021-04-09T12:05:53Z

@andreyvelich Sorry I completely missed your comment yesterday as I was very busy. Hopefully I will have time to continue wit this today (otherwise it'll be in the weekend).

No worries, take your time.
Thank you for your effort on this!

davidspek · 2021-04-10T10:46:56Z

@andreyvelich I've pushed a commit with the katib-cert-manager and katib-with-kubeflow uses this as a base. I've also had the cert-generator create around 10 pods before one was successful so I think this is a good move. I don't really have a way to test this deployment, but when looking at the resulting YAML it makes sense. The resulting kubeflow YAML is what is expected with this setup.

I've also updated the releasing and test scripts so that the image tags are updated in katib-config.yaml.

manifests/v1beta1/installs/katib-cert-manager/kustomization.yaml

andreyvelich · 2021-04-10T18:36:09Z

Overall looks good to me, thank you @davidspek!
/lgtm
Please take a look @yanniszark @gaocegege @johnugeorge

andreyvelich · 2021-04-10T20:03:49Z

/retest

yanniszark · 2021-04-12T15:27:35Z

Thanks a lot for the effort @davidspek @andreyvelich.
I took a quick look and some small comments I have are:

It would be good if the patches were declared in a universal way (separate files). Keep in mind that you can include multiple patches in one file, using patchesStrategicMerge, if the context is similar.
Instead of patching to remove all cert-generator resources, how about creating a separate kustomization for the cert-generator?

And of course, before merging, I would validate that the result of each new kustomization is the same as the result of the respective previous kustomization.

davidspek · 2021-04-12T15:29:47Z

Makes sense to me. @andreyvelich Let me know what you prefer and I'll make the needed changes.

andreyvelich · 2021-04-12T15:39:13Z

I agree with @yanniszark.

Let's have 2 separate folders for the Webhooks:

cert-generator
webhook

davidspek · 2021-04-12T17:52:38Z

@andreyvelich I've just pushed a commit in which I have separated webhook from cert-generator and I'm not using katib-standalone as a base for katib-cert-manager to avoid needing all the patches that delete the cert-generator resources.

manifests/v1beta1/installs/katib-with-kubeflow/patches/remove-resources-patch.yaml

andreyvelich · 2021-04-12T18:53:54Z

Thank you @davidspek.
If the changes look good to you @yanniszark, can you add lgtm, please ?

davidspek · 2021-04-12T20:46:24Z

/retest

gaocegege

/lgtm
/approve

google-oss-robot · 2021-04-13T02:32:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DavidSpek, gaocegege

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:

~~OWNERS~~ [gaocegege]

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

…1514: Create workflow for Go Cherry pick of #1498 #1514 on release-0.11. #1498: fix kustomize manifests for kubeflow #1514: Create workflow for Go (#1515) * fix kustomize manifests for kubeflow * fix standalone and external-db manifests * remove old namespace file * remove PV from kubeflow manifest * fix katib-external-db reference outside of root * fix katib-with-kubeflow-cert-manager * Move image tags to katib-config.yaml and remove patches * use common namespace kustomization * Make kubeflow-cert use kubeflow as a base * Remove katib-cert-generator job from kubeflow-cert-generator manifests * Move pv-patch to patches folder * Create katib-cert-manager and make kubeflowuse this as base * Fix release and CI scripts for new layout * Remove unnecessary cert-generator images from kustomization.yaml * Remove unnecessary SA, CR and CRB from katib-cert-manager * Remove commonLabel from katib-with-kubeflow * Separate cert-generator from webhook kustomization * Create workflow for Go * Add GOPATH env * Move check up * Add env * Add go mod download * Add ls command * Add path * Change path for run * Change GOPATH * Add kubebuilder * Download coveralls * Add node test * Remove Travis * Add coveralls step * Change coveralls use * Add working dir * Remove run * Fix the patch * Remove patch Co-authored-by: DavidSpek <vanderspek.david@gmail.com>

* Add kustomization overlay: katib-standalone-openshift * Rename OpenShift kustomization and remove unused RBAC resources * Update kustomization katib-openshift to support changes in #1498 * katib-openshift: move patches to dedicated dir * katib-openshift: clarify comments * Update katib-openshift image tags

google-oss-robot requested a review from andreyvelich March 30, 2021 15:34

google-oss-robot added the size/L label Mar 30, 2021

davidspek force-pushed the fix-manifests branch from c0b5c65 to 24d2b87 Compare March 30, 2021 16:07

davidspek added 5 commits April 3, 2021 13:04

fix kustomize manifests for kubeflow

7abb7f0

fix standalone and external-db manifests

c438b29

remove old namespace file

b4e89a8

remove PV from kubeflow manifest

7e38c28

fix katib-external-db reference outside of root

6a411ef

davidspek force-pushed the fix-manifests branch 2 times, most recently from a5274b2 to bc8f44a Compare April 3, 2021 11:10

fix katib-with-kubeflow-cert-manager

a744531

davidspek force-pushed the fix-manifests branch from bc8f44a to a744531 Compare April 3, 2021 11:11

andreyvelich reviewed Apr 7, 2021

View reviewed changes

Move image tags to katib-config.yaml and remove patches

43617b6

davidspek added 4 commits April 7, 2021 17:39

use common namespace kustomization

8756b0d

Make kubeflow-cert use kubeflow as a base

617479c

Remove katib-cert-generator job from kubeflow-cert-generator manifests

716c74b

Move pv-patch to patches folder

af3cde5

davidspek mentioned this pull request Apr 7, 2021

Upgrade to use Kustomize v4+ kubeflow/manifests#1797

Closed

andreyvelich mentioned this pull request Apr 8, 2021

Webhook certificates validation fails #1512

Closed

davidspek added 2 commits April 10, 2021 12:35

Create katib-cert-manager and make kubeflowuse this as base

169d693

Fix release and CI scripts for new layout

2e50026

andreyvelich reviewed Apr 10, 2021

View reviewed changes

manifests/v1beta1/installs/katib-cert-manager/kustomization.yaml Show resolved Hide resolved

davidspek added 2 commits April 10, 2021 20:17

Remove unnecessary SA, CR and CRB from katib-cert-manager

371718d

Remove commonLabel from katib-with-kubeflow

335d58b

google-oss-robot assigned andreyvelich Apr 10, 2021

google-oss-robot added the lgtm label Apr 10, 2021

google-oss-robot removed the lgtm label Apr 12, 2021

andreyvelich reviewed Apr 12, 2021

View reviewed changes

manifests/v1beta1/installs/katib-with-kubeflow/patches/remove-resources-patch.yaml Outdated Show resolved Hide resolved

Separate cert-generator from webhook kustomization

02a7f4e

davidspek force-pushed the fix-manifests branch from a97f932 to 02a7f4e Compare April 12, 2021 18:55

gaocegege reviewed Apr 13, 2021

View reviewed changes

google-oss-robot assigned gaocegege Apr 13, 2021

google-oss-robot added the lgtm label Apr 13, 2021

google-oss-robot added the approved label Apr 13, 2021

google-oss-robot merged commit 520f54e into kubeflow:master Apr 13, 2021

davidspek mentioned this pull request Apr 13, 2021

Add kustomization overlay: katib-openshift #1513

Merged

andreyvelich mentioned this pull request Apr 13, 2021

Sync Katib manifests with Kustomize v4 support kubeflow/manifests#1840

Merged

1 task

maanur added a commit to maanur/katib that referenced this pull request Apr 13, 2021

Update kustomization katib-openshift to support changes in kubeflow#1498

7d71691

maanur added a commit to maanur/katib that referenced this pull request Apr 13, 2021

Update kustomization katib-openshift to support changes in kubeflow#1498

83678f1

tenzen-y mentioned this pull request Sep 24, 2021

Change the minimal kustomize version in the developer guide #1675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix kustomize manifests for kubeflow #1498

fix kustomize manifests for kubeflow #1498

davidspek commented Mar 30, 2021

andreyvelich left a comment

andreyvelich Apr 7, 2021

davidspek Apr 7, 2021

andreyvelich Apr 7, 2021

davidspek Apr 7, 2021

andreyvelich Apr 7, 2021

davidspek Apr 7, 2021

andreyvelich Apr 7, 2021

davidspek Apr 7, 2021

andreyvelich commented Apr 7, 2021

yanniszark commented Apr 7, 2021

davidspek commented Apr 7, 2021

andreyvelich commented Apr 7, 2021

andreyvelich commented Apr 7, 2021 •

edited

Loading

davidspek commented Apr 9, 2021

andreyvelich commented Apr 9, 2021

davidspek commented Apr 10, 2021

andreyvelich commented Apr 10, 2021

andreyvelich commented Apr 10, 2021

yanniszark commented Apr 12, 2021

davidspek commented Apr 12, 2021

andreyvelich commented Apr 12, 2021

davidspek commented Apr 12, 2021

andreyvelich commented Apr 12, 2021

davidspek commented Apr 12, 2021

gaocegege left a comment

google-oss-robot commented Apr 13, 2021

fix kustomize manifests for kubeflow #1498

fix kustomize manifests for kubeflow #1498

Conversation

davidspek commented Mar 30, 2021

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Apr 7, 2021

yanniszark commented Apr 7, 2021

davidspek commented Apr 7, 2021

andreyvelich commented Apr 7, 2021

andreyvelich commented Apr 7, 2021 • edited Loading

davidspek commented Apr 9, 2021

andreyvelich commented Apr 9, 2021

davidspek commented Apr 10, 2021

andreyvelich commented Apr 10, 2021

andreyvelich commented Apr 10, 2021

yanniszark commented Apr 12, 2021

davidspek commented Apr 12, 2021

andreyvelich commented Apr 12, 2021

davidspek commented Apr 12, 2021

andreyvelich commented Apr 12, 2021

davidspek commented Apr 12, 2021

gaocegege left a comment

Choose a reason for hiding this comment

google-oss-robot commented Apr 13, 2021

andreyvelich commented Apr 7, 2021 •

edited

Loading