Prometheus integration via control socket #28
Conversation
| /.idea/ |
There was a problem hiding this comment.
I've left a few comments.
I also think this charm's tests/CI need to be brought up to date a bit. There's an ad-hoc script (run_tests) that sets up a virtual environment and runs flake8 and the unit tests, but it looks like that's not run in CI so it'd be easy to forget. We should use tox to manage venvs and run tests. See for example the prometheus-k8s-operator charm's tox.ini and pyproject.toml for an example.
The charm also has very few unit tests. Maybe it's tiny and doesn't need any more, but we should at least add a couple to exercise this new functionality.
There was a problem hiding this comment.
(Would be worth reading through https://juju.is/docs/sdk/styleguide if you haven't already.)
There was a problem hiding this comment.
I agree with you re: tox, but it might be out of scope for this PR.
There was a problem hiding this comment.
Fair enough. However, I'd suggest doing this soon as a follow-up PR. Tox is quite easy to add, and eliminates the need for manually messing with virtual environments and having an ad-hoc run_tests script. With my Charm Tech hat on, this charm wouldn't pass review for a real charm as it is ... which doesn't seem like a good thing for a charm used by Juju itself.
#16301 This PR adds a `controlsocket` worker, which will become the canonical way for the controller charm to communicate with the Juju controller. This worker serves an HTTP API over a Unix socket, which is exposed to the machine/container where the controller charm is running. The worker depends on state, and can interact directly with state when it receives a request. I've added "add metrics user" and "remove metrics user" handlers, which will be used to integrate `prometheus-k8s` with the controller charm. The API is fully compliant with JSON REST: ``` POST /metrics-users {"username":"juju-metrics-r0","password":"bar"} ``` ``` DELETE /metrics-users/{username} ``` ## Checklist - [x] Code style: imports ordered, good names, simple structure, etc - [x] Comments saying why design decisions were made - [x] Go unit tests, with comments saying what you're testing - [x] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing - [x] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages ## QA steps ### Manual Check the worker is starting and the socket is being created: ```shell make [k8s]-operator-update juju bootstrap [cloud] c juju switch controller juju ssh controller/0 stat /var/lib/juju/control.socket ``` Manually curl the socket: ```bash curl -X POST http://a/metrics-users \n -d '{"username":"juju-metrics-test","password":"bar"}' \n --unix-socket /var/lib/juju/control.socket # {"message":"created user \"juju-metrics-test\""} ``` ```bash curl -X DELETE http://a/metrics-users/juju-metrics-test \n --unix-socket /var/lib/juju/control.socket # {"message":"deleted user \"juju-metrics-test\""} ``` ### Integration Integration with Prometheus (using the modified controller charm from juju/juju-controller#28): ```shell juju bootstrap [k8s] c \n --controller-charm-path=barrettj12-cc \n --controller-charm-channel=latest/stable juju switch controller juju deploy prometheus-k8s p --trust juju relate p controller juju users # should include `juju-metrics-r0` ``` See also: #16388 ## Links **Jira card:** JUJU-4649 Rejected PRs: - #15319 - #16263 - #16285
#16388 This PR adds Bash tests verifying integration between `prometheus-k8s` and the controller charm in various scenarios. For these tests to pass, we need #16301 and juju/juju-controller#28 to land. ## Checklist - [ ] Code style: imports ordered, good names, simple structure, etc - [ ] Comments saying why design decisions were made - ~[ ] Go unit tests, with comments saying what you're testing~ - [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing - ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~ ## QA steps The tests can be run on both LXD and k8s. To run them: ```bash export CONTROLLER_CHARM_PATH_IAAS=[path/to/local/ctrl/charm] # if testing on LXD export CONTROLLER_CHARM_PATH_CAAS='barrettj12-cc' export CONTROLLER_CHARM_CHANNEL='latest/stable' cd tests ./main.sh -v -c [cloud] controllercharm ``` ## Links **Jira card:** JUJU-4720 Related PRs: - #16301 - juju/juju-controller#28
barrettj12
force-pushed
the
ctrlsocket
branch
from
413a0cb
to
5d883ce
Compare
|
I added a new test ( Maybe I should add some more tests for the controlsocket client, specifically testing error responses from the controlsocket API? |
barrettj12
force-pushed
the
ctrlsocket
branch
from
0549b2a
to
877051f
Compare
This PR replaces #20.
It builds upon work in juju/juju#16301 to add a "control socket" to Juju, which the controller charm can use to communicate with the Juju controller agent.
QA steps
See juju/juju#16301
Links
Jira card: JUJU-4662
This PR depends on the following Juju PR: juju/juju#16301