Quantum + Melange: DHCP generation for networks owned by default tenant is broken

[Tomasz Paszkowski]

All networks which belongs to default tenant have broken dhcp configuration generation. IP addresses allocated from such networks are not returned by get_dhcp_hosts_text function from quantum/manager.py file. To fix this we need to change line #665 from:

ips = self.ipam.get_allocated_ips(context, subnet_id, project_id)

to:

ips = self.ipam.get_allocated_ips(context, subnet_id, None)

So the functions will return all allocated ips in this network as we have single dhcp server for all tenants in particular network.

Eventual we can modify call to get_dhcp_hosts_text in functions enable_dhcp,update_dhcp to set project_id as None.

[dan wendlandt (danwent)]

Thanks for reporting this Tomasz!

Looking at the code, this does look like a bug.

I do not believe the change suggested above is correct though, as it would only work if all networks were assigned to the default tenant. Rather, the project_id passed to get_allocated_ips should be the project_id of the network, not the project_id of the VM. This means it will correctly be None for networks belonging to the default tenant, but set for networks belonging to a particular tenant. Hence, I think we should grab the project_id from the networks object within the for-loop in enable dhcp, and pass that in, instead of the existing code that always passing in the VM's project id:

diff --git a/nova/network/quantum/manager.py b/nova/network/quantum/manager.py
index 16e782f..96418ae 100644
--- a/nova/network/quantum/manager.py
+++ b/nova/network/quantum/manager.py
@@ -452,7 +452,7 @@ class QuantumManager(manager.FloatingIP, manager.FlatManager):
                         quantum_net_id, interface_id)

             hosts = self.get_dhcp_hosts_text(context,
- subnet['network_id'], project_id)
+ subnet['network_id'], network_ref['project_id'])
             self.driver.update_dhcp_hostfile_with_text(interface_id, hosts)
             self.driver.restart_dhcp(context, interface_id, network_ref)

Tomasz, are you able to test out this patch and confirm?

[Tomasz Paszkowski (ss7pro)]

Hi,

Patch works but please remember that update_dhcp is also calling
get_dhcp_hosts_text.

Regarding your comments I don't catch the idea why dhcp config
generation needs to be aware of tenant_id. IMHO network_id is enough
to fetch hosts which are placed in a given subnet.

On Wed, Mar 7, 2012 at 7:58 PM, dan wendlandt <email address hidden> wrote:
> Thanks for reporting this Tomasz!
>
> Looking at the code, this does look like a bug.
>
> I do not believe the change suggested above is correct though, as it
> would only work if all networks were assigned to the default tenant.
> Rather, the project_id  passed to get_allocated_ips should be the
> project_id of the network, not the project_id of the VM.  This means it
> will correctly be None for networks belonging to the default tenant, but
> set for networks belonging to a particular tenant.  Hence, I think we
> should grab the project_id from the networks object within the for-loop
> in enable dhcp, and pass that in, instead of the existing code that
> always passing in the VM's project id:
>
>
> diff --git a/nova/network/quantum/manager.py b/nova/network/quantum/manager.py
> index 16e782f..96418ae 100644
> --- a/nova/network/quantum/manager.py
> +++ b/nova/network/quantum/manager.py
> @@ -452,7 +452,7 @@ class QuantumManager(manager.FloatingIP, manager.FlatManager):
>                         quantum_net_id, interface_id)
>
>             hosts = self.get_dhcp_hosts_text(context,
> -                subnet['network_id'], project_id)
> +                subnet['network_id'], network_ref['project_id'])
>             self.driver.update_dhcp_hostfile_with_text(interface_id, hosts)
>             self.driver.restart_dhcp(context, interface_id, network_ref)
>
> Tomasz, are you able to test out this patch and confirm?
>
> ** Also affects: nova
>   Importance: Undecided
>       Status: New
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/949234
>
> Title:
>  Quantum DHCP generation for networks owned by default tenant is broken
>
> Status in OpenStack Compute (Nova):
>  New
> Status in OpenStack Quantum (virtual network service):
>  Confirmed
>
> Bug description:
>  All networks which belongs to default tenant have broken dhcp
>  configuration generation. IP addresses allocated from such networks
>  are not returned by get_dhcp_hosts_text function from
>  quantum/manager.py file. To fix this we need to change line #665 from:
>
>  ips = self.ipam.get_allocated_ips(context, subnet_id, project_id)
>
>  to:
>
>  ips = self.ipam.get_allocated_ips(context, subnet_id, None)
>
>
>  So the functions will return all allocated ips in this network as we have single dhcp server for all tenants in particular network.
>
>  Eventual we can modify call to get_dhcp_hosts_text in functions
>  enable_dhcp,update_dhcp to   set project_id as None.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/nova/+bug/949234/+subscriptions

--
Tomasz Paszkowski
SS7, Asterisk, SAN, Datacenter, Cloud Computing
[phone]

[dan wendlandt (danwent)]

Hi Tomasz,

I haven't had a chance to look at this in detail, just enough to know that
always setting the project_id to None wasn't the right approach.

I agree, update_dhcp seems to have the same issue. Same fix of getting the
project_id from the net instead should fix it.

In terms of why project_id is passed in, are you using Melange? If so, the
project_id is needed to make any call to melange. If you are using the
default nova IPAM lib, I don't believe project_id is used at all, and thus
would be surprised if you saw this issue at all (though again, I still need
to look at the code in more detail).

On Wed, Mar 7, 2012 at 12:27 PM, Tomasz Paszkowski <
<email address hidden>> wrote:

> Hi,
>
> Patch works but please remember that update_dhcp is also calling
> get_dhcp_hosts_text.
>
> Regarding your comments I don't catch the idea why dhcp config
> generation needs to be aware of tenant_id. IMHO network_id is enough
> to fetch hosts which are placed in a given subnet.
>
>
> On Wed, Mar 7, 2012 at 7:58 PM, dan wendlandt <email address hidden>
> wrote:
> > Thanks for reporting this Tomasz!
> >
> > Looking at the code, this does look like a bug.
> >
> > I do not believe the change suggested above is correct though, as it
> > would only work if all networks were assigned to the default tenant.
> > Rather, the project_id passed to get_allocated_ips should be the
> > project_id of the network, not the project_id of the VM. This means it
> > will correctly be None for networks belonging to the default tenant, but
> > set for networks belonging to a particular tenant. Hence, I think we
> > should grab the project_id from the networks object within the for-loop
> > in enable dhcp, and pass that in, instead of the existing code that
> > always passing in the VM's project id:
> >
> >
> > diff --git a/nova/network/quantum/manager.py
> b/nova/network/quantum/manager.py
> > index 16e782f..96418ae 100644
> > --- a/nova/network/quantum/manager.py
> > +++ b/nova/network/quantum/manager.py
> > @@ -452,7 +452,7 @@ class QuantumManager(manager.FloatingIP,
> manager.FlatManager):
> > quantum_net_id, interface_id)
> >
> > hosts = self.get_dhcp_hosts_text(context,
> > - subnet['network_id'], project_id)
> > + subnet['network_id'], network_ref['project_id'])
> > self.driver.update_dhcp_hostfile_with_text(interface_id,
> hosts)
> > self.driver.restart_dhcp(context, interface_id, network_ref)
> >
> > Tomasz, are you able to test out this patch and confirm?
> >
> > ** Also affects: nova
> > Importance: Undecided
> > Status: New
> >
> > --
> > You received this bug notification because you are subscribed to the bug
> > report.
> > https://bugs.launchpad.net/bugs/949234
> >
> > Title:
> > Quantum DHCP generation for networks owned by default tenant is broken
> >
> > Status in OpenStack Compute (Nova):
> > New
> > Status in OpenStack Quantum (virtual network service):
> > Confirmed
> >
> > Bug description:
> > All networks which belongs to default tenant have broken dhcp
> > configuration generation. IP addresses allocated from such networks
> > are not...

[Tomasz Paszkowski (ss7pro)]

Hi

I have double checked the code of quantum and melange. The
project/tenant id is ofcourse used but it's optional for getting the
list of allocated_ips and it using this var dosen't bring any
additional value. I may be wrong but I'am just curious why someone
implemented it this way :-)))

Dnia 07-03-2012 o godz. 21:55 dan wendlandt
<email address hidden> napisał(a):

> Hi Tomasz,
>
> I haven't had a chance to look at this in detail, just enough to know that
> always setting the project_id to None wasn't the right approach.
>
> I agree, update_dhcp seems to have the same issue. Same fix of getting the
> project_id from the net instead should fix it.
>
> In terms of why project_id is passed in, are you using Melange? If so, the
> project_id is needed to make any call to melange. If you are using the
> default nova IPAM lib, I don't believe project_id is used at all, and thus
> would be surprised if you saw this issue at all (though again, I still need
> to look at the code in more detail).
>
>
> On Wed, Mar 7, 2012 at 12:27 PM, Tomasz Paszkowski <
> <email address hidden>> wrote:
>
>> Hi,
>>
>> Patch works but please remember that update_dhcp is also calling
>> get_dhcp_hosts_text.
>>
>> Regarding your comments I don't catch the idea why dhcp config
>> generation needs to be aware of tenant_id. IMHO network_id is enough
>> to fetch hosts which are placed in a given subnet.
>>
>>
>> On Wed, Mar 7, 2012 at 7:58 PM, dan wendlandt <email address hidden>
>> wrote:
>>> Thanks for reporting this Tomasz!
>>>
>>> Looking at the code, this does look like a bug.
>>>
>>> I do not believe the change suggested above is correct though, as it
>>> would only work if all networks were assigned to the default tenant.
>>> Rather, the project_id passed to get_allocated_ips should be the
>>> project_id of the network, not the project_id of the VM. This means it
>>> will correctly be None for networks belonging to the default tenant, but
>>> set for networks belonging to a particular tenant. Hence, I think we
>>> should grab the project_id from the networks object within the for-loop
>>> in enable dhcp, and pass that in, instead of the existing code that
>>> always passing in the VM's project id:
>>>
>>>
>>> diff --git a/nova/network/quantum/manager.py
>> b/nova/network/quantum/manager.py
>>> index 16e782f..96418ae 100644
>>> --- a/nova/network/quantum/manager.py
>>> +++ b/nova/network/quantum/manager.py
>>> @@ -452,7 +452,7 @@ class QuantumManager(manager.FloatingIP,
>> manager.FlatManager):
>>> quantum_net_id, interface_id)
>>>
>>> hosts = self.get_dhcp_hosts_text(context,
>>> - subnet['network_id'], project_id)
>>> + subnet['network_id'], network_ref['project_id'])
>>> self.driver.update_dhcp_hostfile_with_text(interface_id,
>> hosts)
>>> self.driver.restart_dhcp(context, interface_id, network_ref)
>>>
>>> Tomasz, are you able to test out this patch and confirm?
>>>
>>> ** Also affects: nova
>>> Importance: Undecided
>>> Status: New
>>>
>>> --
>>> You received this bug notification because you are subscribed to the bug
>>> report.
>>> https://bugs.lau...

[dan wendlandt (danwent)]

On Wed, Mar 7, 2012 at 2:28 PM, Tomasz Paszkowski <<email address hidden>
> wrote:

> Hi
>
> I have double checked the code of quantum and melange. The
> project/tenant id is ofcourse used but it's optional for getting the
> list of allocated_ips and it using this var dosen't bring any
> additional value. I may be wrong but I'am just curious why someone
> implemented it this way :-)))
>

Its not actually optional given how the code currently works. It must be
set for the code to work correctly if Melange is in use. If you didn't
pass it into melange, we would always query IP addresses using the
"default" tenant ID, which would be incorrect for any tenant-specific
network.

Is it correct that you are using Melange? I'm trying to confirm this, as
if you are not using Melange, I would need to explore this more, as it
seems like this is only a problem with Melange.

Dan

>
>
> Dnia 07-03-2012 o godz. 21:55 dan wendlandt
> <email address hidden> napisał(a):
>
> > Hi Tomasz,
> >
> > I haven't had a chance to look at this in detail, just enough to know
> that
> > always setting the project_id to None wasn't the right approach.
> >
> > I agree, update_dhcp seems to have the same issue. Same fix of getting
> the
> > project_id from the net instead should fix it.
> >
> > In terms of why project_id is passed in, are you using Melange? If so,
> the
> > project_id is needed to make any call to melange. If you are using the
> > default nova IPAM lib, I don't believe project_id is used at all, and
> thus
> > would be surprised if you saw this issue at all (though again, I still
> need
> > to look at the code in more detail).
> >
> >
> > On Wed, Mar 7, 2012 at 12:27 PM, Tomasz Paszkowski <
> > <email address hidden>> wrote:
> >
> >> Hi,
> >>
> >> Patch works but please remember that update_dhcp is also calling
> >> get_dhcp_hosts_text.
> >>
> >> Regarding your comments I don't catch the idea why dhcp config
> >> generation needs to be aware of tenant_id. IMHO network_id is enough
> >> to fetch hosts which are placed in a given subnet.
> >>
> >>
> >> On Wed, Mar 7, 2012 at 7:58 PM, dan wendlandt <
> <email address hidden>>
> >> wrote:
> >>> Thanks for reporting this Tomasz!
> >>>
> >>> Looking at the code, this does look like a bug.
> >>>
> >>> I do not believe the change suggested above is correct though, as it
> >>> would only work if all networks were assigned to the default tenant.
> >>> Rather, the project_id passed to get_allocated_ips should be the
> >>> project_id of the network, not the project_id of the VM. This means it
> >>> will correctly be None for networks belonging to the default tenant,
> but
> >>> set for networks belonging to a particular tenant. Hence, I think we
> >>> should grab the project_id from the networks object within the for-loop
> >>> in enable dhcp, and pass that in, instead of the existing code that
> >>> always passing in the VM's project id:
> >>>
> >>>
> >>> diff --git a/nova/network/quantum/manager.py
> >> b/nova/network/quantum/manager.py
> >>> index 16e782f..96418ae 100644
> >>> --- a/nova/network/quantum/manager.py
> >>> +++ b/nova/network/quantum/manager.py
> >>> @@ -452,7 +452,7 @@ class Quan...

[dan wendlandt (danwent)]

Bumping down to high, as this seems to only affect Melange, which is a non-default, but still supported, config.

[Tomasz Paszkowski (ss7pro)]

Yes I'am using melange. Sorry for taking your time but now I see that
project_id is needed to get allocations only for a particular network
! Many thanks for your time.

On Wed, Mar 7, 2012 at 11:57 PM, dan wendlandt
<email address hidden> wrote:
> On Wed, Mar 7, 2012 at 2:28 PM, Tomasz Paszkowski <<email address hidden>
>> wrote:
>
>> Hi
>>
>> I have double checked the code of quantum and melange. The
>> project/tenant id is ofcourse used but it's optional for getting the
>> list of allocated_ips and it using this var dosen't bring any
>> additional value. I may be wrong but I'am just curious why someone
>> implemented it this way :-)))
>>
>
> Its not actually optional given how the code currently works.  It must be
> set for the code to work correctly if Melange is in use.  If you didn't
> pass it into melange, we would always query IP addresses using the
> "default" tenant ID, which would be incorrect for any tenant-specific
> network.
>
> Is it correct that you are using Melange?  I'm trying to confirm this, as
> if you are not using Melange, I would need to explore this more, as it
> seems like this is only a problem with Melange.
>
> Dan
>
>
>>
>>
>> Dnia 07-03-2012 o godz. 21:55 dan wendlandt
>> <email address hidden> napisał(a):
>>
>> > Hi Tomasz,
>> >
>> > I haven't had a chance to look at this in detail, just enough to know
>> that
>> > always setting the project_id to None wasn't the right approach.
>> >
>> > I agree, update_dhcp seems to have the same issue.  Same fix of getting
>> the
>> > project_id from the net instead should fix it.
>> >
>> > In terms of why project_id is passed in, are you using Melange?  If so,
>> the
>> > project_id is needed to make any call to melange.  If you are using the
>> > default nova IPAM lib, I don't believe project_id is used at all, and
>> thus
>> > would be surprised if you saw this issue at all (though again, I still
>> need
>> > to look at the code in more detail).
>> >
>> >
>> > On Wed, Mar 7, 2012 at 12:27 PM, Tomasz Paszkowski <
>> > <email address hidden>> wrote:
>> >
>> >> Hi,
>> >>
>> >> Patch works but please remember that update_dhcp is also calling
>> >> get_dhcp_hosts_text.
>> >>
>> >> Regarding your comments I don't catch the idea why dhcp config
>> >> generation needs to be aware of tenant_id. IMHO network_id is enough
>> >> to fetch hosts which are placed in a given subnet.
>> >>
>> >>
>> >> On Wed, Mar 7, 2012 at 7:58 PM, dan wendlandt <
>> <email address hidden>>
>> >> wrote:
>> >>> Thanks for reporting this Tomasz!
>> >>>
>> >>> Looking at the code, this does look like a bug.
>> >>>
>> >>> I do not believe the change suggested above is correct though, as it
>> >>> would only work if all networks were assigned to the default tenant.
>> >>> Rather, the project_id  passed to get_allocated_ips should be the
>> >>> project_id of the network, not the project_id of the VM.  This means it
>> >>> will correctly be None for networks belonging to the default tenant,
>> but
>> >>> set for networks belonging to a particular tenant.  Hence, I think we
>> >>> should grab the project_id from the networks object within the for-loop
>> >>> in enable dhcp, and pass that in...

[dan wendlandt (danwent)]

Ok, I just need to find time to test the patch this morning, then I will
push the change (unless you want to push the change yourself, in which
case, go ahead).

Dan

On Thu, Mar 8, 2012 at 1:24 AM, Tomasz Paszkowski <<email address hidden>
> wrote:

> Yes I'am using melange. Sorry for taking your time but now I see that
> project_id is needed to get allocations only for a particular network
> ! Many thanks for your time.
>
>
>
> On Wed, Mar 7, 2012 at 11:57 PM, dan wendlandt
> <email address hidden> wrote:
> > On Wed, Mar 7, 2012 at 2:28 PM, Tomasz Paszkowski <
> <email address hidden>
> >> wrote:
> >
> >> Hi
> >>
> >> I have double checked the code of quantum and melange. The
> >> project/tenant id is ofcourse used but it's optional for getting the
> >> list of allocated_ips and it using this var dosen't bring any
> >> additional value. I may be wrong but I'am just curious why someone
> >> implemented it this way :-)))
> >>
> >
> > Its not actually optional given how the code currently works. It must be
> > set for the code to work correctly if Melange is in use. If you didn't
> > pass it into melange, we would always query IP addresses using the
> > "default" tenant ID, which would be incorrect for any tenant-specific
> > network.
> >
> > Is it correct that you are using Melange? I'm trying to confirm this, as
> > if you are not using Melange, I would need to explore this more, as it
> > seems like this is only a problem with Melange.
> >
> > Dan
> >
> >
> >>
> >>
> >> Dnia 07-03-2012 o godz. 21:55 dan wendlandt
> >> <email address hidden> napisał(a):
> >>
> >> > Hi Tomasz,
> >> >
> >> > I haven't had a chance to look at this in detail, just enough to know
> >> that
> >> > always setting the project_id to None wasn't the right approach.
> >> >
> >> > I agree, update_dhcp seems to have the same issue. Same fix of
> getting
> >> the
> >> > project_id from the net instead should fix it.
> >> >
> >> > In terms of why project_id is passed in, are you using Melange? If
> so,
> >> the
> >> > project_id is needed to make any call to melange. If you are using
> the
> >> > default nova IPAM lib, I don't believe project_id is used at all, and
> >> thus
> >> > would be surprised if you saw this issue at all (though again, I still
> >> need
> >> > to look at the code in more detail).
> >> >
> >> >
> >> > On Wed, Mar 7, 2012 at 12:27 PM, Tomasz Paszkowski <
> >> > <email address hidden>> wrote:
> >> >
> >> >> Hi,
> >> >>
> >> >> Patch works but please remember that update_dhcp is also calling
> >> >> get_dhcp_hosts_text.
> >> >>
> >> >> Regarding your comments I don't catch the idea why dhcp config
> >> >> generation needs to be aware of tenant_id. IMHO network_id is enough
> >> >> to fetch hosts which are placed in a given subnet.
> >> >>
> >> >>
> >> >> On Wed, Mar 7, 2012 at 7:58 PM, dan wendlandt <
> >> <email address hidden>>
> >> >> wrote:
> >> >>> Thanks for reporting this Tomasz!
> >> >>>
> >> >>> Looking at the code, this does look like a bug.
> >> >>>
> >> >>> I do not believe the change suggested above is correct though, as it
> >> >>> would only work if all networks were assigned to the default tenant.
> >> >>> Ra...

[Tomasz Paszkowski (ss7pro)]

Hi,

Bad news, network_ref['project_id'] is still not working for a default
tenant. From those what i see here
we shoud ask melange to provide us with all addresses from given
subnet_id. Now code is using wrong API call which is valid only in
single tenant subnets (non default). The valid one is:
/ipam/tenants/{tenant_id}/ip_blocks/{ip_block_id}/ip_addresses (but
this call is currently broken in recent melange code). I have created
bug report for this in melange:
https://bugs.launchpad.net/melange/+bug/951499

Below relevant code:

-- manager.py line #661 --
    def get_dhcp_hosts_text(self, context, subnet_id, project_id=None):
        ips = self.ipam.get_allocated_ips(context, subnet_id, project_id)
        hosts_text = ""

-- melange_ipam_lib.py line #243 --
   def get_allocated_ips(self, context, subnet_id, project_id):
        ips = self.m_conn.get_allocated_ips_for_network(subnet_id, project_id)
        return [(ip['address'], ip['interface_id']) for ip in ips]

-- melange_connection.py line #166 --
    def get_allocated_ips_for_network(self, network_id, project_id=None):
        tenant_scope = "/tenants/%s" % project_id if project_id else ""
        url = ("ipam%(tenant_scope)s/allocated_ip_addresses" % locals())
        # TODO(bgh): This request fails if you add the ".json" to the end so
        # it has to call do_request itself. Melange bug?
        response = self.do_request("GET", url, content_type="")
        return json.loads(response)['ip_addresses']

-- melange/melange/ipam/service.py line # 684 --
    def _allocated_ips_mapper(self, mapper):
        allocated_ips_res = AllocatedIpAddressesController().create_resource()
        _connect(mapper,
                      "/ipam/allocated_ip_addresses",
                      controller=allocated_ips_res,
                      action="index",
                      conditions=dict(method=['GET']))
        _connect(mapper,
                      "/ipam/tenants/{tenant_id}/allocated_ip_addresses",
                      controller=allocated_ips_res,
                      action="index",
                      conditions=dict(method=['GET']))

-- melange/melange/ipam/service.py line # 177 --
class AllocatedIpAddressesController(BaseController):

    def index(self, request, tenant_id=None):
        filter_conditions = utils.filter_dict(request.params, 'used_by_device')
        if tenant_id:
            filter_conditions['used_by_tenant'] = tenant_id
        ips = models.IpAddress.find_all_allocated_ips(**filter_conditions)
        return self._paginated_response('ip_addresses', ips, request)

-- melange/melange/ipam/models.py line # 588 --
    def find_all_allocated_ips(cls, **conditions):
        LOG.debug("Retrieving all allocated IPs.")
        return db.db_query.find_all_allocated_ips(cls, **conditions)

-- melange/melange/db/sqlalchemy/api.py
def find_all_allocated_ips(model, used_by_device=None, used_by_tenant=None,
                           **conditions):
    query = _query_by(ipam.models.IpAddress, **conditions).\
            filter(or_(ipam.models.IpAddress.marked_for_deallocation == None,
                       ipam.models.IpAddress.marked_for_deallocation == False))

    if u...

[dan wendlandt (danwent)]

Any progress here? Otherwise, I'm probably going to have to untarget this as a quantum bug for RC1 (the change required is in nova anyway, but its useful to track in Quantum).

[dan wendlandt (danwent)]

We declared using melange IPAM with Quantum "experimental" in essex, as almost all testing was done with the built-in Nova IPAM.

Melange and Nova IPAM will go away in Folsom, so fixing this issue isn't a high priority.

[Joe Gordon (jogo)]

marking as won't fix in nova, this is very old