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

Sidecar rewrite #71

Closed
misohu opened this issue Jan 29, 2024 · 5 comments · Fixed by #72
Closed

Sidecar rewrite #71

misohu opened this issue Jan 29, 2024 · 5 comments · Fixed by #72
Assignees
Labels
enhancement New feature or request

Comments

@misohu
Copy link
Member

misohu commented Jan 29, 2024

Context

We rewrite all of our charms using the sidecar pattern instead of the old podspec.

What needs to get done

Rewrite the charm using sidecar with base charm pattern.

Definition of Done

Charm is rewritten with sidecar with base charm pattern.
All of the tests are rewritten and passing.

@misohu misohu added the enhancement New feature or request label Jan 29, 2024
Copy link

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/KF-5256.

This message was autogenerated

@DnPlas DnPlas self-assigned this Feb 5, 2024
@DnPlas
Copy link
Contributor

DnPlas commented Feb 5, 2024

For completing this task we will need to consider the following

Base charm usage

Because we are using the base charm standard to refactor this charm, the proposed components are:

  • MlmdPebbleService - for creating and updating the Pebble layer. This component will make sure to run the bin/metadata_store_server command with all required flags.
  • LeadershipGateComponent - to make sure the unit is the leader

Since this is a relatively easy charm, nothing else should be required.

Sqlite to mysql migration

The current implementation of this charm uses sqlite and a volume to store all ML metadata. Ideally we will move away from that approach and integrate with the mysql-k8s charm.
For this, we'll need to make sure the mysql-k8s charm adapts to the requirements of the mlmd workload and shares the information it expects (db host, port, user, password and db name). More investigation is required for completing this sub-task.

k8s-service relation interface replacement

Because this relation interface is based on the old SDI implementation, the proposal is to replace it with the newer version that's worked in canonical/mlops-libs#2. This sub-task requires us to potentially change all the charms that relate to mlmd to ensure a correct data sharing.

@DnPlas
Copy link
Contributor

DnPlas commented Feb 7, 2024

Update on the mysql-k8s integration

We have tried using mysql-k8s to replace sqlite as a database. We have found issues in the past and decided not to go with that approach due to the expectations of both the mlmd workload and the mysql-k8s restrictions. For more details about this, please refer to #64.

Because of the above, we may have to stick with the sqlite solution, which means that we'll need to "manually" configure the volumes of the application containers:

  1. Keeping the filesystem storage defined already in metadata.yaml
  2. Using pebble to push the metadata_store_server_config_file to /config/config.proto

Please note that this solution completely deviates from the upstream deployment, where they are using mysql as their DB.

@kimwnasptd
Copy link
Contributor

@DnPlas since MLMD Charm is using a PVC for holding the data. Then how will the upgrade look like for this Charm (podspec to sidecar) if we need to remove/re-apply the Charm?

If it's indeed a problem we don't have to tackle it in this issue, but let's have a follow-up then and track it in the 1.9 release

@DnPlas
Copy link
Contributor

DnPlas commented Feb 8, 2024

@DnPlas since MLMD Charm is using a PVC for holding the data. Then how will the upgrade look like for this Charm (podspec to sidecar) if we need to remove/re-apply the Charm?

If it's indeed a problem we don't have to tackle it in this issue, but let's have a follow-up then and track it in the 1.9 release

@kimwnasptd I'm not entirely sure what you mean by "we need to remove/re-apply the Charm`. Are you saying this because in the past we had to remove some charms because of incompatibilities?

My understanding for the upgrade story is that the storage and its data should not be lost during the process. When we remove a charm that has a storage attached, the default action juju takes is to detach the storage, and not remove it entirely, which means that after re-deploying the application we must be able to attach it to it. I will test this is the case for this effort.

DnPlas added a commit that referenced this issue Mar 20, 2024
This commit introduces changes that support the refactoring of the charm
from podspec to sidecar pattern.

Fixes #71
DnPlas added a commit that referenced this issue Mar 21, 2024
This commit introduces changes that support the refactoring of the charm
from podspec to sidecar pattern.

Fixes #71
DnPlas added a commit that referenced this issue Apr 8, 2024
The charm has a mysql relation that is not used as it is provided by the mariadb charm which is no longer supported.
The other alternative is to use the relational-db relation to relate to mysql-k8s charm, but due to #64, it is not possible.

In #71 it was decided that instead of an external DB provider, the mlmd charm will use the SQLite implementation it has been using for a while.
In PR#72 the charm code has been modified to exclusively use SQLite.

Fixes #73
DnPlas added a commit that referenced this issue Apr 16, 2024
* refactor: rewrite mlmd charm to sidecar pattern

This commit introduces changes that support the refactoring of the charm
from podspec to sidecar pattern.

Fixes #71
DnPlas added a commit that referenced this issue Jun 21, 2024
The charm has a mysql relation that is not used as it is provided by the mariadb charm which is no longer supported.
The other alternative is to use the relational-db relation to relate to mysql-k8s charm, but due to #64, it is not possible.

In #71 it was decided that instead of an external DB provider, the mlmd charm will use the SQLite implementation it has been using for a while.
In PR#72 the charm code has been modified to exclusively use SQLite.

Fixes #73
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

3 participants