Keep specific homeserver from blueprint #454

MadLittleMods · 2022-08-22T20:27:25Z

Keep specific homeserver from blueprint. When testing in #443, hs1 has a room with many users with many messages in it which takes a while to construct in the blueprint which I want to stay cached, and hs2 is my local Synapse built up over and over to test optimization changes when federating with hs1.

Split off from #443

Usage:

COMPLEMENT_KEEP_BLUEPRINTS="fed.perf_many_messages.hs1" COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestMessagesOverFederation -p 1

Dev notes

Labels on container:

"Labels": {
    "access_token_@charlie:hs2": "syt_Y2hhcmxpZQ_rKpoAAPGqeKxFYkgmAkU_4PG5Hd",
    "complement_blueprint": "perf_many_messages",
    "complement_context": "fed.perf_many_messages.hs2",
    "complement_hs_name": "hs2",
    "complement_pkg": "fed",
    "desktop.docker.io/ports/8008/tcp": "127.0.0.1:0",
    "desktop.docker.io/ports/8448/tcp": "127.0.0.1:0",
    "device_id@charlie:hs2": "NINFGRNLCL",
    "org.opencontainers.image.documentation": "https://github.com/matrix-org/synapse/blob/master/docker/README.md",
    "org.opencontainers.image.licenses": "Apache-2.0",
    "org.opencontainers.image.source": "https://github.com/matrix-org/synapse.git",
    "org.opencontainers.image.url": "https://matrix.org/docs/projects/server/synapse"
}

Split off from #443

MadLittleMods · 2022-08-22T20:28:20Z

internal/docker/builder.go

@@ -180,17 +180,32 @@ func (d *Builder) ConstructBlueprintIfNotExist(bprint b.Blueprint) error {
 	if err != nil {
 		return fmt.Errorf("ConstructBlueprintIfNotExist(%s): failed to ImageList: %w", bprint.Name, err)
 	}
-	if len(images) == 0 {
-		err = d.ConstructBlueprint(bprint)
+	var missingHomeservers []b.Homeserver


There are many ways to tackle this sort of behavior. What's the most idiomatic Complement way to accomplish this? Different argument name?

I don't know what you mean.

This other comment is perfect, #454 (comment)

Just looking for a way forward as I know the the quick and dirty way here wasn't good enough to merge.

MadLittleMods · 2022-08-22T20:29:39Z

internal/docker/builder.go

@@ -124,16 +124,16 @@ func (d *Builder) removeImages() error {
 			d.log("Not cleaning up image with tags: %v", img.RepoTags)
 			continue
 		}
-		bprintName := img.Labels["complement_blueprint"]
+		contextStr := img.Labels[complementLabel]


Re-using the contextStr because it already packs in the blueprint name and homeserver name like fed.perf_many_messages.hs1

MadLittleMods · 2022-08-22T20:31:25Z

internal/docker/builder.go

 		keep := false
 		for _, keepBprint := range d.Config.KeepBlueprints {
-			if bprintName == keepBprint {
+			if contextStr == keepBprint {


We could add in some regex pattern matching here to support keeping all of the homeservers in a blueprint, fed.perf_many_messages.*

This is a breaking backwards incompatible change as you're modfiying the behaviour of COMPLEMENT_KEEP_BLUEPRINTS to be per-HS and not per blueprint. This needs documentation, backwards compat and warning logs before this PR can land. I'm not opposed to changing the behaviour, but this should probably be a different env var name to avoid confusion as you are not specifying blueprint names. See https://github.com/matrix-org/complement/blob/main/internal/config/config.go#L56 as an example of deprecated logging.

Furthermore, you shouldn't be using contextStr here as it bakes in the package namespace which is unreasonable for us to expect the caller to know.

I would change this to be COMPLEMENT_KEEP_BLUEPRINT_SERVERS=bprintname.hsname e.g COMPLEMENT_KEEP_BLUEPRINT_SERVERS=alice.hs1 perf_many_messages.hs2 and then prefix the complement_pkg to make a complete context string you can == with. Then, add a deprecation notice for COMPLEMENT_KEEP_BLUEPRINTS telling people what they should use instead and why: making this long isn't a problem provided we log it once when the config is setup like the example I gave earlier. Something along the lines of:

Deprecated: COMPLEMENT_KEEP_BLUEPRINTS will be removed in a later version. Use COMPLEMENT_KEEP_BLUEPRINT_SERVERS instead which allows you to specify individual servers in a blueprint to keep rather than keeping all servers in the blueprint.

MadLittleMods · 2022-08-22T20:32:56Z

cmd/account-snapshot/README.md

@@ -23,7 +23,7 @@ Then run Homerunner in single-shot mode: (this will take hours or days depending
 ```
 HOMERUNNER_SNAPSHOT_BLUEPRINT=blueprint.json ./homerunner
 ```
-This will execute the blueprint and commit the resulting images so you can push them to docker hub/gitlab. IMPORTANT: Make sure to set `HOMERUNNER_KEEP_BLUEPRINTS=your-blueprint-name` when running homerunner subsequently or **it will clean up the image**.
+This will execute the blueprint and commit the resulting images so you can push them to docker hub/gitlab. IMPORTANT: Make sure to set `HOMERUNNER_KEEP_BLUEPRINTS=your-blueprint-name` (TODO: Update) when running homerunner subsequently or **it will clean up the image**.


Keeping this as a TODO to update until we settle on a way to accomplish this.

kegsay

Overall happy with the idea. We need to be kinder when breaking API compatibility however.

kegsay · 2022-09-20T10:57:54Z

internal/docker/builder.go

 		keep := false
 		for _, keepBprint := range d.Config.KeepBlueprints {
-			if bprintName == keepBprint {
+			if contextStr == keepBprint {


This is a breaking backwards incompatible change as you're modfiying the behaviour of COMPLEMENT_KEEP_BLUEPRINTS to be per-HS and not per blueprint. This needs documentation, backwards compat and warning logs before this PR can land. I'm not opposed to changing the behaviour, but this should probably be a different env var name to avoid confusion as you are not specifying blueprint names. See https://github.com/matrix-org/complement/blob/main/internal/config/config.go#L56 as an example of deprecated logging.

Furthermore, you shouldn't be using contextStr here as it bakes in the package namespace which is unreasonable for us to expect the caller to know.

I would change this to be COMPLEMENT_KEEP_BLUEPRINT_SERVERS=bprintname.hsname e.g COMPLEMENT_KEEP_BLUEPRINT_SERVERS=alice.hs1 perf_many_messages.hs2 and then prefix the complement_pkg to make a complete context string you can == with. Then, add a deprecation notice for COMPLEMENT_KEEP_BLUEPRINTS telling people what they should use instead and why: making this long isn't a problem provided we log it once when the config is setup like the example I gave earlier. Something along the lines of:

Deprecated: COMPLEMENT_KEEP_BLUEPRINTS will be removed in a later version. Use COMPLEMENT_KEEP_BLUEPRINT_SERVERS instead which allows you to specify individual servers in a blueprint to keep rather than keeping all servers in the blueprint.

kegsay · 2022-09-20T10:58:35Z

internal/docker/builder.go

@@ -180,17 +180,32 @@ func (d *Builder) ConstructBlueprintIfNotExist(bprint b.Blueprint) error {
 	if err != nil {
 		return fmt.Errorf("ConstructBlueprintIfNotExist(%s): failed to ImageList: %w", bprint.Name, err)
 	}
-	if len(images) == 0 {
-		err = d.ConstructBlueprint(bprint)
+	var missingHomeservers []b.Homeserver


I don't know what you mean.

kegsay · 2022-09-20T10:59:45Z

internal/docker/builder.go

 		if err != nil {
 			return fmt.Errorf("ConstructBlueprintIfNotExist(%s): failed to ConstructBlueprint: %w", bprint.Name, err)
 		}
 	}
 	return nil
 }

-func (d *Builder) ConstructBlueprint(bprint b.Blueprint) error {
-	errs := d.construct(bprint)
+func (d *Builder) ConstructBlueprint(bprint b.Blueprint, homeserversToConstruct []b.Homeserver) error {


It's a bit sad you need to specify the homeservers here in addition to the blueprint. It would be nicer if this built all servers if homeserversToConstruct is empty. Needs comments either way as this is a public function.

kegsay · 2022-11-18T10:37:55Z

What's the status with this?

Keep specific homeserver from blueprint

44df406

Split off from #443

MadLittleMods commented Aug 22, 2022

View reviewed changes

MadLittleMods requested a review from kegsay August 22, 2022 20:33

kegsay self-assigned this Sep 17, 2022

kegsay requested changes Sep 20, 2022

View reviewed changes

kegsay assigned MadLittleMods Sep 20, 2022

kegsay added the stale This issue or PR is old and may be closed soon label Nov 18, 2022

MadLittleMods closed this Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep specific homeserver from blueprint #454

Keep specific homeserver from blueprint #454

MadLittleMods commented Aug 22, 2022

MadLittleMods Aug 22, 2022

kegsay Sep 20, 2022

MadLittleMods Sep 20, 2022

MadLittleMods Aug 22, 2022

MadLittleMods Aug 22, 2022

kegsay Sep 20, 2022

MadLittleMods Aug 22, 2022

kegsay left a comment

kegsay Sep 20, 2022

kegsay Sep 20, 2022

kegsay Sep 20, 2022

kegsay commented Nov 18, 2022

Keep specific homeserver from blueprint #454

Keep specific homeserver from blueprint #454

Conversation

MadLittleMods commented Aug 22, 2022

Dev notes

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

kegsay 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

kegsay commented Nov 18, 2022