Skip to content
This repository has been archived by the owner on Nov 22, 2021. It is now read-only.

Fixes BZ #1176423: create reservation for VIP ip addresses in DHCP (Do not merge) #402

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sseago
Copy link
Contributor

@sseago sseago commented Jan 13, 2015

https://bugzilla.redhat.com/show_bug.cgi?id=1176423

For VIPs on the provisioning network (with DHCP active), calling unused_ip
is insufficient. We also need to create the actual DHCP reservation
so the allocated IP address won't be reused elsewhere.

This change handles both reserving and freeing IP addresses. For a given
VIP nic, we reserve the IP address in DHCP when initially creating this
VIP on a DHCP network, or moving its network traffic type to such a network.
We delete the reservation when moving the VIP away from a DHCP network,
or on deleting it.

One side effect of this is VIP creation takes still longer than before, due
to the extra DHCP proxy action, but we already have a "waiting..." dialog with
spinner for this step.

https://bugzilla.redhat.com/show_bug.cgi?id=1176423

For VIPs on the provisioning network (with DHCP active), calling unused_ip
is insufficient. We also need to create the actual DHCP reservation
so the allocated IP address won't be reused elsewhere.

This change handles both reserving and freeing IP addresses. For a given
VIP nic, we reserve the IP address in DHCP when initially creating this
VIP on a DHCP network, or moving its network traffic type to such a network.
We delete the reservation when moving the VIP *away from* a DHCP network,
or on deleting it.

One side effect of this is VIP creation takes still longer than before, due
to the extra DHCP proxy action, but we already have a "waiting..." dialog with
spinner for this step.
@bcrochet
Copy link
Member

Visual ACK. Really needs to be tested on a system that can repro the issue reliably.

@mburns72h
Copy link
Contributor

This patch works as expected, creating the reservations for vips in dhcpd.leases. However, it does not resolve the issue of conflicting dhcp entries. New hosts still get conflicting ip addresses despite existing dhcp host entries.

@@ -3,15 +3,46 @@ class VipNic < Nic::Managed
has_one :deployment_vip_nic, :dependent => :destroy, :class_name => 'Staypuft::DeploymentVipNic'
has_one :deployment, :class_name => 'Staypuft::Deployment', :through => :deployment_vip_nic

before_save :reserve_ip
before_save :reserve_ip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick... either make them line up, or don't change the before_save.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ew :) I agree

@sseago sseago changed the title Fixes BZ #1176423: create reservation for VIP ip addresses in DHCP Fixes BZ #1176423: create reservation for VIP ip addresses in DHCP (Do not merge) Jan 21, 2015
@lzap
Copy link
Member

lzap commented Jan 27, 2015

I haven't realized the patch does not actually work. Can you give an example of such a conflict?

@mburns72h
Copy link
Contributor

Example leases file:

http://fpaste.org/169078/21159698/

Look at, for example, ip address 192.168.223.7. It's reserved using host vip_ceilometer_private_000000000101 with mac 00:00:00:00:01:01

but gets leased out to mac 52:54:00:75:3e:57

@lzap
Copy link
Member

lzap commented Jan 27, 2015

Taking the IP address you refer to here is the reservation:

host vip_ceilometer_private_000000000101 {
  dynamic;
  hardware ethernet 00:00:00:00:01:01;
  fixed-address 192.168.223.7;
        supersede host-name = "vip_ceilometer_private_000000000101";
}

But this is not a lease, it's a deleted lease and that is a difference.

lease 192.168.223.7 {
  starts 2 2015/01/13 14:00:47;
  ends 2 2015/01/13 14:10:47;
  tstp 2 2015/01/13 14:10:47;
  cltt 2 2015/01/13 14:00:47;
  binding state free;
  hardware ethernet 52:54:00:75:3e:57;
  uid "\001RT\000u>W";
}

Note the binding state free = this IP address is free again and can be leased or reserved again.

We had a bug in foreman-proxy. Proxy was seeing those "free" leases as "active", but this is already fixed. Can you check you backported it already?

theforeman/smart-proxy#239

@lzap
Copy link
Member

lzap commented Jan 27, 2015

@mburns72h confirms the patch 239 is in.

We have one additional patch, can you check you have it? It solves conflicts caused by Discovery, it was still not merged upstream but review is finished and it will be soon merged into master:

theforeman/foreman#2022

@mburns72h
Copy link
Contributor

Yes, that is in as well

@lzap
Copy link
Member

lzap commented Jan 27, 2015

For the record: it looks like ISC DHCP issues leases for reserved hosts which are offline.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants