Skip to content

Keep specific homeserver from blueprint #454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Keep specific homeserver from blueprint #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/account-snapshot/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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**.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#### Limitations

Expand Down
35 changes: 25 additions & 10 deletions internal/docker/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

keep := false
for _, keepBprint := range d.Config.KeepBlueprints {
if bprintName == keepBprint {
if contextStr == keepBprint {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

keep = true
break
}
}
if keep {
d.log("Keeping image created from blueprint %s", bprintName)
d.log("Keeping image created from blueprint %s", contextStr)
continue
}
_, err = d.Docker.ImageRemove(context.Background(), img.ID, types.ImageRemoveOptions{
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what you mean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for _, homeserver := range bprint.Homeservers {
found := false
for _, image := range images {
if image.Labels["complement_hs_name"] == homeserver.Name {
found = true
break
}
}

if !found {
missingHomeservers = append(missingHomeservers, homeserver)
}
}

if len(images) < len(bprint.Homeservers) {
err = d.ConstructBlueprint(bprint, missingHomeservers)
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

errs := d.construct(bprint, homeserversToConstruct)
if len(errs) > 0 {
for _, err := range errs {
d.log("could not construct blueprint: %s", err)
Expand Down Expand Up @@ -237,7 +252,7 @@ func (d *Builder) ConstructBlueprint(bprint b.Blueprint) error {
}

// construct all Homeservers sequentially then commits them
func (d *Builder) construct(bprint b.Blueprint) (errs []error) {
func (d *Builder) construct(bprint b.Blueprint, homeserversToConstruct []b.Homeserver) (errs []error) {
d.log("Constructing blueprint '%s'", bprint.Name)

networkID, err := createNetworkIfNotExists(d.Docker, d.Config.PackageNamespace, bprint.Name)
Expand All @@ -246,8 +261,8 @@ func (d *Builder) construct(bprint b.Blueprint) (errs []error) {
}

runner := instruction.NewRunner(bprint.Name, d.Config.BestEffort, d.Config.DebugLoggingEnabled)
results := make([]result, len(bprint.Homeservers))
for i, hs := range bprint.Homeservers {
results := make([]result, len(homeserversToConstruct))
for i, hs := range homeserversToConstruct {
res := d.constructHomeserver(bprint.Name, runner, hs, networkID)
if res.err != nil {
errs = append(errs, res.err)
Expand Down