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

fix: explicitly apply minio-service with name #151

Merged
merged 2 commits into from
Oct 17, 2023
Merged

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Oct 13, 2023

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:

  • name should be minio-service
  • there must be a port with name minio and targetPort:9000
  • there must be a port with name console and targetPort:9001
  • the selector must be app.kubernetes.io/name: minio

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

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.
Refer to kubeflow/pipelines#9689 for more information
@DnPlas DnPlas requested a review from a team as a code owner October 13, 2023 15:31
orfeas-k
orfeas-k previously approved these changes Oct 17, 2023
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

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.

@DnPlas
Copy link
Contributor Author

DnPlas commented Oct 17, 2023

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 DnPlas force-pushed the dnplas-change-svc-name branch from fe949f9 to a4f0714 Compare October 17, 2023 12:43
@DnPlas
Copy link
Contributor Author

DnPlas commented Oct 17, 2023

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.

@DnPlas DnPlas enabled auto-merge (squash) October 17, 2023 12:49
@DnPlas DnPlas merged commit b99aad8 into main Oct 17, 2023
@DnPlas DnPlas deleted the dnplas-change-svc-name branch October 17, 2023 12:49
ca-scribner added a commit that referenced this pull request Nov 17, 2023
ca-scribner added a commit that referenced this pull request Nov 17, 2023
ca-scribner added a commit to canonical/kfp-operators that referenced this pull request Nov 17, 2023
This adds to kfp-api a service called `minio-service` which points to the related object-storage's s3 service.  This has been added to address a bug in upstream kfp, as explained [here](canonical/minio-operator#151).  This service was originally added to the minio charm in [minio pr 151](canonical/minio-operator#151), but has been refactored so it is added here instead as described in [minio issue 153](canonical/minio-operator#153).
ca-scribner added a commit to canonical/kfp-operators that referenced this pull request Nov 20, 2023
This adds to kfp-api a service called `minio-service` which points to the related object-storage's s3 service.  This has been added to address a bug in upstream kfp, as explained [here](canonical/minio-operator#151).  This service was originally added to the minio charm in [minio pr 151](canonical/minio-operator#151), but has been refactored so it is added here instead as described in [minio issue 153](canonical/minio-operator#153).

(cherry picked from commit 12572ca)
ca-scribner added a commit that referenced this pull request Nov 21, 2023
* Revert "fix: explicitly apply minio-service with name (#151)"
* feat: add alias for minio service name in charm.py

This reverts commit b99aad8.\
ca-scribner added a commit to canonical/kfp-operators that referenced this pull request Nov 22, 2023
This adds to kfp-api a service called `minio-service` which points to the related object-storage's s3 service.  This has been added to address a bug in upstream kfp, as explained [here](canonical/minio-operator#151).  This service was originally added to the minio charm in [minio pr 151](canonical/minio-operator#151), but has been refactored so it is added here instead as described in [minio issue 153](canonical/minio-operator#153).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants