Fix add del dhcp range simple #620

Merged
merged 10 commits into from Jun 26, 2015

Projects

None yet

3 participants

@Metallion
Member

See #611.

unakatsuo and others added some commits May 29, 2015
@unakatsuo unakatsuo add test 951603e
@unakatsuo unakatsuo add range test dataset method. hit_ranges(). 835abc5
@unakatsuo unakatsuo add test for Network#add_ipv4_dynamic_range. b1c5977
@unakatsuo unakatsuo add test for Network#del_ipv4_dynamic_range. b8479f5
@unakatsuo unakatsuo dcmgr: rewrite add_ipv4_dynamic_range/del_ipv4_dynamic_range totally. 9675f30
@Metallion Metallion Merge branch 'develop' into fix-add-del-dhcp-range-simple
d53604c
@unakatsuo unakatsuo referenced this pull request Jun 17, 2015
Closed

Fix add del dhcp ranges #621

@axsh-bot
Member

d53604c success - wakame-ci/rspec

@axsh-bot
Member

d53604c success - wakame-ci/rpmbuild

@axsh-bot
Member

d53604c success - wakame-ci/to-s3

@axsh-bot
Member

d53604c failure - wakame-ci/lxc.smoke.allowed-failure

@Metallion Metallion commented on an outdated diff Jun 22, 2015
dcmgr/lib/dcmgr/models/dhcp_range.rb
+ # => 192.168.0.10-20, 192.168.0.50-60
+ def_dataset_method(:hit_ranges) { |*ipv4_u32|
+ sql = "(range_begin <= ? AND range_end >= ?)"
+ ary = ipv4_u32.map { |u32|
+ [sql, [u32, u32]]
+ }
+ sql = "(" + ary.map { |_sql, args|
+ _sql
+ }.join(") OR (") + ")"
+
+ filter("(" + sql + ")",
+ *ary.map {|sql, args|
+ args
+ }.flatten
+ ).order(Sequel.asc(:range_begin))
+ }
@Metallion
Metallion Jun 22, 2015 Member

This code is very hard to read. I recommend rewriting it like this.

def_dataset_method(:hit_ranges) { |*ipv4_u32|
  sql_ary = ipv4_u32.map { |u32|
    "(RANGE_BEGIN <= #{u32} AND RANGE_END >= #{u32})"
  }

  sql = sql_ary.join(" OR ")

  where(sql).order(Sequel.asc(:range_begin))
}

I added some unit tests for this method and they confirm that this code has the same behaviour.

