Generate triggers #17156

SimonRichardson · 2024-04-05T11:47:29Z

Triggers were written whereby any update to the database would cause a new insert into the change log, even if the value was identical. This causes a lot of churn in the change log. We can generate the triggers using the columns from the actual database schema. The triggers will always be correct and always up to date.

Part of the change set includes fixing watchers and watcher tests:

Cloud watcher

The watch cloud test wasn't working how it was expected. The upsert was just triggering a new change in the old code, prior to this patch. The cloud-config is currently immutable, I'll open a bug to fix this. As the config is not updated and the trigger prevents a noop change then the test fails. The solution is to then update a field on the cloud that will cause a change. In this case, the endpoint was updated.

Bug about cloud-config: https://bugs.launchpad.net/juju/+bug/2066410

Model config watcher

The model config watch wasn't correct once the generated triggers were added. Firstly, we deleted all of the keys and then re-added them again even if they were the same. This caused a lot of churn in the changestream. The fix was just to ensure that we inserted, updated, and deleted rows correctly.

The second problem was that we would see the initial changes from bootstrap and then see them again when the change stream was processing requests. This might be ok for some watchers (I highly doubt that), but the concept of watchers are that, watch changes and then react. For most things that take information from the API server, the initial
event is thrown away, so it's pointless trying to get that. It's better to just read the state of the world and then respond to changes.

The model_config watcher will now emit an empty initial event and then subsequent events will tell you what has changed.

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

QA steps

$ make rebuild-triggers

$ juju bootstrap lxd test
$ juju add-model default
$ juju deploy ubuntu

Links

Jira card: JUJU-6055

SimonRichardson · 2024-04-08T07:59:19Z

@manadart there is a slight wart with this, as it's compiled into non-test code, the lack of the file or even a typo means you can't create the file again without commenting out the code that depends on it.

manadart · 2024-04-08T08:19:54Z

I was thinking about this over the weekend. The inequality checks don't work if either of the values are NULL, and we can't use arbitrary values for IFNULL on both sides, because we don't know the valid values for every field.

It looks like we will have might have to do the filtering in the specific watchers.

SimonRichardson · 2024-04-08T08:55:33Z

I was thinking about this over the weekend. The inequality checks don't work if either of the values are NULL, and we can't use arbitrary values for IFNULL on both sides, because we don't know the valid values for every field.

We can check for that in the DB schema and work through that.

SimonRichardson · 2024-04-08T13:58:56Z

For example PRAGMA table_info(<table-name>); will tell you if it's expecting NULL or not. Then we can apply the following rules:

If NOT NULL - a != b
If NULL - (IFNULL(a, b) OR a != b)

manadart

This is a better option than the esoteric trigger functions. We'll need to ensure it's understood and accepted as an alternative.

manadart · 2024-04-22T16:40:09Z

domain/blockdevice/state/state.go

@@ -177,24 +177,7 @@ func (st *State) updateBlockDevices(ctx context.Context, tx *sqlair.TX, machineU
 		fsTypeByName[fsType.Name] = fsType.ID
 	}

-	insertQuery := `
-INSERT INTO block_device (uuid, machine_uuid, name, label, device_uuid, hardware_id, wwn, bus_address, serial_id, mount_point, size_mib, filesystem_type_id, in_use)


This should have gone in its own commit.

I'm going to rewind this, I was trying to test the null change.

generate/triggergen/main.go

domain/schema/controller.go

SimonRichardson · 2024-05-06T10:19:23Z

This is on hold until after the beta 4.0 release. Then we can bake these changes into the codebase.

SimonRichardson · 2024-05-22T15:24:21Z

/build

SimonRichardson · 2024-05-22T16:24:40Z

Bug about missing cloud config update https://bugs.launchpad.net/juju/+bug/2066410

SimonRichardson · 2024-05-23T14:29:56Z

domain/schema/changelog.go

 	"github.com/juju/juju/core/database/schema"
 )

+type tableNamespaceID = int


I think we should rethink this at some point. It doesn't provide strong guarantees between schema and change log watchers.

SimonRichardson · 2024-05-23T14:30:28Z

domain/schema/testing/schema.go

@@ -29,3 +33,24 @@ func (s *SchemaApplier) Apply(c *gc.C, ctx context.Context, runner database.TxnR
 	c.Assert(err, gc.IsNil)
 	c.Check(changeSet.Post, gc.Equals, s.Schema.Len())
 }
+
+// DumpChangeLog dumps the change log to the test log.
+func DumpChangeLog(ctx context.Context, c *gc.C, runner database.TxnRunner) {


This is VERY useful for debugging the change log.

manadart

You've already got a conflict, but everything looks good.

Probably worth an email to crew about working with it.

domain/schema/testing/schema.go

generate/triggergen/main.go

Triggers were written whereby any update to the database would cause a new insert into the change log, even if the value was identical. This causes a lot of churn in the change log. Instead we can generate the triggers using the columns from the actual database schema. The triggers will always be correct and always up to date.

To make it easier to build and see, we split each trigger into individual trigger documents.

The watch cloud test wasn't working how it was expected. The upsert was just triggering a new change in the old code, prior to this patch. The cloud config is currently immutable, I'll open a bug to fix this. As the config is not updated and the trigger prevents a noop change then the test fails. The solution is to then update a field on the cloud that will actually cause a change. In this case the endpoint being updated.

The model config watch wasn't correct once the generated triggers were added. Firstly, we deleted all of the keys and then re-added them again even if they where the same. This caused a lot of churn in the changestream. The fix was just ensure that we inserted, updated and deleted rows correctly. The second problem was that we would see the initial changes from bootstrap and then see them again when the change stream was processing requests. This might be ok for some watchers (I highly doubt that), but the concept of watchers are, watch changes and then react. For most things that take information from the API server, the initial event is thrown away, so it's pointless trying to get that. It's better to just read the state of the world then respond to changes. The model_config watcher will now emit an empty initial event and then subsequent events will tell you what has changed.

We need to ensure that the change stream is actually idle. We can do this by checking what the current upper bound is vs what is the MAX change log id. If those match we're done! This is only ever run in test code as we don't care for production.

The trigger gen script was put together to be a prove out that it was possible. The prototype should reuse functions and modularize the content. This change does that by splitting it into different concerns.

We no longer render out the subnet association any more.

SimonRichardson · 2024-05-29T13:00:55Z

/build

We don't need to wait for the change stream to become idle every time. It just needs to be used after an update to the state, not after every change.

SimonRichardson · 2024-05-29T15:03:37Z

/merge

SimonRichardson added the 4.0 label Apr 5, 2024

SimonRichardson requested a review from manadart April 5, 2024 11:47

SimonRichardson self-assigned this Apr 5, 2024

This comment was marked as outdated.

Sign in to view

SimonRichardson force-pushed the generate-triggers branch from 891bb26 to c726b32 Compare April 5, 2024 14:40

SimonRichardson force-pushed the generate-triggers branch 2 times, most recently from 4efd2cf to 70dff6c Compare April 13, 2024 20:31

hpidcock added the has merge conflicts label Apr 18, 2024

SimonRichardson force-pushed the generate-triggers branch from 70dff6c to 039476b Compare April 19, 2024 09:21

SimonRichardson removed the has merge conflicts label Apr 19, 2024

SimonRichardson force-pushed the generate-triggers branch 3 times, most recently from d8e3f24 to d53ea2d Compare April 22, 2024 15:59

manadart reviewed Apr 22, 2024

View reviewed changes

hpidcock added the has merge conflicts label Apr 23, 2024

SimonRichardson force-pushed the generate-triggers branch from d53ea2d to 00fa2b3 Compare May 17, 2024 08:10

SimonRichardson removed the has merge conflicts label May 17, 2024

SimonRichardson force-pushed the generate-triggers branch from 830663b to 1fb87f0 Compare May 17, 2024 10:23

SimonRichardson force-pushed the generate-triggers branch from 1fb87f0 to faf94c0 Compare May 22, 2024 15:45

SimonRichardson force-pushed the generate-triggers branch 3 times, most recently from 36ff90a to bbb17e5 Compare May 23, 2024 14:12

SimonRichardson commented May 23, 2024

View reviewed changes

SimonRichardson force-pushed the generate-triggers branch from ba10e84 to 5078ba1 Compare May 24, 2024 07:48

manadart approved these changes May 29, 2024

View reviewed changes

domain/schema/testing/schema.go Outdated Show resolved Hide resolved

generate/triggergen/main.go Show resolved Hide resolved

SimonRichardson added 8 commits May 29, 2024 13:23

Split the triggers into individual files

78c8458

To make it easier to build and see, we split each trigger into individual trigger documents.

Ensure we add comments to the trigger functions

a0f5b27

Rework change stream idle

fa044c9

We need to ensure that the change stream is actually idle. We can do this by checking what the current upper bound is vs what is the MAX change log id. If those match we're done! This is only ever run in test code as we don't care for production.

Make trigger gen easier to read

413fbb3

The trigger gen script was put together to be a prove out that it was possible. The prototype should reuse functions and modularize the content. This change does that by splitting it into different concerns.

Remove subnet associations

990b258

We no longer render out the subnet association any more.

SimonRichardson force-pushed the generate-triggers branch from b1a2057 to 990b258 Compare May 29, 2024 12:28

Fix the secrets watcher

4606284

We don't need to wait for the change stream to become idle every time. It just needs to be used after an update to the state, not after every change.

jujubot merged commit f179ef9 into juju:main May 29, 2024
14 of 17 checks passed

SimonRichardson deleted the generate-triggers branch May 29, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate triggers #17156

Generate triggers #17156

SimonRichardson commented Apr 5, 2024 •

edited by jira bot

This comment was marked as outdated.

SimonRichardson commented Apr 8, 2024

manadart commented Apr 8, 2024

SimonRichardson commented Apr 8, 2024

SimonRichardson commented Apr 8, 2024

manadart left a comment

manadart Apr 22, 2024

SimonRichardson Apr 22, 2024

SimonRichardson commented May 6, 2024

SimonRichardson commented May 22, 2024

SimonRichardson commented May 22, 2024

SimonRichardson May 23, 2024

SimonRichardson May 23, 2024

manadart left a comment

SimonRichardson commented May 29, 2024

SimonRichardson commented May 29, 2024

Generate triggers #17156

Generate triggers #17156

Conversation

SimonRichardson commented Apr 5, 2024 • edited by jira bot

Cloud watcher

Model config watcher

Checklist

QA steps

Links

This comment was marked as outdated.

SimonRichardson commented Apr 8, 2024

manadart commented Apr 8, 2024

SimonRichardson commented Apr 8, 2024

SimonRichardson commented Apr 8, 2024

manadart left a comment

Choose a reason for hiding this comment

manadart Apr 22, 2024

Choose a reason for hiding this comment

SimonRichardson Apr 22, 2024

Choose a reason for hiding this comment

SimonRichardson commented May 6, 2024

SimonRichardson commented May 22, 2024

SimonRichardson commented May 22, 2024

SimonRichardson May 23, 2024

Choose a reason for hiding this comment

SimonRichardson May 23, 2024

Choose a reason for hiding this comment

manadart left a comment

Choose a reason for hiding this comment

SimonRichardson commented May 29, 2024

SimonRichardson commented May 29, 2024

SimonRichardson commented Apr 5, 2024 •

edited by jira bot