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

Add model registry sync script #2682

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Apr 16, 2024

Which issue is resolved by this Pull Request:
Resolves #2631

Description of your changes:

  • Added script to automatically fetch model-registry manifests, followed the same approach that has been done for other components
  • Updated README adding model-registry row, since there is no version yet I linked main branch

Checklist:

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

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
@lampajr
Copy link
Member Author

lampajr commented Apr 16, 2024

There are a couple of open points:

@rimolive
Copy link
Member

Thank you @lampajr for your contribution!

cc @juliusvonkohout

I believe components in alpha release aren't eligible to be part of the core manifests, correct?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Apr 17, 2024

Thank you @lampajr for your contribution!

cc @juliusvonkohout

I believe components in alpha release aren't eligible to be part of the core manifests, correct?

Just add this to the Agenda of the next Manifests WG meeting. Seldon should be a similar effort.

@lampajr
Copy link
Member Author

lampajr commented Apr 22, 2024

As discussed in the last Kubeflow Manifest WG meeting (2024-04-18) I am working on filling the requirements that are stated https://github.com/kubeflow/manifests/blob/master/proposals/20220926-contrib-component-guidelines.md#component-requirements.

For easier management we decided to add missing files, like manifests README and OWNERS, directly in the Kubeflow Model Registry repository (see kubeflow/model-registry#69). Thus, I think that this PR could be considered as ready for review as it simply adds the script to sink the manifest folder from Kubeflow Model Registry.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Apr 23, 2024

As discussed in the last Kubeflow Manifest WG meeting (2024-04-18) I am working on filling the requirements that are stated https://github.com/kubeflow/manifests/blob/master/proposals/20220926-contrib-component-guidelines.md#component-requirements.

For easier management we decided to add missing files, like manifests README and OWNERS, directly in the Kubeflow Model Registry repository (see kubeflow/model-registry#69). Thus, I think that this PR could be considered as ready for review as it simply adds the script to sink the manifest folder from Kubeflow Model Registry.

Are you sure that you also satisfy

  • All pods must run according to the offical Kubernetes podsecuritystandards restricted set. This means explicitly no root containers and dropping all capabilities in your pods securitycontexts. You can easily test it with a namespace annotation.
  • Running with Istio sidecars
  • Namespace isolation / multi tenancy (except for the ml-metadata backend which is tackled in phase 2)

@lampajr
Copy link
Member Author

lampajr commented Apr 23, 2024

Are you sure that you also satisfy

  • All pods must run according to the offical Kubernetes podsecuritystandards restricted set. This means explicitly no root containers and dropping all capabilities in your pods securitycontexts. You can easily test it with a namespace annotation.
  • Running with Istio sidecars
  • Namespace isolation / multi tenancy (except for the ml-metadata backend which is tackled in phase 2)

This is still an ongoing task to ensure those things.
At the moment with this PR and the other one on KFMR we should add all (or most of) missing files and setup the manifest sync automation.

@tarilabs @rimolive fyi

@juliusvonkohout
Copy link
Member

/lgtm
/approve

then we discuss the details when synchronizing

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout, lampajr

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

@juliusvonkohout
Copy link
Member

/unhold

@google-oss-prow google-oss-prow bot merged commit 8634c24 into kubeflow:master Apr 23, 2024
3 checks passed
@lampajr lampajr deleted the gh2631_kfmr_manifests_sync branch April 23, 2024 08:08
@juliusvonkohout
Copy link
Member

@lampajr do you also need a networkpolicy for the manifest repository? Otherwise there wont be incoming traffic to the Kubeflow namespace for your pods. Please check https://github.com/kubeflow/manifests/tree/master/common/networkpolicies

@lampajr
Copy link
Member Author

lampajr commented Apr 25, 2024

@lampajr do you also need a networkpolicy for the manifest repository?

We will discuss about this in the next KFMR bi-weekly meeting (next Monday)

Otherwise there wont be incoming traffic to the Kubeflow namespace for your pods.

I am not sure I get this sentence, based on the https://github.com/kubeflow/manifests/tree/master/common/networkpolicies it looks like more an optional, second line of defence and especially for those pods that are not secured by Istio. So I don't get why you are saying a generic "there won't be incoming traffic". Do you mean just for those not protected by Istio? Or am I missing something else? 🤔

@tarilabs
Copy link
Member

I have the same questions as @lampajr (thanks btw for contributing to this work).

As the Model Registry registers with Istio: https://github.com/kubeflow/model-registry/tree/main/manifests/kustomize/options/istio
is that enough not to require a network policy to be defined (according to this sentence); can that be confirmed please?

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Apr 25, 2024

I have the same questions as @lampajr (thanks btw for contributing to this work).

As the Model Registry registers with Istio: https://github.com/kubeflow/model-registry/tree/main/manifests/kustomize/options/istio is that enough not to require a network policy to be defined (according to this sentence); can that be confirmed please?

If you have any kind of webhook or service in the Kubeflow namespace that needs access from other namespaces or the apiserver (webhook) you need a networkpolicy, because that is blocked by default. But i can create one in 5 Minutes if I see your deployment ports. We will notice anyway when testing the release if you need one.

@tarilabs
Copy link
Member

thanks for the quick feedback @juliusvonkohout , let's keep posted 👍

doncorsean pushed a commit to doncorsean/kubeflow-manifests that referenced this pull request Jul 18, 2024
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
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.

Write a script to sync Model Registry manifests
4 participants