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 Wed, 04 Jun 2014 15:16:08 GMT
Marcus,

I didn't do the db thing for 4.3 but it is idem-potent and can go in a
Upgrade430to431.java as well. This one doesn't exist yet.

On Wed, Jun 4, 2014 at 4:27 PM, Marcus <shadowsor@gmail.com> wrote:
> That wasn't the patch I thought it was. Regarding
> 5e80e5d33d9a295b91cdba9377f52d9d963d802a, we should probably do that for
> IpAssocCommand as well. I'm not sure we have the db fix in 4.3 yet, and so a
> fix like this would be required for IpAssocCommand (and perhaps other
> unfound things).
>
>
> On Tue, Jun 3, 2014 at 3:22 PM, Marcus <shadowsor@gmail.com> wrote:
>>
>> Hmm.. ok. I guess we can apply the bandaid patch as well
>>
>>
>> On Tue, Jun 3, 2014 at 12:16 PM, Edison Su <Edison.su@citrix.com> wrote:
>>>
>>> I checked in a commit: 5e80e5d33d9a295b91cdba9377f52d9d963d802a, which
>>> will fix some of the mess of vlan id.
>>>
>>> > -----Original Message-----
>>> > From: Marcus [mailto:shadowsor@gmail.com]
>>> > Sent: Tuesday, June 03, 2014 9:57 AM
>>> > To: Daan Hoogland
>>> > Cc: dev
>>> > Subject: Re: VPC's VR missing public NIC eth1
>>> >
>>> > Ok, thanks. It seems there are other cases where the Command being
>>> > passed from the mgmt server has inconsistent broadcastUri as well, this
>>> > should blanket fix them. In the meantime there's a growing group of 4.3
>>> > upgraders who are getting pitchforks out over at CLOUDSTACK-6464, so we
>>> > may want to have something in 4.3.1 too.
>>> >
>>> >
>>> > On Tue, Jun 3, 2014 at 12:30 AM, Daan Hoogland
>>> > <daan.hoogland@gmail.com>
>>> > wrote:
>>> >
>>> > > 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
>>> > >
>>
>>
>



-- 
Daan

Mime
View raw message