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

Update metacontroller #2255

Merged
merged 4 commits into from
Jul 26, 2022
Merged

Conversation

gkcalat
Copy link
Member

@gkcalat gkcalat commented Jul 20, 2022

Which issue is resolved by this Pull Request:

Description of your changes:

  • Upgraded metacontroller to stay in sync for Kubeflow Pipelines.
  • Added README with short instructions on how to manually upgrade it in the future.

Checklist:

  • Unit tests pass:
    Make sure you have installed kustomize == 3.2.1
    1. make generate-changed-only
    2. make test

@gkcalat
Copy link
Member Author

gkcalat commented Jul 20, 2022

/assign @zijianjoy

@gkcalat gkcalat requested a review from zijianjoy July 21, 2022 06:30
@zijianjoy
Copy link
Contributor

/lgtm
/approve

Looks great, thank you so much Ablai!

@google-oss-prow google-oss-prow bot added the lgtm label Jul 21, 2022
@zijianjoy
Copy link
Contributor

/assign @kimwnasptd for approving this change.

@kimwnasptd
Copy link
Member

kimwnasptd commented Jul 25, 2022

Thanks for keeping this up to date @gkcalat @zijianjoy!

Some small questions that come to mind:

  1. why do we need to have the metacontroller under contrib?
  2. what's the difference with the metacontroller in https://github.com/kubeflow/manifests/tree/master/apps/pipeline/upstream/third-party/metacontroller/base?
  3. Right now in our example kustomization we are not installing that package, since KFP is using the metacontroller bundled with them (link in point 2). Should we change this?
  4. Do we need to have both the metacontroller under contrib and inside the KFP manifests?

@zijianjoy
Copy link
Contributor

zijianjoy commented Jul 25, 2022

@kimwnasptd

Good questions: My understanding is that Metacontroller makes it easy to write custom controller in any kubernetes cluster. If you confirm that metacontroller is currently not used in anywhere else except KFP, I think it doesn't matter we have metacontroller by itself in contrib/ folder or within KFP manifests at the moment. However, it might cause implicit conflict if any other Kubeflow component or users decide to adopt Metacontroller. We will need to reverse the process: Extract metacontroller back to contrib/ to maintain consistency. So I feel that the current structure is more future-proof.

@kimwnasptd
Copy link
Member

Thanks for the explanation!

I'm OK with merging this update for now. I made these questions because I want to create a proposal around expectations regarding the /contrib folder. Mostly to ensure each component should have a clear description of:

  1. How could someone use it with Kubeflow, and examples
  2. Why someone would use it [problems it tries to solve]
  3. Status of the KF versions that the component is compatible with

But, again, this will be a proposal of its own. Just wanted to have a rough idea for the state of metacontroller for now.

/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gkcalat, kimwnasptd, zijianjoy

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-prow google-oss-prow bot merged commit a578115 into kubeflow:master Jul 26, 2022
@zijianjoy
Copy link
Contributor

@kimwnasptd Nice idea! I think the three points you made regarding clear description is a good start. Once we reach consent in community, feel free to let me know and I can provide content for the three points for metacontroller.

@annajung
Copy link
Member

Hi @zijianjoy @gkcalat

As Kimonas pointed out in his comment, this PR does not change the default behavior of which metacontroller gets installed since the default installation points to the /third-party.

I also noticed that /third-party/metacontroller/ already contains the changes you have made in the /contrib as part of this PR. Unless you want to change the default installation to use the /contrib, it doesn't seem like this has any behavior changes for 1.6.

That said, I don't think we need to cut another 1.6 RC for this change only.

Since we are planning to cut a new RC in the future for the notebook WG (release blocking issue), we can include this change at that point if you like to continue your work to change the default installation to use the metacontroller from the /contrib

Let me know what you think! thanks!

@zijianjoy
Copy link
Contributor

Hello @annajung , you are right, the change to metacontroller will be automatically picked up once you made a new RC. And based on the Notebook WG issue, there is a need to make a new RC.

kevin85421 pushed a commit to juliusvonkohout/manifests that referenced this pull request Feb 28, 2023
* Upgrade metacontroller to sync with KFP

* Add README.md

* Add README

* Convert into kpt package
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.

4 participants