feat(1-1-restore): adds 1-1-restore cli and service stub #4221

VAveryanov8 · 2025-01-20T11:55:04Z

This adds 1-1 restore cli and stubs.
For now cli just schedules the 1-1 restore task type and when task is executed, service logs "Not yet implemented".

1-1-restore is added as a subcommand to the restore cli, so it looks like below:

$ sctool.dev restore 1-1-restore -c my-cluster --source-cluster-id 0ee498b4-d6b6-4f92-9c19-3118b72d925f --nodes-mapping pkg/command/one2onerestore/testdata/nodesmapping_oneline.txt -L s3:test-bucket -T sm_20241122172732UTC

Note: I've temporarily used v3/pkg/managerclient from the va/fast-restore-part-1 branch as requried changed not in the master yet

P.S. Feel free to suggest another name for this cli and service 😄

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

pkg/command/one2onerestore/cmd.go

pkg/command/one2onerestore/nodesmapping_test.go

pkg/command/one2onerestore/res.yaml

karol-kokoszka · 2025-01-29T11:27:15Z

pkg/service/one2onerestore/model.go

+	SourceClusterID string        `json:"source_cluster_id"`
+	SnapshotTag     string        `json:"snapshot_tag"`
+	NodesMapping    []nodeMapping `json:"nodes_mapping"`
+	BatchSize       int           `json:"batch_size,omitempty"`


I don't think batch size is needed, it's ok to call RClone to copy all SSTables of the given keyspace/table of a given node at once.
RClone tracks the progress on its own, SM just calls for the progress using jobID.

karol-kokoszka · 2025-01-29T11:33:47Z

pkg/service/one2onerestore/model.go

+	Parallel        int           `json:"parallel,omitempty"`
+	Transfers       int           `json:"transfers"`
+	RateLimit       []DCLimit     `json:"rate_limit,omitempty"`
+	AllowCompaction bool          `json:"allow_compaction,omitempty"`


We don;t need this parameter.
Compaction should be disabled to not waste CPU cycles and disk IO.
But it doesn't have an impact on the correctness of the restore.

This adds a new task type for fast vnode restore. Fixes: #4200

This introduces new cli command for fast vnode restore procedure. Fixes: #4200

This adds support for the fast restore task type to the task scheduler

This is the result of running `make docs`

This renames fastrestore to 1-1 restore and also makes it a subcommand of restore cmd.

This describes what 'same topology' means. Also adds references to new subcommands to the 'restore' cli page.

…s uuid This removes 1-1-restore options that are not needed at the moment (we can added them later if nedded). Also changed source-cluster-id type to uuid.UUID

VAveryanov8 · 2025-01-29T15:54:19Z

@karol-kokoszka @Michal-Leszczynski
I've removed fields from Target that are not needed at the moment for 1-1-restore. (If it turns out that some of them are actually needed, I'll add them later.) and addressed your code review comments. Please review one more time

This dowgrades some of the deps that was updated by accident, so now it looks more like the master branch.

Michal-Leszczynski · 2025-01-31T10:12:12Z

pkg/command/one2onerestore/res.yaml

+  The `<dc>` parameter is optional. It allows you to specify the datacenter whose nodes will be used to restore the data
+  from this location in a multi-dc setting, it must match Scylla nodes datacenter.
+  By default, all live nodes are used to restore data from specified locations.
+
+  Note that specifying datacenters closest to backup locations might reduce download time of restored data.


I don't think that it applies to the 1-1-restore.
Since we have node mapping, we shouldn't allow to specify the location DC.

Michal-Leszczynski · 2025-01-31T10:14:09Z

pkg/service/one2onerestore/model.go

+	if _, err := SnapshotTagTime(t.SnapshotTag); err != nil {
+		return err
+	}


nit: you can use IsSnapshotTag instead.

Michal-Leszczynski · 2025-01-31T10:18:03Z

pkg/service/one2onerestore/model.go

+	if _, err := SnapshotTagTime(t.SnapshotTag); err != nil {
+		return err
+	}
+	if t.SourceClusterID.String() == "" {


nit: you can check if it's unset by t.SourceClusterID == uuid.Nil

Michal-Leszczynski · 2025-01-31T10:20:51Z

pkg/service/one2onerestore/service.go

+	"github.com/scylladb/scylla-manager/v3/pkg/util/uuid"
+)
+
+// Service for the 1-2-1 restore.


