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

Re: [libvirt] [PATCH v2 2/3] network: Introduce start and shutdown hooks



On 07.02.2014 14:27, Laine Stump wrote:
On 02/05/2014 12:11 PM, Michal Privoznik wrote:
There might be some use cases, where user wants to prepare the host or
its environment prior to starting a network and do some cleanup after
the network has been shut down. Consider all the functionality that
libvirt doesn't currently have as an example what a hook script can
possibly do.

Signed-off-by: Michal Privoznik <mprivozn redhat com>
---
  docs/hooks.html.in          |  60 ++++++++++++++++++----
  src/lxc/lxc_driver.c        |   4 +-
  src/lxc/lxc_process.c       |   6 +--
  src/network/bridge_driver.c | 120 +++++++++++++++++++++++++++++++++++++++++++-
  src/network/bridge_driver.h |  14 +++---
  src/qemu/qemu_command.c     |   2 +-
  src/qemu/qemu_hotplug.c     |  14 +++---
  src/qemu/qemu_process.c     |   2 +-
  src/util/virhook.c          |  13 ++++-
  src/util/virhook.h          |  11 ++++
  10 files changed, 212 insertions(+), 34 deletions(-)

diff --git a/docs/hooks.html.in b/docs/hooks.html.in
index f0f692b..9deb215 100644
--- a/docs/hooks.html.in
+++ b/docs/hooks.html.in
@@ -16,6 +16,9 @@
            configuration<br/><br/></li>
        <li>A QEMU guest is started or stopped<br/><br/></li>
        <li>An LXC guest is started or stopped<br/><br/></li>


Do we know when support for the above hooks was added? The way it's
written here, it sounds like support for *all* hook types was added in
1.2.2. If the support has been there since those particular hypervisor
drivers were added, then we should put that version.

Okay, I'll append "since 0.8.0" to the first three items (yes, they were all introduced at once in that release).



+      <li>A network is started or stopped or an interface is
+          un-/plugged from/to the network
+          (<span class="since">since 1.2.2</span>)<br/><br/></li>
      </ul>

      <h2><a name="location">Script location</a></h2>
@@ -44,6 +47,9 @@
            Executed when a QEMU guest is started, stopped, or migrated<br/><br/></li>
        <li><code>/etc/libvirt/hooks/lxc</code><br /><br/>
            Executed when an LXC guest is started or stopped</li>
+      <li><code>/etc/libvirt/hooks/network</code><br/><br/>
+          Executed when a network is started or stopped or an
+          interface is un-/plugged from/to the network</li>
      </ul>
      <br/>

@@ -66,6 +72,12 @@
         XML description for the domain on their stdin. This includes items
         such the UUID of the domain and its storage information, and is
         intended to provide all the libvirt information the script needs.</p>
+    <p>The network hook script is fed on its stdin with <b>full</b> XML
+       description of network in which an action occurs. However, in cases of
+       interface being plugged or unplugged to the network, script can expect
+       full XML description of corresponding domain appended after the network
+       XML (this happens when a domain with an &lt;interface/&gt; belonging to
+       the network is started or destroyed).</p>

How about this:

For all cases, stdin of the network hook script is provided with the
full XML description of the network status. In the case of an interface
being plugged/unplugged to the network, the network XML will be followed
with the full XML description of the domain containing the interface
that is being plugged/unplugged.

Okay, I'm not a native speaker, so my documentation may be hard to parse sometimes :)



      <p>The command line arguments take this approach:</p>
      <ol>
@@ -181,23 +193,49 @@
          <pre>/etc/libvirt/hooks/lxc guest_name reconnect begin -</pre>
        </li>
      </ul>
+
+    <h5><a name="network">/etc/libvirt/hooks/network</a></h5>
+    <ul>
+      <li><span class="since">Since 1.2.2</span>, before a network is started,
+        this script is called as:<br/>
+          <pre>/etc/libvirt/hooks/network network_name start begin -</pre></li>
+      <li>After the network is started, up &and; running, the script is
+        called as:<br/>
+          <pre>/etc/libvirt/hooks/network network_name started begin -</pre></li>
+      <li>When a network is shut down, this script is called as:<br/>
+          <pre>/etc/libvirt/hooks/network network_name stopped end -</pre></li>


I wonder if anyone will ever need a "stop" event to be run just before
we begin shutting down a network. I'm undecided if it's better to have
it there from the beginning even if it's never used, or to not clutter
the code until we come up with a reason for it.

Maybe, if they will want prevent the shutdown. That is, if the hook script quits with nonzero status, libvirt may refuse to shutdown the network - for instance, the script knows that network is used by something else that is not under libvirt control.



+      <li>Later, when network is started and there's an interface from a
+        domain to be plugged into the network (plugged may not be the correct
+        expression when it comes to bridgeless netowrks, perhaps allocated is

s/netowrks/networks/

If we want to be completely accurate about what's happening under the
covers, then I think you are correct about using allocate/free rather
than plug/unplug. I don't know if that level of accuracy is all that
important here though. Again, undecided.

I'll rather stick with my version. In general, interfaces are plugged to and unplugged from networks. The fact, that we bend our <network/> definition so that e.g. device passthrough is possible - that's different story.



+        better one then), the hook script is called as:<br/>
+          <pre>/etc/libvirt/hooks/network network_name plugged begin -</pre>
+        Please note, that in this case, the script is passed both network and
+        domain XMLs on its stdin.</li>
+      <li>When the domain from previous case is shutting down, the interface
+        is unplugged. This leads to another script invocation:<br/>
+          <pre>/etc/libvirt/hooks/network network_name unplugged begin -</pre>
+        And again, as in previous case, both network and domain XMLs are passed
+        onto script's stdin.</li>


Again, I'm wondering if anyone will ever be concerned about whether
their hook is executed before or after the network device is
allocated/created/connected, and likewise before/after
freeing/destroying/unplugging/whatever you want to call it.

Currently it doesn't make much sense as the return value of the hook script is ignored. But later somebody may come and demand that the hook script must be able to deny interface plugging.



+    </ul>
+
      <br/>

      <h2><a name="execution">Script execution</a></h2>
      <ul>
-      <li>The "start" operation for the guest hook scripts, qemu and lxc,
-          executes <b>prior</b> to the guest being created.  This allows the
-          guest start operation to be aborted if the script returns indicating
-          failure.<br/><br/></li>
-      <li>The "shutdown" operation for the guest hook scripts, qemu and lxc,
-          executes <b>after</b> the guest has stopped.  If the hook script
-          indicates failure in its return, the shut down of the guest cannot
-          be aborted because it has already been performed.<br/><br/></li>
+      <li>The "start" operation for the guest and network hook scripts,
+          executes <b>prior</b> to the object (guest or network) being created.
+          This allows the object start operation to be aborted if the script
+          returns indicating failure.<br/><br/></li>
+      <li>The "shutdown" operation for the guest and network hook scripts,
+          executes <b>after</b> the object (guest or network) has stopped. If
+          the hook script indicates failure in its return, the shut down of the
+          object cannot be aborted because it has already been performed.
+          <br/><br/></li>
        <li>Hook scripts execute in a synchronous fashion.  Libvirt waits
            for them to return before continuing the given operation.<br/><br/>
-          This is most noticeable with the guest start operation, as a lengthy
-          operation in the hook script can mean an extended wait for the guest
-          to be available to end users.<br/><br/></li>
+          This is most noticeable with the guest or network start operation,
+          as a lengthy operation in the hook script can mean an extended wait
+          for the guest or network to be available to end users.<br/><br/></li>
        <li>For a hook script to be utilised, it must have its execute bit set
            (ie. chmod o+rx <i>qemu</i>), and must be present when the libvirt

(I know this is pre-existing text, but the usage of "ie." above is

@@ -2034,6 +2052,23 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
      if (virNetworkSaveStatus(driverState->stateDir, network) < 0)
          goto cleanup;

+    /* finally we can call the 'started' hook script if any */
+    if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
+        char *xml = virNetworkDefFormat(network->def, 0);
+        int hookret;


I was going to suggest formatting this once and keeping it around to
make things faster, but then realized that the network status will
change between the first time its formatted and now, so it's proper
(even necessary) to recreate it.

One thing I've noticed though is that you're not checking for non-null
return from virNetworkDefFormat (here or in other places), and
virHookCall itself also doesn't treat this as an error. So some problem
formatting the network XML would lead to the script being executed with
no network xml on stdin, rather than an error.

I wonder if that should be an error at this level, or if virHookCall
should *always* have a non-null 6th argument.


Well, in other places we are not checking the return value neither. But you're right. It doesn't hurt to check here. Will do.



+
+        hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
+                              VIR_HOOK_NETWORK_OP_STARTED,
+                              VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL);
+        VIR_FREE(xml);
+
+        /*
+         * If the script raised an error abort the launch
+         */
+        if (hookret < 0)
+            goto cleanup;
+    }
+
      network->active = 1;
      VIR_INFO("Network '%s' started up", network->def->name);
      ret = 0;
@@ -2087,6 +2122,17 @@ static int networkShutdownNetwork(virNetworkDriverStatePtr driver,
          break;
      }

+    /* now that we know it's stopped call the hook if present */
+    if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
+        char *xml = virNetworkDefFormat(network->def, 0);
+
+        /* we can't stop the operation even if the script raised an error */
+        virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
+                    VIR_HOOK_NETWORK_OP_STOPPED, VIR_HOOK_SUBOP_END,
+                    NULL, xml, NULL);
+        VIR_FREE(xml);
+    }
+
      network->active = 0;
      virNetworkObjUnsetDefTransient(network);
      return ret;
@@ -3223,6 +3269,7 @@ finish:
  }

  /* networkAllocateActualDevice:
+ * @dom: domain definition that @iface belongs to
   * @iface: the original NetDef from the domain
   *
   * Looks up the network reference by iface, allocates a physical
@@ -3234,7 +3281,8 @@ finish:
   * Returns 0 on success, -1 on failure.
   */
  int
-networkAllocateActualDevice(virDomainNetDefPtr iface)
+networkAllocateActualDevice(virDomainDefPtr dom,
+                            virDomainNetDefPtr iface)
  {
      virNetworkDriverStatePtr driver = driverState;
      enum virDomainNetType actualType = iface->type;
@@ -3567,6 +3615,42 @@ validate:
          }
      }

+    /* finally we can call the 'plugged' hook script if any */
+    if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
+        /* the XML construction is a bit complicated here,
+         * as we want to pass both domain XML and network XML */
+        virBuffer buf = VIR_BUFFER_INITIALIZER;
+        char *xml, *net_xml, *dom_xml;
+        int hookret;
+
+        net_xml = virNetworkDefFormat(netdef, 0);
+        dom_xml = virDomainDefFormat(dom, 0);
+
+        virBufferAdd(&buf, net_xml, -1);
+        virBufferAdd(&buf, dom_xml, -1);
+
+        VIR_FREE(net_xml);
+        VIR_FREE(dom_xml);
+
+        if (virBufferError(&buf)) {
+            virBufferFreeAndReset(&buf);
+            goto error;
+        }
+
+        xml = virBufferContentAndReset(&buf);
+
+        hookret = virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
+                              VIR_HOOK_NETWORK_OP_IFACE_PLUGGED,
+                              VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL);
+        VIR_FREE(xml);
+
+        /*
+         * If the script raised an error abort the allocation
+         */
+        if (hookret < 0)
+            goto error;
+    }
+
      if (dev) {
          /* we are now assured of success, so mark the allocation */
          dev->connections++;
@@ -3780,6 +3864,7 @@ error:


  /* networkReleaseActualDevice:
+ * @dom: domain definition that @iface belongs to
   * @iface:  a domain's NetDef (interface definition)
   *
   * Given a domain <interface> element that previously had its <actual>
@@ -3790,7 +3875,8 @@ error:
   * Returns 0 on success, -1 on failure.
   */
  int
-networkReleaseActualDevice(virDomainNetDefPtr iface)
+networkReleaseActualDevice(virDomainDefPtr dom,
+                           virDomainNetDefPtr iface)
  {
      virNetworkDriverStatePtr driver = driverState;
      enum virDomainNetType actualType = virDomainNetGetActualType(iface);
@@ -3909,6 +3995,36 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
  success:
      if (iface->data.network.actual)
          netdef->connections--;
+
+    /* finally we can call the 'unplugged' hook script if any */
+    if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
+        /* the XML construction is a bit complicated here,
+         * as we want to pass both domain XML and network XML */
+        virBuffer buf = VIR_BUFFER_INITIALIZER;
+        char *xml, *net_xml, *dom_xml;
+
+        net_xml = virNetworkDefFormat(netdef, 0);
+        dom_xml = virDomainDefFormat(dom, 0);
+
+        virBufferAdd(&buf, net_xml, -1);
+        virBufferAdd(&buf, dom_xml, -1);
+
+        VIR_FREE(net_xml);
+        VIR_FREE(dom_xml);
+
+        if (virBufferError(&buf)) {
+            virBufferFreeAndReset(&buf);
+            goto error;
+        }
+
+        xml = virBufferContentAndReset(&buf);
+
+        virHookCall(VIR_HOOK_DRIVER_NETWORK, network->def->name,
+                    VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
+                    VIR_HOOK_SUBOP_BEGIN, NULL, xml, NULL);
+        VIR_FREE(xml);
+    }
+
      VIR_DEBUG("Releasing network %s, %d connections",
                netdef->name, netdef->connections);
      ret = 0;
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 50258b5..e6640ac 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -34,12 +34,14 @@
  int networkRegister(void);

  # if WITH_NETWORK
-int networkAllocateActualDevice(virDomainNetDefPtr iface)
-    ATTRIBUTE_NONNULL(1);
+int networkAllocateActualDevice(virDomainDefPtr dom,
+                                virDomainNetDefPtr iface)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  int networkNotifyActualDevice(virDomainNetDefPtr iface)
      ATTRIBUTE_NONNULL(1);

Oh, right! The Notify function. This function is called for each
interface connected to a libvirt network whenever libvirtd is restarted.
Right now it's used to verify that the resources used by the guest are
not being used by someone else, and to mark them as used in the
newly-reconstructed network object. We are going to need an event here
so that any iptables rules created during the "plugged" hook can be
recreated - necessary because libvirtd reloads all of a network's
iptables rules every time it is restarted (an example of why this is
necessary is illustrated in a hackish use of the qemu hook here:

http://wiki.libvirt.org/page/Networking#Forwarding_Incoming_Connections

(We should re-write this hook as a network hook script once your patches
are pushed).


Mmmmm. 'kay. Will send v3.

Michal


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