Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Progress UI for the new optimized restore implementation #2003

Closed
wants to merge 22 commits into from

Conversation

Projects
None yet
6 participants
@ifedorenko
Copy link
Contributor

ifedorenko commented Sep 21, 2018

What is the purpose of this change? What does it change?

This PR implements periodic status report for the new optimized multithreaded restore command implementation. It also introduces new command-agnostic ui.ProgressUI interface, which is then used by restore, backup and check long-running commands.

Was the change discussed in an issue or in the forum before?

See #1719

Closes #426

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch from 1e875f8 to a84fbcc Sep 22, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 22, 2018

Codecov Report

Merging #2003 into master will decrease coverage by 4.88%.
The diff coverage is 21.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2003      +/-   ##
==========================================
- Coverage   50.76%   45.88%   -4.89%     
==========================================
  Files         176      180       +4     
  Lines       14188    14436     +248     
==========================================
- Hits         7203     6624     -579     
- Misses       5933     6793     +860     
+ Partials     1052     1019      -33
Impacted Files Coverage Δ
internal/ui/progress.go 0% <0%> (ø)
internal/ui/testing.go 0% <0%> (ø)
cmd/restic/global.go 22.3% <0%> (-4.97%) ⬇️
internal/checker/testing.go 0% <0%> (ø) ⬆️
internal/archiver/progress.go 0% <0%> (ø)
internal/ui/termstatus/status.go 3.84% <0%> (-0.08%) ⬇️
internal/restorer/filerestorer.go 76.38% <100%> (+0.33%) ⬆️
internal/restorer/progress.go 16.66% <16.66%> (ø)
internal/checker/progress.go 36.66% <36.66%> (ø)
internal/restorer/restorer.go 43.39% <45.45%> (+1.11%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1107eef...c5b4d4d. Read the comment docs.

@ifedorenko ifedorenko referenced this pull request Sep 22, 2018

Merged

new optimized multithreaded restore implementation #1719

6 of 7 tasks complete

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch 3 times, most recently from 518e240 to 53614f1 Sep 25, 2018

@fd0 fd0 referenced this pull request Oct 3, 2018

Open

Progress bar for restore #426

@mholt

This comment has been minimized.

Copy link
Contributor

mholt commented Oct 5, 2018

Ooo this looks really nice. Do you think the ProgressUI interface would help a lot with #1944?

And would you be open to having --json flag support for this, similar to #1944? I could potentially contribute it, just need some guidance as to how.

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 5, 2018

My idea here is to decouple what progress is being reported from how the report is being rendered to the user. So yes, --json does look like a good usecase. There is some rudimentary support for different rendering in TermstatusProgressUI.displayProgress already (i.e. ansi-vs-dumb terminal stuff), but I think --json will require something more elaborate. I'll need to think about it before I can suggest anything more specific.

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch from 53614f1 to 037d750 Oct 12, 2018

return eta
}

func (c *CounterTo) FormatETA(now time.Time) string {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.FormatETA should have comment or be unexported

return c.target
}

func (c *CounterTo) ETA(now time.Time) time.Duration {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.ETA should have comment or be unexported

return c.current
}

func (c *CounterTo) Target() uint64 {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.Target should have comment or be unexported

return fmt.Sprintf("%3.2f%%", c.Percent())
}

func (c *CounterTo) Value() uint64 {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.Value should have comment or be unexported

return percent
}

func (c *CounterTo) FormatPercent() string {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.FormatPercent should have comment or be unexported

c.current += uint64(count)
}

func (c *CounterTo) Percent() float64 {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.Percent should have comment or be unexported

return CounterTo{start: start, target: target}
}

func (c *CounterTo) Add(count uint) {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported method CounterTo.Add should have comment or be unexported

target, current uint64 // XXX unsigned integers wrap at zero, it's bad
}

func StartCountTo(start time.Time, target uint64) CounterTo {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 12, 2018

exported function StartCountTo should have comment or be unexported

@fd0 fd0 force-pushed the ifedorenko:mt-restorer-ui branch from 037d750 to 694e63b Oct 14, 2018

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Oct 14, 2018

I've taken the liberty of rebasing this branch on master, thereby removing the commits that are already in master :)

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Oct 14, 2018

I just gave this a try, the UI looks nice.

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 14, 2018

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Oct 14, 2018

Understood, I'll have a look!

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 15, 2018

as for json, I'd like to suggest the following.

each command will be modeled as sequence of phases ("load index", "list all files to restore", "restore file content", and so on).

every few seconds during phase execution emit json message with the following attributes:

  • title of the phase
  • percent of the phase completion, if known
  • eta of the phase completion, if known
  • message description of the phase process, for example for restore file content download it will be something like "1 out of 1024 MiB in 1 out 123345 files"

at the end of each command phase emit summary json message with the following attributes

  • start/end timestamps
  • message that says how many files/bytes were "processed" during the phase and processing speed. to continue with restore file content download example, it will be something like "downloaded 1024 MiB in 123345 files at 1000Gbps".

@mholt what do you think?

@mholt

This comment has been minimized.

Copy link
Contributor

mholt commented Oct 15, 2018

@ifedorenko That sounds perfect -- and, preferably, if each message is encoded using a different struct type, it would be nice if there was a field to indicate the type of message or structure so we can expect certain decoded fields (struct_type is what I used in #1962 or message_type in #1944 -- hmm, maybe I should have standardized on one name). If that makes sense. Maybe there's a better way to do it, I dunno though.

Basically, if each kind of message could have a different structure, it'd be nice as a consumer of the JSON output to be able to decode all the messages into a structure that can fit all the possible values, or to know which kind of message it is so we can decode properly.

Also, the more details (like processing rate, number of files, etc) that you can put into separate fields as plain numbers, the better. (A message is fine, too, as long as it's not the only way the information is given.)

Anyway, sounds great overall 👍

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 15, 2018

I was hoping to avoid fine-grained metrics, like number of bytes downloaded or files written ;-). No big deal, will find a way to have those too.

@fd0 do you have a preferred text template library I can use here, like mustache for Go for example? Not sure I'll need this yet, but one of the ideas I consider is to have a map of fine-grained metrics, which are then used to render user-visible messages via a template. Probably an overkill here, but it feels like the right way to provide uniform way to support json and user-facing UI.

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Oct 15, 2018

I'd prefer text/template from the standard library.

@mholt the json code is not released yet, we can change the name of the type field :)


var _ ProgressUI = &validatingProgressUI{}

func NewValidatingProgressUI() ProgressUI {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported function NewValidatingProgressUI should have comment or be unexported

return FormatSeconds(sec)
}

func FormatDuration(d time.Duration) string {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported function FormatDuration should have comment or be unexported

return FormatDuration(c.ETA(sw))
}

func (c *CounterTo) Target() *Counter {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported method CounterTo.Target should have comment or be unexported

return eta
}

func (c *CounterTo) FormatETA(sw Stopwatch) string {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported method CounterTo.FormatETA should have comment or be unexported

return FormatPercent(uint64(c.value), uint64(c.target.value))
}

func (c *CounterTo) ETA(sw Stopwatch) time.Duration {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported method CounterTo.ETA should have comment or be unexported

return FormatBytes(uint64(c.value))
}

type Stopwatch struct {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported type Stopwatch should have comment or be unexported

return c.value
}

func (c *Counter) FormatBytes() string {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported method Counter.FormatBytes should have comment or be unexported

c.value += int64(count)
}

func (c *Counter) Value() int64 {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported method Counter.Value should have comment or be unexported

value int64
}

func (c *Counter) Add(count int64) {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported method Counter.Add should have comment or be unexported

"time"
)

type Counter struct {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 16, 2018

exported type Counter should have comment or be unexported

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 16, 2018

Just pushed very much WIP implementation of template-based progress UI API. The good part, common TermstatusProgressUI implementation now has access to individual progress metrics and should render json output in generic way, i.e. we won't need to code json for backup, restore and check separately. The bad (or ugly), text templates make it harder to understand how pieces fit together, which makes the code little more fragile (unit tests should help with that somewhat).

@fd0 @mholt what do you think?

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch from bd1a8f7 to 8b3af11 Oct 16, 2018

@mholt

This comment has been minimized.

Copy link
Contributor

mholt commented Oct 16, 2018

@ifedorenko I love the sound of that. Templates aren't really that evil/bad -- they're easy to test, fortunately.

What needs to happen to add support for the --json global flag?

PS. I just tried this branch for the first time and the interactive UI is working great - nice work!

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 16, 2018

@mholt somebody ;-) will have to change TermstatusProgressUI to honour --json. displayProgress and diplaySummary are the two methods will need to change for sure, and maybe Set too.

@mholt

This comment has been minimized.

Copy link
Contributor

mholt commented Oct 16, 2018

If that lays the foundation for outputting the real-time status of backup, restore, check, and prune, I'd be happy to work on that.

}
}

func (p *TermstatusProgressUI) Unset() {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Oct 18, 2018

exported method TermstatusProgressUI.Unset should have comment or be unexported

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Oct 19, 2018

@fd0 do you mind I move all backup-specific progress reporting to archiver package, similarly to how I already did/prototyped for restore and check? And if you have time for some feedback on overall idea and implementation of ProgressUI API, I'll certainly appreciate that too.

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch from 52581b6 to 3b5b0c3 Oct 20, 2018

@fd0

This comment has been minimized.

Copy link
Member

fd0 commented Oct 20, 2018

@ifedorenko I need to find time to look a this, I didn't look at it yet. Sorry for the delay!

@ifedorenko ifedorenko referenced this pull request Nov 26, 2018

Closed

Out order restore #2101

5 of 7 tasks complete

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch 2 times, most recently from a3f84a4 to 0c4343c Nov 27, 2018

ifedorenko added some commits Sep 13, 2018

restore: periodic progress status report UI
* ui.ProgressUI/ui.TermstatusProgressUI provide command-agnostic periodic
  status update UI. Implementation mostly copied from existed ui.Backup
  and can be shared by all long running commands. Can be easily mocked
  for testing too.
* restorer.progressUI builds on ui.ProgressUI to provide restore
  progress UI

Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
WIP restore progress ui ETA
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
high-level overview of what I am trying to achieve
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
WIP template-based progress ui api
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
WIP Reworked check progress using the new ProgressUI API
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
WIP porting backup ui
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
WIP progress status message
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
WIP get rid of templates
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
termstatus: Terminal.SetStatus([]string{}) should clear status
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
cleanup, removed stale code
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
fixed backup progress ETA
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
fixed check progress
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
introduced runWithProgress template function
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
moved backup progress reported to archiver package
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
fixed failing tests
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
unexported and simplified stopwatch
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
got rid of CounterTo, too little code reuse to bother
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
fixed misleader 'checked snapshots' checker metric
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
updated comments and removed unused code
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
restore progress ui changelog entry
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
removed unused code
Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>
backup: fixed intermittent 'tomb.Go called after all goroutines termi…
…nated'

Signed-off-by: Igor Fedorenko <igor@ifedorenko.com>

@ifedorenko ifedorenko force-pushed the ifedorenko:mt-restorer-ui branch from 0c4343c to c5b4d4d Jan 27, 2019

@bbigras

This comment has been minimized.

Copy link

bbigras commented Mar 23, 2019

Any progress on this?

@ifedorenko

This comment has been minimized.

Copy link
Contributor Author

ifedorenko commented Mar 24, 2019

I have no immediate plans to continue work on this PR. The approach I took here was too ambitious and the change is too big to be worth the trouble. I may provide smaller change to implement restorer progress after #2195 is merged, but can't promise at this point. Really depends how long it takes for #2195 to get reviewed and how much time I will be able to dedicate to restic after that.

@ifedorenko ifedorenko closed this Mar 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.