feat(lxdprofiles): add DDL and state layer for adding and retrieving LXD profiles #18140

nvinuesa · 2024-09-25T09:25:35Z

This small patch adds the DDL (creation of the new lxd_profile table) and the state layer to add and retrieve all lxd profiles for a given machine.

Note: the new methods use the internal/errors new convention but I haven't updated the rest of the methods, this will be done little by little or on a following PR since the machines domain will continue to evolve.

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

Only state layer changed, so unit tests must pass.

Links

Jira card: JUJU-6808

profiles This small patch adds the DDL (creation of the new lxd_profile table) and the state layer to add and retrieve all lxd profiles for a given machine.

SimonRichardson · 2024-09-25T09:51:28Z

domain/machine/state/state.go

+	setLXDProfileStmt, err := st.Prepare(`
+INSERT INTO lxd_profile (*)
+VALUES      ($lxdProfile.*)
+ON CONFLICT DO NOTHING`, lxdProfile{})


I think the better approach is to ask which profiles haven't been inserted, and then insert the ones that are missing. This would work in our favor if there are profiles that have already been applied. You'd do nothing. Also, the number of inserts will be less.

Always prefer a read over a write. The fewer writes we have, the less contention we have, which means we're more likely to go brrrrrrrrrrrrr!

gfouillet

Mostly LGTM, I have request on TU.

domain/machine/state/state_test.go

manadart

Couple of changes please.

domain/machine/state/state.go

domain/schema/model/sql/0017-machine.sql

gfouillet

Few more suggestion ;)

domain/machine/state/state.go

gfouillet · 2024-09-25T13:30:53Z

domain/machine/state/state_test.go

+		c.Assert(err, jc.ErrorIsNil)
+		profiles = append(profiles, profile)
+	}
+	c.Check(profiles, gc.HasLen, 2)


todo: remove hasLen check, I think it is redundant with the SameContent check (which already do it)

Fixed in 82a3a30.

gfouillet · 2024-09-25T13:31:00Z

domain/machine/state/state_test.go

+
+	profiles, err := s.state.LXDProfiles(context.Background(), "deadbeef")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Check(profiles, gc.HasLen, 2)


todo: remove hasLen check, I think it is redundant with the SameContent check (which already do it)

Fixed in 82a3a30.

hmlanigan · 2024-09-25T16:16:09Z

domain/schema/model/sql/0017-machine.sql

+    machine_uuid TEXT NOT NULL,
+    name TEXT NOT NULL,
+    PRIMARY KEY (machine_uuid, name),


There is a potential pitfall here. The order in which lxd profiles are applied is important, as each successive profile over writes items defined prior profiles. The order should not change.

When a new profile is applied, it's to the end of the list. This is problematic with upgrading charms as the name of the profile includes the charm revision.

A caveat is that we always start the list of profiles for a machine with the default profile and juju-<model> profile. This never changes.

You could cheat here and when you get the machine_lxd_profile you sort by the magic rowid column.

But in reality, you need to add an index and just use the slice index when adding, and ordering it when it comes out.

Nice! yeah we were just discussing this with @hmlanigan and I was even thinking about adding a new column just for sorting. Because the worst case scenario was to switch back to having one row per machine and profile names would be an array somehow.

There is precedence of this:

juju/domain/schema/model/sql/0015-charm.sql

Line 511 in 3795cb2

array_index INT NOT NULL,

Done, see 82a3a30 @hmlanigan and @SimonRichardson.

SimonRichardson · 2024-09-25T20:17:10Z

domain/machine/state/state.go

+SELECT &lxdProfile.name
+FROM   machine_lxd_profile
+WHERE  machine_uuid = $machineUUID.uuid`


Ensure you order by index on the way out.

Done, see 82a3a30.

SimonRichardson · 2024-09-27T08:11:06Z

domain/machine/state/state.go

+			return internalerrors.Errorf("checking if machine %q exists: %w", mUUID, err)
+		}
+
+		// Make sure to clean up any existing profiles on the given machine.


Before deleting the profiles, can we check that the profiles in the rows don't already match the ones that already exist. It's very common for the SetLXDProfiles to be triggered with the same content. This comes back to only do a write query if you really need to do one.

When we do wire up lxd profiles, we'll need to insert a logical row into the change stream at the end of this to fire the worker. Hence we need that select check to ensure we're not firing additional changes.

Done, see 6189295.

I'm interested in yours and @manadart 's thoughts on what I did, because this includes now some (small amount of) logic in the state layer. This case sits on the edge of where I'd start using RunAtomic, but IMO it can still live in the state layer.

Keep it here for now.

manadart · 2024-09-27T09:44:33Z

domain/machine/state/state.go

+	}
+
+	return db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
+		queryMachineUUID := machineUUID{UUID: mUUID}


Make this at the top, and pass it to the Prepare calls.

…erting

nvinuesa · 2024-09-27T10:05:25Z

/merge

feat(lxdprofiles): add DDL and state layer for adding and retrieving LXD

7a26d38

profiles This small patch adds the DDL (creation of the new lxd_profile table) and the state layer to add and retrieve all lxd profiles for a given machine.

nvinuesa added the 4.0 label Sep 25, 2024

nvinuesa requested review from SimonRichardson, manadart, Aflynn50 and gfouillet September 25, 2024 09:38

SimonRichardson reviewed Sep 25, 2024

View reviewed changes

gfouillet requested changes Sep 25, 2024

View reviewed changes

domain/machine/state/state_test.go Show resolved Hide resolved

domain/machine/state/state_test.go Show resolved Hide resolved

manadart reviewed Sep 25, 2024

View reviewed changes

domain/machine/state/state.go Show resolved Hide resolved

domain/schema/model/sql/0017-machine.sql Outdated Show resolved Hide resolved

nvinuesa requested review from SimonRichardson and gfouillet September 25, 2024 11:47

chore: add checks for lxd profiles before inserting

00f2fd1

nvinuesa force-pushed the juju-6808 branch from a292016 to 00f2fd1 Compare September 25, 2024 13:04

nvinuesa requested a review from manadart September 25, 2024 13:14

gfouillet reviewed Sep 25, 2024

View reviewed changes

hmlanigan reviewed Sep 25, 2024

View reviewed changes

SimonRichardson reviewed Sep 25, 2024

View reviewed changes

chore: remove previous lxd profiles like in legacy state

82a3a30

nvinuesa requested review from gfouillet, hmlanigan and SimonRichardson September 26, 2024 14:10

gfouillet approved these changes Sep 26, 2024

View reviewed changes

SimonRichardson reviewed Sep 27, 2024

View reviewed changes

manadart approved these changes Sep 27, 2024

View reviewed changes

chore: check if input is the same as existing lxd profiles before ins…

6189295

…erting

jujubot merged commit bcc06c7 into juju:main Sep 27, 2024
18 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lxdprofiles): add DDL and state layer for adding and retrieving LXD profiles #18140

feat(lxdprofiles): add DDL and state layer for adding and retrieving LXD profiles #18140

nvinuesa commented Sep 25, 2024 •

edited

Loading

SimonRichardson Sep 25, 2024

gfouillet left a comment

manadart left a comment

gfouillet left a comment

gfouillet Sep 25, 2024

nvinuesa Sep 26, 2024

gfouillet Sep 25, 2024

nvinuesa Sep 26, 2024

hmlanigan Sep 25, 2024

SimonRichardson Sep 25, 2024

nvinuesa Sep 25, 2024

SimonRichardson Sep 25, 2024

nvinuesa Sep 26, 2024

SimonRichardson Sep 25, 2024

nvinuesa Sep 26, 2024

SimonRichardson Sep 27, 2024 •

edited

Loading

nvinuesa Sep 27, 2024

SimonRichardson Sep 27, 2024

manadart Sep 27, 2024

nvinuesa commented Sep 27, 2024

feat(lxdprofiles): add DDL and state layer for adding and retrieving LXD profiles #18140

feat(lxdprofiles): add DDL and state layer for adding and retrieving LXD profiles #18140

Conversation

nvinuesa commented Sep 25, 2024 • edited Loading

Checklist

QA steps

Links

Choose a reason for hiding this comment

gfouillet left a comment

Choose a reason for hiding this comment

manadart left a comment

Choose a reason for hiding this comment

gfouillet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvinuesa commented Sep 27, 2024

nvinuesa commented Sep 25, 2024 •

edited

Loading

SimonRichardson Sep 27, 2024 •

edited

Loading