Skip to content

feat: migration script for V1 to V2 components #162

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

feat: migration script for V1 to V2 components #162

wants to merge 14 commits into from

Conversation

@kimwnasptd
Copy link
Member

@kimwnasptd kimwnasptd commented Oct 14, 2025

Closes #154

Note: This should be merged/reviewed only after we've merged the PRs for #154 (comment)

/cc @juliusvonkohout @thesuperzapper @andyatmiami

@kimwnasptd kimwnasptd mentioned this pull request Oct 14, 2025
Open
4 tasks
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
juliusvonkohout
juliusvonkohout reviewed Oct 23, 2025
View reviewed changes
kimwnasptd and others added 8 commits October 28, 2025 08:33
@kimwnasptd @juliusvonkohout
Update README.md
6f84e02 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update README.md
35a4ea2 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update README.md
23fb0f7 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update README.md
41e1cbb 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update scripts/upgrade_v1_to_v2.sh
11f879c 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update scripts/upgrade_v1_to_v2.sh
953cdfe 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update scripts/upgrade_v1_to_v2.sh
a725173 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd @juliusvonkohout
Update scripts/upgrade_v1_to_v2.sh
186408a 
Co-authored-by: Julius von Kohout <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd
Copy link
Member Author

kimwnasptd commented Oct 28, 2025

Thanks for the review @juliusvonkohout! Does the script functionality also looks good to you, or do you have any suggestions?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 28, 2025

Thanks for the review @juliusvonkohout! Does the script functionality also looks good to you, or do you have any suggestions?

For me it is fine and optional so
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 28, 2025
@kimwnasptd
Copy link
Member Author

kimwnasptd commented Oct 28, 2025

/ok-to-test

andyatmiami
andyatmiami suggested changes Oct 29, 2025
View reviewed changes
Copy link
Contributor

@andyatmiami andyatmiami left a comment

Choose a reason for hiding this comment

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

Really great work getting this spec'd out @kimwnasptd

I have some general questions/comments/observations - but nothing imho critically impacting ...

Will wait for your response/follow up on some of the outstanding questions before I engage in attempting to run this bad boy 💯

scripts/upgrade_v1_to_v2.sh Outdated
echo -e "\nWill remove namespaced resources with labels: $label"
for resource in $namespace_resources; do
echo "Removing all $resource objects..."
kubectl delete -n kubeflow -l $label $resource
Copy link
Contributor

@andyatmiami andyatmiami Oct 29, 2025

Choose a reason for hiding this comment

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

Would we want the --wait=true flag specified on this to ensure the resource(s) are really really gone before proceeding?

could also then specify --timeout=?s to something reasonable (whatever that would be!)

Copy link
Contributor

@andyatmiami andyatmiami Oct 29, 2025

Choose a reason for hiding this comment

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

Not totally sold myself on this - but I wonder if we'd also want to throw a --ignore-not-found=true flag on there as well..

in the event this script bailed for a wild variety of reasons outside our control... ignore-not-found would make subsequent runs less problematic

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

Yes, both are good suggestions as the assumptions of users would be that previous resources would have been completely removed.

I'll update accordingly

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

Regarding --ignore-not-found, I think we shouldn't need this one.

kubectl delete will not return a non-zero exit code if we try to delete objects based on labels. It would though if we would try to delete an object with its name. That's why in this line we have the ||
https://github.com/kubeflow/dashboard/pull/162/files#diff-afd68f06993c6a5670eb569a6764c40acaafa880d46db67999c93e5adadb1a14R48

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

be7d1cf

scripts/upgrade_v1_to_v2.sh Outdated

for resource in $cluster_resources; do
echo "Removing all $resource objects..."
kubectl delete -l $label $resource
Copy link
Contributor

@andyatmiami andyatmiami Oct 29, 2025

Choose a reason for hiding this comment

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

Would we want the --wait=true flag specified on this to ensure the resource(s) are really really gone before proceeding?

could also then specify --timeout=?s to something reasonable (whatever that would be!)

Copy link
Contributor

@andyatmiami andyatmiami Oct 29, 2025

Choose a reason for hiding this comment

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

Not totally sold myself on this - but I wonder if we'd also want to throw a --ignore-not-found=true flag on there as well..

in the event this script bailed for a wild variety of reasons outside our control... ignore-not-found would make subsequent runs less problematic

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

Added be7d1cf to address this one

README.md
* The script will not remove any CR (Custom Resource) or CRD (Custom Resource Definition), to ensure no data loss
* Only Kubernetes resources relevant to the Deployments will be removed (Deployment, ServiceAccount, Service etc)
* The [`NetworkPolicy`](https://github.com/kubeflow/manifests/blob/v1.10-branch/common/networkpolicies/base/centraldashboard.yaml) from the `kubeflow/manifests` repo, of the CentralDashboard, will be removed
2. Install the manifests from this repository for the Dashboard, Profiles Controller and PodDefaults webhook
Copy link
Contributor

@andyatmiami andyatmiami Oct 29, 2025

Choose a reason for hiding this comment

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

Asking b/c I have no idea - but is it "safe" to assume the overlays this script is using when deploying to any potential client (?)

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

I'd say yes, for:

  1. PodDefaults have only the cert-manager overlay, which is currently the only supported way for creating the Webhook certificate
  2. Profiles use the kubeflow overlay, which is what we want to install in this case (platform)

The only one with a bit of a debate would be the dashboard, as the script uses the kserve overlay (had a bug, and was using istio) which in turn expects KServe to be deployed.

Adding a small note in the README. LMKWDYT

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

5cf0e01

image

scripts/upgrade_v1_to_v2.sh Outdated
echo "Installing the updated Dashboard V2 components."
echo "-----------------------------------------------"

echo -e "\nApplying Dashboard component..."
Copy link
Contributor

@andyatmiami andyatmiami Oct 29, 2025

Choose a reason for hiding this comment

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

Minor nitpick - feel free to disagree.. but it seems to me we should deploy Dashboard last as its unusable until profile-controller is running...

Copy link
Member Author

@kimwnasptd kimwnasptd Oct 30, 2025

Choose a reason for hiding this comment

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

SGTM! Addressed in be7d1cf

@google-oss-prow google-oss-prow bot removed the lgtm label Oct 29, 2025
@kimwnasptd @andyatmiami
Update README.md
66e2345 
Co-authored-by: Andy Stoneberg <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@google-oss-prow
Copy link

google-oss-prow bot commented Oct 30, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from juliusvonkohout. For more information see the Kubernetes Code Review Process.

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

kimwnasptd and others added 3 commits October 30, 2025 09:55
@kimwnasptd @andyatmiami
Update README.md
4691fd5 
Co-authored-by: Andy Stoneberg <[email protected]>
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd
review: wait on delete and dashboard order
be7d1cf 
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd
review: explanation of dashboard overlay
5cf0e01 
Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd kimwnasptd force-pushed the feat-migration-script branch from 939ac4b to 5cf0e01 Compare October 30, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusvonkohout juliusvonkohout juliusvonkohout left review comments

@thesuperzapper thesuperzapper Awaiting requested review from thesuperzapper

+1 more reviewer

@andyatmiami andyatmiami andyatmiami requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@andyatmiami andyatmiami

@juliusvonkohout juliusvonkohout

Labels

ok-to-test size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Script for automating the upgrade to the dashboard v2 components from kubeflow/kubeflow

3 participants

@kimwnasptd @juliusvonkohout @andyatmiami