juju-core

Merge lp:~axwalk/juju-core/lp1270252-local-invalid-instances into lp:~go-bot/juju-core/trunk

  1. lp1270252-local-invalid-instances
  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: 2236
Proposed branch: lp:~axwalk/juju-core/lp1270252-local-invalid-instances
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 44 lines (+23/-7)
1 file modified
provider/local/environ.go (+23/-7)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1270252-local-invalid-instances
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+202572@code.launchpad.net

Commit message

provider/local: check ID validity in Instances

The Instances method in the local provider was
not checking whether ids are valid. Thus,
DestroyEnvironment would fail when it maps when
machine in state remain but their instances are
already destroyed.

Instances is updated to check the existence of
instances, and return ErrPartialInstances or
ErrNoInstances accordingly.

Fixes #1270252

https://codereview.appspot.com/51230044/

Description of the change

provider/local: check ID validity in Instances

The Instances method in the local provider was
not checking whether ids are valid. Thus,
DestroyEnvironment would fail when it maps when
machine in state remain but their instances are
already destroyed.

Instances is updated to check the existence of
instances, and return ErrPartialInstances or
ErrNoInstances accordingly.

Fixes #1270252

https://codereview.appspot.com/51230044/

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

Reviewers: mp+202572_code.launchpad.net,

Message:
Please take a look.

Description:
provider/local: check ID validity in Instances

The Instances method in the local provider was
not checking whether ids are valid. Thus,
DestroyEnvironment would fail when it maps when
machine in state remain but their instances are
already destroyed.

Instances is updated to check the existence of
instances, and return ErrPartialInstances or
ErrNoInstances accordingly.

Fixes #1270252

https://code.launchpad.net/~axwalk/juju-core/lp1270252-local-invalid-instances/+merge/202572

(do not edit description out of merge proposal)

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

Affected files (+25, -7 lines):
   A [revision details]
   M provider/local/environ.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-20140121052439-o3gacqa1rvnhb3lb
+New revision: <email address hidden>

Index: provider/local/environ.go
=== modified file 'provider/local/environ.go'
--- provider/local/environ.go 2014-01-05 22:20:24 +0000
+++ provider/local/environ.go 2014-01-21 23:04:24 +0000
@@ -325,17 +325,33 @@

  // Instances is specified in the Environ interface.
  func (env *localEnviron) Instances(ids []instance.Id)
([]instance.Instance, error) {
- // NOTE: do we actually care about checking the existance of the
instances?
- // I posit that here we don't really care, and that we are only called
with
- // instance ids that we know exist.
   if len(ids) == 0 {
    return nil, nil
   }
- insts := make([]instance.Instance, len(ids))
+ insts, err := env.AllInstances()
+ if err != nil {
+ return nil, err
+ }
+ allInstances := make(map[instance.Id]instance.Instance)
+ for _, inst := range insts {
+ allInstances[inst.Id()] = inst
+ }
+ var found int
+ insts = make([]instance.Instance, len(ids))
   for i, id := range ids {
- insts[i] = &localInstance{id, env}
- }
- return insts, nil
+ if inst, ok := allInstances[id]; ok {
+ insts[i] = inst
+ found++
+ }
+ }
+ if found == 0 {
+ insts, err = nil, environs.ErrNoInstances
+ } else if found < len(ids) {
+ err = environs.ErrPartialInstances
+ } else {
+ err = nil
+ }
+ return insts, err
  }

  // AllInstances is specified in the InstanceBroker interface.

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM. Not an issue for this branch, but Instances() and AllInstances()
need tests at some point

https://codereview.appspot.com/51230044/

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

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

ok launchpad.net/juju-core/agent 1.308s
ok launchpad.net/juju-core/agent/tools 0.271s
ok launchpad.net/juju-core/bzr 7.758s
ok launchpad.net/juju-core/cert 3.484s
ok launchpad.net/juju-core/charm 0.600s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.032s
ok launchpad.net/juju-core/cloudinit/sshinit 1.100s
ok launchpad.net/juju-core/cmd 0.246s
ok launchpad.net/juju-core/cmd/charm-admin 0.874s
? 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 220.174s
ok launchpad.net/juju-core/cmd/jujud 54.341s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 10.521s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.027s
ok launchpad.net/juju-core/container 0.055s
ok launchpad.net/juju-core/container/factory 0.054s
ok launchpad.net/juju-core/container/kvm 0.301s
ok launchpad.net/juju-core/container/kvm/mock 0.049s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.380s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.317s
ok launchpad.net/juju-core/environs 3.080s
ok launchpad.net/juju-core/environs/bootstrap 4.474s
ok launchpad.net/juju-core/environs/cloudinit 0.528s
ok launchpad.net/juju-core/environs/config 2.911s
ok launchpad.net/juju-core/environs/configstore 0.048s
ok launchpad.net/juju-core/environs/filestorage 0.033s
ok launchpad.net/juju-core/environs/httpstorage 0.914s
ok launchpad.net/juju-core/environs/imagemetadata 0.498s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.058s
ok launchpad.net/juju-core/environs/jujutest 0.241s
ok launchpad.net/juju-core/environs/manual 10.826s
ok launchpad.net/juju-core/environs/simplestreams 0.382s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.332s
ok launchpad.net/juju-core/environs/storage 1.462s
ok launchpad.net/juju-core/environs/sync 33.416s
ok launchpad.net/juju-core/environs/testing 0.206s
ok launchpad.net/juju-core/environs/tools 6.996s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.017s
ok launchpad.net/juju-core/instance 0.024s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 25.314s
ok launchpad.net/juju-core/juju/osenv 0.020s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.033s
? launchp...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'provider/local/environ.go'
2--- provider/local/environ.go 2014-01-05 22:20:24 +0000
3+++ provider/local/environ.go 2014-01-21 23:12:42 +0000
4@@ -325,17 +325,33 @@
5
6 // Instances is specified in the Environ interface.
7 func (env *localEnviron) Instances(ids []instance.Id) ([]instance.Instance, error) {
8- // NOTE: do we actually care about checking the existance of the instances?
9- // I posit that here we don't really care, and that we are only called with
10- // instance ids that we know exist.
11 if len(ids) == 0 {
12 return nil, nil
13 }
14- insts := make([]instance.Instance, len(ids))
15+ insts, err := env.AllInstances()
16+ if err != nil {
17+ return nil, err
18+ }
19+ allInstances := make(map[instance.Id]instance.Instance)
20+ for _, inst := range insts {
21+ allInstances[inst.Id()] = inst
22+ }
23+ var found int
24+ insts = make([]instance.Instance, len(ids))
25 for i, id := range ids {
26- insts[i] = &localInstance{id, env}
27- }
28- return insts, nil
29+ if inst, ok := allInstances[id]; ok {
30+ insts[i] = inst
31+ found++
32+ }
33+ }
34+ if found == 0 {
35+ insts, err = nil, environs.ErrNoInstances
36+ } else if found < len(ids) {
37+ err = environs.ErrPartialInstances
38+ } else {
39+ err = nil
40+ }
41+ return insts, err
42 }
43
44 // AllInstances is specified in the InstanceBroker interface.

Subscribers

People subscribed via source and target branches

to status/vote changes: