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
