That would be a good idea
On Wed, Jun 4, 2014 at 9:16 AM, Daan Hoogland <daan.hoogl...@gmail.com> wrote: > 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 <shadow...@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 <shadow...@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...@citrix.com> > wrote: > >>> > >>> I checked in a commit: 5e80e5d33d9a295b91cdba9377f52d9d963d802a, which > >>> will fix some of the mess of vlan id. > >>> > >>> > -----Original Message----- > >>> > From: Marcus [mailto:shadow...@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.hoogl...@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 <shadow...@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 <shadow...@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.hoogl...@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.hoogl...@gmail.com> > >>> > > >>> wrote: > >>> > > >>> > On Fri, May 30, 2014 at 10:51 PM, Marcus < > shadow...@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 >