@Metallion Metallion commented on an outdated diff Jun 23, 2015
dcmgr/lib/dcmgr/models/network.rb
- range_begin = IPAddress::IPv4.new("#{range_begin}/#{self[:prefix]}")
- range_end = IPAddress::IPv4.new("#{range_end}/#{self[:prefix]}")
- test_inclusion(*validate_range_args(range_begin, range_end)) { |range, op|
- case op
- when :coverbegin
- range.range_end = range_begin
- when :coverend
- range.range_begin = range_end
- when :inccur
- range.destroy
- when :incnew
- t = range.range_end
- range.range_end = range_begin
- self.add_dhcp_range(:range_begin=>range_end, :range_end=>t)
+ # Acquire locks
+ dhcp_range_dataset.for_update.select(1).single_value
@Metallion
Metallion Jun 23, 2015 Member

You are only locking the first dhcp_range row for this network. At this point you don't know that this is the range we wish to remove.

unakatsuo added some commits Jun 24, 2015
@unakatsuo unakatsuo get rid of 'LIMIT' clause. it seems no way to ignore response from da…
…tabase server.
e9df3f1
@unakatsuo unakatsuo simplify building dataset as suggested.
bcc056b
@axsh-bot
Member

bcc056b success - wakame-ci/rspec

@axsh-bot
Member

bcc056b success - wakame-ci/rpmbuild

@axsh-bot
Member

bcc056b success - wakame-ci/to-s3

@axsh-bot
Member

bcc056b success - wakame-ci/dummy.smoke

@axsh-bot
Member

bcc056b success - wakame-ci/kvm.smoke.allowed-failure

@axsh-bot
Member

bcc056b success - wakame-ci/kvm.smoke

@axsh-bot
Member

bcc056b success - wakame-ci/vz.smoke

@Metallion Metallion and 1 other commented on an outdated diff Jun 25, 2015
dcmgr/lib/dcmgr/models/network.rb
- test_inclusion(*validate_range_args(range_begin, range_end)) { |range, op|
- case op
- when :coverbegin
- range.range_end = range_begin
- when :coverend
- range.range_begin = range_end
- when :inccur
- range.destroy
- when :incnew
- t = range.range_end
- range.range_end = range_begin
- self.add_dhcp_range(:range_begin=>range_end, :range_end=>t)
+ # Acquire locks
+ dhcp_range_dataset.for_update.select(1).all
+ range_begin_u32, range_end_u32 = validate_range_args(range_begin, range_end)
+ hit_range_ds = dhcp_range_dataset.hit_ranges(range_begin_u32, range_end_u32)
@Metallion
Metallion Jun 25, 2015 Member

Why use hit_ranges here? Why not just do this?

ranges_to_destroy = dhcp_range_dataset.where(range_begin: range_begin_u32, range_end: range_end_u32)

raise "Given range not found" if ranges_to_destroy.empty?

ranges_to_destroy.destroy
@Metallion
Metallion Jun 25, 2015 Member

On second thought it might be better to remove only one matching range if multiple identical ranges were in the database.

ranges_to_destroy = dhcp_range_dataset.where(range_begin: range_begin_u32, range_end: range_end_u32)

raise "Given range not found" if ranges_to_destroy.empty?

if ranges_to_destroy.count > 1
  logger.warn "Deleting dhcp range #{range_begin} to #{range_end} but a duplicate range still exists."
end

ranges_to_destroy.first.destroy
@unakatsuo
unakatsuo Jun 25, 2015 Member

adding same range is rejected at add_ipv4_dynamic_range(). so I vote the first idea.

@Metallion Metallion and 1 other commented on an outdated diff Jun 25, 2015
dcmgr/lib/dcmgr/models/network.rb
- when :coverbegin
- range.range_end = range_begin
- when :coverend
- range.range_begin = range_end
- when :inccur
- range.destroy
- when :incnew
- t = range.range_end
- range.range_end = range_begin
- self.add_dhcp_range(:range_begin=>range_end, :range_end=>t)
+ # Acquire locks
+ dhcp_range_dataset.for_update.select(1).all
+ range_begin_u32, range_end_u32 = validate_range_args(range_begin, range_end)
+ hit_range_ds = dhcp_range_dataset.hit_ranges(range_begin_u32, range_end_u32)
+ ranges_count = hit_range_ds.count
+ if ranges_count == 1
@Metallion
Metallion Jun 25, 2015 Member

This line makes it impossible to remove a range if it was added to the database multiple times. Network#add_ipv4_dynamic_range won't allow the same range to be created twice but we can not assume that all ranges are always created through that method.

@unakatsuo
unakatsuo Jun 25, 2015 Member

Need separate discussion that VDC has to work with unexpected dataset which is generated by unsupported tools.
If we assume, we need to review entire SQLs in the code base.

But it is beyond topic for this change.

@unakatsuo unakatsuo find the dhcp range to be deleted using exact match.
86775ac
@axsh-bot
Member

86775ac success - wakame-ci/rspec

@axsh-bot
Member

86775ac success - wakame-ci/rpmbuild

@axsh-bot
Member

86775ac success - wakame-ci/to-s3

@axsh-bot
Member
@axsh-bot
Member
@axsh-bot
Member

86775ac success - wakame-ci/vz.smoke

@Metallion
Member

Look good. Merging. 👍

@Metallion Metallion merged commit fc6ac04 into develop Jun 26, 2015

7 of 8 checks passed

wakame-ci/lxc.smoke.allowed-failure The build was pending on wakame-ci http://ci.wakame.jp/job/lxc.smoke.trigger/1852/ (86775ac8).
wakame-ci/dummy.smoke The build was success on wakame-ci http://ci.wakame.jp/job/dummy.smoke.trigger/898/ (86775ac8).
wakame-ci/kvm.smoke The build was success on wakame-ci http://ci.wakame.jp/job/kvm.smoke.trigger/3392/ (86775ac8).
wakame-ci/kvm.smoke.allowed-failure The build was success on wakame-ci http://ci.wakame.jp/job/kvm.smoke.trigger2/515/ (86775ac8).
wakame-ci/rpmbuild The build was success on wakame-ci http://ci.wakame.jp/job/wakame-vdc-pub6.spot/718/ (86775ac8).
wakame-ci/rspec The build was success on wakame-ci http://ci.wakame.jp/job/wakame-vdc-pub6.rspec/361/ (86775ac8).
wakame-ci/to-s3 The build was success on wakame-ci http://ci.wakame.jp/job/wakame-vdc-pub6.to-s3/881/ (86775ac8).
wakame-ci/vz.smoke The build was success on wakame-ci http://ci.wakame.jp/job/vz.smoke.trigger/3137/ (86775ac8).
@Metallion Metallion deleted the fix-add-del-dhcp-range-simple branch Jun 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment