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

Bump mysql container image to one with an aarch64 image #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexcreasy
Copy link
Contributor

@alexcreasy alexcreasy commented Aug 13, 2024

Description

Bumps the version of the MySQL container image in the overlays/db kustomization to 8.0.39 which is multi-arch (amd64 / aarch64)

How Has This Been Tested?

Deployed to a local kind cluster on an apple silicon MacBook, using official instructions to deploy using kind on macOS.

*https://www.kubeflow.org/docs/components/pipelines/legacy-v1/installation/localcluster-deployment/#kind
*https://www.kubeflow.org/docs/components/model-registry/installation/#standalone-installation

The only variation of above was that I deployed to kind using my branch of the manifest:
kubectl apply -k "https://github.com/alexcreasy/model-registry/manifests/kustomize/overlays/db?ref=kind"

I've verified I'm able to deploy as above, create a port forward to the service, and query the rest endpoint as indicated in the example above and receive a 200 response.

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

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 assign tarilabs for approval. 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

@alexcreasy
Copy link
Contributor Author

alexcreasy commented Aug 13, 2024

Ahhh... yay the old "it worked on my laptop" scenario 🤦

I guess the new image isn't being loaded into kind on the CI jobs.

@rareddy
Copy link
Contributor

rareddy commented Aug 14, 2024

isn't MR using MariaDB not mySQL that too a version that is matched with KFP?

@tarilabs
Copy link
Member

I believe we had to stick with the currently used version, as that's the one tested to be working with MLMD.
Further, this won't solve the issue you linked, for the reasons I've summarized in #266 (comment)

Let me know your thoughts :)

@alexcreasy
Copy link
Contributor Author

alexcreasy commented Aug 15, 2024

@tarilabs thanks for taking a look, I believe it will solve my issues, as I'm trying to deploy model registry without kubeflow (full explanation of what I'm trying to do on the ticket) - granted I've only run a few curl requests against the MR API on my local kind deployment, so there could be some hidden issue I'm not aware of, but so far they work as expected using the modified manifests on my fork.

Let me know if I've misunderstood the core problem you described :)

@rareddy
Copy link
Contributor

rareddy commented Aug 16, 2024

I believe we had to stick with the currently used version, as that's the one tested to be working with MLMD.

I was under impression that KFP team use MariaDB for writing some windowing functions over it which were only supported in MariaDB and working fine for them. May be this only used for Red Hat's distribution.

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

I was under impression that KFP team use MariaDB for writing some windowing functions over it which were only supported in MariaDB and working fine for them. May be this only used for Red Hat's distribution.

I believe the latter (and I believe the "window function" should have parity between mysql and mariadb).

this is as far as I can see KF 1.8:

https://github.com/kubeflow/manifests/blob/a925b2968e68f3085092adc04e9af39d4f5045f3/apps/pipeline/upstream/base/metadata/overlays/db/metadata-db-deployment.yaml#L24

KF 1.9:

https://github.com/kubeflow/manifests/blob/a38c2be88fbafb0844c0231f0062e4b3719d4737/apps/pipeline/upstream/base/metadata/overlays/db/metadata-db-deployment.yaml#L24

and default branch "master":

https://github.com/kubeflow/manifests/blob/bf3dd8878af2a52480e60ee90d650a098eacd113/apps/pipeline/upstream/base/metadata/overlays/db/metadata-db-deployment.yaml#L24

Always mysql:8.0.3.

My recommendation is as follows:

  1. send a PR first to KFP/manifest proposing this change, so we can gauge if pipeline team is in favour of aligning on this version (otherwise, we would be diverging from the common configuration)
  2. for DX, which is the goal of this PR, this could be handled by a different overlay for "dev"
  3. assess why tests are failing (condition on DB is not met)

dhirajsb pushed a commit to dhirajsb/model-registry-kfp that referenced this pull request Aug 30, 2024
* implement Go core changes

* add test coverage

* refactoring

* minor message change

* nit: renamed function
@tarilabs
Copy link
Member

tarilabs commented Sep 9, 2024

I want to augment the comment in #267 (review)
with the information that the default vanilla installation uses a custom image of mysql from google container registry:

https://github.com/kubeflow/manifests/blob/a38c2be88fbafb0844c0231f0062e4b3719d4737/apps/pipeline/upstream/third-party/mysql/base/mysql-deployment.yaml#L51

👉 gcr.io/ml-pipeline/mysql:8.0.26

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.

3 participants