-
Notifications
You must be signed in to change notification settings - Fork 876
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
Updated and tested Seldon Core 1.15.0 #2326
Updated and tested Seldon Core 1.15.0 #2326
Conversation
@axsaucedo you need to sign the CLA |
Thank you @juliusvonkohout - I have signed the CLI a while back (the https://cla.developers.google.com/clas confirms I have valid CLA), it seems the job is still "waiting for status", and I believe I saw it pass in a previous commit, is there a way to re-trigger the check? |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the prompt response in getting the component updated to work with the latest kubeflow release!
I think Seldon also has proper support for PodSecurityStandards restricted. At least that was the case for the previous version. Nevertheless that is something that should also be tested. |
Thank you for the review @annajung - we'll work through the comments. @juliusvonkohout thanks for the re-test - do you know why the CLA is not working? In the site this is what I see: |
Ok it seems the CLA is passing now @juliusvonkohout. @annajung thank you for the comments, these should now be resolved - let me know if anything else required. |
@kimwnasptd ok I have now added the test - for some strange reason I can't seem to get the "Create KinD Cluster" step to pass in the CI, it seems it doesn't find the file |
@axsaucedo we recently changed slightly our files for installing KinD #2331. I'd suggest to do a rebase to the latest code and use the |
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
I think the problem is in this line in the logs https://github.com/kubeflow/manifests/actions/runs/3549119873/jobs/5961121555
I see that in K8s 1.22 The |
@kimwnasptd it seems that we may need to update some of the KIND tests as currently Seldon supports Kubernetes clusters up to 1.24 - currently we are in the process of upgrading the components, particularly HPA as per SeldonIO/seldon-core#4172, however have been careful as it would introduce breaking changes. Is there a way to set up the KIND cluster with kubernetes 1.24? I could add an extra config file to provide a pre-1-25-k8s kind test. Thank you - let me know if you need more details. |
@kimwnasptd thanks again for having a look, let me know your thoughts on the suggestion above, it does seem like the only way from our current state is to run the integration tests on a pre-1.25 k8s cluster, I can have a look at adding a |
@axsaucedo we discussed it with the release team as well in today's meeting (link to notes). In the KF 1.7 release we are targeting for K8s 1.25, but since we expect some vendors to not yet support this version we are also supporting K8s 1.24. I'm OK with having this component working with K8s 1.24, but let's have a clear text somewhere at the top of the README that exposes the K8s version. So that it will be clear to users that will use KF 1.7 that this component will not work with 1.25 (to avoid confusion, since we will be promoting that the target K8s version for KF 1.7 will be K8s 1.25) |
So @axsaucedo to merge this PR could you:
|
Ok perfect @kimwnasptd - very much appreciated. I will look at actioning these three points. Also absolutely agree on making a clear point in the README, as we also have a clear action plan towards ensuring seldon as a whole supports k8s 1.25+, we already have that PR suggested so it's a matter on how we can make sure we provide users with migration paths before we add it. In that case we'll add these changes as suggested and re-trigger the tests - I'll let you know if there are any issues / further questions. Thanks. |
Signed-off-by: Alejandro Saucedo <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
@kimwnasptd I've now updated the integration tests to work with these constraints, and added a note on "requirements" on the readme to reflect the dependency of k8s pre-1.25, as well as a link to the current efforts to upgrade. Let me know if there's anything else needed on the PR 👍 |
on: | ||
pull_request: | ||
paths: | ||
- contrib/seldon/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be okay for now, but @kimwnasptd we should think about other ways to trigger /contrib
tests in the future and not just trigger based on changes to the contrib/seldon
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking the same thing! We could probably use the workflow_dispatch
event as well in these workflows, to be able to manually trigger them https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch
Let's play around with this more once we have the components update their manifests and have some basic tests in place.
Co-authored-by: Anna <[email protected]>
Signed-off-by: Alejandro Saucedo <[email protected]>
Thanks for working with us to update Seldon to meet the contrib guidelines |
Fantastic @annajung - thanks for the update, let me know if anything else needed fromy my side to merge 👍 |
Was about to comment regarding the README.md, but @annajung had covered it already #2326 (comment), nice! @axsaucedo thank you for your time and great work on updating these! The tests will also help us ensuring the quality of the manifests we bring along and even raise the standard. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: axsaucedo, kimwnasptd 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 |
# Profiles + KFAM | ||
- ../apps/profiles/upstream/overlays/kubeflow | ||
# - ../apps/profiles/upstream/overlays/kubeflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to remove Profile controller from Kubeflow example installation?
@axsaucedo why did you delete kubeflow-edit-seldon.yaml? |
* Updated and tested Seldon Core 1.15.0 Signed-off-by: Alejandro Saucedo <[email protected]> * Added testing section Signed-off-by: Alejandro Saucedo <[email protected]> * Added upgrading reference Signed-off-by: Alejandro Saucedo <[email protected]> * Added automated test Signed-off-by: Alejandro Saucedo <[email protected]> * Added automated tests and documentation Signed-off-by: Alejandro Saucedo <[email protected]> * Added upgrade documentation * Added description of product Signed-off-by: Alejandro Saucedo <[email protected]> * Updated contrib to ensure group maps correctly Signed-off-by: Alejandro Saucedo <[email protected]> * Added updated instructions for non-linux Signed-off-by: Alejandro Saucedo <[email protected]> * Added gh action for seldon test Signed-off-by: Alejandro Saucedo <[email protected]> * Added gh action for seldon test Signed-off-by: Alejandro Saucedo <[email protected]> * Added description of product Signed-off-by: Alejandro Saucedo <[email protected]> * Updating path for kind Signed-off-by: Alejandro Saucedo <[email protected]> * Updating path for kind Signed-off-by: Alejandro Saucedo <[email protected]> * Updating path for kind Signed-off-by: Alejandro Saucedo <[email protected]> * Added kind cluster and rebase Signed-off-by: Alejandro Saucedo <[email protected]> * Updated back to running kind script Signed-off-by: Alejandro Saucedo <[email protected]> * Fixed typo Signed-off-by: Alejandro Saucedo <[email protected]> * Updated to include latest manifests Signed-off-by: Alejandro Saucedo <[email protected]> * Create namespace if not exists Signed-off-by: Alejandro Saucedo <[email protected]> * Adding certmanager Signed-off-by: Alejandro Saucedo <[email protected]> * Added namespaces Signed-off-by: Alejandro Saucedo <[email protected]> * Added waiting for resources Signed-off-by: Alejandro Saucedo <[email protected]> * Updating to path Signed-off-by: Alejandro Saucedo <[email protected]> * Added wait for condition Signed-off-by: Alejandro Saucedo <[email protected]> * Added wait for condition Signed-off-by: Alejandro Saucedo <[email protected]> * Added wait for condition Signed-off-by: Alejandro Saucedo <[email protected]> * Added wait for condition Signed-off-by: Alejandro Saucedo <[email protected]> * Added wait for condition Signed-off-by: Alejandro Saucedo <[email protected]> * Added 1.24 kind tests plus note in the readme Signed-off-by: Alejandro Saucedo <[email protected]> * Extending timeout of wait for Signed-off-by: Alejandro Saucedo <[email protected]> * Update contrib/seldon/README.md Co-authored-by: Anna <[email protected]> * Reverted update on readme Signed-off-by: Alejandro Saucedo <[email protected]> Signed-off-by: Alejandro Saucedo <[email protected]> Co-authored-by: Anna <[email protected]>
Signed-off-by: Alejandro Saucedo [email protected]
Which issue is resolved by this Pull Request:
Resolves part of #2311 addressing #2322.
Description of your changes:
make -C contrib/seldon/ test
)Testing
A section was added under
contrib/seldon/README.md
containing the instructions to test, which are below.It is now possible to run the automated tests with
make -C contrib/seldon/ test
.Explicit overview of manual install & testing
Installation can be done with the kustomize command:
We can create a test model once the "Install Seldon Operator" is configured
We can create an echo model with the following command:
We can verify that model is running:
Also we can verify that the correct virtualservice was created:
Finally we can send a request (you will need to fetch the Dex Auth Token / Cookie):