Skip to content

Allow setting architecture of containers #771

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow setting architecture of containers #771

wants to merge 1 commit into from

Conversation

AndrewFerr
Copy link
Member

Support creating containers of multi-platform images for an architecture other than that of the host running Complement.

Signed-off-by: Andrew Ferrazzutti andrewf@element.io

Pull Request Checklist

@AndrewFerr
Allow setting architecture of containers
fe2048c 
Support creating containers of multi-platform images for an architecture
other than that of the host running Complement.
@AndrewFerr AndrewFerr requested review from kegsay and a team as code owners April 1, 2025 19:45
@AndrewFerr
Copy link
Member Author

These changes allowed me to run Complement tests locally on an (emulated) arm64 Synapse image, so I figured it'd be helpful to submit a PR for them.

AndrewFerr
AndrewFerr commented Apr 2, 2025
View reviewed changes
internal/docker/deployer.go
@@ -172,7 +177,7 @@ func (d *Deployer) Deploy(ctx context.Context, blueprintName string) (*Deploymen
// TODO: Make CSAPI port configurable
containerName := fmt.Sprintf("complement_%s_%s_%s_%d", d.config.PackageNamespace, d.DeployNamespace, contextStr, counter)
deployment, err := deployImage(
d.Docker, img.ID, containerName,
d.Docker, img.ID, "", containerName,
Copy link
Member Author

Choose a reason for hiding this comment

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

Is an image ID tied to a particular architecture? If not, the correct arch has to be looked up here (and must consider the overrides per homeserver name).

internal/docker/deployer.go
Comment on lines +411 to +412
OS: "linux",
Architecture: imageArch,
Copy link
Member Author

Choose a reason for hiding this comment

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

Is hardcoding the OS here too rigid? Alternatively, the COMPLEMENT_BASE_ARCH could be BASE_PLATFORM and be a full <os>/<arch> string.

kegsay
kegsay reviewed Apr 4, 2025
View reviewed changes
Copy link
Member

@kegsay kegsay left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This feels rather niche, and can be solved in ways that don't expand the already huge API surface of Complement. What's stopping you from building arch-specific images and then referencing them as say homeserver-arm64:latest?

@AndrewFerr
Copy link
Member Author

What's stopping you from building arch-specific images and then referencing them as say homeserver-arm64:latest?

Nothing in particular; this PR was meant to work with Synapse's scripts-dev/complement.sh after being tweaked to build multi-platform images by putting --platform= on the build commands. But with that said, multi-platform images aren't a standard feature of all builders, which makes it even less worth expanding Complement's API surface for. So, I'm content to not carry this PR any further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kegsay kegsay kegsay left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AndrewFerr @kegsay