nit: let's keep the same naming convention (1-1-restore or one2onerestore).
I guess that we prefer the 1-1-restore and use it when dashes are not a problem.

Michal-Leszczynski · 2025-01-31T10:22:19Z

pkg/service/one2onerestore/service.go

+// One2OneRestore creates and initializes worker performing 1-2-1 vnode restore with given properties.
+func (s *Service) One2OneRestore(ctx context.Context, clusterID, taskID, runID uuid.UUID, properties json.RawMessage) error {
+	s.logger.Info(ctx, "1-1 restore",


nit: align 1-1-restore naming convention (also in the log).

Michal-Leszczynski · 2025-01-31T10:23:52Z

pkg/service/scheduler/model.go

@@ -27,6 +27,7 @@ const (
 	UnknownTask        TaskType = "unknown"
 	BackupTask         TaskType = "backup"
 	RestoreTask        TaskType = "restore"
+	One2OneRestoreTask TaskType = "1_1_restore"


nit: yet another version of writing the 1-1-restore.

This adds usage of IsSnapshotTag and uuid.Nil. Also aligns 1-1-restore service naming convention across the code, comment and docs.

karol-kokoszka

Thanks !
I have no major concerns.

karol-kokoszka · 2025-02-03T11:59:40Z

pkg/cmd/scylla-manager/server.go

+	healthSvc         *healthcheck.Service
+	backupSvc         *backup.Service
+	restoreSvc        *restore.Service
+	one2OneRestoreSvc *one2onerestore.Service


Can you define the interface for the one2OneRestoreSvc.Service (like Servicer) and reference it here instead of the struct ?

sure, I can easily do this, but I'd like to clarify one detail in advance. I would like to define interface in scylla-manger/server.go package, but I see that in the SM code base it's often other way around (e.g ConfigCacher defined in configcache pkg). i think it's better to define interfaces on the consumer side https://go.dev/wiki/CodeReviewComments#interfaces

I understand that the practice in go is to define the interface in the package that consumes the service, but here, the interface defines the API for the whole service which definition and implementation stays in the "one2onerestore" package.
So, for the clarity of what this package is expected to do, I prefer to have this interface that defines the API.

Returning interface from the constructor aligns with the intention to make the service’s API explicit and maintain a clear package boundary, even though it deviates a bit from the “accept interfaces, return concrete types” idiom.

Btw, packages like net/http http.Handler (defined in the same package).
Another example of the same approach (breaking the idiom), is in go-kit https://github.com/go-kit/examples/blob/master/addsvc/pkg/addservice/service.go

I understand that the practice in go is to define the interface in the package that consumes the service, but here, the interface defines the API for the whole service which definition and implementation stays in the "one2onerestore" package.
So, for the clarity of what this package is expected to do, I prefer to have this interface that defines the API.

That make sense and it is a good intention, but why public methods of a Service struct doesn't define API and boundaries of a "one2onerestore" package?

In provided examples, they all have different pattern - interface is public, but implementation is private. In std lib it make sense - when you promise backward compatibility you don't want to expose some helper structs that can be changed in near future, e.g. http.FileServer. (and they use this pattern only for some helper functions).
In go-kit they use public interface and private struct for the services (business logic), because they have a lot of wrappers around Service, to hide details about various transports (e.g. http, grpc) that can be used, see https://github.com/go-kit/examples/blob/master/addsvc/pkg/addendpoint/set.go#L34

P.S. I'm not trying to argue for the sake of arguing or to resist making the change, I'm modestly trying to understand and learn why we want to do that.

VAveryanov8 force-pushed the va/fast-restore-part-2 branch from f07e746 to 94d7777 Compare January 21, 2025 10:09

VAveryanov8 marked this pull request as ready for review January 21, 2025 14:40

VAveryanov8 requested review from karol-kokoszka and Michal-Leszczynski as code owners January 21, 2025 14:40

VAveryanov8 changed the title ~~feat(fastrestore): adds fastrestore cli and service stub~~ feat(fastrestore): adds 1-1-restore cli and service stub Jan 23, 2025

VAveryanov8 changed the title ~~feat(fastrestore): adds 1-1-restore cli and service stub~~ feat(1-1-restore): adds 1-1-restore cli and service stub Jan 23, 2025

Michal-Leszczynski reviewed Jan 27, 2025

View reviewed changes

karol-kokoszka reviewed Jan 29, 2025

View reviewed changes

VAveryanov8 added 2 commits January 29, 2025 14:46

feat(managerclient): adds new task type - fastrestore

b115ad5

This adds a new task type for fast vnode restore. Fixes: #4200

refactor: renames fastrestore to 1-1-restore

ad35965

VAveryanov8 mentioned this pull request Jan 29, 2025

feat(managerclient): adds new task type - 1_1_restore #4219

Merged

VAveryanov8 changed the base branch from master to va/fast-restore-part-1 January 29, 2025 14:16

VAveryanov8 added 13 commits January 29, 2025 15:28

chore(go.mod): updates to the latest version of pkg/managerclient

d412522

feat(fastrestore cli): adds new fastrestore cli to sctool

e7b9831

This introduces new cli command for fast vnode restore procedure. Fixes: #4200

feat(scheduler): adds fastrestore support to the task scheduler

640729c

This adds support for the fast restore task type to the task scheduler

feat(fastrestore): adds fastrestore service stub

fd70ab9

chore(docs): generates docs for the new cli command

239ba40

This is the result of running `make docs`

chore: adds empty line, comment here and there

fe216b5

refactor: renames fastrestore to 1-1 restore

7d7a323

This renames fastrestore to 1-1 restore and also makes it a subcommand of restore cmd.

chore(docs): make docs

28719f5

chore(docs): updates 1-1-restore command docs

04d23e2

This describes what 'same topology' means. Also adds references to new subcommands to the 'restore' cli page.

chore(comments): explains why seperate check for required args is needed

9180b45

chore(tests): adds golden file helpers for nodemapping tests

5ade183

refactor(1-1-restore): removed not needed options && src-cluster-id i…

24985b1

…s uuid This removes 1-1-restore options that are not needed at the moment (we can added them later if nedded). Also changed source-cluster-id type to uuid.UUID

chore(docs): make docs

7e9f877

VAveryanov8 force-pushed the va/fast-restore-part-2 branch from 37409c0 to 7e9f877 Compare January 29, 2025 14:35

chore(go.mod): downgrades some of the deps

36a3058

This dowgrades some of the deps that was updated by accident, so now it looks more like the master branch.

Michal-Leszczynski reviewed Jan 31, 2025

View reviewed changes

VAveryanov8 added 2 commits February 3, 2025 10:06

refactor(1-1-restore): adds small improvements here and there

595c6e6

This adds usage of IsSnapshotTag and uuid.Nil. Also aligns 1-1-restore service naming convention across the code, comment and docs.

chore(docs): generates docs (make docs)

cb3cc42

Base automatically changed from va/fast-restore-part-1 to 1-1-restore-feature-branch February 3, 2025 11:31

karol-kokoszka approved these changes Feb 3, 2025

View reviewed changes

VAveryanov8 requested a review from Michal-Leszczynski February 4, 2025 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(1-1-restore): adds 1-1-restore cli and service stub #4221

feat(1-1-restore): adds 1-1-restore cli and service stub #4221

VAveryanov8 commented Jan 20, 2025 •

edited

Loading

karol-kokoszka Jan 29, 2025

karol-kokoszka Jan 29, 2025

VAveryanov8 commented Jan 29, 2025

Michal-Leszczynski Jan 31, 2025

Michal-Leszczynski Jan 31, 2025

Michal-Leszczynski Jan 31, 2025

Michal-Leszczynski Jan 31, 2025

Michal-Leszczynski Jan 31, 2025

Michal-Leszczynski Jan 31, 2025

karol-kokoszka left a comment

karol-kokoszka Feb 3, 2025

VAveryanov8 Feb 4, 2025

karol-kokoszka Feb 4, 2025

VAveryanov8 Feb 4, 2025 •

edited

Loading

feat(1-1-restore): adds 1-1-restore cli and service stub #4221

Are you sure you want to change the base?

feat(1-1-restore): adds 1-1-restore cli and service stub #4221

Conversation

VAveryanov8 commented Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VAveryanov8 commented Jan 29, 2025

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

karol-kokoszka left a 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

VAveryanov8 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

VAveryanov8 commented Jan 20, 2025 •

edited

Loading

VAveryanov8 Feb 4, 2025 •

edited

Loading