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

Support functionality to override default images for kfp-profile-controller #416

Merged
merged 12 commits into from
Apr 12, 2024

Conversation

misohu
Copy link
Member

@misohu misohu commented Apr 3, 2024

This PR fixes canonical/bundle-kubeflow#851

New functionality:

  • User can override images for ui-artifact and visualizationserver pods through custom-images config setting.
  • Added integration tests which checks that the config change succeeds and that the new profile will have deployments with different images.

CI passes except bundle tests which was previously observed in #399

@misohu misohu requested a review from a team as a code owner April 3, 2024 11:02
@misohu misohu force-pushed the KF-5476-fix-customisable-images-profile-controller branch from c23bb79 to feb5048 Compare April 3, 2024 11:15
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

Minor comments but otherwise lgtm

Is there anything more we should do to demonstrate that this PR solves canonical/bundle-kubeflow#851? We demonstrate that the pods in the user namespace come up with the updated images, so I think that's enough? We could test in airgapped, but I think that is overkill... wdyt?

charms/kfp-profile-controller/requirements-lint.txt Outdated Show resolved Hide resolved
charms/kfp-profile-controller/src/charm.py Show resolved Hide resolved
charms/kfp-profile-controller/src/charm.py Show resolved Hide resolved
@misohu misohu force-pushed the KF-5476-fix-customisable-images-profile-controller branch from d85fa16 to a86ef44 Compare April 3, 2024 14:52
@misohu
Copy link
Member Author

misohu commented Apr 4, 2024

Thanks @ca-scribner for your review I resolved your comments. I dont think airgapped is needed for now (yes it will be overkill). Moreover I believe the proposed integration test is handling the problem from the issue. It demonstrates that if you change the config, desired images will be deployed with new profiles.

Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

minor suggestions otherwise lgtm

ca-scribner
ca-scribner previously approved these changes Apr 11, 2024
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

lgtm, but CI is failing due to this charmcraft issue. That should be fixed ~today by charmcraft 2.6.0, and we should rerun CI once that ships to confirm everything works.

@misohu misohu merged commit e32222f into main Apr 12, 2024
46 checks passed
NohaIhab pushed a commit that referenced this pull request May 9, 2024
switch to jlumbroso/free-disk-space for freeing runner space (#428)

The previous space freeing method (easimon/maximize-build-space) at some point circa 2024-01 stopped freeing as much space, likely due to changes in the runner (~29GB free after it freed space).  Not sure why this happened, but jlumbroso/free-disk-space at time of this commit would get us up to ~45GB free on the runner without negative effects so we've switched to that.

Support functionality to override default images for kfp-profile-controller (#416)

* Support functionality to override default images for kfp-profile-controller

ci: remove destructive mode from integration tests (#441)

The charmcraft issues that forced us to use destructive mode are now fixed.

build: install `jinja2` from binary (#443)

This commit installs jinja2 (an install dependency of charmed-kubeflow-chisme) as a binary
to avoid running into the build time issues described in canonical/bundle-kubeflow#883.

Part of canonical/bundle-kubeflow#883

refactor: use k8s_service_info lib instead of SDI (#436)

* refactor: use k8s_service_info lib instead of SDI

Use the k8s_service_info for receiving the MLMD GRPC Service info instead of using
the SDI, as it will stop being supported soon.
This commit also ensures that mlmd runs with trust=True in the
integration tests.

Fixes #413

fix: Pin integration test dependencies in main (#434)

* pin integration test dependencies

Co-authored-by: Daniela Plascencia <daniela.plascencia@canonical.com>

feat: Integrate ROCK in metadata-writer charm (#439)

chore: Bump o11y libs and remove obsolete juju topology (#446)

Ref canonical/bundle-kubeflow#880
Ref canonical/bundle-kubeflow#849

ci: bump juju to 3.5
NohaIhab pushed a commit that referenced this pull request May 24, 2024
…roller (#416)

* Support functionality to override default images for kfp-profile-controller
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.

Charmed Kubeflow 1.7 Airgapped deployment - user namespace repository
2 participants