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.

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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 4 commits into from

Conversation

alexcreasy
Copy link
Contributor

@alexcreasy alexcreasy commented Aug 13, 2024 •
edited
Loading

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

@google-oss-prow google-oss-prow bot requested review from tarilabs and Tomcli August 13, 2024 16:58
@alexcreasy alexcreasy mentioned this pull request Aug 13, 2024
Open
@alexcreasy
Copy link
Contributor Author

alexcreasy commented Aug 13, 2024 •
edited
Loading

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 •
edited
Loading

@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.

tarilabs
tarilabs reviewed Aug 19, 2024
View reviewed changes
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
@tarilabs
Decouple MLMD types setup from Go core library (kubeflow#267)
655e4da 
* implement Go core changes

* add test coverage

* refactoring

* minor message change

* nit: renamed function
@tarilabs tarilabs mentioned this pull request Sep 2, 2024
Open
@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

@tarilabs
Copy link
Member

I still wonder why it fails at

kubectl wait --for=condition=available -n kubeflow deployment/model-registry-db --timeout=5m

@alexcreasy
Copy link
Contributor Author

@tarilabs I'm not sure, I haven't looked into it yet - it's strange, I can deploy this to kind locally. I'll dig into this next week and see what I can find.

@alexcreasy
Copy link
Contributor Author

I wonder if it's some issue with pulling the image? At a guess (and this is a complete guess) I wonder if the db image needs to be manually loaded into kind for the deployment to be able to use it.

@tarilabs
Copy link
Member

I wonder if it's some issue with pulling the image? At a guess (and this is a complete guess) I wonder if the db image needs to be manually loaded into kind for the deployment to be able to use it.

Maybe, but this looks not being a problem for mysql 8.0.3 🤔

@tarilabs
debug handle for kind deploy
7a8bded 
Signed-off-by: Matteo Mortari <[email protected]>
@google-oss-prow Google OSS Prow
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 tomcli 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

@tarilabs
Copy link
Member

let's see if 7a8bded is of any help in digging further into the actual error 🧐

@tarilabs
more debug
ad2db00 
Signed-off-by: Matteo Mortari <[email protected]>
@tarilabs
Copy link
Member

let's see if 7a8bded is of any help in digging further into the actual error 🧐

with the log extraction, locate the following:

ERROR 2002 (HY000): Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2)

which brings up:

so the solution appears to be removing the AppArmor profile for mysql in the GHA runner...

@google-oss-prow google-oss-prow bot added size/S and removed size/XS labels Nov 22, 2024
@tarilabs
Copy link
Member

All green now, awesome 🚀

Now, if only we had some answer on...

@tarilabs
Copy link
Member

btw, I assume this resolves #266

@tarilabs tarilabs linked an issue Nov 22, 2024 that may be closed by this pull request
Cannot deploy to k8s on AArch64 nodes using manifests in repo #266
Open
@tarilabs
Copy link
Member

btw, this also should be of great advantage to #327 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarilabs tarilabs tarilabs left review comments

@Tomcli Tomcli Awaiting requested review from Tomcli

Assignees
No one assigned
Labels
Area/GitHub Area/Manifests size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot deploy to k8s on AArch64 nodes using manifests in repo
3 participants
@alexcreasy @rareddy @tarilabs