feat(restore): adds --dc-mapping flag to restore command #4213

VAveryanov8 · 2025-01-16T15:54:41Z

This adds support for --dc-mapping flag to restore command.
It specifies mapping between DCs from the backup and DCs in the restored(target) cluster.
All DCs from the source cluster should be explicitly mapped to all DCs in the target cluster. The only exception is when
source and target cluster has exact match: source dcs == target dcs.
Only works with tables restoration (--restore-tables=true).

Also by default now each dc is downloading only data from corresponding dc(s)accordingly to user provided mapping.

Syntax:
"source_dc1,source_dc2=>target_dc1,target_dc2"
Multiple mappings are separated by semicolons (;). Exclamation mark (!) before a DC indicates that it should be ignored during restore.

Examples:

"dc1,dc2=>dc3"      - data from dc1 and dc2 DCs should be restored to dc3 DC.
"dc1,dc2=>dc3,!dc4" - data from dc1 and dc2 DCs should be restored to dc3 DC. Ignoring dc4 DC from target cluster.
"dc1,!dc2=>dc2"     - data from dc1 should be restored to dc2 DC. Ignoring dc2 from source cluster.

Fixes: #3829

Please make sure that:

Code is split to commits that address a single change
Commit messages are informative
Commit titles have module prefix
Commit titles have issue nr. suffix

This adds support for --dc-mapping flag to restore command. It specifies mapping between DCs from the backup and DCs in the restored(target) cluster. All DCs from the source cluster should be explicitly mapped to all DCs in the target cluster. The only exception is when source and target cluster has exact match: source dcs == target dcs. Only works with tables restoration (--restore-tables=true). Syntax: "source_dc1,source_dc2=>target_dc1,target_dc2" Multiple mappings are separated by semicolons (;). Exclamation mark (!) before a DC indicates that it should be ignored during restore. Examples: "dc1,dc2=>dc3" - data from dc1 and dc2 DCs should be restored to dc3 DC. "dc1,dc2=>dc3,!dc4" - data from dc1 and dc2 DCs should be restored to dc3 DC. Ignoring dc4 DC from target cluster. "dc1,!dc2=>dc2" - data from dc1 should be restored to dc2 DC. Ignoring dc2 from source cluster. Fixes: #3829

This introduces use of dc mappings when restoring tables. Now each dc is downloading only data from corresponding dc(s) accordingly to user provided mapping. Also some dcs can be explicitly ignored. Fixes: #3829

This adds another cluster to docker setup, so we can have integration tests for dc-mappings. Fixes: #3829

Fixes: #3829

Michal-Leszczynski · 2025-01-17T08:54:10Z

In terms of syntax:

"dc1,!dc2=>dc2" - data from dc1 should be restored to dc2 DC. Ignoring dc2 from source cluster.

I find it confusing that it's possible to specify both restored and skipped DCs in the same mapping key.
It reads like "restore dc1 and not dc2 into dc2" instead of "don't restore dc2 and restore dc1 into dc2".
IMHO it should look like:

 "!dc2;dc1=>dc2"     - data from dc1 should be restored to dc2 DC. Ignoring dc2 from source cluster.

I would suggest to validate that positive and negative DC occurrences are not mixed in the mapping key and that negative DCs can't be mapped to anything, so that we avoid confusion and typing mistakes.

EDIT: The same goes here:

"dc1,dc2=>dc3,!dc4" - data from dc1 and dc2 DCs should be restored to dc3 DC. Ignoring dc4 DC from target cluster.

The negative DC is placed in the mapping value alongside positive DC?
Now I see that we can't simply allow to write !dc1 because it's ambiguous to whether it's a source or destination DC.

But I'm still in favor of not mixing positive and negative DCs here.
I could look like that:

> "=>!dc4;dc1,dc2=>dc3" - data from dc1 and dc2 DCs should be restored to dc3 DC. Ignoring dc4 DC from target cluster.

On the other hand, if we allow no DCs on either side of => then the ! is not needed anymore, but it's still nice for visibility.
What do you think about removing the ! in front of DC from the syntax and incorporating it into the =>? Something like:

"dc1!=>dc2,dc3;dc2=>dc1"- we want to skip dc1 in src, dc2 and dc3 in dst, and restore from dc2 in src into dc1 in dst

Alternative approach would be to add some special characters for distinguishing between src and dst DCs, but I guess that the relation to the => already takes care of that.

Michal-Leszczynski · 2025-01-17T09:32:35Z

pkg/command/restore/res.yaml

@@ -72,3 +72,16 @@ dry-run: |

 show-tables: |
  Prints table names together with keyspace, used in combination with --dry-run.
+
+dc-mapping: |


The sctool command docs are actually also a part of the regular SM docs.
In order to include those changes in the docs, you should run make docs from repo root dir. In order to see how the changes were rendered, you can run make -C docs preview(or justmake previewfromdocs` dir).

Michal-Leszczynski · 2025-01-17T09:44:18Z

pkg/command/restore/dcmappings_test.go

+			slices.EqualFunc(tc.expectedMappings, mappings, func(a, b dcMapping) bool {
+				return slices.Equal(a.Source, b.Source) &&
+					slices.Equal(a.Target, b.Target) &&
+					slices.Equal(a.IgnoreSource, b.IgnoreSource) &&
+					slices.Equal(a.IgnoreTarget, b.IgnoreTarget)
+			})


I think that you forgot to check the result of the slices.EqualFunc:)

Michal-Leszczynski · 2025-01-17T09:45:42Z

pkg/command/restore/dcmappings_test.go

+			input: "dc1, dc2=>dc1, dc2",
+			expectedMappings: dcMappings{
+				{Source: []string{"dc1", "dc2"}, Target: []string{"dc1", "dc2"}},
+			},


This test case does not pass when the result is validated.
I don't see any code handling white spaces during the parsing, but it would be nice to add it.

Michal-Leszczynski · 2025-01-17T09:47:42Z

pkg/command/restore/cmd.go

+		if cmd.Update() {
+			return wrapper("dc-mapping")
+		}
+		props["dc-mapping"] = cmd.dcMapping


We are using the convention of naming json fields with _ instead of -, but we use - in the flag names:)
It can cause confusion and name mismatchs, but I think that we should follow it since its present in all of the flag names.

Michal-Leszczynski · 2025-01-17T10:12:44Z

pkg/service/restore/batch.go

+			dcsInLoc := locationDC[loc.StringWithoutDC()]
+			hostAllDCs := hostDCs[h]
+			hostDCAccess[h] = append(hostDCAccess[h], strset.Intersection(strset.New(dcsInLoc...), strset.New(hostAllDCs...)).List()...)


I don't think that we should just make an intersection here.
If someone specified that given DC should be restored by given DC, then all hosts from this DC should have access to the appropriate location. This should be done inside the GetTarget initial validation.

Michal-Leszczynski · 2025-01-17T10:19:55Z

pkg/service/restore/model.go

+	DCMappings      DCMappings `json:"dc-mapping"`

 	// Cache for host with access to remote location
 	locationHosts map[Location][]string `json:"-"`
+	// Cache for host and their DC after applying DCMappings
+	hostDCs map[string][]string
+	// Cache for dcs that shouldn't be restored from location
+	ignoredSourceDC []string


Wouldn't it be way simpler if we just had a single mapping map[string][string] of dst DC -> src DC list?
It could replace all locationHosts, hostDCs, ignoredSourceDC.
During batch dispatch we could/should have access to given hosts DC, so it would give us enough information for correctness.
Ignored DCs could be replaced with restored DCs which could be easily obtained from such mapping when needed for filtering.

The assumption is that we don't need to care for cases like: what if only a few hosts from this DC have access to this location. Even when making backup, the whole DC needs to be backed up to a single location and all hosts from given DC needs to have access to it.

Michal-Leszczynski · 2025-01-17T10:22:41Z

pkg/service/restore/worker.go

+	// The only way to have a list of dcs from the source cluster is to
+	// get them from backup locations.
+	sourceDC, err := w.collectDCFromLocation(ctx, t.locationHosts)


Correct, but we are already going through all of the manifests in the GetTarget (initUnits), so we should probably get all of the interesting data within a single iteration.

Michal-Leszczynski · 2025-01-17T10:31:45Z

Makefile

@@ -115,6 +115,7 @@ INTEGRATION_TEST_ARGS := -cluster $(PUBLIC_NET)100 \
 -managed-cluster $(PUBLIC_NET)11,$(PUBLIC_NET)12,$(PUBLIC_NET)13,$(PUBLIC_NET)21,$(PUBLIC_NET)22,$(PUBLIC_NET)23 \
 -test-network $(PUBLIC_NET) \
 -managed-second-cluster $(PUBLIC_NET)31,$(PUBLIC_NET)32 \
+-managed-third-cluster $(PUBLIC_NET)41,$(PUBLIC_NET)42 \


Having 3 clusters for the docker setup seems excessive.
You have written nice unit tests, so we don't need to cover all scenarios with integration tests.
Having said that, could we simply rename the DC in the second cluster (used just for the restore)?
We could even divide this cluster into 2 single nodes DCs if that could help.

Michal-Leszczynski · 2025-01-17T10:35:44Z

@VAveryanov8 I made a general review, but I didn't dive into other details, because it would be better to first discuss the current comments (other things might not matter after that).

Michal-Leszczynski · 2025-01-17T10:53:41Z

Also, we should deprecate the datacenter from the --location flag, as it was previously used to partially control the dc mapping. When dc mapping flag is present, it should be ignored. When it's not, it can still be respected, but in the implementation we would still just parse it into dc mapping on the SM side.

VAveryanov8 added 6 commits January 16, 2025 16:33

feat(restore): uses dc mappings for restoring tables

532ad04

This introduces use of dc mappings when restoring tables. Now each dc is downloading only data from corresponding dc(s) accordingly to user provided mapping. Also some dcs can be explicitly ignored. Fixes: #3829

feat(tests): adds third cluster (dc3) to docker setup

4622a83

This adds another cluster to docker setup, so we can have integration tests for dc-mappings. Fixes: #3829

chore(test): adds dc-mapping integration tests

f0229b4

Fixes: #3829

fix: adds copyright :D

e03889c

fix(test): fixes cluster setup

e30e28a

VAveryanov8 marked this pull request as ready for review January 17, 2025 08:23

VAveryanov8 requested review from karol-kokoszka and Michal-Leszczynski as code owners January 17, 2025 08:23

Michal-Leszczynski reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(restore): adds --dc-mapping flag to restore command #4213

feat(restore): adds --dc-mapping flag to restore command #4213

VAveryanov8 commented Jan 16, 2025 •

edited

Loading

Michal-Leszczynski commented Jan 17, 2025 •

edited

Loading

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski Jan 17, 2025

Michal-Leszczynski commented Jan 17, 2025

Michal-Leszczynski commented Jan 17, 2025

feat(restore): adds --dc-mapping flag to restore command #4213

Are you sure you want to change the base?

feat(restore): adds --dc-mapping flag to restore command #4213

Conversation

VAveryanov8 commented Jan 16, 2025 • edited Loading

Michal-Leszczynski commented Jan 17, 2025 • edited Loading

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

Choose a reason for hiding this comment

Michal-Leszczynski commented Jan 17, 2025

Michal-Leszczynski commented Jan 17, 2025

VAveryanov8 commented Jan 16, 2025 •

edited

Loading

Michal-Leszczynski commented Jan 17, 2025 •

edited

Loading