Use LTS microcluster by masnax · Pull Request #395 · canonical/microceph

masnax · 2024-08-01T23:40:34Z

Updates MicroCeph to use the stable release of microcluster at v2.0.0.

For the most part, the changes to the code in MicroCeph are just package imports and changing the definition of state.State to an interface.

The actual behavioural changes are these:

state.Context is no longer exposed, so request.Context from the API handler is passed around explicitly to each helper. This should actually result in more precise context handling for MicroCeph.
- One caveat is a few async API operations which spawn a goroutine. For these context.Background() is used because the request context may cancel before the goroutine is complete.
Adds a schema update to MicroCeph that changes their foreign key references to core_cluster_members. This table has been renamed from internal_cluster_members to make it clear that accessing it is expected behaviour. I also took the liberty of renaming the previous schema update function to use an ordered naming scheme so that it doesn't become ambiguous where an update fits among the others.
Join tokens now have expiration dates. By default, MicroCeph will set a 3 hour expiration for join tokens, but this can be configured with the --timeout flag added to cluster add.

The project version supplied to microcluster has been set to UNKOWN for now because microcluster requires a value here.

masnax · 2024-08-02T01:15:10Z

Test failures appear to be because of the go module transition. Should be fixed when canonical/microcluster#219 is merged.

UtkarshBhatthere · 2024-08-05T06:59:51Z

Thanks for the change @masnax will take a look :D

UtkarshBhatthere · 2024-08-18T16:41:14Z

microceph/database/schema.go

+// schemaUpdate4 updates all tables referencing `internal_cluster_members` to now reference `core_cluster_members` instead.
+func schemaUpdate4(ctx context.Context, tx *sql.Tx) error {
+	stmt := `
+CREATE TABLE disks2 (


@sabaini It would be good to have a discussion for table names being introduced here.

FWIW, the tables in MicroCeph have not been renamed, it is only the foreign key reference to the microcluster table that has changed. We ensure that foreign key references are not erased when we changed the table name internally, so no data will be lost.

This had to be retroactively applied to all schema updates in case a very old version of MicroCeph tries to update to the latest version.

UtkarshBhatthere · 2024-08-18T16:43:58Z

microceph/cmd/microcephd/main.go

+	daemonArgs := microcluster.DaemonArgs{
+		Verbose:          c.global.flagLogVerbose,
+		Debug:            c.global.flagLogDebug,
+		Version:          "UNKNOWN", // FIXME: Add an explicit version to MicroCeph.


@masnax can you please add some comments to describe what versioning is this.

When using the MicroCeph API, it is useful to determine the current version of MicroCeph so that compatibility can be verified. We have an endpoint at /core/1.0 that provides basic identifying information like the cluster name, address, and project version.

For example, from MicroCloud, we should be able to tell from the API that MicroCeph is installed from a revision belonging to a supported snap track/risk. Like MicroCloud LTS should verify that MicroCeph is also installed as an LTS version.

Microcluster validates that a version must be present, but I wasn't able to find any value in the project that matched this criteria so I put UNKOWN here.

I'll update the commit with the above information as well.

I've added a comment in-line with the Version field now.

microceph/cmd/microceph/status.go

UtkarshBhatthere

Did a round of reviews, have left questions and comments inline.

roosterfish

Thanks @masnax, looks good to me from microcluster perspective.

Aside from those changes I am wondering if we should parse the --timeout duration from string instead of requiring specifying it in seconds.
This way if you intend to use a duration of a few hours you don't have to provide this big int which represents the seconds but instead provide only 6h or 1d.
Something like we do in the microcluster example package.

masnax · 2024-08-28T17:35:29Z

Thanks @masnax, looks good to me from microcluster perspective.

Aside from those changes I am wondering if we should parse the --timeout duration from string instead of requiring specifying it in seconds. This way if you intend to use a duration of a few hours you don't have to provide this big int which represents the seconds but instead provide only 6h or 1d. Something like we do in the microcluster example package.

Good idea, this has been applied now.

sabaini

This lgtm, thanks @masnax!
One comment inline wrt to versioning

microceph/cmd/microcephd/main.go