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

Katib: Move manifests development upstream #1432

Conversation

yanniszark
Copy link
Contributor

Issue Resolved

Resolves: #1431
Umbrella issue: kubeflow/manifests#1740

Description

As part of the work of wg-manifests for 1.3
(kubeflow/manifests#1735), we are moving manifests
development in upstream repos. This gives the application developers full
ownership of their manifests, tracked in a single place.

This PR copies the manifests for application Katib
from path apps/katib/upstream of kubeflow/manifests to path
manifests/v1beta1 of the upstream repo (https://github.com/kubeflow/katib).

cc @kubeflow/wg-automl-leads

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@yanniszark
Copy link
Contributor Author

/retest

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

Thank you for doing this @yanniszark!
Do we have a unique structure for the kustomize manifests for each kubeflow component ?
Or every WG should decide how to store manifest by themselves?

@@ -0,0 +1,53 @@
apiVersion: apps/v1
Copy link
Member

Choose a reason for hiding this comment

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

Should we move all of the YAMLs from the manifests/v1beta1/katib-controller/base to the /manifests/v1beta1/components/<appropriate-component-name/ ?
Since we left the old structure for the manifests to not brake kfdef configs before we transfer to the v3 manifests ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I was suspecting something like this happened.

@@ -0,0 +1,65 @@
apiVersion: app.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to maintain these overlays ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the application overlays, we are discussing about moving away from them soon:
kubeflow/manifests#1715

What we could do is put them in an installs/kubeflow kustomization, since they are used when Katib is installed alongside other Kubeflow components.

Copy link
Member

Choose a reason for hiding this comment

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

@yanniszark So should we add the application overlay for Katib now or it's better to remove all labels and add them later with an Application, once we need it.
Since currently, many Katib labels are incorrect for the Application: kubeflow/manifests#1669.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreyvelich in general, we are moving away from using the Application CR: kubeflow/manifests#1715 (comment)

For this PR, I would suggest leaving them as is. The initial plan is to just get the manifests under the control of the upstream repo (katib) and then we can make changes incrementally.

@yanniszark
Copy link
Contributor Author

yanniszark commented Feb 21, 2021

@andreyvelich those are some good questions. I tried to answer in the comments above. As for the final question:

Do we have a unique structure for the kustomize manifests for each kubeflow component ?
Or every WG should decide how to store manifest by themselves?

We plan to propose a common way to organize manifests soon (step 4 in this plan kubeflow/manifests#1735). The first goal is to get the content of kubeflow/manifests back to the respective upstream repos.

In general, applications are expected to provide kustomizations that can be used to install the application.

@yanniszark yanniszark force-pushed the feature-move-katib-manifests-upstream branch from 5ce880a to bfda6be Compare March 1, 2021 20:58
@yanniszark
Copy link
Contributor Author

@andreyvelich I have made the changed suggested here:

Should we move all of the YAMLs from the manifests/v1beta1/katib-controller/base to the /manifests/v1beta1/components/<appropriate-component-name/ ?

Could you take another look?

@andreyvelich
Copy link
Member

@yanniszark Thank you for the update!

  1. Do you need istio-authz-policy-odl.yaml and istio-authz-policy-old.yaml in this PR ?
  2. Please rebase to fix CI.

uri: /katib/
route:
- destination:
host: katib-ui.$(katib-ui-namespace).svc.$(clusterDomain)
Copy link
Member

Choose a reason for hiding this comment

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

Will we get these values from the appropriate stack.
For example: https://github.com/kubeflow/manifests/blob/master/distributions/stacks/gcp/kustomization.yaml ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly! Let's keep those as they are and we can introduce changes afterwards if we find it's not a good pattern.

@yanniszark yanniszark force-pushed the feature-move-katib-manifests-upstream branch 3 times, most recently from 4b12a58 to 6a1f705 Compare March 4, 2021 15:46
@yanniszark
Copy link
Contributor Author

Do you need istio-authz-policy-odl.yaml and istio-authz-policy-old.yaml in this PR ?
No, this was a mistaken git add.

Please rebase to fix CI.
I rebased but the Github Actions CI seems broken. Here is the Github worker message:

/home/runner/work/_temp/47c2af1d-bea5-4f4b-85f5-af8d0b594511.sh: line 1: juju: command not found

@andreyvelich
Copy link
Member

Thank you @yanniszark.
Yes there are some problems with CI as I mentioned here: #1450 (comment).

@yanniszark
Copy link
Contributor Author

@andreyvelich thanks, so what are the next steps for the CI?
Wait for #1450 to be merged?

@andreyvelich
Copy link
Member

@yanniszark Yeah, please can you rebase and we can move forward?

@yanniszark yanniszark force-pushed the feature-move-katib-manifests-upstream branch from 6a1f705 to aea241f Compare March 6, 2021 19:01
As part of the work of wg-manifests for 1.3
(kubeflow/manifests#1735), we are moving manifests
development in upstream repos. This gives the application developers full
ownership of their manifests, tracked in a single place.

This commit copies the manifests for application `Katib`
from path `apps/katib/upstream` of kubeflow/manifests to path
`manifests/v1beta1` of the upstream repo (https://github.com/kubeflow/katib).

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
@yanniszark yanniszark force-pushed the feature-move-katib-manifests-upstream branch from aea241f to f61c903 Compare March 6, 2021 19:02
@yanniszark
Copy link
Contributor Author

@andreyvelich great, I just rebased!

@andreyvelich
Copy link
Member

Thanks @yanniszark.
/lgtm
/approve
/retest

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, yanniszark

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

@google-oss-robot google-oss-robot merged commit ac4f525 into kubeflow:master Mar 6, 2021
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.

Katib: Move manifests development upstream
4 participants