juju-core

Merge lp:~axwalk/juju-core/sshstorage-put-stdin into lp:~go-bot/juju-core/trunk

  1. sshstorage-put-stdin
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 1865
Proposed branch: lp:~axwalk/juju-core/sshstorage-put-stdin
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 88 lines (+28/-9)
2 files modified
environs/sshstorage/storage.go (+21/-9)
environs/sshstorage/storage_test.go (+7/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/sshstorage-put-stdin
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+186936@code.launchpad.net

Commit message

environs/sshstorage: fix Put of large files

Write file contents via stdin, rather than
as a here document on the command line. This
enables writing of large files (e.g. juju
tools archives).

https://codereview.appspot.com/13507045/

Description of the change

environs/sshstorage: fix Put of large files

Write file contents via stdin, rather than
as a here document on the command line. This
enables writing of large files (e.g. juju
tools archives).

https://codereview.appspot.com/13507045/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.9 KiB)

Reviewers: mp+186936_code.launchpad.net,

Message:
Please take a look.

Description:
environs/sshstorage: fix Put of large files

Write file contents via stdin, rather than
as a here document on the command line. This
enables writing of large files (e.g. juju
tools archives).

https://code.launchpad.net/~axwalk/juju-core/sshstorage-put-stdin/+merge/186936

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13507045/

Affected files (+25, -9 lines):
   A [revision details]
   M environs/sshstorage/storage.go
   M environs/sshstorage/storage_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130920052504-qqe61zp28ipkvyh6
+New revision: <email address hidden>

Index: environs/sshstorage/storage.go
=== modified file 'environs/sshstorage/storage.go'
--- environs/sshstorage/storage.go 2013-09-20 03:46:57 +0000
+++ environs/sshstorage/storage.go 2013-09-22 13:32:02 +0000
@@ -123,21 +123,30 @@

  func (s *SSHStorage) runf(flockmode flockmode, command string,
args ...interface{}) (string, error) {
   command = fmt.Sprintf(command, args...)
- return s.run(flockmode, command)
+ return s.run(flockmode, command, nil)
  }

-func (s *SSHStorage) run(flockmode flockmode, command string) (string,
error) {
+func (s *SSHStorage) run(flockmode flockmode, command string, input
[]byte) (string, error) {
   const rcPrefix = "JUJU-RC: "
   command = fmt.Sprintf(
- "(SHELL=/bin/bash flock %s %s -c %s) 2>&1; echo %s$?",
+ "SHELL=/bin/bash flock %s %s -c %s",
    flockmode,
    s.remotepath,
    utils.ShQuote(command),
- rcPrefix,
   )
- if _, err := s.stdin.Write([]byte(command + "\r\n")); err != nil {
+ if input != nil {
+ command = fmt.Sprintf("line | base64 -d | (%s)", command)
+ }
+ command = fmt.Sprintf("(%s) 2>&1; echo %s$?", command, rcPrefix)
+ if _, err := s.stdin.Write([]byte(command + "\n")); err != nil {
    return "", fmt.Errorf("failed to write command: %v", err)
   }
+ if input != nil {
+ encoded := base64.StdEncoding.EncodeToString(input)
+ if _, err := s.stdin.Write([]byte(encoded + "\n")); err != nil {
+ return "", fmt.Errorf("failed to write input: %v", err)
+ }
+ }
   var output []string
   for s.scanner.Scan() {
    line := s.scanner.Text()
@@ -243,7 +252,6 @@
   if _, err := r.Read(buf); err != nil {
    return err
   }
- encoded := base64.StdEncoding.EncodeToString(buf)
   path = utils.ShQuote(path)

   tmpdir := s.tmpdir
@@ -253,7 +261,7 @@
   tmpdir = utils.ShQuote(tmpdir)

   // Write to a temporary file ($TMPFILE), then mv atomically.
- command := fmt.Sprintf("mkdir -p `dirname %s` && base64 -d > $TMPFILE",
path)
+ command := fmt.Sprintf("mkdir -p `dirname %s` && cat > $TMPFILE", path)
   command = fmt.Sprintf(
    "export TMPDIR=%s && TMPFILE=`mktemp` && ((%s && mv $TMPFILE %s) || rm
-f $TMPFILE)",
    tmpdir, command, path,
@@ -269,8 +277,7 @@
    command = fmt.Sprintf("%s && (%s); rc=$?; %s; exit $rc", installTmpdir,
command, removeTmpdir)
   }

- command = fmt.Sprintf("(%s...

Read more...

Revision history for this message
Tim Penhey (thumper) wrote :

LGTM

https://codereview.appspot.com/13507045/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (7.4 KiB)

The attempt to merge lp:~axwalk/juju-core/sshstorage-put-stdin into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.779s
ok launchpad.net/juju-core/agent/tools 0.251s
ok launchpad.net/juju-core/bzr 6.991s
ok launchpad.net/juju-core/cert 2.291s
ok launchpad.net/juju-core/charm 0.625s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.021s
ok launchpad.net/juju-core/cmd 0.233s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 158.117s
ok launchpad.net/juju-core/cmd/jujud 39.868s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.072s
ok launchpad.net/juju-core/constraints 0.027s
ok launchpad.net/juju-core/container/lxc 0.374s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.299s
ok launchpad.net/juju-core/environs 1.884s
ok launchpad.net/juju-core/environs/bootstrap 4.957s
ok launchpad.net/juju-core/environs/cloudinit 0.394s
ok launchpad.net/juju-core/environs/config 0.818s
ok launchpad.net/juju-core/environs/configstore 0.031s
ok launchpad.net/juju-core/environs/filestorage 0.034s
ok launchpad.net/juju-core/environs/httpstorage 0.049s
ok launchpad.net/juju-core/environs/imagemetadata 0.496s
ok launchpad.net/juju-core/environs/instances 0.053s
ok launchpad.net/juju-core/environs/jujutest 0.225s
ok launchpad.net/juju-core/environs/manual 4.810s
ok launchpad.net/juju-core/environs/simplestreams 0.251s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]

----------------------------------------------------------------------
FAIL: storage_test.go:159: storageSuite.TestPut

storage_test.go:167:
    c.Assert(out, gc.DeepEquals, data)
... obtained []uint8 = []byte{}
... expected []uint8 = []byte{0x61, 0x62, 0x63, 0x0, 0x64, 0x65, 0x66}

----------------------------------------------------------------------
FAIL: storage_test.go:308: storageSuite.TestPutTmpDir

storage_test.go:334:
    c.Assert(err, gc.ErrorMatches, "mktemp: failed to create file.*No such file or directory")
... error string = "" +
... "bash: line 2: line: command not found\n" +
... "mktemp: failed to create file via template `/tmp/gocheck-5577006791947779410/10.tmp/tmp.XXXXXXXXXX': No such file or directory"
... regex string = "mktemp: failed to create file.*No such file or directory"

OOPS: 12 passed, 2 FAILED
--- FAIL: Test (1.42 seconds)
FAIL
FAIL launchpad.net/juju-core/environs/sshstorage 1.452s
ok launchpad.net/juju-core/environs/storage 0.013s
ok launchpad.net/juju-core/environs/sync 27.972s
ok launchpad.net/juju-core/environs/testing 0.013s
ok launchpad.net/juju-core/environs/tools 10.407s
? launchpad.net/juju-core/environs/tools/testing [no test files]
? launchpad.net/juju-core/errors [no test files]
ok launchpad.net/juju-core/instance 0.022s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 20.511s
ok launchpad.net/juju-co...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/sshstorage/storage.go'
2--- environs/sshstorage/storage.go 2013-09-20 03:46:57 +0000
3+++ environs/sshstorage/storage.go 2013-09-23 04:28:01 +0000
4@@ -123,21 +123,35 @@
5
6 func (s *SSHStorage) runf(flockmode flockmode, command string, args ...interface{}) (string, error) {
7 command = fmt.Sprintf(command, args...)
8- return s.run(flockmode, command)
9+ return s.run(flockmode, command, nil)
10 }
11
12-func (s *SSHStorage) run(flockmode flockmode, command string) (string, error) {
13+func (s *SSHStorage) run(flockmode flockmode, command string, input []byte) (string, error) {
14 const rcPrefix = "JUJU-RC: "
15 command = fmt.Sprintf(
16- "(SHELL=/bin/bash flock %s %s -c %s) 2>&1; echo %s$?",
17+ "SHELL=/bin/bash flock %s %s -c %s",
18 flockmode,
19 s.remotepath,
20 utils.ShQuote(command),
21- rcPrefix,
22 )
23- if _, err := s.stdin.Write([]byte(command + "\r\n")); err != nil {
24+ var encoded string
25+ if input != nil {
26+ encoded = base64.StdEncoding.EncodeToString(input)
27+ command = fmt.Sprintf(
28+ "head -q -c %d | base64 -d | (%s)",
29+ len(encoded),
30+ command,
31+ )
32+ }
33+ command = fmt.Sprintf("(%s) 2>&1; echo %s$?", command, rcPrefix)
34+ if _, err := s.stdin.Write([]byte(command + "\n")); err != nil {
35 return "", fmt.Errorf("failed to write command: %v", err)
36 }
37+ if input != nil {
38+ if _, err := s.stdin.Write([]byte(encoded)); err != nil {
39+ return "", fmt.Errorf("failed to write input: %v", err)
40+ }
41+ }
42 var output []string
43 for s.scanner.Scan() {
44 line := s.scanner.Text()
45@@ -243,7 +257,6 @@
46 if _, err := r.Read(buf); err != nil {
47 return err
48 }
49- encoded := base64.StdEncoding.EncodeToString(buf)
50 path = utils.ShQuote(path)
51
52 tmpdir := s.tmpdir
53@@ -253,7 +266,7 @@
54 tmpdir = utils.ShQuote(tmpdir)
55
56 // Write to a temporary file ($TMPFILE), then mv atomically.
57- command := fmt.Sprintf("mkdir -p `dirname %s` && base64 -d > $TMPFILE", path)
58+ command := fmt.Sprintf("mkdir -p `dirname %s` && cat > $TMPFILE", path)
59 command = fmt.Sprintf(
60 "export TMPDIR=%s && TMPFILE=`mktemp` && ((%s && mv $TMPFILE %s) || rm -f $TMPFILE)",
61 tmpdir, command, path,
62@@ -269,8 +282,7 @@
63 command = fmt.Sprintf("%s && (%s); rc=$?; %s; exit $rc", installTmpdir, command, removeTmpdir)
64 }
65
66- command = fmt.Sprintf("(%s) << EOF\n%s\nEOF", command, encoded)
67- _, err = s.runf(flockExclusive, command+"\n")
68+ _, err = s.run(flockExclusive, command+"\n", buf)
69 return err
70 }
71
72
73=== modified file 'environs/sshstorage/storage_test.go'
74--- environs/sshstorage/storage_test.go 2013-09-20 04:07:02 +0000
75+++ environs/sshstorage/storage_test.go 2013-09-23 04:28:01 +0000
76@@ -298,6 +298,13 @@
77 c.Assert(err, gc.NotNil)
78 }
79
80+func (s *storageSuite) TestPutLarge(c *gc.C) {
81+ stor, _ := s.makeStorage(c)
82+ buf := make([]byte, 1048576)
83+ err := stor.Put("ohmy", bytes.NewBuffer(buf), int64(len(buf)))
84+ c.Assert(err, gc.IsNil)
85+}
86+
87 func (s *storageSuite) TestPutTmpDir(c *gc.C) {
88 stor, storageDir := s.makeStorage(c)
89

Subscribers

People subscribed via source and target branches

to status/vote changes: