Re: [PATCHv2 0.5/4] network: fix virNetworkObjAssignDef and persistence

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 04/25/2014 01:55 PM, Laine Stump wrote:
> On 04/24/2014 07:09 PM, John Ferlan wrote:
>>
>> On 04/23/2014 09:49 AM, Laine Stump wrote:
>>> Experimentation showed that if virNetworkCreateXML() was called for a
>>> network that was already defined, and then the network was
>>> subsequently shutdown, the network would continue to be persistent
>>> after the shutdown (expected/desired), but the original config would
>>> be lost in favor of the transient config sent in with
>>> virNetworkCreateXML() (which would then be the new persistent config)
>>> (obviously unexpected/not desired).
>>>
>>> To fix this, virNetworkObjAssignDef() has been changed to
>>>
>>> 1) properly save/free network->def and network->newDef for all the
>>> various combinations of live/active/persistent, including some
>>> combinations that were previously considered to be an error but didn't
>>> need to be (e.g. setting a "live" config for a network that isn't yet
>>> active but soon will be - that was previously considered an error,
>>> even though in practice it can be very useful).
>>>
>>> 2) automatically set the persistent flag whenever a new non-live
>>> config is assigned to the network (and clear it when the non-live
>>> config is set to NULL). the libvirt network driver no longer directly
>>> manipulates network->persistent, but instead relies entirely on
>>> virNetworkObjAssignDef() to do the right thing automatically.
>>>
>>> After this patch, the following sequence will behave as expected:
>>>
>>> virNetworkDefineXML(X)
>>> virNetworkCreateXML(X') (same name but some config different)
>>> virNetworkDestroy(X)
>>>
>>> At the end of these calls, the network config will remain as it was
>>> after the initial virNetworkDefine(), whereas previously it would take
>>> on the changes given during virNetworkCreateXML().
>>>
>>> Another effect of this tighter coupling between a) setting a !live def
>>> and b) setting/clearing the "persistent" flag, is that future patches
>>> which change the details of network lifecycle management
>>> (e.g. upcoming patches to fix detection of "active" networks when
>>> libvirtd is restarted) will find it much more difficult to break
>>> persistence functionality.
>>> ---
>>>  src/conf/network_conf.c           | 78 +++++++++++++++++++++++----------------
>>>  src/conf/network_conf.h           |  6 +--
>>>  src/network/bridge_driver.c       | 34 +++++++----------
>>>  src/parallels/parallels_network.c |  4 +-
>>>  src/test/test_driver.c            |  5 +--
>>>  5 files changed, 64 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> index 56c4a09..66945ce 100644
>>> --- a/src/conf/network_conf.c
>>> +++ b/src/conf/network_conf.c
>>> @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets)
>>>  /*
>>>   * virNetworkObjAssignDef:
>>>   * @network: the network object to update
>>> - * @def: the new NetworkDef (will be consumed by this function iff successful)
>>> + * @def: the new NetworkDef (will be consumed by this function)
>>>   * @live: is this new def the "live" version, or the "persistent" version
>>>   *
>>> - * Replace the appropriate copy of the given network's NetworkDef
>>> + * Replace the appropriate copy of the given network's def or newDef
>>>   * with def. Use "live" and current state of the network to determine
>>> - * which to replace.
>>> + * which to replace and what to do with the old defs. When a non-live
>>> + * def is set, indicate that the network is now persistent.
>>> + *
>>> + * NB: a persistent network can be made transient by calling with:
>>> + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the
>>> + * persistent def to NULL)
>>>   *
>>>   * Returns 0 on success, -1 on failure.
>>>   */
>>> -int
>>> +void
>> Well this doesn't jive with the comment above about what it returns...
> 
> Yeah, when all the failure paths disappeared I changed the function to
> void, but forgot to remove that comment. I'll fix that now.
> 
>>>  virNetworkObjAssignDef(virNetworkObjPtr network,
>>>                         virNetworkDefPtr def,
>>>                         bool live)
>>>  {
>>> -    if (virNetworkObjIsActive(network)) {
>>> -        if (live) {
>>> +    if (live) {
>>> +        /* before setting new live def, save (into newDef) any
>>> +         * existing persistent (!live) def to be restored when the
>>> +         * network is destroyed, unless there is one already saved.
>>> +         */
>>> +        if (network->persistent && !network->newDef)
>>> +            network->newDef = network->def;
>>> +        else
>>>              virNetworkDefFree(network->def);
>>> -            network->def = def;
>>> -        } else if (network->persistent) {
>>> -            /* save current configuration to be restored on network shutdown */
>>> -            virNetworkDefFree(network->newDef);
>>> +        network->def = def;
>>> +    } else { /* !live */
>>> +        virNetworkDefFree(network->newDef);
>>> +        if (virNetworkObjIsActive(network)) {
>>> +            /* save new configuration to be restored on network
>>> +             * shutdown, leaving current live def alone
>>> +             */
>>>              network->newDef = def;
>>> -        } else {
>>> -            virReportError(VIR_ERR_OPERATION_INVALID,
>>> -                           _("cannot save persistent config of transient "
>>> -                             "network '%s'"), network->def->name);
>>> -            return -1;
>>> +        } else { /* !live and !active */
>>> +            if (network->def && !network->persistent) {
>>> +                /* network isn't (yet) marked active or persistent,
>>> +                 * but already has a "live" def set. This means we are
>>> +                 * currently setting the persistent def as a part of
>>> +                 * the process of starting the network, so we need to
>>> +                 * preserve the "not yet live" def in network->def.
>>> +                 */
>>> +                network->newDef = def;
>>> +            } else {
>>> +                /* either there is no live def set, or this network
>>> +                 * was already set as persistent, so the proper thing
>>> +                 * is to overwrite network->def.
>>> +                 */
>>> +                network->newDef = NULL;
>> If I'm reading correctly - the newDef is virNetworkDefFree()'d above as
>> we enter the "} else { /* !live */" clause, which is probably where this
>> statement should be moved.
> 
> No, I just did some reduction of common code by moving it above the if
> statement. You'll notice that network->newDef is set to *something* in
> every path through those ifs and elses, so at the end of the function,
> it is either NULL or pointing to a valid object.
> 
>>
>>> +                virNetworkDefFree(network->def);
>>> +                network->def = def;
>>
>> NOTE: I think this is what is causing issues in networkUndefine() -
>> that'll call this function with '!live' - thus you'll do the free the
>> 'def' and set it to NULL... Back in the caller there's all sorts of
>> touching of the network->def-> structure
> 
> Yes, indirectly. But the *real* problem is that we shouldn't be calling
> this function until after we're finished using anything in network->def.
> See below for the small diff I'm squashing in to fix this.
> 
>>
>>
>>> +            }
>>>          }
>>> -    } else if (!live) {
>>> -        virNetworkDefFree(network->newDef);
>>> -        virNetworkDefFree(network->def);
>>> -        network->newDef = NULL;
>>> -        network->def = def;
>>> -    } else {
>>> -        virReportError(VIR_ERR_OPERATION_INVALID,
>>> -                       _("cannot save live config of inactive "
>>> -                         "network '%s'"), network->def->name);
>>> -        return -1;
>>> +        network->persistent = !!def;
>>>      }
>>> -    return 0;
>>>  }
>>>  
>>>  /*
>>> @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>>>      virNetworkObjPtr network;
>>>  
>>>      if ((network = virNetworkFindByName(nets, def->name))) {
>>> -        if (virNetworkObjAssignDef(network, def, live) < 0) {
>>> -            virNetworkObjUnlock(network);
>>> -            return NULL;
>>> -        }
>>> +        virNetworkObjAssignDef(network, def, live);
>>>          return network;
>>>      }
>>>  
>>> @@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>>>      ignore_value(virBitmapSetBit(network->class_id, 2));
>>>  
>>>      network->def = def;
>>> -
>>> +    network->persistent = !live;
>>>      return network;
>>> +
>>>   error:
>>>      virNetworkObjUnlock(network);
>>>      virNetworkObjFree(network);
>>> @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets,
>>>          goto error;
>>>  
>>>      net->autostart = autostart;
>>> -    net->persistent = 1;
>>>  
>>>      VIR_FREE(configFile);
>>>      VIR_FREE(autostartLink);
>>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>>> index 1a864de..90da16f 100644
>>> --- a/src/conf/network_conf.h
>>> +++ b/src/conf/network_conf.h
>>> @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn,
>>>  virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets,
>>>                                       virNetworkDefPtr def,
>>>                                       bool live);
>>> -int virNetworkObjAssignDef(virNetworkObjPtr network,
>>> -                           virNetworkDefPtr def,
>>> -                           bool live);
>>> +void virNetworkObjAssignDef(virNetworkObjPtr network,
>>> +                            virNetworkDefPtr def,
>>> +                            bool live);
>>>  int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live);
>>>  void virNetworkObjUnsetDefTransient(virNetworkObjPtr network);
>>>  virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network);
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index eb276cd..62d1c25 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml)
>>>      if (networkValidate(driver, def, true) < 0)
>>>         goto cleanup;
>>>  
>>> -    /* NB: "live" is false because this transient network hasn't yet
>>> -     * been started
>>> +    /* NB: even though this transient network hasn't yet been started,
>>> +     * we assign the def with live = true in anticipation that it will
>>> +     * be started momentarily.
>>>       */
>>> -    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>>> +    if (!(network = virNetworkAssignDef(&driver->networks, def, true)))
>>>          goto cleanup;
>>>      def = NULL;
>>>  
>>> @@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
>>>      if (networkValidate(driver, def, false) < 0)
>>>         goto cleanup;
>>>  
>>> -    if ((network = virNetworkFindByName(&driver->networks, def->name))) {
>>> -        network->persistent = 1;
>>> -        if (virNetworkObjAssignDef(network, def, false) < 0)
>>> -            goto cleanup;
>>> -    } else {
>>> -        if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>>> -            goto cleanup;
>>> -    }
>>> -
>>> -    /* define makes the network persistent - always */
>>> -    network->persistent = 1;
>>> +    if (!(network = virNetworkAssignDef(&driver->networks, def, false)))
>>> +       goto cleanup;
>>>  
>>> -    /* def was asigned */
>>> +    /* def was assigned to network object */
>>>      freeDef = false;
>>>  
>>>      if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) {
>>> @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml)
>>>              network = NULL;
>>>              goto cleanup;
>>>          }
>>> -        network->persistent = 0;
>>> -        virNetworkDefFree(network->newDef);
>>> -        network->newDef = NULL;
>>> +        /* if network was active already, just undo new persistent
>>> +         * definition by making it transient.
>>> +         * XXX - this isn't necessarily the correct thing to do.
>>> +         */
>> Leaving the XXX comment in here leaves me wondering what is the correct
>> thing to do...
> 
> The correct thing to do is... well, to exactly rollback to what you had
> before the failure.
> 
> However, the current code (both before *and* after this patch) does the
> (same) wrong thing in this case - if you fail during an attempt to
> redefine an already persistent+active network, you'll end up with a
> transient+active network (so it will disappear as soon as the network is
> stopped).
> 
> It would be better if such a failure caused us to revert back to the
> previous persistent def. That would require saving off the existing def
> before trying to assign the new one or writing to disk. But that is a
> separate problem and should be in a separate patch anyway. The only
> reason I touched that area of the code in this patch was that I was
> replacing direct assignment of network->persistent with calling
> virNetworkObjAssignDef() (while preserving the existing functionality).
> I did think it was worth noting the problem with the existing
> functionality while touching the code, however.
> 
> So, while I think this needs fixing, it's pre-existing and I'd rather
> not have these other patches delayed because of it.
> 
>>
>>> +        virNetworkObjAssignDef(network, NULL, false);
>>>          goto cleanup;
>>>      }
>>>  
>>> @@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net)
>>>          goto cleanup;
>>>  
>>>      /* make the network transient */
>>> -    network->persistent = 0;
>>>      network->autostart = 0;
>>> -    virNetworkDefFree(network->newDef);
>>> -    network->newDef = NULL;
>>> +    virNetworkObjAssignDef(network, NULL, false);
>> This seems to be the cause of the crash I noted in private chat. If I
>> restore just these lines and not make the ObjAssignDef() call, then
>> things work again.  That includes issues I was seeing in virt-test for
>> patches 3 & 4 as well.
> 
> Yep. That call needs to be moved down to below the call to
> networkRemoveInactive. I've done that and all the tests complete without
> failure (as you've also separately reported on IRC). Here is what I
> squashed into this patch:
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 309d967..2a54aa3 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2899,14 +2899,12 @@ networkUndefine(virNetworkPtr net)
>      if (virNetworkObjIsActive(network))
>          active = true;
>  
> +    /* remove autostart link */
>      if (virNetworkDeleteConfig(driver->networkConfigDir,
>                                 driver->networkAutostartDir,
>                                 network) < 0)
>          goto cleanup;
> -
> -    /* make the network transient */
>      network->autostart = 0;
> -    virNetworkObjAssignDef(network, NULL, false);
>  
>      event = virNetworkEventLifecycleNew(network->def->name,
>                                          network->def->uuid,
> @@ -2920,6 +2918,12 @@ networkUndefine(virNetworkPtr net)
>              goto cleanup;
>          }
>          network = NULL;
> +    } else {
> +
> +        /* if the network still exists, it was active, and we need to make
> +         * it transient (by deleting the persistent def

s/def/def)/

> +         */
> +        virNetworkObjAssignDef(network, NULL, false);
>      }
>  
>      ret = 0;
> --
> 
>>
>> Just figured I'd close the loop on this review at least...
> 
> Do my responses clear up your concerns?
> 
> 
> 

Yep - all set, thanks for the extra details...

You have an ACK with that extra ) above



John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]