feat: Parse `kopia snapshot restore` progress output. #2776

e-sumin · 2024-03-21T11:08:56Z

Change Overview

Kopia does have progress reporting for restore operation (when using CLI), which is slightly different from snapshotting progress. This PR contains logic for parsing this progress. It will be used for parsing snapshot restore progress and will be propagated to progress stored in ActionSet status.

Pull request type

Please check the type of change your PR introduces:

Issues

fixes #issue-number

Test Plan

💪 Manual
⚡ Unit test
💚 E2E

pkg/kopia/command/parse_command_output.go

Reformat comments Co-authored-by: Pavan Navarathna <[email protected]>

viveksinghggits · 2024-05-17T13:27:55Z

pkg/kopia/command/parse_command_output.go

+// SnapshotStatsFromSnapshotRestore parses the output of a `kopia snapshot
+// restore` execution for a log of the stats for that execution.


sorry, I was not able to understand this comment properly, especially the part execution for a log of the stats for that execution..

The comment was done in same styling (wording) as in similar function:
https://github.com/kanisterio/kanister/pull/2776/files#diff-00217f95488de292da931fcdc8bdc46e611d503b26e17e13119cb4ce04548892R214-R215

In both cases it means that we have output of some command, we are parsing it and trying to find stats logged by command.

Possible rephrasing (for both cases) :
// XXX parses the output of 'ZZZZZ' line-by-line in search of progress statistics.
// It returns nil if no statistics are found, or the most recent statistic if multiple are encountered.

WDYT ?

sounds good.

Replaced for both SnapshotStatsFromSnapshotCreate and RestoreStatsFromRestoreOutput

viveksinghggits · 2024-05-17T13:30:03Z

pkg/kopia/command/parse_command_output.go

@@ -327,6 +331,93 @@ func parseKopiaProgressLine(line string, matchOnlyFinished bool) (stats *Snapsho
 	}
 }

