Merge lp:~gz/goose/never_decrement_next_id into lp:goose

[Martin Packman]

Never decrement next id in nova testservce

Adjusting the next id downwards is not necessary and can cause breakage
across testcases when cleanup does not exactly match creation, which is
common in juju.

Never decrement next id in nova testservce

Adjusting the next id downwards is not necessary and can cause breakage
across testcases when cleanup does not exactly match creation, which is
common in juju.

For example:
* TestA creates two rules: next id is 3, rules is {1, 2}
* TestA deletes rule 1: next id is 2, rules is {2}
* TestB creates one rule: boom... only when TestA runs first

https://codereview.appspot.com/7307066/

[Martin Packman (gz)]

Reviewers: mp+147208_code.launchpad.net,

Message:
Please take a look.

Description:
Never decrement next id in nova testservce

Adjusting the next id downwards is not necessary and can cause breakage
across testcases when cleanup does not exactly match creation, which is
common in juju.

For example:
* TestA creates two rules: next id is 3, rules is {1, 2}
* TestA deletes rule 1: next id is 2, rules is {2}
* TestB creates one rule: boom... only when TestA runs first

https://code.launchpad.net/~gz/goose/never_decrement_next_id/+merge/147208

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M testservices/novaservice/service_http.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-20130207052913-e5vfzm0pg3786t9i
+New revision: <email address hidden>

Index: testservices/novaservice/service_http.go
=== modified file 'testservices/novaservice/service_http.go'
--- testservices/novaservice/service_http.go 2013-02-04 15:53:04 +0000
+++ testservices/novaservice/service_http.go 2013-02-07 18:43:34 +0000
@@ -847,9 +847,6 @@
     if err := n.removeSecurityGroup(group.Id); err != nil {
      return err
     }
- if n.nextGroupId > 0 {
- n.nextGroupId--
- }
     writeResponse(w, http.StatusAccepted, nil)
     return nil
    } else if err == errNoGroupId {
@@ -914,9 +911,6 @@
     if err = n.removeSecurityGroupRule(id); err != nil {
      return err
     }
- if n.nextRuleId > 0 {
- n.nextRuleId--
- }
     writeResponse(w, http.StatusAccepted, nil)
     return nil
    }
@@ -976,9 +970,6 @@
    if ipId := path.Base(r.URL.Path); ipId != "os-floating-ips" {
     if nId, err := strconv.Atoi(ipId); err == nil {
      if err := n.removeFloatingIP(nId); err == nil {
- if n.nextIPId > 0 {
- n.nextIPId--
- }
       writeResponse(w, http.StatusAccepted, nil)
       return nil
      }

[John A Meinel (jameinel)]

LGTM. It isn't like we are worried about overflowing an int here. :)

I'm curious what the real behavior is, would it be hard to test creating
and releasing a bunch of Ids on canonistack and seeing how the numbers
change (Maybe not ideal, since there is noise in that system.)

Maybe audit the openstack code, but both seem like a lot of time spent
for something trivial.

https://codereview.appspot.com/7307066/

[Dimiter Naydenov (dimitern)]

LGTM. I wasn't sure if I had to do that in the first place (the
decrementing), but it seemed cleaner then. It's a trivial change anyway
and since it helps tests, perfect!

https://codereview.appspot.com/7307066/

[Martin Packman (gz)]

On 2013/02/07 19:49:08, jameinel wrote:

> I'm curious what the real behavior is, would it be hard to test
creating and
> releasing a bunch of Ids on canonistack and seeing how the numbers
change (Maybe
> not ideal, since there is noise in that system.)

Pretty sure it's just an auto-increment in mysql for the things that
aren't UUIDs still.

https://codereview.appspot.com/7307066/

=== modified file 'testservices/novaservice/service_http.go'
--- testservices/novaservice/service_http.go	2013-02-04 15:53:04 +0000
+++ testservices/novaservice/service_http.go	2013-02-07 18:50:40 +0000
@@ -847,9 +847,6 @@
 			if err := n.removeSecurityGroup(group.Id); err != nil {
 				return err
 			}
-			if n.nextGroupId > 0 {
-				n.nextGroupId--
-			}
 			writeResponse(w, http.StatusAccepted, nil)
 			return nil
 		} else if err == errNoGroupId {
@@ -914,9 +911,6 @@
 			if err = n.removeSecurityGroupRule(id); err != nil {
 				return err
 			}
-			if n.nextRuleId > 0 {
-				n.nextRuleId--
-			}
 			writeResponse(w, http.StatusAccepted, nil)
 			return nil
 		}
@@ -976,9 +970,6 @@
 		if ipId := path.Base(r.URL.Path); ipId != "os-floating-ips" {
 			if nId, err := strconv.Atoi(ipId); err == nil {
 				if err := n.removeFloatingIP(nId); err == nil {
-					if n.nextIPId > 0 {
-						n.nextIPId--
-					}
 					writeResponse(w, http.StatusAccepted, nil)
 					return nil
 				}