/juju

Show progress for kvm containers #7122

Merged
merged 6 commits into from Mar 20, 2017

Conversation

Reviewers

@axw axw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jameinel @jujubot @axw
@jameinel
Owner

jameinel commented Mar 19, 2017 •
Edited 1 time

Description of change

KVM containers were only showing "Starting container, this might take a while". But not giving any sense for what it was actually doing. This makes it slightly better by giving a progress message (number of bytes copied, progress %) while the container is being started, and then changing the final message to indicate the machine is running.

QA steps

  $ juju bootstrap maas
  $ juju switch controller
  $ juju add-machine kvm:0
  $ juju status
  $ watch juju show-machine 0

Without this patch, there would only be a message:

        instance-id: juju-3cab08-0-kvm-1
        machine-status:
          current: allocating
          message: Creating container; it might take some time

With this patch, you should see messages like:

 copying http://cloud-images.ubuntu.com/server/releases/trusty/release-20170307/ubuntu-14.04-server-cloudimg-amd64-disk1.img 5% 1.4MB/s

And a final status of:

        machine-status:
          current: running
          message: Container started
          since: 19 Mar 2017 16:25:39+04:00

this matches the LXD container status. The progress message should be pretty close, and the final status is the same.

Documentation changes

This is user visible, but as it is informative, I don't think it needs documentation changes.

Bug reference

lp:1674074

jameinel added some commits Mar 19, 2017

@jameinel
Using gogland and Windows more files show up.
c1a0529
@jameinel
Add progress messages for KVM images. 
This does 2 main things:
1) Add a final "Running" message, so the machine doesn't stay 'allocating' forever.
2) Add a progress indicator while the data is being copied, so we have some idea of what is happening.
cfa4a95
@jameinel
Owner

jameinel commented Mar 19, 2017

Note, the minimal fix for https://bugs.launchpad.net/juju/+bug/1674074 could be done by just adding the one line that sets the container status to Running. But this is a bit nicer as it can take a couple minutes to download the container image.

I tested this live on a MAAS setup, since that is one of the few places that easily supports KVM instances.

jameinel added some commits Mar 19, 2017

@jameinel
Change the progress message to show MB/s. 
Match the structure of the progress messages for LXD. Only give a progress
tick when we change by at least 1% and calculate the MB/s based on bytes
transferred since we started copying.
1cc7501
@jameinel
Add tests for progressWriter and toBPS. 
Change progressWriter to use a clock object so we can easily
test it. Add tests that we only send 100 progress messages.
3515401
@jameinel
put the MB/s into parentheses
3731111
@axw

axw approved these changes Mar 20, 2017
View changes

container/kvm/sync.go
// Sync updates the local cached images by reading the simplestreams data and
// caching if an image matching the contrainsts doesn't exist. It retrieves
// metadata information from Oner and updates local cache via Fetcher.
-func Sync(o Oner, f Fetcher) error {
+func Sync(o Oner, f Fetcher, progress ProgressCallback) error {
@axw

axw Mar 20, 2017

Member

can you please add to the doc comment, mentioning that progress may be nil?

container/kvm/sync.go
+
+var _ (io.Writer) = (*progressWriter)(nil)
+
+var modifiers = []string{"k", "M", "G"}
@axw

axw Mar 20, 2017

Member

FYI we already have dustin/go-humanize:

fmt.Sprintf("%s/s", humanize.IBytes(bytes))
@jameinel
Owner

jameinel commented Mar 20, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 20, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 20, 2017

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10498
@jameinel
Owner

jameinel commented Mar 20, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 20, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 20, 2017

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10499
@jameinel
review feedback. 
Switch to dustin/go-humanize instead of a bespoke BPS check.
Document the 'progress' parameter for Sync.
7f172c4
@jameinel
Owner

jameinel commented Mar 20, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 20, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 5a93c5e into juju:develop Mar 20, 2017

@jameinel jameinel deleted the jameinel:2.2-kvm-status-1674074 branch Apr 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment