feat: atomic commit hook changes for relation settings by hmlanigan · Pull Request #22090 · juju/juju

hmlanigan · 2026-03-25T21:06:25Z

Move updating relation setting changes in CommitHookChanges from individual transactions to the single transaction in the unitstate domain call. Functionality of the relation domain's SetRelationApplicationAndUnitSettings has been moved/copied to the unitstate domain.

It may be possible in the future to improve the performance of this functionality as it updates each relation's settings individually.

getManagementCaveat ensures that CommitHookChanges is called with EnsureLeadership provided that application settings need to be changed. It is verified via unit test.

There is no outward change to functionality.

Clean up to rename state structures to be reusable with SQL queries.
Move to using the Unit UUID when calling the state version of CommitHookChanges.

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

$ juju add-model test && juju deploy juju-qa-dummy-source source && juju deploy juju-qa-dummy-sink sink && juju integrate sink source

# wait for the model to settle
$ juju config  source token="goat"

$ juju status
Model  Controller  Cloud/Region         Version  Timestamp
test   thursday    localhost/localhost  4.0.4.1  20:00:40Z

App     Version  Status  Scale  Charm                 Channel        Rev  Exposed  Message
sink             active      1  juju-qa-dummy-sink    latest/stable    7  no       Token is goat
source           active      1  juju-qa-dummy-source  latest/stable    6  no       Token is goat

Unit       Workload  Agent  Machine  Public address  Ports  Message
sink/1*    active    idle   1        10.21.51.151           Token is goat
source/0*  active    idle   0        10.21.51.52            Token is goat

Machine  State    Address       Inst id        Base          AZ              Message
0        started  10.21.51.52   juju-f9d572-0  ubuntu@20.04  ubuntu-nuc-two  Running
1        started  10.21.51.151  juju-f9d572-1  ubuntu@20.04  ubuntu-nuc-two  Running

Links

Jira card: JUJU-9031

gfouillet

There is a lot of duplicated code from relation state.

I wonder if we should consider the option to import relation state in unit state.

Otherwise, QA ok, but few issues spotted.

jack-w-shaw

Only one small thing

hmlanigan · 2026-03-27T14:29:23Z

@gfouillet

There is a lot of duplicated code from relation state.
I wonder if we should consider the option to import relation state in unit state.

@manadart's preference is to duplicate code rather than have domains sharing state. There are limited exceptions, though this wouldn't be one. Secrets might be though.

In this case, a good bit of the code was moved from the relation domain to unitstate domain.

nvinuesa · 2026-03-27T14:55:38Z

+	ctx context.Context, in []unitstate.RelationSettings,
+) ([]internal.RelationSettings, error) {
+	return transform.SliceOrErr(in, func(in unitstate.RelationSettings) (internal.RelationSettings, error) {
+		relationUUID, err := s.getRelationUUIDByKey(ctx, in.RelationKey)


If we move it down to the state, then this can be done in the same txn, also avoiding more db hits.

Reads are less of a hit than an even longer txn.

Simon also wants the relation settings calculations moved to the service.

SimonRichardson

We should try our best to do as much of the reads up front. If for any reason this transaction turns into a write, and then fails after the write, then we've just wasted a lot of time blocking other transactions potentially making progress. Essentially this call can tank the throughput of juju.

SimonRichardson · 2026-03-30T08:43:56Z

+	// Determine the keys to set and unset.
+	var set []relationUnitSetting
+	var unset keys
+	for k, v := range settings {
+		if v == "" {
+			unset = append(unset, k)
+		} else {
+			set = append(set, relationUnitSetting{
+				UUID:  relUnitUUID,
+				Key:   k,
+				Value: v,
+			})
+		}
+	}


This should be in the service.

I agree this is not performant, however it is how it works today. It will take iterations to fully resolve all of this code. I'll add a todo and try to get some of it updated here an in the relation domain.

SimonRichardson · 2026-03-30T08:56:43Z

+	// Determine the keys to set and unset.
+	var set []relationApplicationSetting
+	var unset keys
+	for k, v := range settings {
+		if v == "" {
+			unset = append(unset, k)
+		} else {
+			set = append(set, relationApplicationSetting{
+				UUID:  endpointUUID,
+				Key:   k,
+				Value: v,
+			})
+		}
+	}


In the service.

Added a todo for here and the relation domain.

SimonRichardson · 2026-03-30T09:01:58Z

+	err = tx.Query(ctx, stmt, getUnitRelAndApp{UnitUUID: unitUUID, RelationUUID: relationUUID}).Get(&row)
+	if errors.Is(err, sqlair.ErrNoRows) {
+		return "", "", errors.Errorf(
+			"relation unit/application not found for unit %q and relation %q",


I'm not a fan of not knowing which you can't find.

manadart

Minor things, the most important of which is the error translation for non-leader failures.

I will think about what to do about the duplications we've introduced, but we will live with them for now.

manadart · 2026-03-30T09:32:47Z

 }

-func (s *LeadershipService) getManagementCaveat(arg unitstate.CommitHookChangesArg) (func(context.Context, func(context.Context) error) error, error) {
+func (s *LeadershipService) getManagementCaveat(


Capture/annotate these errors.

SimonRichardson

I'm going to block on the performance issue.

As a follow up, there will be a PR that ensures the integrity of the entities before any insertions and updates.

Will be used for commit hook functionality.

Use the relation keys in the RelationSettings to get relation UUIDs for CommitHookChanges in state. This validates the data and simplies the state methods.

setRelationApplicationAndUnitSettings is copied from the relation domain state and made a private method which can be called within a transaction. It has been adapted to get the relation unit uuid from known data rather than expecting one.

Move updates to relations settings during CommitHookChanges to the atomic version.

SetRelationApplicationAndUnitSettings has moved to the unitstate domain state package for atomic commit hook changes.

hmlanigan · 2026-03-31T18:46:52Z

/merge

hmlanigan · 2026-03-31T19:15:19Z

/merge

@SimonRichardson

#22150 Resolve comment from #22090 Just in case UUIDs retrieve in the unitstate service no longer exists, check that all still exist in state. All UUIDs added to CommitHookChangesArg need to be checked as well. Drive-by: use UnitNotFound from the application errors package rather than a local version in unit state. No longer a need for an errors package. ## 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~ - [ ] [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 Deploy an application and relate to another. There should be no change in behavior. ## Links **Jira card:** [JUJU-9540](https://warthogs.atlassian.net/browse/JUJU-9540) [JUJU-9540]: https://warthogs.atlassian.net/browse/JUJU-9540?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

#22137 Resolves (ToDo](#22090 (comment)) from #22090 1. refactor: move setting calculations to service Moved relation setting change/removal calculations out of the transaction path and into the service layer. This reduces transaction work and keeps the database interaction focused on persistence. The result is a cleaner separation of responsibilities and less overhead during updates. 1. fix: update method to do what the comment says Renamed the internal relation settings helper to reflect its actual behavior more clearly. Updated relation application settings handling so existing settings are replaced rather than merged. The method now deletes current settings before inserting the new values, matching the documented behavior. This change also applies to the shared SetRelationApplicationSettings path. Migration/import behavior is preserved where settings are inserted rather than updated. The implementation now aligns better with the documented contract for relation settings replacement. This change brought to light that empty values in the relation_*_setting tables where possible while some of the code used an empty value to indicate the setting should be deleted. This is partially based on whether the service code path indicated insert, insert/update, or replace/insert functionality. Added checks to ensure where empty values did not indicate a setting should be removed, the keys were dropped and a warning message logged. Due to the low probability, the table SQL will not be updated in 4.0, but in the main branch. ## 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 - [ ] ~[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 Test Migration with unit settings, ``` $ juju_36 bootstrap localhost src $ juju_36 add-model moveme && juju_36 deploy juju-qa-dummy-source source && juju_36 deploy juju-qa-dummy-sink sink && juju_36 relate sink source && juju_36 config source token=goat $ juju bootstrap localhost dst $ juju migration src:moveme dst $ juju status Model Controller Cloud/Region Version Timestamp moveme dst localhost/localhost 4.0.6.1 20:16:51Z App Version Status Scale Charm Channel Rev Exposed Message sink active 1 juju-qa-dummy-sink latest/stable 7 no Token is goat source active 1 juju-qa-dummy-source latest/stable 6 no Token is goat Unit Workload Agent Machine Public address Ports Message sink/0* active idle 1 10.21.51.18 Token is goat source/0* active idle 0 10.21.51.205 Token is goat Machine State Address Inst id Base AZ Message 0 started 10.21.51.205 juju-ad2d53-0 ubuntu@20.04 ubuntu-nuc-two Running 1 started 10.21.51.18 juju-ad2d53-1 ubuntu@20.04 ubuntu-nuc-two Running $ juju config source token=heyya $ juju status Model Controller Cloud/Region Version Timestamp moveme dst localhost/localhost 4.0.6.1 20:17:09Z App Version Status Scale Charm Channel Rev Exposed Message sink active 1 juju-qa-dummy-sink latest/stable 7 no Token is heyya source active 1 juju-qa-dummy-source latest/stable 6 no Token is heyya Unit Workload Agent Machine Public address Ports Message sink/0* active idle 1 10.21.51.18 Token is heyya source/0* active idle 0 10.21.51.205 Token is heyya Machine State Address Inst id Base AZ Message 0 started 10.21.51.205 juju-ad2d53-0 ubuntu@20.04 ubuntu-nuc-two Running 1 started 10.21.51.18 juju-ad2d53-1 ubuntu@20.04 ubuntu-nuc-two Running ``` Test with CMR as well. ## Links **Jira card:** [JUJU-9539](https://warthogs.atlassian.net/browse/JUJU-9539) [JUJU-9539]: https://warthogs.atlassian.net/browse/JUJU-9539?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

hmlanigan requested a review from gfouillet as a code owner March 25, 2026 21:06

jujubot assigned hmlanigan Mar 25, 2026

jujubot added the 4.0 label Mar 25, 2026

hmlanigan requested review from iyiguncevik, jack-w-shaw and nvinuesa March 25, 2026 21:07

hmlanigan force-pushed the atomic-commit-hook-settings branch from 1ad4946 to 0ddfa5d Compare March 26, 2026 15:59

hmlanigan mentioned this pull request Mar 26, 2026

feat: atomic commit hook update network info #22077

Closed

5 tasks

gfouillet reviewed Mar 27, 2026

View reviewed changes

Comment thread domain/unitstate/service/commithook.go Outdated

Comment thread domain/unitstate/service/commithook.go Outdated

Comment thread domain/unitstate/service/commithook.go

Comment thread domain/unitstate/state/commithook.go Outdated

jack-w-shaw approved these changes Mar 27, 2026

View reviewed changes

Comment thread domain/unitstate/state/unitrelations.go

hmlanigan force-pushed the atomic-commit-hook-settings branch from 0ddfa5d to 69e0cf7 Compare March 27, 2026 14:47

nvinuesa reviewed Mar 27, 2026

View reviewed changes

hmlanigan requested a review from gfouillet March 27, 2026 20:12

SimonRichardson reviewed Mar 30, 2026

View reviewed changes

manadart approved these changes Mar 30, 2026

View reviewed changes

SimonRichardson previously requested changes Mar 30, 2026

View reviewed changes

hmlanigan added 7 commits March 31, 2026 17:42

feat: add GetUnitUUIDByName to unitstate state pkg

2abb75c

Will be used for commit hook functionality.

feat: get unit uuid in service CommitHook method

d9746d2

refactor: rename unitUUID to entityUUID to facalitate reuse

76ea945

feat: use relation UUIDs when calling state version of CommitHookChanges

dc48349

Use the relation keys in the RelationSettings to get relation UUIDs for CommitHookChanges in state. This validates the data and simplies the state methods.

feat: update relations settings in atomic commit hook

6b61e64

Move updates to relations settings during CommitHookChanges to the atomic version.

refactor: remove unused code

2785841

SetRelationApplicationAndUnitSettings has moved to the unitstate domain state package for atomic commit hook changes.

hmlanigan force-pushed the atomic-commit-hook-settings branch from 69e0cf7 to 2785841 Compare March 31, 2026 18:27

jujubot merged commit 918d102 into juju:4.0 Mar 31, 2026
24 of 26 checks passed

hmlanigan deleted the atomic-commit-hook-settings branch March 31, 2026 19:22

hmlanigan mentioned this pull request Apr 1, 2026

refactor: move relation application settings replacement behavior #22137

Merged

5 tasks

iyiguncevik mentioned this pull request Apr 2, 2026

chore: merge 4.0 into main #22139

Merged

hmlanigan mentioned this pull request Apr 2, 2026

feat: validate incoming UUIDs for state method of CommitHookChanges #22150

Merged

5 tasks

Conversation

hmlanigan commented Mar 25, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

QA steps

Links

Uh oh!

gfouillet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jack-w-shaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hmlanigan commented Mar 27, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonRichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

manadart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SimonRichardson left a comment

Choose a reason for hiding this comment

Uh oh!

hmlanigan commented Mar 31, 2026

Uh oh!

hmlanigan commented Mar 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hmlanigan commented Mar 25, 2026 •

edited by atlassian Bot

Loading