Skip to content

Commit

Permalink
cmd/create: Don't block user interaction while fetching the image size
Browse files Browse the repository at this point in the history
It takes 'skopeo inspect' a few seconds to fetch the image size from the
remote registry, and while that happens the user can't interact with the
image download prompt:
  $ toolbox create
  Image required to create toolbox container.
  <wait for a few seconds>
  Download registry.fedoraproject.org/fedora-toolbox:39 (359.8MB)? [y/N]:

This feels awkward because it's not clear to the user what's going on
during those few seconds.  Moreover, while knowing the image size can be
convenient at times, for example when disk space and network bandwidth
are limited, it's not always important.

It will be better if 'skopeo inspect' ran in the background, while
waiting for the user to respond to the image download prompt, and once
the image size has been fetched, the image download prompt can be
updated to include it.

So, initially:
  $ toolbox create
  Image required to create toolbox container.
  Download registry.fedoraproject.org/fedora-toolbox:39 ( ... MB)? [y/N]:

... and then once the size is available:
  $ toolbox create
  Image required to create toolbox container.
  Download registry.fedoraproject.org/fedora-toolbox:39 (359.8MB)? [y/N]:

If skopeo(1) is missing or too old, then the prompt can continue without
the size, as it did before:
  $ toolbox create
  Image required to create toolbox container.
  Download registry.fedoraproject.org/fedora-toolbox:39 [y/N]:

The placeholder for the missing image size (ie., ' ... MB') was chosen
to have seven characters, so that it matches the most common sizes.  The
human-readable representation of the image size is capped at four valid
numbers [1].  Unless it's a perfect round number like 1KB or 1.2MB, it
will likely use all four numbers and the decimal point, which is five
characters.  Then two more for the unit, because it's very unlikely that
there will be an image that's less than 1KB in size and will be shown in
bytes with a B.  That makes it seven characters in total.

Updating the image download prompt with the results of 'skopeo inspect'
is vulnerable to races.  At the same time as the terminal's cursor is
being moved to the beginning of the current line to overwrite the
earlier prompt with the new one, the user can keep typing and keep
moving the cursor forward.  This competition over the cursor can lead to
awkward outcomes.

For example, the prompt can overwrite the characters typed in by the
user, leaving characters in the terminal's input buffer waiting for the
user to hit ENTER, even though they are not visible on the screen.
Another example is that hitting BACKSPACE can end up deleting parts of
the prompt, instead of stopping at the edge.

This is solved by putting the terminal device into non-canonical mode
input and disabling the echoing of input characters, while the prompt is
being updated.  This prevents input from moving the terminal's cursor
forward, and from accumulating in the terminal's input buffer even if
it might not be visible.  Any input during this interim period is
discarded and replaced by '...', and a fresh new prompt is shown in the
following line.

In practice, this race shouldn't be too common.  It can only happen if
the user is typing right when the prompt is being updated, which is
unlikely because it's only supposed to be a short 'yes' or 'no' input.

The use of the context.Cause and context.WithCancelCause functions [2]
requires Go >= 1.20.  Bumping the Go version in src/go.mod then requires
a 'go mod tidy'.  Otherwise, it leads to:
  $ meson compile -C builddir --verbose
  ...
  /home/rishi/devel/containers/git/toolbox/src/go-build-wrapper
    /home/rishi/devel/containers/git/toolbox/src
    /home/rishi/devel/containers/git/toolbox/builddir src/toolbox
    0.0.99.4 cc /lib64/ld-linux-x86-64.so.2 false
  go: updates to go.mod needed; to update it:
          go mod tidy
  ninja: build stopped: subcommand failed.

[1] https://pkg.go.dev/github.com/docker/go-units#HumanSize

[2] https://pkg.go.dev/context

#752
#1263
  • Loading branch information
debarshiray committed Dec 16, 2023
1 parent 5ce96da commit 5784754
Show file tree
Hide file tree
Showing 4 changed files with 273 additions and 21 deletions.
6 changes: 5 additions & 1 deletion .github/workflows/ubuntu-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ jobs:
fish \
gcc \
go-md2man \
golang \
golang-1.20 \
meson \
ninja-build \
openssl \
Expand All @@ -54,6 +54,10 @@ jobs:
systemd \
udisks2
- name: Set up PATH for Go 1.20
run: |
echo "PATH=/usr/lib/go-1.20/bin:$PATH" >> "$GITHUB_ENV"
- name: Checkout Bats
uses: actions/checkout@v3
with:
Expand Down
259 changes: 245 additions & 14 deletions src/cmd/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ import (
"github.com/spf13/cobra"
)

type promptForDownloadError struct {
ImageSize string
}

const (
alpha = `abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ`
num = `0123456789`
Expand Down Expand Up @@ -573,8 +577,7 @@ func getFullyQualifiedImageFromRepoTags(image string) (string, error) {
return imageFull, nil
}

func getImageSizeFromRegistry(imageFull string) (string, error) {
ctx := context.Background()
func getImageSizeFromRegistry(ctx context.Context, imageFull string) (string, error) {
image, err := skopeo.Inspect(ctx, imageFull)
if err != nil {
return "", err
Expand All @@ -598,6 +601,23 @@ func getImageSizeFromRegistry(imageFull string) (string, error) {
return imageSizeHuman, nil
}

func getImageSizeFromRegistryAsync(ctx context.Context, imageFull string) (<-chan string, <-chan error) {
retValCh := make(chan string)
errCh := make(chan error)

go func() {
imageSize, err := getImageSizeFromRegistry(ctx, imageFull)
if err != nil {
errCh <- err
return
}

retValCh <- imageSize
}()

return retValCh, errCh
}

func getServiceSocket(serviceName string, unitName string) (string, error) {
logrus.Debugf("Resolving path to the %s socket", serviceName)

Expand Down Expand Up @@ -710,18 +730,7 @@ func pullImage(image, release, authFile string) (bool, error) {
}

if promptForDownload {
fmt.Println("Image required to create toolbox container.")

var prompt string

if imageSize, err := getImageSizeFromRegistry(imageFull); err != nil {
logrus.Debugf("Getting image size failed: %s", err)
prompt = fmt.Sprintf("Download %s? [y/N]:", imageFull)
} else {
prompt = fmt.Sprintf("Download %s (%s)? [y/N]:", imageFull, imageSize)
}

shouldPullImage = askForConfirmation(prompt)
shouldPullImage = showPromptForDownload(imageFull)
}

if !shouldPullImage {
Expand Down Expand Up @@ -751,6 +760,214 @@ func pullImage(image, release, authFile string) (bool, error) {
return true, nil
}

func createPromptForDownload(imageFull, imageSize string) string {
var prompt string
if imageSize == "" {
prompt = fmt.Sprintf("Download %s? [y/N]:", imageFull)
} else {
prompt = fmt.Sprintf("Download %s (%s)? [y/N]:", imageFull, imageSize)
}

return prompt
}

func showPromptForDownloadFirst(imageFull string) (bool, error) {
prompt := createPromptForDownload(imageFull, " ... MB")

parentCtx := context.Background()
askCtx, askCancel := context.WithCancelCause(parentCtx)
defer askCancel(errors.New("clean-up"))

askCh, askErrCh := askForConfirmationAsync(askCtx, prompt, nil)

imageSizeCtx, imageSizeCancel := context.WithCancelCause(parentCtx)
defer imageSizeCancel(errors.New("clean-up"))

imageSizeCh, imageSizeErrCh := getImageSizeFromRegistryAsync(imageSizeCtx, imageFull)

var imageSize string
var shouldPullImage bool

select {
case val := <-askCh:
shouldPullImage = val
cause := fmt.Errorf("%w: received confirmation without image size", context.Canceled)
imageSizeCancel(cause)
case err := <-askErrCh:
shouldPullImage = false
cause := fmt.Errorf("failed to ask for confirmation without image size: %w", err)
imageSizeCancel(cause)
case val := <-imageSizeCh:
imageSize = val
cause := fmt.Errorf("%w: received image size", context.Canceled)
askCancel(cause)
case err := <-imageSizeErrCh:
cause := fmt.Errorf("failed to get image size: %w", err)
askCancel(cause)
}

if imageSizeCtx.Err() != nil && askCtx.Err() == nil {
cause := context.Cause(imageSizeCtx)
logrus.Debugf("Show prompt for download: image size canceled: %s", cause)
return shouldPullImage, nil
}

var done bool

if imageSizeCtx.Err() == nil && askCtx.Err() != nil {
select {
case val := <-askCh:
logrus.Debugf("Show prompt for download: received pending confirmation without image size")
shouldPullImage = val
done = true
case err := <-askErrCh:
logrus.Debugf("Show prompt for download: failed to ask for confirmation without image size: %s",
err)
}
} else {
panic("code should not be reached")
}

cause := context.Cause(askCtx)
logrus.Debugf("Show prompt for download: ask canceled: %s", cause)

if done {
return shouldPullImage, nil
}

return false, &promptForDownloadError{imageSize}
}

func showPromptForDownloadSecond(imageFull string, errFirst *promptForDownloadError) bool {
oldState, err := term.GetState(os.Stdin)
if err != nil {
logrus.Debugf("Show prompt for download: failed to get terminal state: %s", err)
return false
}

defer term.SetState(os.Stdin, oldState)

lockedState := term.NewStateFrom(oldState,
term.WithVMIN(1),
term.WithVTIME(0),
term.WithoutECHO(),
term.WithoutICANON())

if err := term.SetState(os.Stdin, lockedState); err != nil {
logrus.Debugf("Show prompt for download: failed to set terminal state: %s", err)
return false
}

parentCtx := context.Background()
discardCtx, discardCancel := context.WithCancelCause(parentCtx)
defer discardCancel(errors.New("clean-up"))

discardCh, discardErrCh := discardInputAsync(discardCtx)

var prompt string
if errors.Is(errFirst, context.Canceled) {
prompt = createPromptForDownload(imageFull, errFirst.ImageSize)
} else {
prompt = createPromptForDownload(imageFull, "")
}

fmt.Printf("\r")

askCtx, askCancel := context.WithCancelCause(parentCtx)
defer askCancel(errors.New("clean-up"))

var askForConfirmationPreFnDone bool
askForConfirmationPreFn := func() error {
defer discardCancel(errors.New("clean-up"))
if askForConfirmationPreFnDone {
return nil
}

// Erase to end of line
fmt.Printf("\033[K")

// Save the cursor position.
fmt.Printf("\033[s")

if err := term.SetState(os.Stdin, oldState); err != nil {
return fmt.Errorf("failed to restore terminal state: %w", err)
}

cause := errors.New("terminal restored")
discardCancel(cause)

err := <-discardErrCh
if !errors.Is(err, context.Canceled) {
return fmt.Errorf("failed to discard input: %w", err)
}

logrus.Debugf("Show prompt for download: stopped discarding input: %s", err)

discardTotal := <-discardCh
logrus.Debugf("Show prompt for download: discarded input: %d bytes", discardTotal)

if discardTotal == 0 {
askForConfirmationPreFnDone = true
return nil
}

if err := term.SetState(os.Stdin, lockedState); err != nil {
return fmt.Errorf("failed to set terminal state: %w", err)
}

discardCtx, discardCancel = context.WithCancelCause(parentCtx)
// A deferred call can't be used for this CancelCauseFunc,
// because the 'discard' operation needs to continue running
// until the next invocation of this function. It relies on
// the guarantee that AskForConfirmationAsync will always call
// its askForConfirmationPreFunc as long as the function
// returns errContinue.

discardCh, discardErrCh = discardInputAsync(discardCtx)

// Restore the cursor position
fmt.Printf("\033[u")

// Erase to end of line
fmt.Printf("\033[K")

fmt.Printf("...\n")
return errContinue
}

askCh, askErrCh := askForConfirmationAsync(askCtx, prompt, askForConfirmationPreFn)
var shouldPullImage bool

select {
case val := <-askCh:
logrus.Debug("Show prompt for download: received confirmation with image size")
shouldPullImage = val
case err := <-askErrCh:
logrus.Debugf("Show prompt for download: failed to ask for confirmation with image size: %s", err)
shouldPullImage = false
}

return shouldPullImage
}

func showPromptForDownload(imageFull string) bool {
fmt.Println("Image required to create toolbox container.")

shouldPullImage, err := showPromptForDownloadFirst(imageFull)
if err == nil {
return shouldPullImage
}

var errPromptForDownload *promptForDownloadError
if !errors.As(err, &errPromptForDownload) {
panicMsg := fmt.Sprintf("unexpected %T: %s", err, err)
panic(panicMsg)
}

shouldPullImage = showPromptForDownloadSecond(imageFull, errPromptForDownload)
return shouldPullImage
}

// systemdNeedsEscape checks whether a byte in a potential dbus ObjectPath needs to be escaped
func systemdNeedsEscape(i int, b byte) bool {
// Escape everything that is not a-z-A-Z-0-9
Expand Down Expand Up @@ -778,3 +995,17 @@ func systemdPathBusEscape(path string) string {
}
return string(n)
}

func (err *promptForDownloadError) Error() string {
innerErr := err.Unwrap()
errMsg := innerErr.Error()
return errMsg
}

func (err *promptForDownloadError) Unwrap() error {
if err.ImageSize == "" {
return errors.New("failed to get image size")
}

return context.Canceled
}
24 changes: 23 additions & 1 deletion src/go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/containers/toolbox

go 1.14
go 1.20

require (
github.com/HarryMichal/go-version v1.0.1
Expand All @@ -15,3 +15,25 @@ require (
github.com/stretchr/testify v1.7.0
golang.org/x/sys v0.1.0
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/fatih/color v1.13.0 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/magiconair/properties v1.8.5 // indirect
github.com/mattn/go-colorable v0.1.12 // indirect
github.com/mattn/go-isatty v0.0.14 // indirect
github.com/mitchellh/mapstructure v1.4.3 // indirect
github.com/pelletier/go-toml v1.9.4 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/spf13/afero v1.6.0 // indirect
github.com/spf13/cast v1.4.1 // indirect
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/subosito/gotenv v1.2.0 // indirect
golang.org/x/text v0.3.7 // indirect
gopkg.in/ini.v1 v1.66.2 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
)
5 changes: 0 additions & 5 deletions src/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,6 @@ github.com/googleapis/gax-go/v2 v2.1.0/go.mod h1:Q3nei7sK6ybPYH7twZdmQpAd1MKb7pf
github.com/googleapis/gax-go/v2 v2.1.1/go.mod h1:hddJymUZASv3XPyGkUpKj8pPO47Rmb0eJc8R6ouapiM=
github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
github.com/hashicorp/consul/api v1.11.0/go.mod h1:XjsvQN+RJGWI2TWy1/kqaE16HrR2J/FWgkYjdZQsX9M=
github.com/hashicorp/consul/api v1.12.0/go.mod h1:6pVBMo0ebnYdt2S3H87XhekM/HHrUoTD2XXb/VrZVy0=
github.com/hashicorp/consul/sdk v0.8.0/go.mod h1:GBvyrGALthsZObzUGsfgHZQDXjg4lOjagTIwIR1vPms=
github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80=
Expand Down Expand Up @@ -319,7 +318,6 @@ github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFR
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
github.com/sagikazarmark/crypt v0.3.0/go.mod h1:uD/D+6UF4SrIR1uGEv7bBNkNqLGqUr43MRiaGWX1Nig=
github.com/sagikazarmark/crypt v0.4.0/go.mod h1:ALv2SRj7GxYV4HO9elxH9nS6M9gW+xDNxqmyJ6RfDFM=
github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE=
Expand Down Expand Up @@ -551,7 +549,6 @@ golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20211007075335-d3039528d8ac/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211124211545-fe61309f8881/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211205182925-97ca703d548d/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211210111614-af8b64212486/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
Expand Down Expand Up @@ -658,7 +655,6 @@ google.golang.org/api v0.57.0/go.mod h1:dVPlbZyBo2/OjBpmvNdpn2GRm6rPy75jyU7bmhdr
google.golang.org/api v0.59.0/go.mod h1:sT2boj7M9YJxZzgeZqXogmhfmRWDtPzT31xkieUbuZU=
google.golang.org/api v0.61.0/go.mod h1:xQRti5UdCmoCEqFxcz93fTl338AVqDgyaDRuOZ3hg9I=
google.golang.org/api v0.62.0/go.mod h1:dKmwPCydfsad4qCH08MSdgWjfHOyfpd4VtDGgRFdavw=
google.golang.org/api v0.63.0/go.mod h1:gs4ij2ffTRXwuzzgJl/56BdwJaA194ijkfn++9tDuPo=
google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM=
google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4=
google.golang.org/appengine v1.5.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4=
Expand Down Expand Up @@ -756,7 +752,6 @@ google.golang.org/grpc v1.39.1/go.mod h1:PImNr+rS9TWYb2O4/emRugxiyHZ5JyHW5F+RPnD
google.golang.org/grpc v1.40.0/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34=
google.golang.org/grpc v1.40.1/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34=
google.golang.org/grpc v1.42.0/go.mod h1:k+4IHHFw41K8+bbowsex27ge2rCb65oeWqe4jJ590SU=
google.golang.org/grpc v1.43.0/go.mod h1:k+4IHHFw41K8+bbowsex27ge2rCb65oeWqe4jJ590SU=
google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw=
google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
Expand Down

0 comments on commit 5784754

Please sign in to comment.