From 95f644174642d59bd19b08789943780feedba226 Mon Sep 17 00:00:00 2001 From: Jason Lynch Date: Mon, 27 Apr 2026 15:01:53 -0400 Subject: [PATCH] fix: update existing instances first in node update Modifies the node update process to ensure that existing instances are updated before adding any new replicas. This ensures that we apply configuration updates to the primary and other existing replicas first. This fixes a bug for systemd where the new replica could be unable to bootstrap from the primary due to missing pg_hba.conf entries. PLAT-566 --- e2e/add_replica_test.go | 107 ++++ .../TestUpdateDatabase/adding_a_replica.json | 26 +- .../adding_a_replica_with_primary_update.json | 138 +++++ .../adding_multiple_replicas_concurrent.json | 40 +- .../adding_multiple_replicas_rolling.json | 56 +- .../database/operations/update_database.go | 4 +- .../operations/update_database_test.go | 22 + .../database/operations/update_nodes.go | 42 +- .../database/operations/update_nodes_test.go | 529 +++++++++++++++++- 9 files changed, 841 insertions(+), 123 deletions(-) create mode 100644 e2e/add_replica_test.go create mode 100644 server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica_with_primary_update.json diff --git a/e2e/add_replica_test.go b/e2e/add_replica_test.go new file mode 100644 index 00000000..3ed59cb3 --- /dev/null +++ b/e2e/add_replica_test.go @@ -0,0 +1,107 @@ +//go:build e2e_test + +package e2e + +import ( + "testing" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" + + api "github.com/pgEdge/control-plane/api/apiv1/gen/control_plane" +) + +func TestAddReplica(t *testing.T) { + t.Parallel() + + host1 := fixture.HostIDs()[0] + host2 := fixture.HostIDs()[1] + host3 := fixture.HostIDs()[2] + + username := "admin" + password := "password" + + tLog(t, "creating database") + + ctx := t.Context() + db := fixture.NewDatabaseFixture(ctx, t, &api.CreateDatabaseRequest{ + Spec: &api.DatabaseSpec{ + DatabaseName: "add_replica", + DatabaseUsers: []*api.DatabaseUserSpec{ + { + Username: username, + Password: pointerTo(password), + DbOwner: pointerTo(true), + Attributes: []string{"LOGIN", "SUPERUSER"}, + }, + }, + Port: pointerTo(0), + PatroniPort: pointerTo(0), + Nodes: []*api.DatabaseNodeSpec{ + {Name: "n1", HostIds: []api.Identifier{api.Identifier(host1)}}, + {Name: "n2", HostIds: []api.Identifier{api.Identifier(host2)}}, + }, + }, + }) + + tLog(t, "creating test data") + + opts := ConnectionOptions{ + Username: username, + Password: password, + } + db.WithConnection(ctx, opts, t, func(conn *pgx.Conn) { + _, err := conn.Exec(ctx, "CREATE TABLE foo (id int primary key, val text)") + require.NoError(t, err) + _, err = conn.Exec(ctx, "INSERT INTO foo (id, val) VALUES (1, 'foo')") + require.NoError(t, err) + }) + + tLog(t, "adding a replica") + + err := db.Update(ctx, UpdateOptions{ + Spec: &api.DatabaseSpec{ + DatabaseName: "add_replica", + DatabaseUsers: []*api.DatabaseUserSpec{ + { + Username: username, + DbOwner: pointerTo(true), + Attributes: []string{"LOGIN", "SUPERUSER"}, + }, + }, + Port: pointerTo(0), + PatroniPort: pointerTo(0), + Nodes: []*api.DatabaseNodeSpec{ + { + Name: "n1", + HostIds: []api.Identifier{ + api.Identifier(host1), + api.Identifier(host3), + }, + }, + {Name: "n2", HostIds: []api.Identifier{api.Identifier(host2)}}, + }, + }, + }) + require.NoError(t, err) + + tLog(t, "validating that replica exists and is populated") + + opts = ConnectionOptions{ + Username: username, + Password: password, + Matcher: And(WithNode("n1"), WithRole("replica")), + } + db.WithConnection(ctx, opts, t, func(conn *pgx.Conn) { + const isInRecoveryQuery = "SELECT pg_is_in_recovery()" + const valQuery = "SELECT val FROM foo WHERE id = 1" + + var isInRecovery bool + require.NoError(t, conn.QueryRow(ctx, isInRecoveryQuery).Scan(&isInRecovery)) + require.True(t, isInRecovery) + + var val string + require.NoError(t, conn.QueryRow(ctx, valQuery).Scan(&val)) + require.Equal(t, "foo", val) + }) +} diff --git a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.json b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.json index 1749aca7..c16a14f7 100644 --- a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.json +++ b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.json @@ -15,21 +15,13 @@ "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "update", "resource_id": "database.node::n1", - "reason": "has_diff", - "diff": [ - { - "value": "n1-instance-2-id", - "op": "add", - "path": "/instance_ids/-" - } - ] + "reason": "dependency_updated", + "diff": null } ], [ @@ -61,21 +53,21 @@ "reason": "dependency_updated", "diff": null } - ], + ] + ], + [ [ { "type": "create", - "resource_id": "database.switchover::n1-instance-1-id", + "resource_id": "monitor.instance::n1-instance-2-id", "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "create", - "resource_id": "monitor.instance::n1-instance-2-id", + "resource_id": "database.switchover::n1-instance-1-id", "reason": "does_not_exist", "diff": null } diff --git a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica_with_primary_update.json b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica_with_primary_update.json new file mode 100644 index 00000000..a1ad69e6 --- /dev/null +++ b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica_with_primary_update.json @@ -0,0 +1,138 @@ +[ + [ + [ + { + "type": "create", + "resource_id": "orchestrator.resource::n1-instance-1-dep-2-id", + "reason": "does_not_exist", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.instance::n1-instance-1-id", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.node::n1", + "reason": "dependency_updated", + "diff": null + }, + { + "type": "update", + "resource_id": "monitor.instance::n1-instance-1-id", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.postgres_database::n1:test", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.replication_slot::n1:n2:test", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.subscription::n1:n2:test", + "reason": "dependency_updated", + "diff": null + }, + { + "type": "update", + "resource_id": "database.subscription::n2:n1:test", + "reason": "dependency_updated", + "diff": null + } + ] + ], + [ + [ + { + "type": "create", + "resource_id": "orchestrator.resource::n1-instance-2-dep-1-id", + "reason": "does_not_exist", + "diff": null + } + ], + [ + { + "type": "create", + "resource_id": "database.instance::n1-instance-2-id", + "reason": "does_not_exist", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.node::n1", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.postgres_database::n1:test", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.replication_slot::n1:n2:test", + "reason": "dependency_updated", + "diff": null + } + ], + [ + { + "type": "update", + "resource_id": "database.subscription::n1:n2:test", + "reason": "dependency_updated", + "diff": null + }, + { + "type": "update", + "resource_id": "database.subscription::n2:n1:test", + "reason": "dependency_updated", + "diff": null + } + ] + ], + [ + [ + { + "type": "create", + "resource_id": "monitor.instance::n1-instance-2-id", + "reason": "does_not_exist", + "diff": null + } + ], + [ + { + "type": "create", + "resource_id": "database.switchover::n1-instance-1-id", + "reason": "does_not_exist", + "diff": null + } + ] + ] +] \ No newline at end of file diff --git a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.json b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.json index 4a2f78c6..5590eb51 100644 --- a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.json +++ b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.json @@ -27,33 +27,19 @@ "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "update", "resource_id": "database.node::n1", - "reason": "has_diff", - "diff": [ - { - "value": "n1-instance-2-id", - "op": "add", - "path": "/instance_ids/-" - } - ] + "reason": "dependency_updated", + "diff": null }, { "type": "update", "resource_id": "database.node::n2", - "reason": "has_diff", - "diff": [ - { - "value": "n2-instance-2-id", - "op": "add", - "path": "/instance_ids/-" - } - ] + "reason": "dependency_updated", + "diff": null } ], [ @@ -97,33 +83,33 @@ "reason": "dependency_updated", "diff": null } - ], + ] + ], + [ [ { "type": "create", - "resource_id": "database.switchover::n1-instance-1-id", + "resource_id": "monitor.instance::n1-instance-2-id", "reason": "does_not_exist", "diff": null }, { "type": "create", - "resource_id": "database.switchover::n2-instance-1-id", + "resource_id": "monitor.instance::n2-instance-2-id", "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "create", - "resource_id": "monitor.instance::n1-instance-2-id", + "resource_id": "database.switchover::n1-instance-1-id", "reason": "does_not_exist", "diff": null }, { "type": "create", - "resource_id": "monitor.instance::n2-instance-2-id", + "resource_id": "database.switchover::n2-instance-1-id", "reason": "does_not_exist", "diff": null } diff --git a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.json b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.json index 8b88961a..022d3516 100644 --- a/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.json +++ b/server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.json @@ -15,21 +15,13 @@ "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "update", "resource_id": "database.node::n1", - "reason": "has_diff", - "diff": [ - { - "value": "n1-instance-2-id", - "op": "add", - "path": "/instance_ids/-" - } - ] + "reason": "dependency_updated", + "diff": null } ], [ @@ -61,14 +53,6 @@ "reason": "dependency_updated", "diff": null } - ], - [ - { - "type": "create", - "resource_id": "database.switchover::n1-instance-1-id", - "reason": "does_not_exist", - "diff": null - } ] ], [ @@ -87,21 +71,13 @@ "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "update", "resource_id": "database.node::n2", - "reason": "has_diff", - "diff": [ - { - "value": "n2-instance-2-id", - "op": "add", - "path": "/instance_ids/-" - } - ] + "reason": "dependency_updated", + "diff": null } ], [ @@ -133,33 +109,33 @@ "reason": "dependency_updated", "diff": null } - ], + ] + ], + [ [ { - "type": "update", - "resource_id": "database.switchover::n1-instance-1-id", - "reason": "dependency_updated", + "type": "create", + "resource_id": "monitor.instance::n1-instance-2-id", + "reason": "does_not_exist", "diff": null }, { "type": "create", - "resource_id": "database.switchover::n2-instance-1-id", + "resource_id": "monitor.instance::n2-instance-2-id", "reason": "does_not_exist", "diff": null } - ] - ], - [ + ], [ { "type": "create", - "resource_id": "monitor.instance::n1-instance-2-id", + "resource_id": "database.switchover::n1-instance-1-id", "reason": "does_not_exist", "diff": null }, { "type": "create", - "resource_id": "monitor.instance::n2-instance-2-id", + "resource_id": "database.switchover::n2-instance-1-id", "reason": "does_not_exist", "diff": null } diff --git a/server/internal/database/operations/update_database.go b/server/internal/database/operations/update_database.go index 5cac9cbb..69b019cb 100644 --- a/server/internal/database/operations/update_database.go +++ b/server/internal/database/operations/update_database.go @@ -60,7 +60,7 @@ func UpdateDatabase( // is available to be a source node. var states []*resource.State if len(updates) > 0 { - u, err := update(updates) + u, err := update(start, updates) if err != nil { return nil, err } @@ -126,7 +126,7 @@ func partitionNodes(start *resource.State, nodes []*NodeResources) ([]*NodeResou return updates, adds, nil } -func updateFunc(options UpdateDatabaseOptions) (func([]*NodeResources) ([]*resource.State, error), error) { +func updateFunc(options UpdateDatabaseOptions) (func(*resource.State, []*NodeResources) ([]*resource.State, error), error) { switch options.NodeUpdateStrategy { case "", NodeUpdateStrategyRolling: return RollingUpdateNodes, nil diff --git a/server/internal/database/operations/update_database_test.go b/server/internal/database/operations/update_database_test.go index c1ab528c..fbcd178c 100644 --- a/server/internal/database/operations/update_database_test.go +++ b/server/internal/database/operations/update_database_test.go @@ -341,6 +341,28 @@ func TestUpdateDatabase(t *testing.T) { }, }, }, + { + name: "adding a replica with primary update", + options: operations.UpdateDatabaseOptions{}, + start: twoNodeState, + // The primary instance should get updated before the replica is + // added. + nodes: []*operations.NodeResources{ + { + NodeName: "n1", + InstanceResources: []*database.InstanceResources{ + n1Instance1WithNewDependency, + n1Instance2, + }, + DatabaseName: "test", + }, + { + NodeName: "n2", + InstanceResources: []*database.InstanceResources{n2Instance1}, + DatabaseName: "test", + }, + }, + }, { name: "add an instance dependency with forced update", options: operations.UpdateDatabaseOptions{ diff --git a/server/internal/database/operations/update_nodes.go b/server/internal/database/operations/update_nodes.go index 03328267..951b4647 100644 --- a/server/internal/database/operations/update_nodes.go +++ b/server/internal/database/operations/update_nodes.go @@ -2,6 +2,7 @@ package operations import ( "fmt" + "slices" "github.com/pgEdge/control-plane/server/internal/database" "github.com/pgEdge/control-plane/server/internal/patroni" @@ -12,28 +13,32 @@ import ( // state only contains changed resources from the previous state. These states // should be cumulatively merged with previous states to produce the sequence of // desired states. This function always updates the node's replica instances. -func UpdateNode(node *NodeResources) ([]*resource.State, error) { - // Updates are performed on replica instances first +func UpdateNode(start *resource.State, node *NodeResources) ([]*resource.State, error) { var primary *resource.State - + var replicaUpdates, replicaAdds []*resource.State var primaryHostID string - states := make([]*resource.State, 0, len(node.InstanceResources)) - for _, inst := range node.InstanceResources { - instanceID := inst.InstanceID() + for _, inst := range node.InstanceResources { state, err := inst.InstanceState() if err != nil { return nil, err } - if instanceID == node.PrimaryInstanceID { + + switch { + case inst.InstanceID() == node.PrimaryInstanceID: + if !start.HasResources(inst.Instance.Identifier()) { + return nil, fmt.Errorf("invalid state: node %s exists, but its primary instance '%s' hasn't been created yet", node.NodeName, node.PrimaryInstanceID) + } primary = state primaryHostID = inst.HostID() - } else { - states = append(states, state) + case start.HasResources(inst.Instance.Identifier()): + replicaUpdates = append(replicaUpdates, state) + default: + replicaAdds = append(replicaAdds, state) } } if primary == nil { - // TODO(PLAT-240): This is another place where we assume that a node has + // TODO(PLAT-582): This is another place where we assume that a node has // a primary instance. We're returning an error here so that we don't // break downstream components that make the same assumption. We'll need // to change this if we want workflows to handle nodes without a primary @@ -41,8 +46,8 @@ func UpdateNode(node *NodeResources) ([]*resource.State, error) { return nil, fmt.Errorf("node %s has no primary instance", node.NodeName) } - // This condition is true when we have replicas - if len(states) != 0 { + // This condition is true when we have existing replicas + if len(replicaUpdates) != 0 { // Ensure that we always switch back to the original primary err := primary.AddResource(&database.SwitchoverResource{ HostID: primaryHostID, @@ -54,7 +59,10 @@ func UpdateNode(node *NodeResources) ([]*resource.State, error) { } } - states = append(states, primary) + // Existing replicas should be updated first and new replicas should be + // added last. + states := slices.Concat(replicaUpdates, []*resource.State{primary}, replicaAdds) + nodeState, err := node.nodeResourceState() if err != nil { return nil, err @@ -67,11 +75,11 @@ func UpdateNode(node *NodeResources) ([]*resource.State, error) { // RollingUpdateNodes returns a sequence of state diffs where each of the given // nodes is updated in-order sequentially. This function retains the // replica-first order from UpdateNode. -func RollingUpdateNodes(nodes []*NodeResources) ([]*resource.State, error) { +func RollingUpdateNodes(start *resource.State, nodes []*NodeResources) ([]*resource.State, error) { // Updates each node sequentially var states []*resource.State for _, node := range nodes { - update, err := UpdateNode(node) + update, err := UpdateNode(start, node) if err != nil { return nil, err } @@ -83,11 +91,11 @@ func RollingUpdateNodes(nodes []*NodeResources) ([]*resource.State, error) { // ConcurrentUpdateNodes returns a sequence of state diffs where each of the // given nodes is updated simultaneously. This function retains the // replica-first order from UpdateNode. -func ConcurrentUpdateNodes(nodes []*NodeResources) ([]*resource.State, error) { +func ConcurrentUpdateNodes(start *resource.State, nodes []*NodeResources) ([]*resource.State, error) { // Updates each node simultaneously states := make([][]*resource.State, len(nodes)) for i, node := range nodes { - update, err := UpdateNode(node) + update, err := UpdateNode(start, node) if err != nil { return nil, err } diff --git a/server/internal/database/operations/update_nodes_test.go b/server/internal/database/operations/update_nodes_test.go index 84109f0d..697652b8 100644 --- a/server/internal/database/operations/update_nodes_test.go +++ b/server/internal/database/operations/update_nodes_test.go @@ -19,7 +19,8 @@ func TestUpdateNode(t *testing.T) { for _, tc := range []struct { name string - input *operations.NodeResources + start *resource.State + node *operations.NodeResources expected []*resource.State expectedErr string }{ @@ -27,7 +28,17 @@ func TestUpdateNode(t *testing.T) { // When there's one instance, we should produce one state with the // instance and the node resource. name: "one instance", - input: &operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{instance1.InstanceID()}, + }, + }, + instance1.InstanceDependencies, + ), + node: &operations.NodeResources{ DatabaseName: "test", NodeName: "n1", PrimaryInstanceID: instance1.InstanceID(), @@ -51,7 +62,24 @@ func TestUpdateNode(t *testing.T) { // instance and a second state with the primary instance and the // node resource. name: "two instances", - input: &operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + instance1.Instance, + instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + instance1.InstanceID(), + instance2.InstanceID(), + }, + }, + }, + slices.Concat( + instance1.InstanceDependencies, + instance2.InstanceDependencies, + ), + ), + node: &operations.NodeResources{ DatabaseName: "test", NodeName: "n1", PrimaryInstanceID: instance1.InstanceID(), @@ -91,7 +119,27 @@ func TestUpdateNode(t *testing.T) { // With 3 instances, we should produce three states, where the last // state contains the primary instance and the node resource. name: "three instances", - input: &operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + instance1.Instance, + instance2.Instance, + instance3.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + instance1.InstanceID(), + instance2.InstanceID(), + instance3.InstanceID(), + }, + }, + }, + slices.Concat( + instance1.InstanceDependencies, + instance2.InstanceDependencies, + instance3.InstanceDependencies, + ), + ), + node: &operations.NodeResources{ DatabaseName: "test", NodeName: "n1", PrimaryInstanceID: instance1.InstanceID(), @@ -136,11 +184,58 @@ func TestUpdateNode(t *testing.T) { }, }, { - // TODO(PLAT-240): we need to decide how to handle this case. For + // New instances are processed after existing ones. + name: "adding a replica", + start: makeState(t, + []resource.Resource{ + instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + instance1.InstanceID(), + }, + }, + }, + instance1.InstanceDependencies, + ), + node: &operations.NodeResources{ + DatabaseName: "test", + NodeName: "n1", + PrimaryInstanceID: instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{ + instance1, + instance2, + }, + }, + expected: []*resource.State{ + makeState(t, + []resource.Resource{ + instance1.Instance, + }, + instance1.InstanceDependencies, + ), + makeState(t, + []resource.Resource{ + instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + instance1.InstanceID(), + instance2.InstanceID(), + }, + }, + }, + instance2.InstanceDependencies, + ), + }, + }, + { + // TODO(PLAT-582): we need to decide how to handle this case. For // now, this produces an error to avoid breaking downstream // components. - name: "no primary", - input: &operations.NodeResources{ + name: "no primary", + start: resource.NewState(), + node: &operations.NodeResources{ DatabaseName: "test", NodeName: "n1", InstanceResources: []*database.InstanceResources{ @@ -149,9 +244,22 @@ func TestUpdateNode(t *testing.T) { }, expectedErr: "node n1 has no primary instance", }, + { + name: "primary not created", + start: resource.NewState(), + node: &operations.NodeResources{ + DatabaseName: "test", + NodeName: "n1", + PrimaryInstanceID: instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{ + instance1, + }, + }, + expectedErr: "invalid state: node n1 exists, but its primary instance 'n1-instance-1-id' hasn't been created yet", + }, } { t.Run(tc.name, func(t *testing.T) { - out, err := operations.UpdateNode(tc.input) + out, err := operations.UpdateNode(tc.start, tc.node) if tc.expectedErr != "" { assert.Nil(t, out) assert.ErrorContains(t, err, tc.expectedErr) @@ -171,14 +279,25 @@ func TestRollingUpdateNodes(t *testing.T) { for _, tc := range []struct { name string - input []*operations.NodeResources + start *resource.State + nodes []*operations.NodeResources expected []*resource.State expectedErr string }{ { // This should look identical to the UpdateNode output. name: "one node with one instance", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + }, + n1Instance1.InstanceDependencies, + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -202,7 +321,25 @@ func TestRollingUpdateNodes(t *testing.T) { { // This should produce two states with one instance in each. name: "two nodes with one instance each", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -244,7 +381,30 @@ func TestRollingUpdateNodes(t *testing.T) { // primary instance and node resource, n2's instance and node // resource. name: "two nodes with one replica", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n1Instance2.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n1Instance2.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -302,7 +462,35 @@ func TestRollingUpdateNodes(t *testing.T) { // This should produce four states: n1's replica, n1's primary + // node, n2's replica, n2's primary + node. name: "two nodes with two replicas", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n1Instance2.Instance, + n2Instance1.Instance, + n2Instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{ + n2Instance1.InstanceID(), + n2Instance2.InstanceID(), + }, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n1Instance2.InstanceDependencies, + n2Instance1.InstanceDependencies, + n2Instance2.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -373,9 +561,81 @@ func TestRollingUpdateNodes(t *testing.T) { ), }, }, + { + // This should produce three states: n1's primary instance, n1's + // replica instance and node resource, n2's instance and node + // resource. + name: "two nodes with one new replica", + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ + { + DatabaseName: "test", + NodeName: "n1", + PrimaryInstanceID: n1Instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{ + n1Instance1, + n1Instance2, + }, + }, + { + DatabaseName: "test", + NodeName: "n2", + PrimaryInstanceID: n2Instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{n2Instance1}, + }, + }, + expected: []*resource.State{ + makeState(t, + []resource.Resource{ + n1Instance1.Instance, + }, + n1Instance1.InstanceDependencies, + ), + makeState(t, + []resource.Resource{ + n1Instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + }, + n1Instance2.InstanceDependencies, + ), + makeState(t, + []resource.Resource{ + n2Instance1.Instance, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + n2Instance1.InstanceDependencies, + ), + }, + }, } { t.Run(tc.name, func(t *testing.T) { - out, err := operations.RollingUpdateNodes(tc.input) + out, err := operations.RollingUpdateNodes(tc.start, tc.nodes) if tc.expectedErr != "" { assert.Nil(t, out) assert.ErrorContains(t, err, tc.expectedErr) @@ -395,14 +655,25 @@ func TestConcurrentUpdateNodes(t *testing.T) { for _, tc := range []struct { name string - input []*operations.NodeResources + start *resource.State + nodes []*operations.NodeResources expected []*resource.State expectedErr string }{ { // This should look identical to the UpdateNode output. name: "one node with one instance", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + }, + n1Instance1.InstanceDependencies, + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -426,7 +697,25 @@ func TestConcurrentUpdateNodes(t *testing.T) { { // This should produce one state with both instances/nodes. name: "two nodes with one instance each", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -466,7 +755,30 @@ func TestConcurrentUpdateNodes(t *testing.T) { // primary instance + node, followed by n1's primary instance and // node resource. name: "two nodes with one replica", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n1Instance2.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n1Instance2.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -522,7 +834,35 @@ func TestConcurrentUpdateNodes(t *testing.T) { // This should produce two states: n1's replica and n2's replica, // followed by n1's primary + node and n2's primary + node. name: "two nodes with two replicas", - input: []*operations.NodeResources{ + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n1Instance2.Instance, + n2Instance1.Instance, + n2Instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{ + n2Instance1.InstanceID(), + n2Instance2.InstanceID(), + }, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n1Instance2.InstanceDependencies, + n2Instance1.InstanceDependencies, + n2Instance2.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ { DatabaseName: "test", NodeName: "n1", @@ -589,9 +929,158 @@ func TestConcurrentUpdateNodes(t *testing.T) { ), }, }, + { + // This should produce two states: n1's primary instance + n2's + // instance and node resource, n1's replica instance and node + // resource. + name: "two nodes with one new replica", + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ + { + DatabaseName: "test", + NodeName: "n1", + PrimaryInstanceID: n1Instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{ + n1Instance1, + n1Instance2, + }, + }, + { + DatabaseName: "test", + NodeName: "n2", + PrimaryInstanceID: n2Instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{n2Instance1}, + }, + }, + expected: []*resource.State{ + makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + makeState(t, + []resource.Resource{ + n1Instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + }, + n1Instance2.InstanceDependencies, + ), + }, + }, + { + // This should produce two states: n1 and n2's primary instances, n1 + // and n2's replica instances and node resources. + name: "two nodes with two new replicas", + start: makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{n1Instance1.InstanceID()}, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{n2Instance1.InstanceID()}, + }, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + nodes: []*operations.NodeResources{ + { + DatabaseName: "test", + NodeName: "n1", + PrimaryInstanceID: n1Instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{ + n1Instance1, + n1Instance2, + }, + }, + { + DatabaseName: "test", + NodeName: "n2", + PrimaryInstanceID: n2Instance1.InstanceID(), + InstanceResources: []*database.InstanceResources{ + n2Instance1, + n2Instance2, + }, + }, + }, + expected: []*resource.State{ + makeState(t, + []resource.Resource{ + n1Instance1.Instance, + n2Instance1.Instance, + }, + slices.Concat( + n1Instance1.InstanceDependencies, + n2Instance1.InstanceDependencies, + ), + ), + makeState(t, + []resource.Resource{ + n1Instance2.Instance, + n2Instance2.Instance, + &database.NodeResource{ + Name: "n1", + InstanceIDs: []string{ + n1Instance1.InstanceID(), + n1Instance2.InstanceID(), + }, + }, + &database.NodeResource{ + Name: "n2", + InstanceIDs: []string{ + n2Instance1.InstanceID(), + n2Instance2.InstanceID(), + }, + }, + }, + slices.Concat( + n1Instance2.InstanceDependencies, + n2Instance2.InstanceDependencies, + ), + ), + }, + }, } { t.Run(tc.name, func(t *testing.T) { - out, err := operations.ConcurrentUpdateNodes(tc.input) + out, err := operations.ConcurrentUpdateNodes(tc.start, tc.nodes) if tc.expectedErr != "" { assert.Nil(t, out) assert.ErrorContains(t, err, tc.expectedErr)