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 >
