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 metacontroller for kubeflow 1.4 #1

Merged
merged 27 commits into from
Nov 23, 2021
Merged

Add metacontroller for kubeflow 1.4 #1

merged 27 commits into from
Nov 23, 2021

Conversation

ca-scribner
Copy link
Contributor

@ca-scribner ca-scribner commented Sep 22, 2021

This is an MVP of the charm. A fully implemented charm should have better status handling, teardown, etc.

PR is now fully fleshed out charm for the Metacontroller. If things are missing, I missed them!

* install hook has MVP functionality.  It installs correctly and monitors the stateful until it is ready.  Does not monitor/check other objs
* includes basic test coverage

Todo:
* update-status hook
* remove hook
* all TODO items in code
Hook fails because kubectl cannot access environment.  Maybe juju sets up the remove hook env differently than install?
Current version will restore the state if it notices the statefulset is missing/incomplete
.github/workflows/publish.yaml Outdated Show resolved Hide resolved
metadata.yaml Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@ca-scribner
Copy link
Contributor Author

This charm is missing RBAC needed for the status checking. Currently it breaks when RBAC is enabled

@ca-scribner ca-scribner marked this pull request as ready for review November 12, 2021 20:04
@ca-scribner ca-scribner requested a review from a team as a code owner November 12, 2021 20:04
.flake8 Outdated Show resolved Hide resolved
.flake8 Outdated Show resolved Hide resolved
.flake8 Outdated Show resolved Hide resolved
.github/workflows/integrate.yaml Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@ca-scribner
Copy link
Contributor Author

  • Unit tests work locally but fail in GH because I forgot to mock away a call to lightkube.Client() and it needs k8s config
  • integration tests fail I think because the charm is trying to create the service account multiple times. Will debug tomorrow.

Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @ca-scribner, few comments.

.github/workflows/integrate.yaml Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
charmcraft.yaml Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@jnsgruk
Copy link
Member

jnsgruk commented Nov 19, 2021

  • Unit tests work locally but fail in GH because I forgot to mock away a call to lightkube.Client() and it needs k8s config
  • integration tests fail I think because the charm is trying to create the service account multiple times. Will debug tomorrow.

You can stop the behaviour where lightkube is looking for a kubeconfig with something like:

@patch("lightkube.core.client.GenericSyncClient", Mock)

client fails when kube config cannot be found on system (such as in GH runner)
Install now attempts to create all resources, failing over to patching any that already exist (and logging this to debug-log).  This is not foolproof (it could overwrite existing resources)

Also changed:
* k8s resource names now slightly more unique, attempting to avoid conflicts
* changed CI name for `test_build_and_deploy` to `test_build_and_deploy_with_trust` to make it easier to run just that test during debugging
Seems to be colliding with the regular deploy test, maybe because the second test starts before the first is fully torn down?  Not sure, but both work in isolation locally.
* refactor _create and _render to minimize duplicated code
* misc minor changes
Copy link
Contributor

@DomFleischmann DomFleischmann left a comment

Choose a reason for hiding this comment

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

This is looking good, lets resolve the last comments from @DnPlas and @knkski and get this thing merged

src/charm.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
Includes:
* Removing properties for simplicity
* Using tenacity.retry instead of while loop for checking resources
* Using CheckFailed exception for raising errors in helpers
* Scheduled weekly integration CI
Restore assignment of mock object that was removed by mistake
Copy link
Contributor

@DomFleischmann DomFleischmann left a comment

Choose a reason for hiding this comment

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

LGTM

@DomFleischmann DomFleischmann merged commit c19d314 into main Nov 23, 2021
@DomFleischmann DomFleischmann deleted the kf1.4-release branch November 23, 2021 17:03
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.

6 participants