cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: VPC's VR missing public NIC eth1
Date Tue, 03 Jun 2014 06:30:29 GMT
one clarification, I was not suggesting changing vlan://x back to x,
just the case where x==untagged. I had a little analog discussion with
Hugo and he convinced me that untagged has no special meaning in SDN
cases, maybe for vxlan. So the problem I saw is at least smaller then
in my mind.

I have committed the db change to update 4.3.0 to 4.4.0. It will need
heavy testing. And I didn't extensively look into other tables that
need such a change. networks is the likely candidate but there may be
others.


On Mon, Jun 2, 2014 at 6:38 PM, Marcus <shadowsor@gmail.com> wrote:
> Just to recap... I was trying to review the issue in my head and thought it
> might be useful to write it down.
>
> in 4.3 we got the BroadcastDomainType enum introduced, and many parts of the
> code were changed to use that when dealing with the vlan id. This code,
> among other things, returns a vlan id in URI format, describing both the
> technology used to provide the virtual lan, along with the id. Along the way
> this seems to have caused the value itself to be stored as a URI (still not
> sure where, by whom, or if it was intentional). That was fine and seemed to
> work after some fixing, until there was an upgrade done where the existing
> database value was NOT in URI format. We had a few places where the code was
> never changed to use BroadcastDomainType to 'normalize' the info from the
> database (e.g. the IpAssocVpcCommand the mgmt server constructs), so
> upgrades are broken.
>
> Most places in the code as it is now are working with a live value of
> 'vlan://x', regardless of whether the database has 'vlan://x' or just 'x',
> thanks to this code it returns the same 'vlan://' for either stored value.
> For these places it shouldn't matter if we fix the old databases to store
> 'vlan://x' or the 4.3 installs to go back to 'x'.
>
> However, there are a few places that are broken, like this IpAssocVpcCommand
> the mgmt server creates and CLOUDSTACK-5505. If we switch the db value back,
> we have to identify all of the outstanding ones and fix them. In addition,
> new code since then may have perhaps assumed that the db value is 'vlan://',
> and might have bothered to pass through the interpolation, so they may break
> as well. If we had full coverage on the test suite it would be easy to
> change the value back in the DB of a 4.3 or 4.4 install and see what breaks.
>
> If we don't switch the value back, and instead update old databases to the
> current way, it fixes the immediate issue but we end up with code doing the
> same thing in two different ways. Some places will be using the raw db value
> and other places will be asking for it to be normalized, and both will have
> the same result, which is kind of messy and prone to causing issues down the
> road if something changes again to separate these two.
>
>
> On Mon, Jun 2, 2014 at 10:01 AM, Marcus <shadowsor@gmail.com> wrote:
>>
>> I'm not sure the KVM code needs to be changed, you're asking it to deal
>> with an inconsistency from the mgmt server. Don't you find it odd that one
>> Command from the mgmt server provides broadcastUri="vlan://untagged" and
>> another provides broadcastUri="untagged"?  I'm not sure I understand why
>> changing 'untagged' into a URI format changes its meaning, but it seems like
>> that doesn't make any sense to you, so perhaps we can break that out into a
>> separate column so that we can still capture the info, if needed.
>>
>> If we don't like URI format for the vlan id, that's fine, but we need to
>> do changes to the 4.3 installs and fix 4.4. As mentioned, I remember there
>> being a decent amount of work to handle the "vlan://" when it was
>> introduced, and that will need to be done again to change it back. I'm not
>> against that, but I'm not going to be the one doing that work, either :-)
>>
>>
>>
>> On Mon, Jun 2, 2014 at 3:47 AM, Daan Hoogland <daan.hoogland@gmail.com>
>> wrote:
>>>
>>> I don't think this should be solved this way afterall. 'untagged'
>>> actually means no vlan, so it should not be prepended with 'vlan://'.
>>> I think the kvm code should be fixed for this not the generic code.
>>>
>>> On Fri, May 30, 2014 at 10:59 PM, Daan Hoogland <daan.hoogland@gmail.com>
>>> wrote:
>>> > On Fri, May 30, 2014 at 10:51 PM, Marcus <shadowsor@gmail.com> wrote:
>>> >> Looks good to me, aside from he debug statement.
>>> >
>>> > Ah, the first line was not in my line of sight.
>>> > --
>>> > Daan
>>>
>>>
>>>
>>> --
>>> Daan
>>
>>
>



-- 
Daan

Mime
View raw message