feat: Add support for creation of multi-tenancy resources in Kubeflow profiles#34
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds optional S3/object-storage integration to the data-kubeflow-integrator charm so it can generate and dispatch the Kubeflow multi-tenancy resources needed for KFP pipelines (Secret + ConfigMaps) into profile namespaces via resource-dispatcher.
Changes:
- Introduces a new
s3-credentialsrelation and anS3Managerthat rendersmlpipeline-minio-artifact(Secret) plusartifact-repositoriesandkfp-launcher(ConfigMaps). - Adds a new
config-mapskubernetes_manifest relation and wires manifest dispatching for ConfigMaps alongside existing secrets/roles/etc. - Adds unit + integration coverage for S3 behavior, and extends integration test harness to support
--model/--charm-path.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_s3.py | New unit tests validating S3 relation behavior, endpoint parsing, and dispatched manifests. |
| tests/spread/test_s3/task.yaml | New spread task to run the S3 integration test module. |
| tests/integration/test_s3.py | New integration tests that deploy s3-integrator, relate it, and assert k8s resources are created/removed. |
| tests/integration/conftest.py | Adds --model/--charm-path options and increases Juju wait timeout for integration runs. |
| src/utils/k8s_models.py | Extends ReconciledManifests to carry ConfigMaps for dispatching. |
| src/utils/helpers_manifests.py | Adds helpers to render the Secret + ConfigMaps and endpoint parsing logic. |
| src/managers/s3.py | New manager to validate S3 connection info, set status, and generate S3-related manifests. |
| src/events/s3.py | New events handler to react to S3 credentials changed/gone and trigger reconciliation. |
| src/events/manifest.py | Adds a ConfigMaps manifests wrapper and includes S3 manifests in reconciliation + dispatch. |
| src/core/statuses.py | Adds a blocked status for missing S3 fields after a relation is established. |
| src/core/state.py | Adds S3Requirer, S3 config accessors, and relation presence helpers for config-maps and S3. |
| src/core/config.py | Adds S3Config model for s3-default-pipeline-root. |
| src/constants.py | Adds S3 relation/constants and names/keys for the generated KFP multi-tenancy resources. |
| src/charm.py | Wires S3 manager + events into the charm lifecycle/status handling. |
| metadata.yaml | Declares s3-credentials and config-maps relations. |
| lib/charms/data_platform_libs/v0/s3.py | Vendors the S3 interface library used by the charm. |
| config.yaml | Adds the s3-default-pipeline-root config option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
theoctober19th
left a comment
There was a problem hiding this comment.
Great work, Manos!
I've raised a few small points, but the major TODO thing here is to replace the s3 lib with object-storage-charmlib from PyPI.
Regarding the addition of option to disable auto creation of the bucket, if the only blocker it puts is forcing us to simulate microceph setup, I actually think the S3 Integrator did the right thing not passing the credentials to the data-integrator charm, because then we would be testing the behavior closer to the actual production environment, because I don't think we would ever want to have a setup where S3 Integrator passes something to the related charms where that something does not exist at all.
But then the decision to whether add that option or not would be a Data Platform decision, so we could delegate this question / request to the team nevertheless.
deusebio
left a comment
There was a problem hiding this comment.
The structure is there and compliant with the current code base. Also the tests are sensible and well structure, modulo the comment from bikalpa to also tests unhappy path with blocked status.
However, as discussed, could you please review the code to be less general towards s3, but more KFP specifi in the config/endpoints/class names?
A few other minor comments.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
theoctober19th
left a comment
There was a problem hiding this comment.
Adding on what @deusebio has commented, I agree that the name could be improved. Also, I am against naming this just kfp which is very confusing, maybe a self explanatory prefix like kfp_s3 or kfp_s3_storage everywhere would be a nice idea. (For classes, KFPS3StorageManager and so on..).
deusebio
left a comment
There was a problem hiding this comment.
Looks great! Please only add some extra safety checks/block when the pipeline root is using something else other than minio, and that would not work, also saying that although we specify minio, any s3 compatible backend should be supported, right?
Once that is addressed, I believe we should be good to go for my part!
|
To address the comment from @theoctober19th, renamed all resources related to KFP S3 storage to something more descriptive (af2c191) |
|
To address the comments from @deusebio, I raised kubeflow/pipelines#13717, and added some validation for blocking an |
theoctober19th
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedbacks!
Summary
kfp-s3-storagevia s3 interface. When integrated, creates 3 multi-tenancy resources needed to launch KFP pipelines:kfp-launcherconfigMapmlpipeline-minio-artifactSecretsecretsandconfig-mapsrelations.config-mapsto achieve the above point.kfp-s3-storagerelation is optional, but if there is a relation made, block if there are missing values.kfp-pipeline-rootconfig option, similar tokfp-profile-controller, and pass this value to thekfp-launcherconfigmaptest_s3.py, andhelpers_s3.pyto deploy S3 storage that are taken directly from object-storage-integratorconftest.pyto make use of a--modeland--charm-pathoption, similar to how we do in other repositories, see feast-operators. This way we can run a test withNotes
src/templates, similar to the other resources.securevalue for thekfp-launcheris similar to how we handle it inkfp-operators.test_s3.py, we don't actually make use of actual S3 storage, since we only want to confirm whether the credentials froms3-integratorare passed properly to the multi-tenancy resources. Unfortunately, the2/edgebranch ofs3-integratorcurrently blocks and doesn't transmit the credentials if it cannot create a bucket. I created Addensure_bucketconfig option to disable automatic creation of buckets object-storage-integrator#180 to add a new config option in the charm.Closes #33