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

Convert to Ops #32

Merged
merged 19 commits into from Jul 6, 2023
Merged

Convert to Ops #32

merged 19 commits into from Jul 6, 2023

Conversation

stonepreston
Copy link
Contributor

@stonepreston stonepreston commented Jun 23, 2023

This PR converts metallb from podspec to Ops. Due to the speaker being a daemonset, it was not possible to use the sidecar pattern. Because of this, a simpler architecture was chosen and the charm simply handles applying an upstream manifest that is manipulated using the ops-lib-manifest library. This simplifies the charm quite a bit (1 metallb charm instead of separate charms for the speaker and controller)

The new charm maintains feature parity (L2 support). Metallb shifted away from a config file based approach to CRDs in v13. The charm still has an iprange config option, but instead of updating a config file it creates and manages an IPAddressPool resource. The CRD method of doing things is more flexible as users can rely on the default ip address pool created by the charm, but can create additional IP Address Pool resources in the cluster if needed.

The old charms did not handle RBAC resources for the user and required installing rbac resources manually. Initially I made whether or not to deploy RBAC resources configurable, but decided against it in the end as if you disabled RBAC for the charm in an RBAC enabled cluster things could break. Instead I opted to just install any necessary RBAC resources from the manifest, and if the cluster does not have RBAC enabled, then no harm is done and you will just have a handful of RBAC resources laying around unused.

To make switching over from the old charms (generally installed in a model named metallb-system) easier, there is a namespace config option that controls what namespace the resources get created in. It defaults to metallb-system, but if you already have that as an existing namespace from the old charms the user can supply a different ns to be created. To switch over from the old charms, you can just create a new model, deploy the new charm with your desired namespace specified in config, and the charm will create that namespace, and install all the metallb resources there. Once metallb is up in that new namespace, it will takeover managing existing load balancer svc IPs, and then the old metallb-system model can be removed.

Unit tests currently have 100% coverage, and the integration tests test that that the assigned IP to a microbot load balancer service can be contacted in both RBAC and non-RBAC enabled microk8s clusters.

@stonepreston stonepreston changed the title Conver to Ops Convert to Ops Jun 23, 2023
Copy link
Member

@addyess addyess left a comment

Choose a reason for hiding this comment

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

I've left suggestions for things i think are missing/wrong/need work.

Thanks for the good work @stonepreston

.gitignore Outdated Show resolved Hide resolved
LICENSE Show resolved Hide resolved
metadata.yaml Outdated Show resolved Hide resolved
charmcraft.yaml Outdated Show resolved Hide resolved
src/charm.py Outdated
else:
# surface any other errors besides not found
logger.error(e.status.message)
raise
Copy link
Member

Choose a reason for hiding this comment

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

it feels odd to intentionally raise on the update_status hook. this will likely stall the charm in error state forever unless retry hooks is on. I guess it raises eyebows though. Opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Kubernetes API could be temporarily unavailable for any number of reasons. Charms need to handle that gracefully.

It's unfortunate that automatically-retry-hooks=false is as widely used as it is. It basically disables fault tolerance and is harmful to the overall robustness of charms. But given that it is used, we just have to work under the assumption that hook errors are catastrophic.

With that in mind, this feels like a "log a traceback, set blocked or waiting status, retry later" situation.

summary: |
This charm deploys MetalLB in a Kubernetes model, which provides a software
defined load balancer.
docs: https://discourse.charmhub.io/t/metallb/6320
Copy link
Contributor

@Cynerva Cynerva Jul 5, 2023

Choose a reason for hiding this comment

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

I went ahead and pointed this at the docs page that the current metallb-controller and metallb-speaker charms use for now. But that page will need to be updated.

Of course, if I update that page now, it will no longer be correct for current stable charms.

@evilnick I really need a good way to stage doc changes for this, to go live at a later date with the CK 1.28 release. Any recommendations?

@Cynerva
Copy link
Contributor

Cynerva commented Jul 6, 2023

@addyess I think this is ready to go, can you review again following my recent updates?

Copy link
Member

@addyess addyess left a comment

Choose a reason for hiding this comment

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

🚀 🍰 LGTM 🍰 🚀

@addyess addyess merged commit ec09d06 into main Jul 6, 2023
8 checks passed
@addyess addyess deleted the ops-conversion branch July 6, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants