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

Add UI, PostgreSQL, Minio and S3 relations #6

Merged
merged 8 commits into from
Jun 4, 2024
Merged

Add UI, PostgreSQL, Minio and S3 relations #6

merged 8 commits into from
Jun 4, 2024

Conversation

kelkawi-a
Copy link
Collaborator

This PR does the following:

  • Adds a relation to the PostgreSQL charm to act as Airbyte's DB.
  • Adds a relation to the Minio charm as an option for object storage.
  • Adds a relation to the S3 integrator charm as another option for object storage.
  • Adds a relation to the Airbyte UI charm to enable communication of server status and service name between the two. The corresponding UI PR can be found here.
  • Adds a configuration value for logs TTL, which gets added as a lifecycle policy for the selected object storage bucket.
  • Adds logic for creating object storage buckets on Minio/S3 storage on relation creation or on config change.
  • Refactors charm code.

Note: I've tested the functionality of adding source and destination connectors, and running a sync job. The only shortcoming thus far is that I am not able to get the Airbyte worker to store the logs in object storage (the bucket creation at the charm level works, Airbyte is just not communicating with the object storage properly to upload the logs). I am currently investigating this issue on different Airbyte slack channels and threads [1,2,3]. I have also tested the S3 integration with RadosGW, but Airbyte seems to have hardcoded this to AWS S3 storage, not just any S3-compatible storage. Regardless, this PR can be considered ready for review, with minor fixes to come potentially on some environment variables later.

@mertalpt
Copy link

Before I review the PR, I noticed something:

I have also tested the S3 integration with RadosGW, but Airbyte seems to have hardcoded this to AWS S3 storage, not just any S3-compatible storage.

@kelkawi-a How does minio work in the current Airbyte deployment? Does it use a different storage class?

Copy link

@mertalpt mertalpt left a comment

Choose a reason for hiding this comment

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

LGTM overall but not approving until the "issue" thread is resolved.

@kelkawi-a
Copy link
Collaborator Author

Before I review the PR, I noticed something:

I have also tested the S3 integration with RadosGW, but Airbyte seems to have hardcoded this to AWS S3 storage, not just any S3-compatible storage.

@kelkawi-a How does minio work in the current Airbyte deployment? Does it use a different storage class?

Minio on k8s stores on persistent storage.

Copy link

@gtato gtato left a comment

Choose a reason for hiding this comment

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

left some minor comments and questions
it's nice to see this taking shape
great job!

try:
charm = self.charm
# Hack: get_interfaces checks for peer relation which does not exist under requires/provides list in charmcraft.yaml
del charm.meta.relations["peer"]
Copy link

Choose a reason for hiding this comment

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

so if you don't delete the peer from meta, then what.. get_interfaces fails?
is there a way to get a specific interface... since you don't care about the others?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, get_interfaces iterates through all the relations and searches for them under the list of relations defined in requires and provides, but the peer relation is a special case which does not exist under them, so the method fails. Anyway, we do not care for searching for the peer relation, hence why it can be removed.

@@ -0,0 +1,141 @@
# Copyright 2024 Canonical Ltd.
Copy link

Choose a reason for hiding this comment

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

are these 2 files s3 and s3_helpers coming from somewhere? another charm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are defined for this charm, but there are occurences of the helpers in other charms that relate to the s3 integrator as well (e.g. the create_bucket_if_not_exists method)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants