juju-core

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

  1. sshstorage
  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: 1813
Proposed branch: lp:~axwalk/juju-core/sshstorage
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 612 lines (+597/-0)
3 files modified
environs/sshstorage/error.go (+20/-0)
environs/sshstorage/storage.go (+272/-0)
environs/sshstorage/storage_test.go (+305/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/sshstorage
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+184708@code.launchpad.net

Commit message

environs/sshstorage: SSH-based environs.Storage

This is a new storage implementation for use in
manual bootstrapping. If the environment's storage
is managed by the bootstrap node (as the null provider's
will be), you need an alternative storage mechanism
while bootstrapping.

https://codereview.appspot.com/13243046/

Description of the change

environs/sshstorage: SSH-based environs.Storage

This is a new storage implementation for use in
manual bootstrapping. If the environment's storage
is managed by the bootstrap node (as the null provider's
will be), you need an alternative storage mechanism
while bootstrapping.

https://codereview.appspot.com/13243046/

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

Reviewers: mp+184708_code.launchpad.net,

Message:
Please take a look.

Description:
environs/sshstorage: SSH-based environs.Storage

This is a new storage implementation for use in
manual bootstrapping. If the environment's storage
is managed by the bootstrap node (as the null provider's
will be), you need an alternative storage mechanism
while bootstrapping.

https://code.launchpad.net/~axwalk/juju-core/sshstorage/+merge/184708

(do not edit description out of merge proposal)

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

Affected files (+480, -0 lines):
   A [revision details]
   A environs/sshstorage/error.go
   A environs/sshstorage/storage.go
   A environs/sshstorage/storage_test.go

Revision history for this message
William Reade (fwereade) wrote :

LGTM given my understanding of the use case, but please ponder the
comment and either document-and-land or fix-and-repropose.

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage.go#newcode57
environs/sshstorage/storage.go:57: script := fmt.Sprintf("mkdir -p %s &&
chown $SUDO_UID:$SUDO_GID %s", remotepath, remotepath)
AIUI, "install" is preferred over "mkdir;chown"

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage.go#newcode213
environs/sshstorage/storage.go:213: _, err = s.runf("mkdir -p `dirname
%s` && base64 -d > %s << EOF\n%s\nEOF\n", path, path, encoded)
Ehh. I suppose this is a bootstrap-only Storage, right? If so, and
there's no risk of concurrent access[0], this is fine; but please
document clearly that it's not suitable for wider use.

Or alternatively, perhaps, figure out some reasonably clean way to write
the files atomically?

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage_test.go
File environs/sshstorage/storage_test.go (right):

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage_test.go#newcode54
environs/sshstorage/storage_test.go:54: s.restoreEnv =
testing.PatchEnvironment("PATH", bin+":"+os.Getenv("PATH"))
I'm wondering if we do this enough to make
`testing.PrependEnvironmentPATH(bin)` useful.

https://codereview.appspot.com/13243046/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Thanks for the review. I've made it synchronised, despite the intention
being for it to be only used in bootstrap.

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage.go#newcode57
environs/sshstorage/storage.go:57: script := fmt.Sprintf("mkdir -p %s &&
chown $SUDO_UID:$SUDO_GID %s", remotepath, remotepath)
On 2013/09/11 12:46:29, fwereade wrote:
> AIUI, "install" is preferred over "mkdir;chown"

Done, thanks.

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage.go#newcode213
environs/sshstorage/storage.go:213: _, err = s.runf("mkdir -p `dirname
%s` && base64 -d > %s << EOF\n%s\nEOF\n", path, path, encoded)
On 2013/09/11 12:46:29, fwereade wrote:
> Ehh. I suppose this is a bootstrap-only Storage, right? If so, and
 there's no
> risk of concurrent access[0], this is fine; but please document
 clearly that
> it's not suitable for wider use.

It is meant purely for bootstrap-only, but fair enough, people (like me)
tend to use things for purposes other than originally intended.

> Or alternatively, perhaps, figure out some reasonably clean way to
write the
> files atomically?

flock on the storage dir. Dumb, but it'll keep people from shooting
their feet off at least.

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage_test.go
File environs/sshstorage/storage_test.go (right):

https://codereview.appspot.com/13243046/diff/1/environs/sshstorage/storage_test.go#newcode54
environs/sshstorage/storage_test.go:54: s.restoreEnv =
testing.PatchEnvironment("PATH", bin+":"+os.Getenv("PATH"))
On 2013/09/11 12:46:29, fwereade wrote:
> I'm wondering if we do this enough to make
 `testing.PrependEnvironmentPATH(bin)`
> useful.

I don't find what we do now onerous. *shrug*

https://codereview.appspot.com/13243046/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/13243046/

Revision history for this message
William Reade (fwereade) wrote :

Looking good, thanks for the fix. Just one more, I think.

https://codereview.appspot.com/13243046/diff/7001/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/13243046/diff/7001/environs/sshstorage/storage.go#newcode222
environs/sshstorage/storage.go:222: _, err = s.runf(flockExclusive,
"mkdir -p `dirname %s` && base64 -d > %s << EOF\n%s\nEOF\n", path, path,
encoded)
Is there anywhere we can put this to one side for an atomic move into
place later? Would prefer to avoid leaving half-files lying around where
possible.

https://codereview.appspot.com/13243046/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/13243046/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/13243046/diff/7001/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/13243046/diff/7001/environs/sshstorage/storage.go#newcode222
environs/sshstorage/storage.go:222: _, err = s.runf(flockExclusive,
"mkdir -p `dirname %s` && base64 -d > %s << EOF\n%s\nEOF\n", path, path,
encoded)
On 2013/09/12 07:40:20, fwereade wrote:
> Is there anywhere we can put this to one side for an atomic move into
 place
> later? Would prefer to avoid leaving half-files lying around where
 possible.

Done.

https://codereview.appspot.com/13243046/

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

I might not be understanding this in full, but effectively this is client only storage over ssh. How does a machine agent retrieve charms from this. Wouldn't something like webdav or the local provider http storage be more appropriate?

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Kapil, this is only used during bootstrap to get the tools to the bootstrap host. Once the tools are in place and the machine agent is installed, it manages a "local storage" just like the local provider.

Revision history for this message
William Reade (fwereade) wrote :

Sorry, a couple more tweaks needed for the atomic bit.

But still, this CL allows progress and is only problematic in relatively
rare circumstances, so LGTM as long as there's an immediate followup
addressing the comments.

https://codereview.appspot.com/13243046/diff/12001/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/13243046/diff/12001/environs/sshstorage/storage.go#newcode224
environs/sshstorage/storage.go:224: command = fmt.Sprintf("T=`mktemp` &&
((%s && mv $T %s) || rm -f $T)", command, path)
/tmp isn't necessarily on the same filesystem as the destination, so the
mv is not necessarily atomic.

Also, could we call it something a bit more expressive than $T please?

https://codereview.appspot.com/13243046/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/13243046/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/13243046/diff/12001/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/13243046/diff/12001/environs/sshstorage/storage.go#newcode224
environs/sshstorage/storage.go:224: command = fmt.Sprintf("T=`mktemp` &&
((%s && mv $T %s) || rm -f $T)", command, path)
On 2013/09/13 07:32:51, fwereade wrote:
> /tmp isn't necessarily on the same filesystem as the destination, so
 the mv is
> not necessarily atomic.

Right, thanks for that. I've changed the code so that files are now
stored under a subdirectory (content), and there's now a temporary dir
(tmp) that we set TMPDIR to at initialisation time.

> Also, could we call it something a bit more expressive than $T please?

Sure. $TMPFILE, in keeping with $TMPDIR.

https://codereview.appspot.com/13243046/

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

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

ok launchpad.net/juju-core/agent 0.780s
ok launchpad.net/juju-core/agent/tools 0.266s
ok launchpad.net/juju-core/bzr 7.454s
ok launchpad.net/juju-core/cert 3.512s
ok launchpad.net/juju-core/charm 0.531s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.020s
ok launchpad.net/juju-core/cmd 0.218s
? 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 135.422s
ok launchpad.net/juju-core/cmd/jujud 39.847s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 1.905s
ok launchpad.net/juju-core/constraints 0.030s
ok launchpad.net/juju-core/container/lxc 0.272s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.261s
ok launchpad.net/juju-core/environs 1.715s
ok launchpad.net/juju-core/environs/bootstrap 4.972s
ok launchpad.net/juju-core/environs/cloudinit 0.413s
ok launchpad.net/juju-core/environs/config 0.715s
ok launchpad.net/juju-core/environs/filestorage 0.055s
ok launchpad.net/juju-core/environs/imagemetadata 0.510s
ok launchpad.net/juju-core/environs/instances 0.213s
ok launchpad.net/juju-core/environs/jujutest 0.242s
ok launchpad.net/juju-core/environs/localstorage 0.233s
ok launchpad.net/juju-core/environs/manual 1.491s
ok launchpad.net/juju-core/environs/simplestreams 0.315s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]

----------------------------------------------------------------------
FAIL: storage_test.go:123: storageSuite.TestGet

storage_test.go:141:
    c.Assert(err, jc.Satisfies, coreerrors.IsNotFoundError)
... obtained sshstorage.SSHStorageError = sshstorage.SSHStorageError{Output:"/bin/sh: 1: cannot open /tmp/gocheck-5577006791947779410/2/content/notthere: No such file", ExitCode:2} ("/bin/sh: 1: cannot open /tmp/gocheck-5577006791947779410/2/content/notthere: No such file")
... func(T) bool func(error) bool = (func(error) bool)(0x4af890)

OOPS: 9 passed, 1 FAILED
--- FAIL: Test (0.44 seconds)
FAIL
FAIL launchpad.net/juju-core/environs/sshstorage 0.663s
ok launchpad.net/juju-core/environs/sync 0.262s
ok launchpad.net/juju-core/environs/testing 0.011s
ok launchpad.net/juju-core/environs/tools 33.970s
? 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.021s
ok launchpad.net/juju-core/juju 16.961s
ok launchpad.net/juju-core/juju/osenv 0.209s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.017s
ok launchpad.net/juju-core/log/syslog 0.022s
ok launchpad.net/juju-core/names 0.028s
ok launchpad.net/juju-core/provider 0.294s
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 6.487s
ok launchpad.net/juju-core/provider/dummy 19.302s
ok launchpad.net/juju-core/provider/ec2 5.43...

Read more...

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

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

ok launchpad.net/juju-core/agent 0.763s
ok launchpad.net/juju-core/agent/tools 0.238s
ok launchpad.net/juju-core/bzr 7.619s
ok launchpad.net/juju-core/cert 2.185s
ok launchpad.net/juju-core/charm 0.544s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.022s
ok launchpad.net/juju-core/cmd 0.251s
? 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 134.663s
ok launchpad.net/juju-core/cmd/jujud 39.788s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 1.917s
ok launchpad.net/juju-core/constraints 0.039s
ok launchpad.net/juju-core/container/lxc 0.358s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.301s
ok launchpad.net/juju-core/environs 1.650s
ok launchpad.net/juju-core/environs/bootstrap 5.046s
ok launchpad.net/juju-core/environs/cloudinit 0.426s
ok launchpad.net/juju-core/environs/config 0.735s
ok launchpad.net/juju-core/environs/filestorage 0.060s
ok launchpad.net/juju-core/environs/imagemetadata 0.501s
ok launchpad.net/juju-core/environs/instances 0.262s
ok launchpad.net/juju-core/environs/jujutest 0.211s
ok launchpad.net/juju-core/environs/localstorage 0.247s
ok launchpad.net/juju-core/environs/manual 1.356s
ok launchpad.net/juju-core/environs/simplestreams 0.237s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.691s
ok launchpad.net/juju-core/environs/sync 0.318s
ok launchpad.net/juju-core/environs/testing 0.011s
ok launchpad.net/juju-core/environs/tools 33.319s
? 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.019s
ok launchpad.net/juju-core/juju 17.342s
ok launchpad.net/juju-core/juju/osenv 0.227s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.020s
ok launchpad.net/juju-core/log/syslog 0.030s
ok launchpad.net/juju-core/names 0.023s
ok launchpad.net/juju-core/provider 0.302s
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 6.591s
ok launchpad.net/juju-core/provider/dummy 19.485s
ok launchpad.net/juju-core/provider/ec2 5.472s
ok launchpad.net/juju-core/provider/local 1.746s
? launchpad.net/juju-core/provider/local/storage [no test files]
ok launchpad.net/juju-core/provider/maas 3.644s
ok launchpad.net/juju-core/provider/openstack 4.617s
ok launchpad.net/juju-core/rpc 0.249s
ok launchpad.net/juju-core/rpc/jsoncodec 0.208s
ok launchpad.net/juju-core/schema 0.018s
ok launchpad.net/juju-core/state 70.568s
ok launchpad.net/juju-core/state/api 1.464s
ok launchpad.net/juju-core/state/api/agent 2.075s
? launchpad.net/juju-core/state/api/common [no test files]
ok launchpad.net/juju-core/state/api/d...

Read more...

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

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

ok launchpad.net/juju-core/agent 0.711s
ok launchpad.net/juju-core/agent/tools 0.287s
ok launchpad.net/juju-core/bzr 7.013s
ok launchpad.net/juju-core/cert 2.084s
ok launchpad.net/juju-core/charm 0.558s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.020s
ok launchpad.net/juju-core/cmd 0.214s
? 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 135.938s
ok launchpad.net/juju-core/cmd/jujud 39.837s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 1.931s
ok launchpad.net/juju-core/constraints 0.046s
ok launchpad.net/juju-core/container/lxc 0.316s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.279s
ok launchpad.net/juju-core/environs 1.572s
ok launchpad.net/juju-core/environs/bootstrap 5.030s
ok launchpad.net/juju-core/environs/cloudinit 0.452s
ok launchpad.net/juju-core/environs/config 0.775s
ok launchpad.net/juju-core/environs/filestorage 0.058s
ok launchpad.net/juju-core/environs/imagemetadata 0.560s
ok launchpad.net/juju-core/environs/instances 0.260s
ok launchpad.net/juju-core/environs/jujutest 0.250s
ok launchpad.net/juju-core/environs/localstorage 0.251s
ok launchpad.net/juju-core/environs/manual 0.995s
ok launchpad.net/juju-core/environs/simplestreams 0.243s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.590s
ok launchpad.net/juju-core/environs/sync 0.282s
ok launchpad.net/juju-core/environs/testing 0.011s
ok launchpad.net/juju-core/environs/tools 34.272s
? 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.020s
ok launchpad.net/juju-core/juju 17.329s
ok launchpad.net/juju-core/juju/osenv 0.246s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.017s
ok launchpad.net/juju-core/log/syslog 0.034s
ok launchpad.net/juju-core/names 0.036s
ok launchpad.net/juju-core/provider 0.292s
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 6.673s
ok launchpad.net/juju-core/provider/dummy 18.727s
ok launchpad.net/juju-core/provider/ec2 5.276s
ok launchpad.net/juju-core/provider/local 1.824s
? launchpad.net/juju-core/provider/local/storage [no test files]
ok launchpad.net/juju-core/provider/maas 3.663s
ok launchpad.net/juju-core/provider/openstack 4.874s
ok launchpad.net/juju-core/rpc 0.310s
ok launchpad.net/juju-core/rpc/jsoncodec 0.243s
ok launchpad.net/juju-core/schema 0.020s
ok launchpad.net/juju-core/state 69.152s
ok launchpad.net/juju-core/state/api 1.438s
ok launchpad.net/juju-core/state/api/agent 2.096s
? launchpad.net/juju-core/state/api/common [no test files]
ok launchpad.net/juju-core/state/api/d...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'environs/sshstorage'
2=== added file 'environs/sshstorage/error.go'
3--- environs/sshstorage/error.go 1970-01-01 00:00:00 +0000
4+++ environs/sshstorage/error.go 2013-09-13 08:39:27 +0000
5@@ -0,0 +1,20 @@
6+// Copyright 2013 Canonical Ltd.
7+// Licensed under the AGPLv3, see LICENCE file for details.
8+
9+package sshstorage
10+
11+import (
12+ "fmt"
13+)
14+
15+type SSHStorageError struct {
16+ Output string
17+ ExitCode int
18+}
19+
20+func (e SSHStorageError) Error() string {
21+ if e.Output == "" {
22+ return fmt.Sprintf("exit code %d", e.ExitCode)
23+ }
24+ return e.Output
25+}
26
27=== added file 'environs/sshstorage/storage.go'
28--- environs/sshstorage/storage.go 1970-01-01 00:00:00 +0000
29+++ environs/sshstorage/storage.go 2013-09-13 08:39:27 +0000
30@@ -0,0 +1,272 @@
31+// Copyright 2013 Canonical Ltd.
32+// Licensed under the AGPLv3, see LICENCE file for details.
33+
34+package sshstorage
35+
36+import (
37+ "bufio"
38+ "bytes"
39+ "encoding/base64"
40+ "fmt"
41+ "io"
42+ "io/ioutil"
43+ "os"
44+ "os/exec"
45+ "path"
46+ "sort"
47+ "strconv"
48+ "strings"
49+
50+ coreerrors "launchpad.net/juju-core/errors"
51+ "launchpad.net/juju-core/utils"
52+)
53+
54+const (
55+ // tmpdir is the name of the subdirectory
56+ // inside the remote storage directory where
57+ // temporary files are created.
58+ tmpdir = "tmp"
59+
60+ // contentdir is the name of the subdirectory
61+ // inside the remote storage directory where
62+ // files are stored.
63+ contentdir = "content"
64+)
65+
66+// SSHStorage implements environs.Storage.
67+//
68+// The storage is created under sudo, and ownership given over to the
69+// login uid/gid. This is done so that we don't require sudo, and by
70+// consequence, don't require a pty, so we can interact with a script
71+// via stdin.
72+type SSHStorage struct {
73+ host string
74+ remotepath string
75+
76+ cmd *exec.Cmd
77+ stdin io.WriteCloser
78+ stdout io.ReadCloser
79+ scanner *bufio.Scanner
80+}
81+
82+var sshCommand = func(host string, tty bool, command string) *exec.Cmd {
83+ sshArgs := []string{host}
84+ if tty {
85+ sshArgs = append(sshArgs, "-t")
86+ }
87+ sshArgs = append(sshArgs, "--", command)
88+ return exec.Command("ssh", sshArgs...)
89+}
90+
91+type flockmode string
92+
93+const (
94+ flockShared flockmode = "-s"
95+ flockExclusive flockmode = "-x"
96+)
97+
98+// NewSSHStorage creates a new SSHStorage, connected to the
99+// specified host, managing state under the specified remote path.
100+func NewSSHStorage(host string, remotepath string) (*SSHStorage, error) {
101+ contentdir := path.Join(remotepath, contentdir)
102+ tmpdir := path.Join(remotepath, tmpdir)
103+ script := fmt.Sprintf("install -d -g $SUDO_GID -o $SUDO_UID %s %s", contentdir, tmpdir)
104+ cmd := sshCommand(host, true, fmt.Sprintf("sudo bash -c '%s'", script))
105+ cmd.Stdin = os.Stdin
106+ if out, err := cmd.CombinedOutput(); err != nil {
107+ err = fmt.Errorf("failed to create storage dir: %v (%v)", err, strings.TrimSpace(string(out)))
108+ return nil, err
109+ }
110+
111+ // We could use sftp, but then we'd be at the mercy of
112+ // sftp's output messages for checking errors. Instead,
113+ // we execute an interactive bash shell.
114+ cmd = sshCommand(host, false, "bash")
115+ stdin, err := cmd.StdinPipe()
116+ if err != nil {
117+ return nil, err
118+ }
119+ stdout, err := cmd.StdoutPipe()
120+ if err != nil {
121+ stdin.Close()
122+ return nil, err
123+ }
124+ storage := &SSHStorage{
125+ host: host,
126+ remotepath: remotepath,
127+ cmd: cmd,
128+ stdin: stdin,
129+ stdout: stdout,
130+ scanner: bufio.NewScanner(stdout),
131+ }
132+ cmd.Start()
133+
134+ // Verify we have write permissions, and set the temporary directory.
135+ _, err = storage.runf(
136+ flockExclusive,
137+ "touch %s && export TMPDIR=%s",
138+ utils.ShQuote(remotepath),
139+ utils.ShQuote(tmpdir),
140+ )
141+ if err != nil {
142+ stdin.Close()
143+ stdout.Close()
144+ cmd.Wait()
145+ return nil, err
146+ }
147+ return storage, nil
148+}
149+
150+// Close cleanly terminates the underlying SSH connection.
151+func (s *SSHStorage) Close() error {
152+ s.stdin.Close()
153+ s.stdout.Close()
154+ return s.cmd.Wait()
155+}
156+
157+func (s *SSHStorage) runf(flockmode flockmode, command string, args ...interface{}) (string, error) {
158+ command = fmt.Sprintf(command, args...)
159+ return s.run(flockmode, command)
160+}
161+
162+func (s *SSHStorage) run(flockmode flockmode, command string) (string, error) {
163+ const rcPrefix = "JUJU-RC: "
164+ command = fmt.Sprintf(
165+ "(flock %s %s -c %s) 2>&1; echo %s$?",
166+ flockmode,
167+ s.remotepath,
168+ utils.ShQuote(command),
169+ rcPrefix,
170+ )
171+ if _, err := s.stdin.Write([]byte(command + "\r\n")); err != nil {
172+ return "", fmt.Errorf("failed to write command: %v", err)
173+ }
174+ var output []string
175+ for s.scanner.Scan() {
176+ line := s.scanner.Text()
177+ if strings.HasPrefix(line, rcPrefix) {
178+ line := line[len(rcPrefix):]
179+ rc, err := strconv.Atoi(line)
180+ if err != nil {
181+ return "", fmt.Errorf("failed to parse exit code %q: %v", line, err)
182+ }
183+ outputJoined := strings.Join(output, "\n")
184+ if rc == 0 {
185+ return outputJoined, nil
186+ }
187+ return "", SSHStorageError{outputJoined, rc}
188+ } else {
189+ output = append(output, line)
190+ }
191+ }
192+ return "", s.scanner.Err()
193+}
194+
195+// path returns a remote absolute path for a storage object name.
196+func (s *SSHStorage) path(name string) (string, error) {
197+ contentdir := path.Join(s.remotepath, contentdir)
198+ remotepath := path.Clean(path.Join(contentdir, name))
199+ if !strings.HasPrefix(remotepath, contentdir) {
200+ return "", fmt.Errorf("%q escapes storage directory", name)
201+ }
202+ return remotepath, nil
203+}
204+
205+// Get implements environs.StorageReader.Get.
206+func (s *SSHStorage) Get(name string) (io.ReadCloser, error) {
207+ path, err := s.path(name)
208+ if err != nil {
209+ return nil, err
210+ }
211+ out, err := s.runf(flockShared, "base64 < %s", utils.ShQuote(path))
212+ if err != nil {
213+ err := err.(SSHStorageError)
214+ if strings.Contains(err.Output, "No such file") {
215+ return nil, coreerrors.NewNotFoundError(err, "")
216+ }
217+ return nil, err
218+ }
219+ decoded, err := base64.StdEncoding.DecodeString(out)
220+ if err != nil {
221+ return nil, err
222+ }
223+ return ioutil.NopCloser(bytes.NewBuffer(decoded)), nil
224+}
225+
226+// List implements environs.StorageReader.List.
227+func (s *SSHStorage) List(prefix string) ([]string, error) {
228+ remotepath, err := s.path(prefix)
229+ if err != nil {
230+ return nil, err
231+ }
232+ dir, prefix := path.Split(remotepath)
233+ quotedDir := utils.ShQuote(dir)
234+ out, err := s.runf(flockShared, "(test -d %s && find %s -type f) || true", quotedDir, quotedDir)
235+ if err != nil {
236+ return nil, err
237+ }
238+ if out == "" {
239+ return nil, nil
240+ }
241+ var names []string
242+ contentdir := path.Join(s.remotepath, contentdir)
243+ for _, name := range strings.Split(out, "\n") {
244+ if strings.HasPrefix(name[len(dir):], prefix) {
245+ names = append(names, name[len(contentdir)+1:])
246+ }
247+ }
248+ sort.Strings(names)
249+ return names, nil
250+}
251+
252+// URL implements environs.StorageReader.URL.
253+func (s *SSHStorage) URL(name string) (string, error) {
254+ path, err := s.path(name)
255+ if err != nil {
256+ return "", err
257+ }
258+ return fmt.Sprintf("sftp://%s/%s", s.host, path), nil
259+}
260+
261+// ConsistencyStrategy implements environs.StorageReader.ConsistencyStrategy.
262+func (s *SSHStorage) ConsistencyStrategy() utils.AttemptStrategy {
263+ return utils.AttemptStrategy{}
264+}
265+
266+// Put implements environs.StorageWriter.Put
267+func (s *SSHStorage) Put(name string, r io.Reader, length int64) error {
268+ path, err := s.path(name)
269+ if err != nil {
270+ return err
271+ }
272+ buf := make([]byte, length)
273+ if _, err := r.Read(buf); err != nil {
274+ return err
275+ }
276+ encoded := base64.StdEncoding.EncodeToString(buf)
277+ path = utils.ShQuote(path)
278+ // Write to a temporary file ($TMPFILE), then mv atomically.
279+ command := fmt.Sprintf("mkdir -p `dirname %s` && base64 -d > $TMPFILE", path)
280+ command = fmt.Sprintf("TMPFILE=`mktemp` && ((%s && mv $TMPFILE %s) || rm -f $TMPFILE)", command, path)
281+ command = fmt.Sprintf("(%s) << EOF\n%s\nEOF", command, encoded)
282+ _, err = s.runf(flockExclusive, command+"\n")
283+ return err
284+}
285+
286+// Remove implements environs.StorageWriter.Remove
287+func (s *SSHStorage) Remove(name string) error {
288+ path, err := s.path(name)
289+ if err != nil {
290+ return err
291+ }
292+ path = utils.ShQuote(path)
293+ _, err = s.runf(flockExclusive, "rm -f %s", path)
294+ return err
295+}
296+
297+// RemoveAll implements environs.StorageWriter.RemoveAll
298+func (s *SSHStorage) RemoveAll() error {
299+ contentdir := path.Join(s.remotepath, contentdir)
300+ _, err := s.runf(flockExclusive, "rm -fr %s/*", utils.ShQuote(contentdir))
301+ return err
302+}
303
304=== added file 'environs/sshstorage/storage_test.go'
305--- environs/sshstorage/storage_test.go 1970-01-01 00:00:00 +0000
306+++ environs/sshstorage/storage_test.go 2013-09-13 08:39:27 +0000
307@@ -0,0 +1,305 @@
308+// Copyright 2013 Canonical Ltd.
309+// Licensed under the AGPLv3, see LICENCE file for details.
310+
311+package sshstorage
312+
313+import (
314+ "bytes"
315+ "fmt"
316+ "io/ioutil"
317+ "os"
318+ "os/exec"
319+ "path"
320+ "path/filepath"
321+ "regexp"
322+ stdtesting "testing"
323+ "time"
324+
325+ gc "launchpad.net/gocheck"
326+
327+ "launchpad.net/juju-core/environs"
328+ coreerrors "launchpad.net/juju-core/errors"
329+ "launchpad.net/juju-core/testing"
330+ jc "launchpad.net/juju-core/testing/checkers"
331+ "launchpad.net/juju-core/utils"
332+)
333+
334+type storageSuite struct {
335+ testing.LoggingSuite
336+ restoreEnv func()
337+}
338+
339+var _ = gc.Suite(&storageSuite{})
340+
341+func Test(t *stdtesting.T) {
342+ gc.TestingT(t)
343+}
344+
345+var sshCommandOrig = sshCommand
346+
347+func sshCommandTesting(host string, tty bool, command string) *exec.Cmd {
348+ cmd := exec.Command("bash", "-c", command)
349+ uid := fmt.Sprint(os.Getuid())
350+ gid := fmt.Sprint(os.Getgid())
351+ defer testing.PatchEnvironment("SUDO_UID", uid)()
352+ defer testing.PatchEnvironment("SUDO_GID", gid)()
353+ cmd.Env = os.Environ()
354+ return cmd
355+}
356+
357+// flockBin is the path to the original "flock" binary.
358+var flockBin string
359+
360+func (s *storageSuite) SetUpSuite(c *gc.C) {
361+ s.LoggingSuite.SetUpSuite(c)
362+
363+ var err error
364+ flockBin, err = exec.LookPath("flock")
365+ c.Assert(err, gc.IsNil)
366+
367+ bin := c.MkDir()
368+ s.restoreEnv = testing.PatchEnvironment("PATH", bin+":"+os.Getenv("PATH"))
369+
370+ // Create a "sudo" command which just executes its args.
371+ c.Assert(os.Symlink("/usr/bin/env", filepath.Join(bin, "sudo")), gc.IsNil)
372+ sshCommand = sshCommandTesting
373+
374+ // Create a new "flock" which calls the original, but in non-blocking mode.
375+ data := []byte(fmt.Sprintf("#!/bin/sh\nexec %s --nonblock \"$@\"", flockBin))
376+ c.Assert(ioutil.WriteFile(filepath.Join(bin, "flock"), data, 0755), gc.IsNil)
377+}
378+
379+func (s *storageSuite) TearDownSuite(c *gc.C) {
380+ sshCommand = sshCommandOrig
381+ s.restoreEnv()
382+ s.LoggingSuite.TearDownSuite(c)
383+}
384+
385+func (s *storageSuite) TestNewSSHStorage(c *gc.C) {
386+ storageDir := c.MkDir()
387+ for i := 0; i < 2; i++ {
388+ storage, err := NewSSHStorage("example.com", storageDir)
389+ c.Assert(err, gc.IsNil)
390+ c.Assert(storage, gc.NotNil)
391+ c.Assert(storage.Close(), gc.IsNil)
392+ }
393+ c.Assert(os.RemoveAll(storageDir), gc.IsNil)
394+
395+ // You must have permissions to create the directory.
396+ storageDir = c.MkDir()
397+ c.Assert(os.Chmod(storageDir, 0555), gc.IsNil)
398+ _, err := NewSSHStorage("example.com", filepath.Join(storageDir))
399+ c.Assert(err, gc.ErrorMatches, "(.|\n)*cannot change owner and permissions of(.|\n)*")
400+}
401+
402+func (s *storageSuite) TestPathValidity(c *gc.C) {
403+ storageDir := c.MkDir()
404+ storage, err := NewSSHStorage("example.com", storageDir)
405+ c.Assert(err, gc.IsNil)
406+ defer storage.Close()
407+
408+ c.Assert(os.Mkdir(filepath.Join(storageDir, contentdir, "a"), 0755), gc.IsNil)
409+ f, err := os.Create(filepath.Join(storageDir, contentdir, "a", "b"))
410+ c.Assert(err, gc.IsNil)
411+ c.Assert(f.Close(), gc.IsNil)
412+
413+ for _, prefix := range []string{"..", "a/../.."} {
414+ c.Logf("prefix: %q", prefix)
415+ _, err := storage.List(prefix)
416+ c.Check(err, gc.ErrorMatches, regexp.QuoteMeta(fmt.Sprintf("%q escapes storage directory", prefix)))
417+ }
418+
419+ // Paths are always relative, so a leading "/" may as well not be there.
420+ names, err := storage.List("/")
421+ c.Assert(err, gc.IsNil)
422+ c.Assert(names, gc.DeepEquals, []string{"a/b"})
423+
424+ // Paths will be canonicalised.
425+ names, err = storage.List("a/..")
426+ c.Assert(err, gc.IsNil)
427+ c.Assert(names, gc.DeepEquals, []string{"a/b"})
428+}
429+
430+func (s *storageSuite) TestGet(c *gc.C) {
431+ storageDir := c.MkDir()
432+ storage, err := NewSSHStorage("example.com", storageDir)
433+ c.Assert(err, gc.IsNil)
434+ defer storage.Close()
435+ data := []byte("abc\000def")
436+ c.Assert(os.Mkdir(filepath.Join(storageDir, contentdir, "a"), 0755), gc.IsNil)
437+ for _, name := range []string{"b", filepath.Join("a", "b")} {
438+ err = ioutil.WriteFile(filepath.Join(storageDir, contentdir, name), data, 0644)
439+ c.Assert(err, gc.IsNil)
440+ r, err := storage.Get(name)
441+ c.Assert(err, gc.IsNil)
442+ out, err := ioutil.ReadAll(r)
443+ c.Assert(err, gc.IsNil)
444+ c.Assert(out, gc.DeepEquals, data)
445+ }
446+
447+ _, err = storage.Get("notthere")
448+ c.Assert(err, jc.Satisfies, coreerrors.IsNotFoundError)
449+}
450+
451+func (s *storageSuite) TestPut(c *gc.C) {
452+ storageDir := c.MkDir()
453+ storage, err := NewSSHStorage("example.com", storageDir)
454+ c.Assert(err, gc.IsNil)
455+ defer storage.Close()
456+ data := []byte("abc\000def")
457+ for _, name := range []string{"b", filepath.Join("a", "b")} {
458+ err = storage.Put(name, bytes.NewBuffer(data), int64(len(data)))
459+ c.Assert(err, gc.IsNil)
460+ out, err := ioutil.ReadFile(filepath.Join(storageDir, contentdir, name))
461+ c.Assert(err, gc.IsNil)
462+ c.Assert(out, gc.DeepEquals, data)
463+ }
464+}
465+
466+func (s *storageSuite) assertList(c *gc.C, storage environs.StorageReader, prefix string, expected []string) {
467+ c.Logf("List: %v", prefix)
468+ names, err := storage.List(prefix)
469+ c.Assert(err, gc.IsNil)
470+ c.Assert(names, gc.DeepEquals, expected)
471+}
472+
473+func (s *storageSuite) TestList(c *gc.C) {
474+ storageDir := c.MkDir()
475+ storage, err := NewSSHStorage("example.com", storageDir)
476+ c.Assert(err, gc.IsNil)
477+ defer storage.Close()
478+ s.assertList(c, storage, "", nil)
479+
480+ // Directories don't show up in List.
481+ contentDir := filepath.Join(storageDir, contentdir)
482+ c.Assert(os.Mkdir(filepath.Join(contentDir, "a"), 0755), gc.IsNil)
483+ s.assertList(c, storage, "", nil)
484+ s.assertList(c, storage, "a", nil)
485+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "a", "b1"), nil, 0), gc.IsNil)
486+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "a", "b2"), nil, 0), gc.IsNil)
487+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "b"), nil, 0), gc.IsNil)
488+ s.assertList(c, storage, "", []string{"a/b1", "a/b2", "b"})
489+ s.assertList(c, storage, "a", []string{"a/b1", "a/b2"})
490+ s.assertList(c, storage, "a/b", []string{"a/b1", "a/b2"})
491+ s.assertList(c, storage, "a/b1", []string{"a/b1"})
492+ s.assertList(c, storage, "a/b3", nil)
493+ s.assertList(c, storage, "a/b/c", nil)
494+ s.assertList(c, storage, "b", []string{"b"})
495+}
496+
497+func (s *storageSuite) TestRemove(c *gc.C) {
498+ storageDir := c.MkDir()
499+ storage, err := NewSSHStorage("example.com", storageDir)
500+ c.Assert(err, gc.IsNil)
501+ defer storage.Close()
502+
503+ contentDir := filepath.Join(storageDir, contentdir)
504+ c.Assert(os.Mkdir(filepath.Join(contentDir, "a"), 0755), gc.IsNil)
505+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "a", "b1"), nil, 0), gc.IsNil)
506+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "a", "b2"), nil, 0), gc.IsNil)
507+ c.Assert(storage.Remove("a"), gc.ErrorMatches, "rm: cannot remove.*Is a directory")
508+ s.assertList(c, storage, "", []string{"a/b1", "a/b2"})
509+ c.Assert(storage.Remove("a/b"), gc.IsNil) // doesn't exist; not an error
510+ s.assertList(c, storage, "", []string{"a/b1", "a/b2"})
511+ c.Assert(storage.Remove("a/b2"), gc.IsNil)
512+ s.assertList(c, storage, "", []string{"a/b1"})
513+ c.Assert(storage.Remove("a/b1"), gc.IsNil)
514+ s.assertList(c, storage, "", nil)
515+}
516+
517+func (s *storageSuite) TestRemoveAll(c *gc.C) {
518+ storageDir := c.MkDir()
519+ storage, err := NewSSHStorage("example.com", storageDir)
520+ c.Assert(err, gc.IsNil)
521+ defer storage.Close()
522+
523+ contentDir := filepath.Join(storageDir, contentdir)
524+ c.Assert(os.Mkdir(filepath.Join(contentDir, "a"), 0755), gc.IsNil)
525+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "a", "b1"), nil, 0), gc.IsNil)
526+ c.Assert(ioutil.WriteFile(filepath.Join(contentDir, "a", "b2"), nil, 0), gc.IsNil)
527+ s.assertList(c, storage, "", []string{"a/b1", "a/b2"})
528+ c.Assert(storage.RemoveAll(), gc.IsNil)
529+ s.assertList(c, storage, "", nil)
530+
531+ // RemoveAll does not remove the base storage directory.
532+ _, err = os.Stat(contentDir)
533+ c.Assert(err, gc.IsNil)
534+}
535+
536+func (s *storageSuite) TestURL(c *gc.C) {
537+ storageDir := c.MkDir()
538+ storage, err := NewSSHStorage("example.com", storageDir)
539+ c.Assert(err, gc.IsNil)
540+ defer storage.Close()
541+ url, err := storage.URL("a/b")
542+ c.Assert(err, gc.IsNil)
543+ c.Assert(url, gc.Equals, "sftp://example.com/"+path.Join(storageDir, contentdir, "a/b"))
544+}
545+
546+func (s *storageSuite) TestConsistencyStrategy(c *gc.C) {
547+ storageDir := c.MkDir()
548+ storage, err := NewSSHStorage("example.com", storageDir)
549+ c.Assert(err, gc.IsNil)
550+ defer storage.Close()
551+ c.Assert(storage.ConsistencyStrategy(), gc.Equals, utils.AttemptStrategy{})
552+}
553+
554+// flock is a test helper that flocks a file,
555+// executes "sleep" with the specified duration,
556+// and returns the *Cmd so it can be early terminated.
557+func (s *storageSuite) flock(c *gc.C, mode flockmode, lockfile string, duration time.Duration) *os.Process {
558+ sleepcmd := fmt.Sprintf("sleep %vs", duration.Seconds())
559+ cmd := exec.Command(flockBin, "--nonblock", "--close", string(mode), lockfile, "-c", sleepcmd)
560+ c.Assert(cmd.Start(), gc.IsNil)
561+ return cmd.Process
562+}
563+
564+const defaultFlockTimeout = 5 * time.Second
565+
566+func (s *storageSuite) TestSynchronisation(c *gc.C) {
567+ storageDir := c.MkDir()
568+ proc := s.flock(c, flockShared, storageDir, defaultFlockTimeout)
569+ defer proc.Wait()
570+ defer proc.Kill()
571+
572+ // Creating storage requires an exclusive lock initially.
573+ //
574+ // flock exits with exit code 1 if it can't acquire the
575+ // lock immediately in non-blocking mode (which the tests force).
576+ _, err := NewSSHStorage("example.com", storageDir)
577+ c.Assert(err, gc.ErrorMatches, "exit code 1")
578+
579+ proc.Kill()
580+ proc.Wait()
581+ storage, err := NewSSHStorage("example.com", storageDir)
582+ c.Assert(err, gc.IsNil)
583+
584+ // Get and List should be able to proceed with a shared lock.
585+ // All other methods should fail.
586+ data := []byte("abc\000def")
587+ c.Assert(ioutil.WriteFile(filepath.Join(storageDir, contentdir, "a"), data, 0644), gc.IsNil)
588+
589+ proc = s.flock(c, flockShared, storageDir, defaultFlockTimeout)
590+ _, err = storage.Get("a")
591+ c.Assert(err, gc.IsNil)
592+ _, err = storage.List("")
593+ c.Assert(err, gc.IsNil)
594+ c.Assert(storage.Put("a", bytes.NewBuffer(nil), 0), gc.NotNil)
595+ c.Assert(storage.Remove("a"), gc.NotNil)
596+ c.Assert(storage.RemoveAll(), gc.NotNil)
597+ proc.Kill()
598+ proc.Wait()
599+
600+ // None of the methods (apart from URL) should be able to do anything
601+ // while an exclusive lock is held.
602+ proc = s.flock(c, flockExclusive, storageDir, defaultFlockTimeout)
603+ _, err = storage.URL("a")
604+ c.Assert(err, gc.IsNil)
605+ c.Assert(storage.Put("a", bytes.NewBuffer(nil), 0), gc.NotNil)
606+ c.Assert(storage.Remove("a"), gc.NotNil)
607+ c.Assert(storage.RemoveAll(), gc.NotNil)
608+ _, err = storage.Get("a")
609+ c.Assert(err, gc.NotNil)
610+ _, err = storage.List("")
611+ c.Assert(err, gc.NotNil)
612+}

Subscribers

People subscribed via source and target branches

to status/vote changes: