canonical / minio-operator

MinIO Operator
Apache License 2.0
2 stars 9 forks source link

fix: explicitly apply minio-service with name #151

Closed DnPlas closed 9 months ago

DnPlas commented 9 months ago

The upstream pipelines project hardcodes the name of the object storage service to minio-service. The current implementation sets the service to just minio, which collides with what pipelines expect. This PR ensures the Service is created with the expected name and ports.

Testing

To test this change, you can build and deploy the charm and look for the Service minio-service. It should have the following:

These are the logs we are trying to avoid with the changes in this PR:

failed to execute component: failed to upload output artifact "output_dataset_one" to remote storage URI "minio://mlpipeline/v2/artifacts/tutorial-data-passing/731d5ddd-7b18-49ce-9c31-e72ee1b5b252/preprocess/output_dataset_one": uploadFile(): unable to complete copying "/minio/mlpipeline/v2/artifacts/tutorial-data-passing/731d5ddd-7b18-49ce-9c31-e72ee1b5b252/preprocess/output_dataset_one" to remote storage "preprocess/output_dataset_one": failed to close Writer for bucket: blob (key "preprocess/output_dataset_one") (code=Unknown): RequestError: send request failed
caused by: Put "http://minio-service.kubeflow:9000/mlpipeline/v2/artifacts/tutorial-data-passing/731d5ddd-7b18-49ce-9c31-e72ee1b5b252/preprocess/output_dataset_one": dial tcp: lookup minio-service.kubeflow on 10.152.183.10:53: no such host

Refer to kubeflow/pipelines#9689 for more information

DnPlas commented 9 months ago

Thank you @DnPlas for the PR.

Deployed mino from latest/edge and the one built from the PR and verified that it now creates a service named minio-service which is identical to the one we had (meaning that we do not change any other configuration).

Note also that the charm still creates the minio service, thus we end up with 2 identical services (minio and minio-service). Since this doesn't create an issue though, I 'll go ahead and approve.

Hey @orfeas-k, thanks for reviewing. You have made a good point, explicitly defining a Service in the charm code will create a second Service with a different name, but identical in all other fields. I have changed the approach to avoid this and instead used the KubernetesServicePatcher to make sure the Service is configured as we want.

DnPlas commented 9 months ago

Thank you @DnPlas for the PR. Deployed mino from latest/edge and the one built from the PR and verified that it now creates a service named minio-service which is identical to the one we had (meaning that we do not change any other configuration). Note also that the charm still creates the minio service, thus we end up with 2 identical services (minio and minio-service). Since this doesn't create an issue though, I 'll go ahead and approve.

Hey @orfeas-k, thanks for reviewing. You have made a good point, explicitly defining a Service in the charm code will create a second Service with a different name, but identical in all other fields. I have changed the approach to avoid this and instead used the KubernetesServicePatcher to make sure the Service is configured as we want.

After trying this approach, we noticed that it won't work as the charm has to be trusted. Since this is a podspec charm, that is not possible. Reverting back to the initial approach of defining a Service directly as a Kubernetes Resource to be applied.

For now, having two Services should not be an issue as the Service data that is actually used and shared comes from the minio-service and not from the juju created minio Service. When we migrate to sidecar, this inconvenience will be gone.