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

Upgrading metacontroller to v4.11.22 #2988

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tarekabouzeid
Copy link
Member

@tarekabouzeid tarekabouzeid commented Feb 10, 2025

Pull Request Template for Kubeflow Manifests

✏️ Summary of Changes

Upgrading metacontroller to v4.11.22 , issue
Upstream sync PR

📦 Dependencies

List any dependencies or related PRs (e.g., "Depends on #123").

🐛 Related Issues

Link any issues that are resolved or affected by this PR.

✅ Contributor Checklist


You can join the CNCF Slack and access our meetings at the Kubeflow Community website. Our channel on the CNCF Slack is here #kubeflow-platform.

@tarekabouzeid
Copy link
Member Author

cc @juliusvonkohout @varodrig @biswassri

@tarekabouzeid
Copy link
Member Author

tarekabouzeid commented Feb 14, 2025

Reference here to a similar issue, started since metacontroller V3 , where metacontroller supports only watching CRDs.

A possible solution is changing PPC parent resource to Kubeflow profiles instead of namespaces, and update the hook in upstream to be able to process namespace from Kubeflow profiles CRD .

Signed-off-by: Tarek Abouzeid <tarek.abouzeid@teliacompany.com>
Signed-off-by: Tarek Abouzeid <tarek.abouzeid@teliacompany.com>
@tarekabouzeid tarekabouzeid force-pushed the metacontroller-upgrade-4x branch from 6b3ea65 to 5be8bb5 Compare February 14, 2025 23:39
Signed-off-by: Tarek Abouzeid <tarek.abouzeid@teliacompany.com>
@juliusvonkohout
Copy link
Member

Thank you for the effort. We definitely need to watch namespaces and should not change the order. Because in practice there will be many more profiles created after installing Kubeflow and the metacontroller.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Feb 16, 2025

canonical/metacontroller-operator#84

Probably means that we have to switch from composite controller to decorator controller. This can be done in the same PR.

Signed-off-by: Tarek Abouzeid <tarek.abouzeid@teliacompany.com>
Signed-off-by: Tarek Abouzeid <tarek.abouzeid@teliacompany.com>
@tarekabouzeid
Copy link
Member Author

canonical/metacontroller-operator#84

Probably means that we have to switch from composite controller to decorator controller. This can be done in the same PR.

@juliusvonkohout Thanks for feedback. I found a PR from canonical and done the changes here accordingly.

Signed-off-by: Tarek Abouzeid <tarek.abouzeid@teliacompany.com>
@juliusvonkohout
Copy link
Member

juliusvonkohout commented Feb 19, 2025

Thank you very much. Can you create the same PR against kubeflow/pipelines, such that we have it synchronized?
Please tag me and @hbelmiro and @HumairAK in that PR. Hopefully we can then merge the PR in kubeflow/pipelines first and then this one afterwards.

Did you also try this locally with some pipelines in a fresh kind cluster?

@tarekabouzeid
Copy link
Member Author

Thank you very much. Can you create the same PR against kubeflow/pipelines, such that we have it synchronized? Please tag me and @hbelmiro and @HumairAK in that PR. Hopefully we can then merge the PR in kubeflow/pipelines first and then this one afterwards.

Did you also try this locally with some pipelines in a fresh kind cluster?

Thank you Julius, I created PR in KFP as well.

I have installed this in a fresh Kind cluster and verified that secret is created. I will try to run some more pipelines locally as well.

@juliusvonkohout
Copy link
Member

Please check kubeflow/pipelines#11656 (comment)

@juliusvonkohout
Copy link
Member

/lgtm
/approve

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Feb 23, 2025

@tarekabouzeid can you fix the merge conflict?

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout

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

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.

2 participants