Merge lp:~soren/nova/lp714577 into lp:~hudson-openstack/nova/trunk

Proposed by Soren Hansen on 2011-02-09
Status: Merged
Approved by: Devin Carlen on 2011-02-09
Approved revision: 655
Merged at revision: 657
Proposed branch: lp:~soren/nova/lp714577
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 10 lines (+1/-1)
1 file modified
nova/db/sqlalchemy/api.py (+1/-1)
To merge this branch: bzr merge lp:~soren/nova/lp714577
Reviewer Review Type Date Requested Status
Devin Carlen (community) Approve on 2011-02-09
Vish Ishaya (community) Approve on 2011-02-09
Jay Pipes (community) 2011-02-09 Approve on 2011-02-09
Review via email: mp+49127@code.launchpad.net

Commit Message

Pass timestamps to the db layer in fixed_ip_disassociate_all_by_timeout rather than converting to strings ahead of time, otherwise comparison between timestamps would often fail.

Description of the Change

Leave it to the db layer to get comparisons between timestamps right.

Right now, we convert from a datetime object to a string using isoformat(). This yields a string such as '2011-02-09T19:52:01.355919' which sorts a greater than e.g. '2011-02-09 20:01:43.789045' due to being compared alphanumerically rather than as timestamps. This caused the cleanup routing in the network manager's periodic_task to prematurely expire leases, leading them to be recycled before dnsmasq was actually willing to let it go.

To post a comment you must log in.
Jay Pipes (jaypipes) wrote :

lgtm. thx for the excellent patience in narrowing down this issue.

review: Approve
Soren Hansen (soren) wrote :

https://hudson.linux2go.dk/job/Nova-user-test/buildTimeTrend

The impact of this patch is quite obvious from this graph.

The two failures towards the end was me trying to revert the change for a few runs to gather the info I provided on the bug.

Vish Ishaya (vishvananda) wrote :

lgtm. Looks like you are making some progress with automated testing as well. You should sync up with termie and jaypipes so we don't duplicate work.

review: Approve
Devin Carlen (devcamcar) wrote :

lgtm

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/db/sqlalchemy/api.py'
2--- nova/db/sqlalchemy/api.py 2011-01-28 23:31:23 +0000
3+++ nova/db/sqlalchemy/api.py 2011-02-09 20:15:10 +0000
4@@ -579,7 +579,7 @@
5 'AND instance_id IS NOT NULL '
6 'AND allocated = 0',
7 {'host': host,
8- 'time': time.isoformat()})
9+ 'time': time})
10 return result.rowcount
11
12