Skip to content

Commit

Permalink
Merge pull request juju#18497 from Aflynn50/wire-up-uniter-applicatio…
Browse files Browse the repository at this point in the history
…n-watcher

juju#18497

This PR includes juju#18448.

Before this change, the uniter API embedded the AgentEntityWatcher. This
provided the Watch method on the api and would look up the entity in
state and return a notify watcher.

The attach-resource command increments the CharmModifiedField in state
and therefor triggers the watcher.

With resources being moved to DQLite, and the charm_modified_version
integer now being in the application table in domain, this watcher needs
to be watching this table instead.

Not all application functionality has yet been moved over to DQLite.
This could pose an issue, as the uniter checks the CharmModifiedVersion
and the CharmURL when the watcher is triggered. The charm URL should
work as expected as this is currently written in DQLite.

The CharmModifiedVersion is incremented in state in two places. I found
this via the incCharmModifiedVersionOps method and couldn't find any
other places it was set. They are:

- When a resource is set - this will be take care of in the upcoming
 resources work.
- When a charm is changed with SetCharm (there are two callsites in this
 method to incCharmModifiedVersionOps) - The only call remaining for
 this method is doulbe written to DQLite.

Watching the application table in DQLite instead of these should be safe
for the reasons above.

The change made is to remove the AgentEntityWatcher from the UniterAPI
and implement a Watch and WatchApplication on the UniterAPI directly.
The Watch method is kept only so that the uniter API can work with 3.x
clients.

The ApplicationWatcher uses the watcher from the application service,
and the Watch method checks if the tag is an application, if it is,
the Watcher used is from the application service. If it is a unit
watcher, it still watches mongo state.

<!-- 
The PR title should match: <type>(optional <scope>): <description>.

Please also ensure all commits in this PR comply with our conventional commits specification:
https://github.com/juju/juju/blob/main/doc/conventional-commits.md
-->

<!-- Why this change is needed and what it does. -->

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [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
- [ ] [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
Using [`juju sql`](https://discourse.charmhub.io/t/juju-4-0-shortcut-to-get-the-dqlite-repl-from-the-command-line/15394)
```
juju bootstrap lxd test-watcher
juju add-model m
juju model-config logging-config="<root>=DEBUG"
juju deploy juju-qa-test

# Get the model UUID
juju show-model
# Go to DQLite repl on the model database
juju sql 0 <model-uuid>
# Verify that the application is there and get its UUID
dqlite> SELECT prdesc FROM application
dqlite> UPDATE application SET charm_modified_version=1 WHERE uuid="<application-uuid>"
# ssh into the unit
lxc exec <unit-machine> bash
cd /var/log/juju
vim unit-juju-qa-test-0.log
```
You should find the follow lines from just after when you did the update:
```
2024-12-05 11:24:38 DEBUG juju.worker.uniter.remotestate loggo.go:44 got application change for juju-qa-test/0
...
2024-12-05 11:24:38 DEBUG juju.worker.uniter resolver.go:333 upgrade from CharmModifiedVersion 0 to 1
```


I tried to trigger the only remote state debug messages with ` juju model-config -m controller logging-config="juju.worker.uniter.remotestate=DEBUG"`, just doing `<root>=DEBUG` gives you quite a lot of output if anyone knows why that isn't working for me let me know! 

### Migration test
Test that a 3.6 agent can communicate with a 4.0 controller.

This QA caught that the watch unit method also used the generic watch, and we should also add a WatchUnit method to the v20 facade and use that.
```
snap refresh juju --channel=3.6/stable
/snap/bin/juju bootstrap lxd source
/snap/bin/juju add-model mig
/snap/bin/juju deploy juju-qa-test
juju bootstrap lxd target
juju migrate source:mig target
juju status
# Check debug logs for errors 
juju debug-log
juju debug-log -m controller
```
Adding a unit does not work because the charm cannot be fetched from model storage, this is an unrelated breakage.
### K8s resource test
```
sudo make microk8s-operator-update
juju bootstrap microk8s test-k8s
juju add-model m
juju deploy hello-kubecon
watch juju status
```
Check that the app is up and running with no issues.
## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->


**Jira card:** [JUJU-7106](https://warthogs.atlassian.net/browse/JUJU-7106)



[JUJU-7106]: https://warthogs.atlassian.net/browse/JUJU-7106?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
  • Loading branch information
jujubot authored Dec 9, 2024
2 parents 65a4a2d + 1736de3 commit 0b190a9
Show file tree
Hide file tree
Showing 14 changed files with 384 additions and 94 deletions.
16 changes: 14 additions & 2 deletions api/agent/uniter/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import (
"github.com/juju/errors"
"github.com/juju/names/v5"

"github.com/juju/juju/api/common"
apiwatcher "github.com/juju/juju/api/watcher"
apiservererrors "github.com/juju/juju/apiserver/errors"
"github.com/juju/juju/core/life"
"github.com/juju/juju/core/status"
"github.com/juju/juju/core/watcher"
Expand Down Expand Up @@ -44,7 +45,18 @@ func (s *Application) String() string {

// Watch returns a watcher for observing changes to an application.
func (s *Application) Watch(ctx context.Context) (watcher.NotifyWatcher, error) {
return common.Watch(ctx, s.client.facade, "Watch", s.tag)
arg := params.Entity{Tag: s.tag.String()}
var result params.NotifyWatchResult

err := s.client.facade.FacadeCall(ctx, "WatchApplication", arg, &result)
if err != nil {
return nil, errors.Trace(apiservererrors.RestoreError(err))
}

if result.Error != nil {
return nil, result.Error
}
return apiwatcher.NewNotifyWatcher(s.client.facade.RawAPICaller(), result), nil
}

// Life returns the application's current life state.
Expand Down
12 changes: 5 additions & 7 deletions api/agent/uniter/application_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,11 @@ func (s *applicationSuite) apiCallerFunc(c *gc.C) basetesting.APICallerFunc {
Life: s.life,
}},
}
case "Watch":
c.Assert(arg, jc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "application-mysql"}}})
c.Assert(result, gc.FitsTypeOf, &params.NotifyWatchResults{})
*(result.(*params.NotifyWatchResults)) = params.NotifyWatchResults{
Results: []params.NotifyWatchResult{{
NotifyWatcherId: "1",
}},
case "WatchApplication":
c.Assert(arg, jc.DeepEquals, params.Entity{Tag: "application-mysql"})
c.Assert(result, gc.FitsTypeOf, &params.NotifyWatchResult{})
*(result.(*params.NotifyWatchResult)) = params.NotifyWatchResult{
NotifyWatcherId: "1",
}
case "CharmURL":
c.Assert(arg, jc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "application-mysql"}}})
Expand Down
17 changes: 14 additions & 3 deletions api/agent/uniter/unit.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,20 @@ func (u *Unit) EnsureDead(ctx context.Context) error {
return result.OneError()
}

// Watch returns a watcher for observing changes to the unit.
func (u *Unit) Watch(ctx context.Context) (watcher.NotifyWatcher, error) {
return common.Watch(ctx, u.client.facade, "Watch", u.tag)
// Watch returns a watcher for observing changes to an application.
func (s *Unit) Watch(ctx context.Context) (watcher.NotifyWatcher, error) {
arg := params.Entity{Tag: s.tag.String()}
var result params.NotifyWatchResult

err := s.client.facade.FacadeCall(ctx, "WatchUnit", arg, &result)
if err != nil {
return nil, errors.Trace(apiservererrors.RestoreError(err))
}

if result.Error != nil {
return nil, result.Error
}
return apiwatcher.NewNotifyWatcher(s.client.facade.RawAPICaller(), result), nil
}

// WatchRelations returns a StringsWatcher that notifies of changes to
Expand Down
12 changes: 5 additions & 7 deletions api/agent/uniter/unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,11 @@ func (s *unitSuite) TestWatch(c *gc.C) {
return nil
}
c.Assert(objType, gc.Equals, "Uniter")
c.Assert(request, gc.Equals, "Watch")
c.Assert(arg, gc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "unit-mysql-0"}}})
c.Assert(result, gc.FitsTypeOf, &params.NotifyWatchResults{})
*(result.(*params.NotifyWatchResults)) = params.NotifyWatchResults{
Results: []params.NotifyWatchResult{{
NotifyWatcherId: "1",
}},
c.Assert(request, gc.Equals, "WatchUnit")
c.Assert(arg, gc.DeepEquals, params.Entity{Tag: "unit-mysql-0"})
c.Assert(result, gc.FitsTypeOf, &params.NotifyWatchResult{})
*(result.(*params.NotifyWatchResult)) = params.NotifyWatchResult{
NotifyWatcherId: "1",
}
return nil
})
Expand Down
2 changes: 1 addition & 1 deletion api/facadeversions.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ var facadeVersions = facades.FacadeVersions{
"Subnets": {5},
"Undertaker": {1},
"UnitAssigner": {1},
"Uniter": {19},
"Uniter": {19, 20},
"Upgrader": {1},
"UpgradeSteps": {3},
"UserManager": {3},
Expand Down
33 changes: 22 additions & 11 deletions apiserver/facades/agent-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -10747,7 +10747,7 @@
{
"Name": "Uniter",
"Description": "",
"Version": 19,
"Version": 20,
"AvailableTo": [
"controller-machine-agent",
"machine-agent",
Expand Down Expand Up @@ -11448,33 +11448,33 @@
}
}
},
"Watch": {
"WatchAPIHostPorts": {
"type": "object",
"properties": {
"Params": {
"$ref": "#/definitions/Entities"
},
"Result": {
"$ref": "#/definitions/NotifyWatchResults"
"$ref": "#/definitions/NotifyWatchResult"
}
}
},
"WatchAPIHostPorts": {
"WatchActionNotifications": {
"type": "object",
"properties": {
"Params": {
"$ref": "#/definitions/Entities"
},
"Result": {
"$ref": "#/definitions/NotifyWatchResult"
"$ref": "#/definitions/StringsWatchResults"
}
}
},
"WatchActionNotifications": {
"WatchApplication": {
"type": "object",
"properties": {
"Params": {
"$ref": "#/definitions/Entities"
"$ref": "#/definitions/Entity"
},
"Result": {
"$ref": "#/definitions/StringsWatchResults"
"$ref": "#/definitions/NotifyWatchResult"
}
}
},
Expand Down Expand Up @@ -11552,6 +11552,17 @@
}
}
},
"WatchUnit": {
"type": "object",
"properties": {
"Params": {
"$ref": "#/definitions/Entity"
},
"Result": {
"$ref": "#/definitions/NotifyWatchResult"
}
}
},
"WatchUnitAddressesHash": {
"type": "object",
"properties": {
Expand Down
1 change: 1 addition & 0 deletions apiserver/facades/agent/uniter/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ var (
WatchStorageAttachment = watchStorageAttachment

NewUniterAPI = newUniterAPI
NewUniterAPIv19 = newUniterAPIv19
NewUniterAPIWithServices = newUniterAPIWithServices
)

Expand Down
10 changes: 10 additions & 0 deletions apiserver/facades/agent/uniter/package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,16 @@ func (s *uniterSuiteBase) newUniterAPI(c *gc.C, st *state.State, auth facade.Aut
return uniterAPI
}

func (s *uniterSuiteBase) newUniterAPIv19(c *gc.C, st *state.State, auth facade.Authorizer) *uniter.UniterAPIv19 {
facadeContext := s.facadeContext(c)
facadeContext.State_ = st
facadeContext.Auth_ = auth
facadeContext.LeadershipRevoker_ = s.leadershipRevoker
uniterAPI, err := uniter.NewUniterAPIv19(context.Background(), facadeContext)
c.Assert(err, jc.ErrorIsNil)
return uniterAPI
}

func (s *uniterSuiteBase) addRelation(c *gc.C, first, second string) *state.Relation {
st := s.ControllerModel(c).State()
eps, err := st.InferEndpoints(first, second)
Expand Down
13 changes: 12 additions & 1 deletion apiserver/facades/agent/uniter/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,21 @@ import (
// Register is called to expose a package of facades onto a given registry.
func Register(registry facade.FacadeRegistry) {
registry.MustRegister("Uniter", 19, func(stdCtx context.Context, ctx facade.ModelContext) (facade.Facade, error) {
return newUniterAPIv19(stdCtx, ctx)
}, reflect.TypeOf((*UniterAPIv19)(nil)))
registry.MustRegister("Uniter", 20, func(stdCtx context.Context, ctx facade.ModelContext) (facade.Facade, error) {
return newUniterAPI(stdCtx, ctx)
}, reflect.TypeOf((*UniterAPI)(nil)))
}

func newUniterAPIv19(stdCtx context.Context, ctx facade.ModelContext) (*UniterAPIv19, error) {
api, err := newUniterAPI(stdCtx, ctx)
if err != nil {
return nil, err
}
return &UniterAPIv19{UniterAPI: api}, nil
}

// newUniterAPI creates a new instance of the core Uniter API.
func newUniterAPI(stdCtx context.Context, ctx facade.ModelContext) (*UniterAPI, error) {
domainServices := ctx.DomainServices()
Expand Down Expand Up @@ -131,7 +142,6 @@ func newUniterAPIWithServices(
}
logger := context.Logger().Child("uniter")
return &UniterAPI{
AgentEntityWatcher: common.NewAgentEntityWatcher(st, watcherRegistry, accessUnitOrApplication),
APIAddresser: common.NewAPIAddresser(systemState, resources),
ModelConfigWatcher: common.NewModelConfigWatcher(modelConfigService, context.WatcherRegistry()),
RebootRequester: common.NewRebootRequester(machineService, accessMachine),
Expand Down Expand Up @@ -169,5 +179,6 @@ func newUniterAPIWithServices(
StorageAPI: storageAPI,
logger: logger,
store: context.ObjectStore(),
watcherRegistry: watcherRegistry,
}, nil
}
20 changes: 20 additions & 0 deletions apiserver/facades/agent/uniter/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/juju/juju/cloud"
"github.com/juju/juju/controller"
coreapplication "github.com/juju/juju/core/application"
"github.com/juju/juju/core/credential"
"github.com/juju/juju/core/leadership"
"github.com/juju/juju/core/life"
Expand Down Expand Up @@ -73,6 +74,25 @@ type ApplicationService interface {

// DestroyUnit prepares a unit for removal from the model.
DestroyUnit(ctx context.Context, unitName coreunit.Name) error

// WatchApplication returns a NotifyWatcher for changes to the application.
WatchApplication(ctx context.Context, name string) (watcher.NotifyWatcher, error)

// GetApplicationIDByUnitName returns the application ID for the named unit.
//
// Returns [github.com/juju/juju/domain/application.UnitNotFound] if the
// unit is not found.
GetApplicationIDByUnitName(ctx context.Context, unitName coreunit.Name) (coreapplication.ID, error)

// GetApplicationIDByName returns an application ID by application name.
//
// Returns [github.com/juju/juju/domain/application.ApplicationNotFound] if
// the application is not found.
GetApplicationIDByName(ctx context.Context, name string) (coreapplication.ID, error)

// GetCharmModifiedVersion looks up the charm modified version of the given
// application.
GetCharmModifiedVersion(ctx context.Context, id coreapplication.ID) (int, error)
}

// UnitStateService describes the ability to retrieve and persist
Expand Down
Loading

0 comments on commit 0b190a9

Please sign in to comment.