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 kubeflow/kubeflow manifests from v1.9.0-rc.0 #2734

Conversation

kimwnasptd
Copy link
Member

This PR updates the manifests from kubeflow/kubeflow components in the manifests for 1.9.

The manifests are copied from https://github.com/kubeflow/kubeflow/releases/tag/v1.9.0-rc.0

/cc @thesuperzapper @juliusvonkohout @rimolive

Copy link

[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 kimwnasptd. 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

@juliusvonkohout
Copy link
Member

@kimwnasptd looks good on a first glimpse. The failing test is the only major thing.

@kimwnasptd
Copy link
Member Author

@juliusvonkohout the failure is unrelated to this PR though.

I see the following

Error: accumulating resources: accumulation err='accumulating resources from '../apps/centraldashboard/upstream/overlays/oauth2-proxy': evalsymlink failure on '/home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy' : lstat /home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy: no such file or directory': must build at directory: not a valid directory: evalsymlink failure on '/home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy' : lstat /home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy: no such file or directory

Are the oauth2-proxy manifests as expected?

@kimwnasptd
Copy link
Member Author

It looks like a manifest file was introduced in the manifests repo, but was not part of the source manifests
https://github.com/kubeflow/manifests/blob/v1.9-branch/apps/centraldashboard/upstream/overlays/oauth2-proxy/
https://github.com/kubeflow/kubeflow/tree/v1.9-branch/components/centraldashboard/manifests/overlays

Is this the intended behavior?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented May 29, 2024

It looks like a manifest file was introduced in the manifests repo, but was not part of the source manifests https://github.com/kubeflow/manifests/blob/v1.9-branch/apps/centraldashboard/upstream/overlays/oauth2-proxy/ https://github.com/kubeflow/kubeflow/tree/v1.9-branch/components/centraldashboard/manifests/overlays

Is this the intended behavior?

It is used to patch the logout https://github.com/kubeflow/manifests/blob/v1.9-branch/common/oidc-client/oauth2-proxy/components/central-dashboard/patches/deployment.logout-url.yaml

we need to find a new place for that patch, because it should be maintained within kubeflow/manifests. You can also readd it for now and we tackle that in RC.2.

CC @kromanow94

| Tensorboards Web App | apps/tensorboard/tensorboards-web-app/upstream | [v1.8.0](https://github.com/kubeflow/kubeflow/tree/v1.8.0/components/crud-web-apps/tensorboards/manifests) |
| Volumes Web App | apps/volumes-web-app/upstream | [v1.8.0](https://github.com/kubeflow/kubeflow/tree/v1.8.0/components/crud-web-apps/volumes/manifests) |
| Notebook Controller | apps/jupyter/notebook-controller/upstream | [v1.9.0-rc.0](https://github.com/kubeflow/kubeflow/tree/v1.9.0-rc.0/components/notebook-controller/config) |
| PVC Viewer Controller | apps/pvcviewer-roller/upstream | [v1.9.0-rc.0](https://github.com/kubeflow/kubeflow/tree/v1.9.0-rc.0/components/pvcviewer-controller/config) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo?

Suggested change
| PVC Viewer Controller | apps/pvcviewer-roller/upstream | [v1.9.0-rc.0](https://github.com/kubeflow/kubeflow/tree/v1.9.0-rc.0/components/pvcviewer-controller/config) |
| PVC Viewer Controller | apps/pvcviewer-controller/upstream | [v1.9.0-rc.0](https://github.com/kubeflow/kubeflow/tree/v1.9.0-rc.0/components/pvcviewer-controller/config) |

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's fix it

@rimolive
Copy link
Member

rimolive commented May 31, 2024

The error is in an overlay for oauth2 for central dashboard. I thought this was tested in kubeflow/kubeflow repo before, wasn't it?

EDIT: I got the whole thread now. What was the final decision about it? It looks like a simple change, so what's the ETA to get this done?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented May 31, 2024

The error is in an overlay for oauth2 for central dashboard. I thought this was tested in kubeflow/kubeflow repo before, wasn't it?

EDIT: I got the whole thread now. What was the final decision about it? It looks like a simple change, so what's the ETA to get this done?

I am in favour of just re-adding it (5 minutes of work) for now and fixing it in RC.2. RC.2 can be used for that and upgrading istio and the sometimes slow oauth2-proxy installation. Let's resynchronize on Monday for that.

@juliusvonkohout
Copy link
Member

@kimwnasptd Lets close this. It is syncing into the wrong branch. We need to sync to master first and then from master into 1.9.
Otherwise we end up in synchronization hell ;-)

@rimolive Can you run the script to create a new PR against the master branch and undelete /manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy.

Then we need to synchronize the 1.9 branch to master and you can tag RC.1 this week with #2739

@rimolive
Copy link
Member

rimolive commented Jun 3, 2024

As per Julius suggestion, will close this PR and create a new one with the missing manifest.

/close

@google-oss-prow google-oss-prow bot closed this Jun 3, 2024
Copy link

@rimolive: Closed this PR.

In response to this:

As per Julius suggestion, will close this PR and create a new one with the missing manifest.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@thesuperzapper
Copy link
Member

@rimolive @juliusvonkohout aside the PR target issue (the PR should obviously be into master).

It's probably better if we just move anything that was manually added to the "upstream" folder (this ouath2-proxy stuff) into its own folder, so we can sync easily in the future?

@rimolive
Copy link
Member

rimolive commented Jun 3, 2024

@thesuperzapper I agree with that. @juliusvonkohout let's work on this in future releases, or check if it's possible to make that change for 1.9.

@juliusvonkohout
Copy link
Member

Yes i hope so for 1.9 RC.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants