[libvirt] [PATCH 3/5] network: read status from old location and move to new

[John Ferlan]

On 04/10/2014 09:19 AM, Laine Stump wrote:
> This eliminates problems with upgrading from older libvirt that stores
> network status in /var/lib/libvirt/network to newer libvirt that
> stores it in /var/run/libvirt/network, by also reading any status
> files from the old location, saving them to the new location, and
> removing them from the old location.
> 
> This will not help those trying to downgrade, but in practice this
> will only be problematic in two cases
> 
> 1) If there are networks with network-wide bandwidth limits configured
>    and in use by a guest during a downgrade to "old" libvirt. In this
>    case, the class ID's used for that network's tc rules, as well as
>    the currently in-use bandwidth "floor" will be forgotten
> 
> 2) If someone does this: 1) upgrade, 2) downgrade, 3) modify running
>    state of network (e.g. add a static dhcp host, etc), 4) upgrade. In
>    this case, the modifications to the running network will be lost
>    (but not any persistent changes to the network's config).
> 
> I have an idea of how to make these cases work properly as well
> (involving symlinks in /var/lib pointing to /var/run) but want to
> avoid keeping such an ugly legacy around (forever) unlesss others
> think it is absolutely necessary to support flawless downgrades across
> this line *while remaining in service*.

I would have to believe/think the downgrade issue has happened before
with some other change to config or status file locations. Of course, if
some state/status is meant to "live" beyond the current execution, then
perhaps /run/ isn't the right place to store that.  Wouldn't something
like that end up in /etc/libvirt... somewhere?   That is usage of
'--live' vs. '--config' when configuring limits?

> ---
>  src/network/bridge_driver.c          | 40 ++++++++++++++++++++++++++++++++++++
>  src/network/bridge_driver_platform.h |  3 ++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index a027b47..98d10bd 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -459,6 +459,8 @@ networkStateInitialize(bool privileged,
>                         SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 ||
>              VIR_STRDUP(driverState->stateDir,
>                         LOCALSTATEDIR "/run/libvirt/network") < 0 ||
> +            VIR_STRDUP(driverState->oldStateDir,
> +                       LOCALSTATEDIR "/lib/libvirt/network") < 0 ||

You'll need a corresponding VIR_FREE() in networkStateCleanup()

>              VIR_STRDUP(driverState->pidDir,
>                         LOCALSTATEDIR "/run/libvirt/network") < 0 ||
>              VIR_STRDUP(driverState->dnsmasqStateDir,
> @@ -498,6 +500,44 @@ networkStateInitialize(bool privileged,
>      /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */
>      driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ);
>  
> +    /* Due to a change in location of network state xml beginning in
> +     * libvirt 1.2.4, we must check for state files in two
> +     * locations. Anything found in the old location must be written
> +     * to the new location, then erased from the old location.
> +     */

ahh... so this isn't done for the non privileged case...  So if
privileged and the network state exists in the old state dir...  Also
could use STRNEQ_NULLABLE() right?

> +    if (driverState->oldStateDir &&
> +        STRNEQ(driverState->stateDir, driverState->oldStateDir)) {
> +        size_t i;
> +
> +        /* read all */
> +        if (virNetworkLoadAllState(&driverState->networks,
> +                                   driverState->oldStateDir) < 0)
> +            goto error;

So any/all failures to read the old entry/style causes failure...  Could
this be problematic if something ends up existing in the "new" format
that isn't in the "old" format and thus causes exit by the changed
parsing code?  This is perhaps the opposite problem to flawless
downgrades. It also points out the question of how long do we keep this
old reading code around before it's removed?

> +
> +        for (i = 0; i < driverState->networks.count; i++) {
> +            virNetworkObjPtr network = driverState->networks.objs[i];
> +            char *oldFile;
> +            int status;
> +
> +            /* save in new location */
> +            virNetworkObjLock(network);
> +            status = virNetworkSaveStatus(driverState->stateDir, network);
> +            virNetworkObjUnlock(network);
> +            if (status < 0)
> +                goto cleanup;
> +
> +            /* remove from old location */
> +            oldFile = virNetworkConfigFile(driverState->oldStateDir,
> +                                           network->def->name);
> +            if (!oldFile)
> +                goto cleanup;
> +            unlink(oldFile);

Alternatively to removing - rename to some suffix which at least gives a
chance for someone going through a downgrade process to recover the old
file. Of course documenting this would be nice too.

> +            VIR_FREE(oldFile);
> +        }
> +        /* these will all be re-read from their new location */
> +        virNetworkObjListFree(&driverState->networks);
> +    }
> +

This hunk should be it's own local/static function...

ACK pending your thoughts regarding my comments...

John

>      if (virNetworkLoadAllState(&driverState->networks,
>                                 driverState->stateDir) < 0)
>          goto error;
> diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
> index 6a571da..491068a 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -1,7 +1,7 @@
>  /*
>   * bridge_driver_platform.h: platform specific routines for bridge driver
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -39,6 +39,7 @@ struct _virNetworkDriverState {
>      char *networkConfigDir;
>      char *networkAutostartDir;
>      char *stateDir;
> +    char *oldStateDir;
>      char *pidDir;
>      char *dnsmasqStateDir;
>      char *radvdStateDir;
>