JU048: Add support for Pebble Notices #16428

benhoyt · 2023-10-13T04:04:48Z

This PR implements support for waiting for Pebble Notices and firing them as events to the charm. Pebble only has one notice type implemented right now, the custom notice, so that's all we care about for now. This gets sent to the charm as a <container>-pebble-custom-notice event. In this event context, the following environment variables are set:

JUJU_NOTICE_ID: the unique ID of the notice
JUJU_NOTICE_TYPE: the notice type (currently only custom)
JUJU_NOTICE_KEY: the notice key, for example example.com/foo

The change adds a new worker, pebbleNoticer, which is similar to the existing pebblePoller worker that handles <container>-pebble-ready. It uses the Pebble client function WaitNotices to wait for new notices to arrive, and when custom notices arrive, it sends them to the charm using the existing WorkloadEvents abstraction. There's a tomb for the worker, and one run goroutine per container.

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, with comments saying what you're testing
~~doc.go added or updated in changed packages~~

QA steps

# Pack and deploy the test charm
$ juju add-model t
$ cd testcharms/charms/pebble-notices
$ charmcraft pack
$ juju deploy ./juju-qa-pebble-notices_ubuntu-22.04-amd64.charm --resource redis-image=redis
$ juju status  # unit status should settle to "active" after pebble-ready

# Record a notice
$ juju ssh --container redis juju-qa-pebble-notices/0 /charm/bin/pebble notify foo.com/bar key=val
$ juju status  # will now say status "maintenance" with message "notice type=custom key=foo.com/bar"

# List notices:
$ juju ssh --container redis juju-qa-pebble-notices/0 /charm/bin/pebble notices
ID   User  Type    Key          First               Repeated            Occurrences
1    0     custom  foo.com/bar  today at 03:16 UTC  today at 03:16 UTC  1

Documentation changes

There's an item on the Charm Tech roadmap to add these -- being tracked separately.

Links

Pebble Notices spec JU048

go.mod

worker/uniter/hook/hook.go

benhoyt · 2023-10-20T05:04:02Z

worker/uniter/pebblewatcher.go

@@ -0,0 +1,266 @@
+// Copyright 2021 Canonical Ltd.


Note that the first half of this file and the runPoller loop is existing code, but git didn't pick up the file rename as there were too many changes for the new notices-watcher stuff.

wallyworld

I think it needs a tweak.

go.mod

worker/uniter/runner/context/context.go

worker/uniter/pebblewatcher.go

benhoyt · 2023-11-21T01:57:02Z

/build

wallyworld

Soon we will not deploy charms without a manifest.yaml, so we should start adding that to any new test charms.
Also, as per in line comment, we need to upload test charm to store first and make test deploy from there, not a hard coded local path.

tests/suites/sidecar/sidecar.sh

worker/uniter/runner/context/context.go

worker/uniter/pebblepoller_test.go

wallyworld · 2023-11-21T04:17:06Z

worker/uniter/pebblepoller_test.go

+
+func noticeMatches(notice *pebbleclient.Notice, opts *pebbleclient.NoticesOptions) bool {
+	if opts == nil || opts.Types != nil || opts.Keys != nil {
+		panic("not supported")


Can't we pass in a C and assert on that instead of panicing?

Not easily. This is called by the implementation of WaitNotices which is fulfilling an interface and so we can't adjust the parameters being passed. We could pass the C into the fakePebbleClient struct, but that feels weird. The test fails with a panic in the case this happens, which it shouldn't -- it's really just an assertion.

wallyworld · 2023-11-21T04:23:01Z

worker/uniter/pebblenotices.go

+				n.logger.Debugf("container %q: socket %q not found, waiting %s",
+					containerName, socketNotFound.Path, errorDelay)
+			} else {
+				n.logger.Errorf("container %q: WaitNotices error, waiting %s: %v",
+					containerName, errorDelay, err)


Do we need to recreate the pebble client if these occur?

No, I don't think so. Underlying the pebble Client is a regular net/http client and it will just retry the next time around.

hpidcock · 2023-12-13T06:12:34Z

@benhoyt this should probably target juju 3.4

hpidcock

LGTM except a few flaky tests.

worker/uniter/pebblenotices_test.go

worker/uniter/pebblenotices.go

worker/uniter/pebblenotices_test.go

them as events to the charm. Pebble only has one notice type implemented right now, the `custom` notice, so that's all we care about for now. This gets sent to the charm as a `<container>-pebble-custom-notice` event. In this event context, the following environment variables are set: * `JUJU_NOTICE_ID`: the unique ID of the notice * `JUJU_NOTICE_TYPE`: the notice type (currently only `custom`) * `JUJU_NOTICE_KEY`: the notice key, for example `example.com/foo` The change adds a new worker, `pebbleNoticer`, which is similar to the existing `pebblePoller` worker that handles `<container>-pebble-ready`. It uses the Pebble client function `WaitNotices` to wait for new notices to arrive, and when custom notices arrive, it sends them to the charm using the existing `WorkloadEvents` abstraction. There's a tomb for the worker, and one `run` goroutine per container.

benhoyt · 2023-12-15T02:37:46Z

/merge

#16724 Merge 3.4 #16637 [from hpidcock/improve-indexes](07162f1) #16632 [from SimonRichardson/fix-spaces-matcher](e88b7d2) #16634 [from SimonRichardson/check-client-imports](55cabf0a82322f0b5807e5bfeb0f347cc9e9ecea)https://github.com/juju/juju/pull/16634 [from SimonRichardson/check-client-imports](55cabf0) #16651 [from nvinuesa/juju-5092](c1e063c) #16652 [from hpidcock/fix-sidecar-teardown-app-dead](070f3e8) #16653 [from ycliuhw/simplify-vault4CI](e185779) #16604 [from ycliuhw/secret-grant-info](ffa2ac9) #16513 [from hmlanigan/implement-todo](0302c4a) #16664 [from jack-w-shaw/JUJU-5053_print_action_fai…](1b140b8) #16667 [from jack-w-shaw/JUJU-5053_update_action_he…](0cd207d) #16428 [from benhoyt/pebble-notices](6d7ceae) #16706 [from wallyworld/update-names-v5](75c2f63) #16719 [from wallyworld/gomod-updates](246a6e9) #16704 [from wallyworld/appoffertag-uuid](627357c) #16701 [from jack-w-shaw/JUJU-4960_break_up_charm_api](b735335) Conflicts were mainly imports, plus changes where state has been removed in 4.0 ``` # Conflicts: # agent/addons/addons.go # agent/addons/engine_mock_test.go # agent/agentbootstrap/bootstrap_test.go # agent/engine/metrics.go # api/agent/agent/machine_test.go # api/agent/agent/unit_test.go # api/agent/caasoperator/client.go # api/agent/caasoperator/client_test.go # api/agent/deployer/deployer.go # api/agent/deployer/unit.go # api/agent/diskmanager/diskmanager.go # api/agent/hostkeyreporter/facade.go # api/agent/instancemutater/mocks/caller_mock.go # api/agent/machiner/machiner_test.go # api/agent/provisioner/mocks/caller_mock.go # api/agent/provisioner/provisioner_test.go # api/agent/secretsdrain/mocks/facade_mock.go # api/agent/uniter/podspec_test.go # api/base/mocks/caller_mock.go # api/base/mocks/clientfacade_mock.go # api/client/charms/client.go # api/client/charms/mocks/objectstore_mock.go # api/client/modelupgrader/mocks/apibase_mock.go # api/client/modelupgrader/mocks/context_mock.go # api/common/secretbackends/mocks/facade_mock.go # api/common/secretsdrain/mocks/facade_mock.go # api/controller/caasoperatorprovisioner/client.go # api/controller/caasoperatorprovisioner/client_test.go # api/controller/caasunitprovisioner/client_test.go # api/controller/controller/legacy_test.go # api/controller/firewaller/machine_test.go # api/controller/usersecretsdrain/mocks/facade_mock.go # api/export_test.go # api/state_test.go # api/watcher/watcher_test.go # apiserver/admin.go # apiserver/apiserver_test.go # apiserver/common/action.go # apiserver/common/credentialcommon/mocks/credentialcommon_mock.go # apiserver/common/credentialcommon/modelcredential.go # apiserver/common/credentialcommon/modelcredential_test.go # apiserver/common/crossmodel/crossmodel.go # apiserver/common/crossmodel/interface.go # apiserver/common/crossmodel/state.go # apiserver/common/instanceidgetter.go # apiserver/common/mocks/controllerconfig_mock.go # apiserver/common/mocks/storage.go # apiserver/common/mocks/tools_mock.go # apiserver/common/modelmachineswatcher_test.go # apiserver/common/secrets/drain.go # apiserver/common/secrets/drain_test.go # apiserver/common/secrets/mocks/commonsecrets_mock.go # apiserver/common/secrets/mocks/provider_mock.go # apiserver/common/secrets/state.go # apiserver/export_test.go # apiserver/facade/interface.go # apiserver/facade/mocks/facade_mock.go # apiserver/facades/agent/caasapplication/application_test.go # apiserver/facades/agent/caasoperator/mock_test.go # apiserver/facades/agent/caasoperator/operator.go # apiserver/facades/agent/caasoperator/operator_test.go # apiserver/facades/agent/caasoperator/state.go # apiserver/facades/agent/credentialvalidator/backend.go # apiserver/facades/agent/credentialvalidator/backend_test.go # apiserver/facades/agent/credentialvalidator/state.go # apiserver/facades/agent/diskmanager/diskmanager.go # apiserver/facades/agent/hostkeyreporter/facade.go # apiserver/facades/agent/hostkeyreporter/facade_test.go # apiserver/facades/agent/instancemutater/instancemutater_test.go # apiserver/facades/agent/instancemutater/lxdprofilewatcher.go # apiserver/facades/agent/instancemutater/mocks/instancemutater_mock.go # apiserver/facades/agent/keyupdater/authorisedkeys_test.go # apiserver/facades/agent/logger/logger_test.go # apiserver/facades/agent/machineactions/machineactions.go ```

This is basically just adding the notice type in the right places, with the effect that we'll now respond to Pebble change-update notices by sending a pebble-change-updated event to the charm. This PR also updates "sidecar" integration tests and the pebble-notices test charm with a change-update notice test. For reference, Pebble Notices support (for the "custom" notice type) was originally introduced in PR juju#16428.

#17118 This wires up the `change-update` notice type in the right places, with the effect that we'll now respond to Pebble change-update notices by sending a `pebble-change-updated` event to the charm. For reference, Pebble Notices support (for the "custom" notice type) was originally introduced in PR #16428. ## 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 - [ ] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~ ## QA steps Manual QA steps below (similar to the integration test): ``` # Pack and deploy the test charm $ juju add-model t $ cd testcharms/charms/pebble-notices $ charmcraft pack $ juju deploy ./juju-qa-pebble-notices_ubuntu-22.04-amd64.charm --resource redis-image=redis $ juju status # unit status should settle to "active" after pebble-ready # Perform an exec (or any operation that records a change) $ juju ssh --container redis juju-qa-pebble-notices/0 /charm/bin/pebble exec -- echo foo $ juju status # will now say status "maintenance" with message "notice type=change-update kind=exec status=Done" # List notices: $ juju ssh --container redis juju-qa-pebble-notices/0 /charm/bin/pebble notices ID User Type Key First Repeated Occurrences 1 public change-update 1 today at 02:15 UTC today at 02:15 UTC 3 # Get notice details for the above notice ID $ juju ssh --container redis juju-qa-pebble-notices/0 /charm/bin/pebble notice 1 id: "1" user-id: null type: change-update key: "1" first-occurred: 2024-04-04T02:15:46.249636939Z last-occurred: 2024-04-04T02:15:46.297160558Z last-repeated: 2024-04-04T02:15:46.297160558Z occurrences: 3 last-data: kind: exec expire-after: 168h0m0s # Get tasks for corresponding change (the notice key "1" above) $ juju ssh --container redis juju-qa-pebble-notices/0 /charm/bin/pebble tasks 1 Status Spawn Ready Summary Done today at 02:15 UTC today at 02:15 UTC Execute command "echo" ``` ## Documentation changes Ben to add Juju SDK documentation for this (CHARMTECH-35). ## Links Original spec: [JU048](https://docs.google.com/document/d/16PJ85fefalQd7JbWSxkRWn0Ye-Hs8S1yE99eW7pk8fA/edit#).

hpidcock added the 3.1 label Oct 17, 2023

benhoyt changed the title ~~WIP: Support for Pebble Notices~~ JU048: Add support for Pebble Notices Oct 18, 2023

benhoyt force-pushed the pebble-notices branch from b21d5fc to a290649 Compare October 18, 2023 01:37

benhoyt marked this pull request as ready for review October 18, 2023 01:38

benhoyt force-pushed the pebble-notices branch 3 times, most recently from b3554ef to aa80924 Compare October 19, 2023 00:42

benhoyt commented Oct 19, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

benhoyt force-pushed the pebble-notices branch from aa80924 to 480a47f Compare October 19, 2023 01:50

wallyworld reviewed Oct 19, 2023

View reviewed changes

worker/uniter/hook/hook.go Outdated Show resolved Hide resolved

benhoyt force-pushed the pebble-notices branch 2 times, most recently from 6eeecb9 to 341dd29 Compare October 20, 2023 05:00

benhoyt commented Oct 20, 2023

View reviewed changes

benhoyt force-pushed the pebble-notices branch from 341dd29 to 7f047cb Compare October 20, 2023 05:05

wallyworld requested changes Oct 23, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

worker/uniter/runner/context/context.go Outdated Show resolved Hide resolved

worker/uniter/pebblewatcher.go Outdated Show resolved Hide resolved

benhoyt force-pushed the pebble-notices branch from 7f047cb to 2ca29db Compare November 20, 2023 03:36

wallyworld approved these changes Nov 21, 2023

View reviewed changes

benhoyt force-pushed the pebble-notices branch from 91f4632 to 0f05cd2 Compare November 21, 2023 21:42

benhoyt added the do not merge Even if a PR has been approved, do not merge the PR! label Nov 21, 2023

hpidcock added the has merge conflicts label Nov 28, 2023

benhoyt mentioned this pull request Dec 13, 2023

feat: support for Pebble Notices canonical/operator#1086

Merged

benhoyt changed the base branch from 3.1 to 3.4 December 13, 2023 19:10

benhoyt removed the has merge conflicts label Dec 14, 2023

benhoyt force-pushed the pebble-notices branch from 0ca6c33 to a57525e Compare December 14, 2023 23:58

benhoyt removed the do not merge Even if a PR has been approved, do not merge the PR! label Dec 14, 2023

hpidcock added 3.4 and removed 3.1 labels Dec 15, 2023

hpidcock approved these changes Dec 15, 2023

View reviewed changes

worker/uniter/pebblenotices_test.go Outdated Show resolved Hide resolved

worker/uniter/pebblenotices.go Outdated Show resolved Hide resolved

worker/uniter/pebblenotices_test.go Show resolved Hide resolved

worker/uniter/pebblenotices_test.go Show resolved Hide resolved

benhoyt force-pushed the pebble-notices branch from a57525e to 8f19e4f Compare December 15, 2023 01:37

benhoyt force-pushed the pebble-notices branch from 8f19e4f to d56d18f Compare December 15, 2023 02:37

jujubot merged commit 6d7ceae into juju:3.4 Dec 15, 2023
16 of 19 checks passed

benhoyt deleted the pebble-notices branch December 15, 2023 03:01

wallyworld mentioned this pull request Jan 3, 2024

Merge 3.4 #16724

Merged

benhoyt mentioned this pull request Mar 28, 2024

Add support for Pebble change-update notice #17118

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JU048: Add support for Pebble Notices #16428

JU048: Add support for Pebble Notices #16428

benhoyt commented Oct 13, 2023 •

edited

Loading

benhoyt Oct 20, 2023

wallyworld left a comment

benhoyt commented Nov 21, 2023

wallyworld left a comment

wallyworld Nov 21, 2023

benhoyt Nov 21, 2023

wallyworld Nov 21, 2023

benhoyt Nov 21, 2023

hpidcock commented Dec 13, 2023

hpidcock left a comment

benhoyt commented Dec 15, 2023

JU048: Add support for Pebble Notices #16428

JU048: Add support for Pebble Notices #16428

Conversation

benhoyt commented Oct 13, 2023 • edited Loading

Checklist

QA steps

Documentation changes

Links

benhoyt Oct 20, 2023

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

benhoyt commented Nov 21, 2023

wallyworld left a comment

Choose a reason for hiding this comment

wallyworld Nov 21, 2023

Choose a reason for hiding this comment

benhoyt Nov 21, 2023

Choose a reason for hiding this comment

wallyworld Nov 21, 2023

Choose a reason for hiding this comment

benhoyt Nov 21, 2023

Choose a reason for hiding this comment

hpidcock commented Dec 13, 2023

hpidcock left a comment

Choose a reason for hiding this comment

benhoyt commented Dec 15, 2023

benhoyt commented Oct 13, 2023 •

edited

Loading