+// SnapshotRestoreStats is a container for stats parsed from the output of a
+// `kopia snapshot restore` command.
+type SnapshotRestoreStats struct {


can it just be called RestoreStats? i quickly checked what kopia restore command is called and its kopia restore so I think just RestoreStats should also be ok.

But please feel free to disagree.

Actually I was always using kopia snapshot restore but I've just checked documentation, and you are right. Seems that snapshot restore is an alias to restore. So your suggestion makes sense.

viveksinghggits · 2024-05-17T13:30:44Z

pkg/kopia/command/parse_command_output.go

+
+// SnapshotStatsFromSnapshotRestore parses the output of a `kopia snapshot
+// restore` execution for a log of the stats for that execution.
+func SnapshotStatsFromSnapshotRestore(snapRestoreStderrOutput string) (stats *SnapshotRestoreStats) {


Suggested change

func SnapshotStatsFromSnapshotRestore(snapRestoreStderrOutput string) (stats *SnapshotRestoreStats) {

func RestoreStatsFromRestoreOP(snapRestoreStderrOutput string) (stats *SnapshotRestoreStats) {

maybe this or similar?

I've renamed it to RestoreStatsFromRestoreOutput to make it as straightforward as possible.

viveksinghggits · 2024-05-17T13:33:21Z

pkg/kopia/command/parse_command_output.go

+}
+
+func parseKopiaSnapshotRestoreProgressLine(line string) (stats *SnapshotRestoreStats) {
+	match := kopiaSnapshotRestorePattern.FindStringSubmatch(line)


I think it would be a good idea if we can also share (in comments) how exactly this line looks like.

viveksinghggits

can be merged after addressing small nits.

viveksinghggits · 2024-05-20T11:32:59Z

pkg/kopia/command/parse_command_output.go

+}
+
+// RestoreStatsFromRestoreOutput parses the output of a `kopia restore`
+// execution for a log of the stats for that execution.


Suggested change

// execution for a log of the stats for that execution.

// and figures out the progress stats for that operation.

?

I've rephrased it according to my previous suggestion: #2776 (comment)

viveksinghggits · 2024-05-20T11:33:45Z

pkg/kopia/command/parse_command_output.go

+// parseKopiaSnapshotRestoreProgressLine parses restore stats from line
+// which expected to be in the following format:
+// Processed 5 (1.4 GB) of 5 (1.8 GB) 291.1 MB/s (75.2%) remaining 1s.
+func parseKopiaSnapshotRestoreProgressLine(line string) (stats *RestoreStats) {


is snapshotrestore required here? or just kopiarestore would be enough?

viveksinghggits · 2024-05-20T11:36:03Z

pkg/kopia/command/parse_command_output.go

+// parseKopiaSnapshotRestoreProgressLine parses restore stats from line
+// which expected to be in the following format:
+// Processed 5 (1.4 GB) of 5 (1.8 GB) 291.1 MB/s (75.2%) remaining 1s.


Suggested change

// parseKopiaSnapshotRestoreProgressLine parses restore stats from line

// which expected to be in the following format:

// Processed 5 (1.4 GB) of 5 (1.8 GB) 291.1 MB/s (75.2%) remaining 1s.

// parseKopiaSnapshotRestoreProgressLine parses restore stats from the output log line,

// which is expected to be in the following format:

// Processed 5 (1.4 GB) of 5 (1.8 GB) 291.1 MB/s (75.2%) remaining 1s.

Accepted, but due to renamed function, in a separate commit

pkg/kopia/command/parse_command_output.go

Co-authored-by: Vivek Singh <[email protected]>

Parse kopia snapshot restore progress output.

f7fad5f

e-sumin requested review from hairyhum, PrasadG193 and viveksinghggits March 21, 2024 11:08

pavannd1 requested review from ankitjain235 and redgoat650 and removed request for viveksinghggits and hairyhum March 26, 2024 22:31

e-sumin requested a review from viveksinghggits May 15, 2024 09:56

pavannd1 reviewed May 15, 2024

View reviewed changes

pkg/kopia/command/parse_command_output.go Outdated Show resolved Hide resolved

pkg/kopia/command/parse_command_output.go Outdated Show resolved Hide resolved

pkg/kopia/command/parse_command_output.go Outdated Show resolved Hide resolved

Apply suggestions from code review

7e4a04c

Reformat comments Co-authored-by: Pavan Navarathna <[email protected]>

e-sumin requested a review from pavannd1 May 15, 2024 23:20

Merge branch 'master' into kopia_progress_parsing

466fc65

e-sumin changed the title ~~Parse kopia snapshot restore progress output.~~ feat: Parse kopia snapshot restore progress output. May 16, 2024

viveksinghggits reviewed May 17, 2024

View reviewed changes

e-sumin added 3 commits May 20, 2024 06:24

Address review notes

f932ade

Address review notes

fc19368

Address review notes

cfefd78

e-sumin requested a review from viveksinghggits May 20, 2024 04:37

viveksinghggits approved these changes May 20, 2024

View reviewed changes

e-sumin and others added 3 commits May 20, 2024 20:36

Apply suggestions from code review

222b34b

Co-authored-by: Vivek Singh <[email protected]>

Address review notes

2547a8d

Rename SnapshotRestore to Restore

92c4a3a

e-sumin force-pushed the kopia_progress_parsing branch from 4d5aff7 to 92c4a3a Compare May 21, 2024 08:05

e-sumin added the kueue label May 21, 2024

Merge branch 'master' into kopia_progress_parsing

a9c5dfb

mergify bot merged commit 05db153 into master May 21, 2024
15 checks passed

mergify bot deleted the kopia_progress_parsing branch May 21, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Parse `kopia snapshot restore` progress output. #2776

feat: Parse `kopia snapshot restore` progress output. #2776

e-sumin commented Mar 21, 2024

viveksinghggits May 17, 2024

e-sumin May 20, 2024

viveksinghggits May 20, 2024

e-sumin May 20, 2024

viveksinghggits May 17, 2024

viveksinghggits May 17, 2024

e-sumin May 20, 2024

viveksinghggits May 17, 2024

e-sumin May 20, 2024

viveksinghggits May 17, 2024

e-sumin May 20, 2024

viveksinghggits left a comment

viveksinghggits May 20, 2024

e-sumin May 20, 2024

viveksinghggits May 20, 2024

viveksinghggits May 20, 2024

e-sumin May 20, 2024

		// SnapshotStatsFromSnapshotRestore parses the output of a `kopia snapshot
		// restore` execution for a log of the stats for that execution.

	func SnapshotStatsFromSnapshotRestore(snapRestoreStderrOutput string) (stats *SnapshotRestoreStats) {
	func RestoreStatsFromRestoreOP(snapRestoreStderrOutput string) (stats *SnapshotRestoreStats) {

	// execution for a log of the stats for that execution.
	// and figures out the progress stats for that operation.

feat: Parse kopia snapshot restore progress output. #2776

feat: Parse kopia snapshot restore progress output. #2776

Conversation

e-sumin commented Mar 21, 2024

Change Overview

Pull request type

Issues

Test Plan

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viveksinghggits 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Parse `kopia snapshot restore` progress output. #2776

feat: Parse `kopia snapshot restore` progress output. #2776