feature: add pouch save functionality #1592

xiechengsheng · 2018-06-25T07:48:43Z

Signed-off-by: xiechengsheng XIE1995@whut.edu.cn

Ⅰ. Describe what this PR did

add pouch save functionality

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

$ pouch save -o busybox.tar busybox:latest
$ pouch load -i busybox.tar foo
$ pouch images
IMAGE ID       IMAGE NAME                                           SIZE
8c811b4aec35   registry.hub.docker.com/library/busybox:latest       710.81 KB
8c811b4aec35   foo:latest                                           710.81 KB

Ⅴ. Special notes for reviews

xiechengsheng · 2018-06-25T07:55:45Z

/assign @fuweid Please take a look this pr, thanks a lot~

fuweid · 2018-06-26T15:19:08Z

apis/server/image_bridge.go

+	}
+
+	output := newWriteFlusher(rw)
+	if _, err := io.Copy(output, r); err != nil {


Is it better if we just return io.Copy(output, r)

Emmm, I don't think so, io.Copy(output, r) returns two variables, will make this saveImage function more complicated.

fuweid · 2018-06-26T15:56:59Z

cli/save.go

+		Use:   "save [OPTIONS] IMAGE [IMAGE...]",
+		Short: "Save an image to a tar archive or STDOUT",
+		Long:  saveDescription,
+		Args:  cobra.MinimumNArgs(1),


I don't think we can save several images into one tar file.

fuweid · 2018-06-26T15:58:42Z

cli/save.go

+	ctx := context.Background()
+	apiClient := save.cli.Client()
+
+	r, err := apiClient.ImageSave(ctx, args[0])


It is very confusing to me because args[0] vs MinimumNArgs

fuweid · 2018-06-26T16:00:12Z

cli/save.go

+	if err := out.Close(); err != nil {
+		return err
+	}
+	return r.Close()


could we defer the r.Close() if the one of io.Copy or out.Close() fails?

fuweid · 2018-06-26T16:05:09Z

ctrd/image.go

+			return nil, err
+		}
+
+		if ociRefName := namedRef.(reference.Tagged).Tag(); ociRefName != "" {


how do you make sure the namedRef is the reference.Tagged? if not, it will panic.

fuweid · 2018-06-26T16:08:12Z

@xiechengsheng , Could you give more information about this feature? For example, about oci-ref-name? Without those information, I can't understand what you do. Thanks :)

xiechengsheng · 2018-06-27T12:19:33Z

@fuweid I have updated codes according to your comments. desc.Annotations[ocispec.AnnotationRefName] is the tag of image to be saved.

codecov-io · 2018-06-27T12:49:43Z

Codecov Report

Merging #1592 into master will increase coverage by 0.08%.
The diff coverage is 36.04%.

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
+ Coverage   41.26%   41.34%   +0.08%     
==========================================
  Files         274      277       +3     
  Lines       18123    18209      +86     
==========================================
+ Hits         7478     7529      +51     
- Misses       9733     9749      +16     
- Partials      912      931      +19

Impacted Files	Coverage Δ
daemon/mgr/image_load.go	`47.36% <ø> (+47.36%)`	⬆️
daemon/mgr/image.go	`68.46% <ø> (ø)`	⬆️
cli/save.go	`0% <0%> (ø)`
cli/main.go	`0% <0%> (ø)`	⬆️
client/image_save.go	`100% <100%> (ø)`
apis/server/router.go	`91.42% <100%> (+0.06%)`	⬆️
daemon/mgr/image_save.go	`50% <50%> (ø)`
ctrd/image.go	`79.2% <60%> (+1.73%)`	⬆️
apis/server/image_bridge.go	`70% <63.63%> (+4.17%)`	⬆️
apis/server/utils.go	`61.9% <0%> (-4.77%)`	⬇️
... and 7 more

shaloulcy · 2018-06-28T02:44:15Z

cli/save.go

+	apiClient := save.cli.Client()
+
+	r, err := apiClient.ImageSave(ctx, args[0])
+	defer r.Close()


if err!=nil{
return err
}
defer r.Close()

ok, will fix soon.

shaloulcy · 2018-06-28T02:49:23Z

cli/save.go

+	if _, err := io.Copy(out, r); err != nil {
+		return err
+	}
+	if err := out.Close(); err != nil {


if out is os.Stdout, we should not close it

@shaloulcy I'm just curious to ask why we shouldn't close client stdout...

if we did like

func main() { out := os.Stdout fmt.Println(out.Close()) fmt.Println("haha") }

The following stdout will missing so that we can't close the stdout.

shaloulcy · 2018-06-28T02:49:54Z

cli/save.go

+		out, err = os.Create(save.output)
+		if err != nil {
+			return nil
+		}


defer out.Close()

please put the defer out.Close() in the if save.output != "" so that we should not close stdout.

shaloulcy · 2018-06-28T03:01:01Z

apis/server/image_bridge.go

+
+	rw.Header().Set("Content-Type", "application/x-tar")
+
+	r, err := s.ImageMgr.SaveImage(ctx, imageName)


we should close the reader

ok, will fix soon.

allencloud · 2018-07-05T03:30:15Z

cli/save.go

+		Use:   "save [OPTIONS] IMAGE [IMAGE...]",
+		Short: "Save an image to a tar archive or STDOUT",
+		Long:  saveDescription,
+		Args:  cobra.ExactArgs(1),


I think this part is incompatible with save [OPTIONS] IMAGE [IMAGE...]. please make sure the input args number. @xiechengsheng

allencloud · 2018-07-05T03:32:22Z

ctrd/image.go

+	if desc.Annotations == nil {
+		desc.Annotations = make(map[string]string)
+	}
+	if s, ok := desc.Annotations[ocispec.AnnotationRefName]; !ok || s == "" {


how about using exist rather than ok? I think it seems to improve the readability.

xiechengsheng · 2018-07-05T05:40:58Z

Hi, I have updated codes, PTAL, thanks a lot.

allencloud · 2018-07-12T06:05:44Z

This is a quite useful feature for PouchContainer. I update the priority to P2 to show the emergency. @xiechengsheng @fuweid

fuweid · 2018-07-12T06:57:22Z

apis/swagger.yml

+          schema:
+            type: "string"
+            format: "binary"
+        404:


For 404, 500 status code, please use format like

404: $ref: "#/responses/404ErrorResponse" 500: $ref: "#/responses/500ErrorResponse"

Since the someone has changed the file, please rebase your commit and regenerate code.

fuweid · 2018-07-12T06:58:58Z

cli/save.go

+	ctx := context.Background()
+	apiClient := save.cli.Client()
+
+	r, err := apiClient.ImageSave(ctx, args[0])


fuweid · 2018-07-12T07:05:04Z

cli/save.go

+		out, err = os.Create(save.output)
+		if err != nil {
+			return nil
+		}


please put the defer out.Close() in the if save.output != "" so that we should not close stdout.

fuweid · 2018-07-12T07:06:06Z

cli/save.go

+	if _, err := io.Copy(out, r); err != nil {
+		return err
+	}
+	if err := out.Close(); err != nil {


if we did like

func main() { out := os.Stdout fmt.Println(out.Close()) fmt.Println("haha") }

The following stdout will missing so that we can't close the stdout.

fuweid · 2018-07-12T07:11:02Z

test/cli_save_and_load_test.go

+		c.Errorf("failed to decode inspect output: %v", err)
+	}
+
+	dir, err := filepath.Abs(filepath.Dir(os.Args[0]))


Please use tmp dir to handle this.

fuweid · 2018-07-12T07:11:45Z

test/cli_save_and_load_test.go

+	}
+	dir = strings.Replace(dir, "\\", "/", -1)
+
+	res = command.PouchRun("save", "-o", dir+"busybox.tar", busyboxImage)


filepath.Join(dir, "busybox.tar") instead of dir+"busybox.tar"

fuweid · 2018-07-12T07:12:28Z

test/cli_save_and_load_test.go

+		c.Errorf("failed to decode inspect output: %v", err)
+	}
+
+	err = os.Remove(dir + "busybox.tar")


use defer to remove the file after the load success

xiechengsheng · 2018-07-12T07:53:36Z

Hi @fuweid I have fixed codes.

fuweid · 2018-07-12T08:16:21Z

cli/save.go

+	if _, err := io.Copy(out, r); err != nil {
+		return err
+	}
+	if save.output != "" {


this if condition is duplicate, please remove it.

ok, I misunderstood yours meaning.

Signed-off-by: xiechengsheng <XIE1995@whut.edu.cn>

fuweid · 2018-07-12T09:36:29Z

LGTM

fuweid · 2018-07-12T09:40:45Z

Next Step we can add API level test for that.

xiechengsheng force-pushed the add-save branch from c4ff93c to 4262dee Compare June 25, 2018 07:52

fuweid reviewed Jun 26, 2018

View reviewed changes

xiechengsheng force-pushed the add-save branch from 4262dee to 827e9f2 Compare June 27, 2018 12:17

shaloulcy reviewed Jun 28, 2018

View reviewed changes

allencloud reviewed Jul 5, 2018

View reviewed changes

xiechengsheng force-pushed the add-save branch from 827e9f2 to 7df5a40 Compare July 5, 2018 05:39

pouchrobot added the size/XL label Jul 5, 2018

allencloud added kind/feature priority/P2 labels Jul 12, 2018

fuweid reviewed Jul 12, 2018

View reviewed changes

xiechengsheng force-pushed the add-save branch from 7df5a40 to 5824474 Compare July 12, 2018 07:43

xiechengsheng force-pushed the add-save branch from 5824474 to 175b63f Compare July 12, 2018 07:59

fuweid reviewed Jul 12, 2018

View reviewed changes

xiechengsheng force-pushed the add-save branch from 175b63f to 22f5d66 Compare July 12, 2018 08:39

feature: add pouch save functionality

6f1e362

Signed-off-by: xiechengsheng <XIE1995@whut.edu.cn>

xiechengsheng force-pushed the add-save branch from 22f5d66 to 6f1e362 Compare July 12, 2018 09:09

fuweid merged commit ca12fe6 into AliyunContainerService:master Jul 12, 2018

pouchrobot mentioned this pull request Jul 19, 2018

Weekly Report of PouchContainer from 2018-07-12 to 2018-07-19 #1752

Closed


		rw.Header().Set("Content-Type", "application/x-tar")

		r, err := s.ImageMgr.SaveImage(ctx, imageName)

feature: add pouch save functionality #1592

feature: add pouch save functionality #1592

Conversation

xiechengsheng commented Jun 25, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

xiechengsheng commented Jun 25, 2018

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

fuweid commented Jun 26, 2018 • edited Loading

xiechengsheng commented Jun 27, 2018

codecov-io commented Jun 27, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

xiechengsheng commented Jul 5, 2018

allencloud commented Jul 12, 2018

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

xiechengsheng commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Jul 12, 2018

fuweid commented Jul 12, 2018

fuweid commented Jun 26, 2018 •

edited

Loading

codecov-io commented Jun 27, 2018 •

edited

Loading