Custom `Deployment` quality of life improvements (pass `t`, config) by MadLittleMods · Pull Request #778 · matrix-org/complement

MadLittleMods · 2025-05-13T02:08:07Z

Spawning from a real use-case with a custom Deployment/Deployer (Element-internal). I think this is the first working example of a custom deployment so it makes sense that we ran into a couple oversights.

This PR introduces some quality of life for working with a custom Deployment. For reference, custom deployments were introduced in #750.

Pass t to allow the custom deployment creation callback (complement.WithDeployment(callback)) to handle errors gracefully
Also pass in complement.Config so the custom Deployment can align behavior to what's configured

Other custom Deployment changes: #780

Dev notes

Reference: Using a custom Deployment with Complement

func TestMain(m *testing.M) {
    complement.TestMain(m, "custom_deployment",
        complement.WithDeployment(complement.WithDeployment(func(numServers int) Deployment {
            // return the custom deployment which implements `complement.Deployment`
        })
    )
}

Pull Request Checklist

Pull request includes a sign off

test_main.go

Example: ``` alice.MustJoinRoom(t, bobRoomID, []string{ shardDeployment2.GetFullyQualifiedHomeserverName(t, "hs1"), }) ```

internal/docker/deployment.go

devonh · 2025-05-14T23:16:14Z

Do you know what those CI failures are about? The error doesn't seem related to these changes.

internal/docker/deployment.go

…match issues See #778 (comment)

MadLittleMods · 2025-05-15T02:10:55Z

test_package.go

+	// Returns the resolvable server name (host) of a homeserver given its short alias
+	// (e.g., "hs1", "hs2").
+	//
+	// In the case of the standard Docker deployment, this will be the same `hs1`, `hs2`
+	// but may be different for other custom deployments (ex.
+	// `shardDeployment1.GetFullyQualifiedHomeserverName(t, "hs1")` -> `hs1.shard1:8081`).
+	GetFullyQualifiedHomeserverName(t ct.TestLike, hsName string) string


@kegsay Just to double-check with the Complement taste-maker, do these changes make sense to you?

Are you okay with this breaking change to the complement.Deployment interface?

Are you okay with the breaking changes to complement.WithDeployment(...) / complement.WithCleanup(...)?

I'm happy with adding more params to With... options.

I'm unhappy with adding GetFullyQualifiedHomeserverName. See https://github.com/matrix-org/complement/pull/778/files#r2091384008

Thanks for the review @kegsay!

The GetFullyQualifiedHomeserverName changes have been split off to #780

This reverts commit 10578f6.

This reverts commit 9035d88.

MadLittleMods · 2025-05-16T16:00:16Z

Thanks for the review @devonh and @kegsay 🦉

…t custom `Deployment` (#780) *Split off from #778 per [discussion](#778 (comment) Spawning from a real use-case with a custom `Deployment`/`Deployer` (Element-internal). Introduce `complement.Deployment.GetFullyQualifiedHomeserverName(hsName)` to allow the per-deployment short homeserver aliases like `hs1` to be mapped to something else that's resolvable in your custom deployments context. Example: `hs1` -> `hs1.shard1:8481`. This is useful for situations like the following where you have to specify the via servers in a federated join request during a test: ```go alice.MustJoinRoom(t, bobRoomID, []string{ deployment.GetFullyQualifiedHomeserverName(t, "hs2"), }) ``` ### But why does this have to be part of the `complement.Deployment` interface instead of your own custom deployment? - Tests only have access to the generic `complement.Deployment` interface - We can't derive fully-qualified homeserver names from the existing `complement.Deployment` interface - While we could cheekily cast the generic `complement.Deployment` back to `CustomDeployment` in our own tests (and have the helper in the `CustomDeployment` instead), if we start using something exotic in our out-of-repo Complement tests, the suite of existing Complement tests in this repo will not be compatible. (also see below) ### Motivating custom `Deployment` use case [`complement.Deployment`](https://github.com/matrix-org/complement/blob/d2e04c995666fbeb0948e6a4ed52d3fbb45fbdf7/test_package.go#L21-L69) is an interface that can be backed by anything. For reference, custom deployments were introduced in #750. The [default `Deployment` naming scheme in Complement is `hs1`, `hs2`, etc](https://github.com/matrix-org/complement/blob/6b63eff50804beb334ca215650f5027ddf02ae9a/test_package.go#L198). It's really nice and convenient to be able to simply refer to homeservers as `hs1`, etc within a deployment. And using consistent names like this makes tests compatible with each other regardless of which `Deployment` is being used. The built-in `Deployment` in Complement has each homeserver in a Docker container which already has network aliases like `hs1`, `hs2`, etc so no translation is needed from friendly name to resolvable address. When one homeserver needs to federate with the other, it can simply make a request to `https://hs1:8448/...` per [spec on resolving server names](https://spec.matrix.org/v1.13/server-server-api/#resolving-server-names). Right-now, we hard-code `hs1` across the tests when we specify ["via" servers in join requests](https://spec.matrix.org/v1.13/client-server-api/#post_matrixclientv3joinroomidoralias) but that only works if you follow the strict single-deployment naming scheme. https://github.com/matrix-org/complement/blob/6b63eff50804beb334ca215650f5027ddf02ae9a/tests/federation_rooms_invite_test.go#L112 In the current setup of our custom `Deployment`, each `Deployment` is a "shard" application that can deploy multiple homeserver "tenants". We specifically want to test that homeservers between multiple shards can federate with each other as a sanity check (make sure our shards can deploy homeserver tenants correctly). If we keep using the consistent `hs1`, `hs2` naming scheme for each `Deployment` we're going to have conflicts. This is where `deployment.GetFullyQualifiedHomeserverName(t, hsName)` comes in handy. We can call `deployment1.GetFullyQualifiedHomeserverName(t, "hs1")` -> `hs1.shard1` and also `deployment2.GetFullyQualifiedHomeserverName(t, "hs1")` -> `hs1.shard2` to get their unique resolvable addresses in the network. Additionally, the helper removes the constraint of needing the network to strictly resolve `hs1`, `hs2` hostnames to their respective homeservers. Whenever you need to refer to another homeserver, use `deployment.GetFullyQualifiedHomeserverName(hsName)` to take care of the nuance of environment that the given `Deployment` creates. Example of a cross-deployment test: ```go func TestMain(m *testing.M) { complement.TestMain(m, "custom_tests", complement.WithDeployment(internal.MakeCustomDeployment()), ) } func TestCrossShardFederation(t *testing.T) { // Create two shards with their own homeserver tenants shardDeployment1 := complement.Deploy(t, 1) defer shardDeployment1.Destroy(t) shardDeployment2 := complement.Deploy(t, 1) defer shardDeployment2.Destroy(t) alice := shardDeployment1.Register(t, "hs1", helpers.RegistrationOpts{}) bob := shardDeployment2.Register(t, "hs1", helpers.RegistrationOpts{}) aliceRoomID := alice.MustCreateRoom(t, map[string]any{ "preset": "public_chat", }) bobRoomID := bob.MustCreateRoom(t, map[string]any{ "preset": "public_chat", }) t.Run("parallel", func(t *testing.T) { t.Run("shard1 -> shard2", func(t *testing.T) { // Since these tests use the same config, they can be run in parallel t.Parallel() alice.MustJoinRoom(t, bobRoomID, []string{ shardDeployment2.GetFullyQualifiedHomeserverName(t, "hs1"), }) bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, bobRoomID)) }) t.Run("shard2 -> shard1", func(t *testing.T) { // Since these tests use the same config, they can be run in parallel t.Parallel() bob.MustJoinRoom(t, aliceRoomID, []string{ shardDeployment1.GetFullyQualifiedHomeserverName(t, "hs1"), }) alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, aliceRoomID)) }) }) } ``` Per the discussion in #780 (comment), multiple-deployments per test doesn't work with Complement's `Deployment` implementation yet and the `Deployment` is meant to encapsulate an _entire_ deployment, all servers and network links between them. This was the motivating use case but use at your own discretion until further guidance is given.

…atrix-org#778) Spawning from a real use-case with a custom `Deployment`/`Deployer` (Element-internal). I think this is the first working example of a custom deployment so it makes sense that we ran into a couple oversights. This PR introduces some quality of life for working with a custom `Deployment`. For reference, custom deployments were introduced in matrix-org#750. - Pass `t` to allow the custom deployment creation callback (`complement.WithDeployment(callback)`) to handle errors gracefully - Also pass in `complement.Config` so the custom Deployment can align behavior to what's configured

…t custom `Deployment` (matrix-org#780) *Split off from matrix-org#778 per [discussion](matrix-org#778 (comment) Spawning from a real use-case with a custom `Deployment`/`Deployer` (Element-internal). Introduce `complement.Deployment.GetFullyQualifiedHomeserverName(hsName)` to allow the per-deployment short homeserver aliases like `hs1` to be mapped to something else that's resolvable in your custom deployments context. Example: `hs1` -> `hs1.shard1:8481`. This is useful for situations like the following where you have to specify the via servers in a federated join request during a test: ```go alice.MustJoinRoom(t, bobRoomID, []string{ deployment.GetFullyQualifiedHomeserverName(t, "hs2"), }) ``` ### But why does this have to be part of the `complement.Deployment` interface instead of your own custom deployment? - Tests only have access to the generic `complement.Deployment` interface - We can't derive fully-qualified homeserver names from the existing `complement.Deployment` interface - While we could cheekily cast the generic `complement.Deployment` back to `CustomDeployment` in our own tests (and have the helper in the `CustomDeployment` instead), if we start using something exotic in our out-of-repo Complement tests, the suite of existing Complement tests in this repo will not be compatible. (also see below) ### Motivating custom `Deployment` use case [`complement.Deployment`](https://github.com/matrix-org/complement/blob/d2e04c995666fbeb0948e6a4ed52d3fbb45fbdf7/test_package.go#L21-L69) is an interface that can be backed by anything. For reference, custom deployments were introduced in matrix-org#750. The [default `Deployment` naming scheme in Complement is `hs1`, `hs2`, etc](https://github.com/matrix-org/complement/blob/6b63eff50804beb334ca215650f5027ddf02ae9a/test_package.go#L198). It's really nice and convenient to be able to simply refer to homeservers as `hs1`, etc within a deployment. And using consistent names like this makes tests compatible with each other regardless of which `Deployment` is being used. The built-in `Deployment` in Complement has each homeserver in a Docker container which already has network aliases like `hs1`, `hs2`, etc so no translation is needed from friendly name to resolvable address. When one homeserver needs to federate with the other, it can simply make a request to `https://hs1:8448/...` per [spec on resolving server names](https://spec.matrix.org/v1.13/server-server-api/#resolving-server-names). Right-now, we hard-code `hs1` across the tests when we specify ["via" servers in join requests](https://spec.matrix.org/v1.13/client-server-api/#post_matrixclientv3joinroomidoralias) but that only works if you follow the strict single-deployment naming scheme. https://github.com/matrix-org/complement/blob/6b63eff50804beb334ca215650f5027ddf02ae9a/tests/federation_rooms_invite_test.go#L112 In the current setup of our custom `Deployment`, each `Deployment` is a "shard" application that can deploy multiple homeserver "tenants". We specifically want to test that homeservers between multiple shards can federate with each other as a sanity check (make sure our shards can deploy homeserver tenants correctly). If we keep using the consistent `hs1`, `hs2` naming scheme for each `Deployment` we're going to have conflicts. This is where `deployment.GetFullyQualifiedHomeserverName(t, hsName)` comes in handy. We can call `deployment1.GetFullyQualifiedHomeserverName(t, "hs1")` -> `hs1.shard1` and also `deployment2.GetFullyQualifiedHomeserverName(t, "hs1")` -> `hs1.shard2` to get their unique resolvable addresses in the network. Additionally, the helper removes the constraint of needing the network to strictly resolve `hs1`, `hs2` hostnames to their respective homeservers. Whenever you need to refer to another homeserver, use `deployment.GetFullyQualifiedHomeserverName(hsName)` to take care of the nuance of environment that the given `Deployment` creates. Example of a cross-deployment test: ```go func TestMain(m *testing.M) { complement.TestMain(m, "custom_tests", complement.WithDeployment(internal.MakeCustomDeployment()), ) } func TestCrossShardFederation(t *testing.T) { // Create two shards with their own homeserver tenants shardDeployment1 := complement.Deploy(t, 1) defer shardDeployment1.Destroy(t) shardDeployment2 := complement.Deploy(t, 1) defer shardDeployment2.Destroy(t) alice := shardDeployment1.Register(t, "hs1", helpers.RegistrationOpts{}) bob := shardDeployment2.Register(t, "hs1", helpers.RegistrationOpts{}) aliceRoomID := alice.MustCreateRoom(t, map[string]any{ "preset": "public_chat", }) bobRoomID := bob.MustCreateRoom(t, map[string]any{ "preset": "public_chat", }) t.Run("parallel", func(t *testing.T) { t.Run("shard1 -> shard2", func(t *testing.T) { // Since these tests use the same config, they can be run in parallel t.Parallel() alice.MustJoinRoom(t, bobRoomID, []string{ shardDeployment2.GetFullyQualifiedHomeserverName(t, "hs1"), }) bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, bobRoomID)) }) t.Run("shard2 -> shard1", func(t *testing.T) { // Since these tests use the same config, they can be run in parallel t.Parallel() bob.MustJoinRoom(t, aliceRoomID, []string{ shardDeployment1.GetFullyQualifiedHomeserverName(t, "hs1"), }) alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, aliceRoomID)) }) }) } ``` Per the discussion in matrix-org#780 (comment), multiple-deployments per test doesn't work with Complement's `Deployment` implementation yet and the `Deployment` is meant to encapsulate an _entire_ deployment, all servers and network links between them. This was the motivating use case but use at your own discretion until further guidance is given.

MadLittleMods added 2 commits May 12, 2025 12:05

Add args to custom deployment/cleanup

94a8e61

Remove TODO docs

896ab4f

MadLittleMods commented May 13, 2025

View reviewed changes

test_main.go Show resolved Hide resolved

MadLittleMods added 2 commits May 13, 2025 15:45

Introduce GetFullyQualifiedHomeserverName(...)

9035d88

Make GetFullyQualifiedHomeserverName easier to use in tests

10578f6

Example: ``` alice.MustJoinRoom(t, bobRoomID, []string{ shardDeployment2.GetFullyQualifiedHomeserverName(t, "hs1"), }) ```

MadLittleMods changed the title ~~Add extra context needed to handle errors and align with Complement config when using a custom Deployment~~ Custom Deployment quality of life improvements (pass t, config, and FQDN helper) May 13, 2025

MadLittleMods marked this pull request as ready for review May 13, 2025 22:49

MadLittleMods requested review from a team and kegsay as code owners May 13, 2025 22:49

devonh reviewed May 14, 2025

View reviewed changes

internal/docker/deployment.go Outdated Show resolved Hide resolved

MadLittleMods commented May 14, 2025

View reviewed changes

internal/docker/deployment.go Show resolved Hide resolved

MadLittleMods added 2 commits May 14, 2025 18:36

Try using WithAPIVersionNegotiation to avoid Docker API version mis…

8fb49da

…match issues See #778 (comment)

Use NewClientWithOpts in both places

d591b4d

devonh approved these changes May 14, 2025

View reviewed changes

MadLittleMods requested a review from devonh May 14, 2025 23:50

MadLittleMods commented May 15, 2025

View reviewed changes

MadLittleMods requested review from kegsay and removed request for kegsay May 15, 2025 15:56

MadLittleMods added 2 commits May 16, 2025 10:44

Revert "Make GetFullyQualifiedHomeserverName easier to use in tests"

c443481

This reverts commit 10578f6.

Revert "Introduce GetFullyQualifiedHomeserverName(...)"

ae8cf42

This reverts commit 9035d88.

MadLittleMods mentioned this pull request May 16, 2025

Add deployment.GetFullyQualifiedHomeserverName(t, hsName) to support custom Deployment #780

Merged

13 tasks

MadLittleMods changed the title ~~Custom Deployment quality of life improvements (pass t, config, and FQDN helper)~~ Custom Deployment quality of life improvements (pass t, config) May 16, 2025

MadLittleMods merged commit 6b63eff into main May 16, 2025
4 checks passed

MadLittleMods deleted the madlittlemods/customer-deployer-args branch May 16, 2025 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom `Deployment` quality of life improvements (pass `t`, config)#778

Custom `Deployment` quality of life improvements (pass `t`, config)#778
MadLittleMods merged 8 commits intomainfrom
madlittlemods/customer-deployer-args

MadLittleMods commented May 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

devonh commented May 14, 2025

Uh oh!

Uh oh!

MadLittleMods May 15, 2025 •

edited

Loading

Uh oh!

kegsay May 15, 2025

Uh oh!

MadLittleMods May 16, 2025

Uh oh!

Uh oh!

MadLittleMods commented May 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

MadLittleMods commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Pull Request Checklist

Uh oh!

Uh oh!

Uh oh!

devonh commented May 14, 2025

Uh oh!

Uh oh!

MadLittleMods May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kegsay May 15, 2025

Choose a reason for hiding this comment

Uh oh!

MadLittleMods May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods commented May 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MadLittleMods commented May 13, 2025 •

edited

Loading

MadLittleMods May 15, 2025 •

edited

Loading