Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On Thu, Jul 17, 2014 at 9:38 PM, Eric Blake ebl...@redhat.com wrote: On 07/17/2014 09:18 AM, Peter Krempa wrote: A second issue that comes into my mind is compatibility with already existing files. Does this work when you already have a lease file? (I didn't try it, I'm just asking). If we use the --leasefile-ro option, then although this method will work, but no, the lease file will *not* be read/written at all. So even if an old one exists, its of no use. But then again, the use of --leasefile-ro is mandatory, so that all events are captured by our helper program. For example, renew of a lease. My concerns are whether this will work in the case you already used the leases helper as the patch is adding a few fields to the stored format. In particular, this scenario MUST work: A user installs libvirt 1.2.6, and starts a guest (which creates a leases file and longrunning helper app). Then the user upgrades to libvirt 1.2.7 and restarts libvirtd. The new libvirt MUST be able to interact with the running guest that is still using the older-style leases file and helper app, with no loss of behavior to the guest. Also, this scenario MUST work: A user installs libvirt 1.2.6. Then they upgrade to libvirt 1.2.7, but without restarting libvirtd. Then they start a guest. Note that the upgrade means that the leaseshelper application that will be started is the 1.2.7 build, but the command line to start it will be triggered by the 1.2.6 libvirtd. The new helper must do everything that the old helper did, so that the old libvirtd does not have any hiccups in operation. The easiest way to ensure that both directions of version mismatch are well-behaved is to make sure that either side can be newer than the other, that the changes are purely additive in nature, and that there are sane defaults in place in the newer code when dealing with the older data that lacks the additions. Also, while upgrade is a required scenario, downgrade generally is not (going from 1.2.7 back to 1.2.6 while a guest is still running, or expecting 1.2.7 libvirtd to be able to use 1.2.6 leaseshelper, is nice if it works but not a showstopper if it doesn't). This patch only adds functionality, and is backwards compatible with the version of leases file generated by older version of leaseshelper. Anyway, the older version wasn't much helpful after the lease expired. This patch rectifies it. I'll send a v2 shortly. Regards, Nehal J Wani -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On 07/16/14 17:31, Nehal J Wani wrote: cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); virCommandAddArgFormat(cmd, --conf-file=%s, configfile); -virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path); +virCommandAddArgFormat(cmd, --dhcp-script=%s, pseudo_leaseshelper_path); +virCommandAddArgFormat(cmd, --leasefile-ro); Does dnsmasq pass through the rest of the environment we pass to it at this point? The leaseshelper extracts quite a lot of stuff from the environment variables that are passed by dnsmasq. In case it does we could use an env variable to pass the interface name instead of linking the helper and using different names. If I use the following line of code in the function networkBuildDhcpDaemonCommandLine ... setenv(VIR_BRIDGE_NAME, network-def-bridge, 1); .. then yes, the helper program invoked by dnsmasq does get to see this variable. (It does so in Fedora20). (Looks like I acted in haste, should've waited for more replies on RFC, sigh) I wanted to reply to that but I was a bit busy and didn't manage to do it soon enough. We have wrappers for running a separate command with a env variable virCommandAddEnv*... you can use easily for this. So, in the next version that I will send, should I first make a wrapper by the name virSetEnvBlockSUID for setenv, just like we have virGetEnvBlockSUID for getenv? No, please use the API mentioned above A second issue that comes into my mind is compatibility with already existing files. Does this work when you already have a lease file? (I didn't try it, I'm just asking). If we use the --leasefile-ro option, then although this method will work, but no, the lease file will *not* be read/written at all. So even if an old one exists, its of no use. But then again, the use of --leasefile-ro is mandatory, so that all events are captured by our helper program. For example, renew of a lease. My concerns are whether this will work in the case you already used the leases helper as the patch is adding a few fields to the stored format. Regards, Nehal J Wani Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
My concerns are whether this will work in the case you already used the leases helper as the patch is adding a few fields to the stored format. In this patch, I am only adding server-duid as an extra JSON object. I am not adding more fields to an existing JSON object. The old leases file will certainly work, even if server-duid is not present. Server-DUID will get added in any event which will get triggered from the ipv6 interface. But all of this will happen assuming dnsmasq and libvirt have been restarted. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On 07/17/2014 09:18 AM, Peter Krempa wrote: A second issue that comes into my mind is compatibility with already existing files. Does this work when you already have a lease file? (I didn't try it, I'm just asking). If we use the --leasefile-ro option, then although this method will work, but no, the lease file will *not* be read/written at all. So even if an old one exists, its of no use. But then again, the use of --leasefile-ro is mandatory, so that all events are captured by our helper program. For example, renew of a lease. My concerns are whether this will work in the case you already used the leases helper as the patch is adding a few fields to the stored format. In particular, this scenario MUST work: A user installs libvirt 1.2.6, and starts a guest (which creates a leases file and longrunning helper app). Then the user upgrades to libvirt 1.2.7 and restarts libvirtd. The new libvirt MUST be able to interact with the running guest that is still using the older-style leases file and helper app, with no loss of behavior to the guest. Also, this scenario MUST work: A user installs libvirt 1.2.6. Then they upgrade to libvirt 1.2.7, but without restarting libvirtd. Then they start a guest. Note that the upgrade means that the leaseshelper application that will be started is the 1.2.7 build, but the command line to start it will be triggered by the 1.2.6 libvirtd. The new helper must do everything that the old helper did, so that the old libvirtd does not have any hiccups in operation. The easiest way to ensure that both directions of version mismatch are well-behaved is to make sure that either side can be newer than the other, that the changes are purely additive in nature, and that there are sane defaults in place in the newer code when dealing with the older data that lacks the additions. Also, while upgrade is a required scenario, downgrade generally is not (going from 1.2.7 back to 1.2.6 while a guest is still running, or expecting 1.2.7 libvirtd to be able to use 1.2.6 leaseshelper, is nice if it works but not a showstopper if it doesn't). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On Fri, Jul 11, 2014 at 5:49 AM, Nehal J Wani nehaljw.k...@gmail.com wrote: This patch enables the helper program to detect event(s) triggered when there is a change in lease length or expiry and client-id. This transfers complete control of leases database to libvirt and suppresses use of the lease database file (network-name.leases). That file will not be created, read, or written. This is achieved by adding the option --leasefile-ro to dnsmasq and applying the symlink technique, which helps us map events related to leases with their corresponding network bridges. Example: /var/lib/libvirt/dnsmasq/virbr0.helper - /home/wani/libvirt/src/libvirt_leaseshelper /var/lib/libvirt/dnsmasq/virbr3.helper - /home/wani/libvirt/src/libvirt_leaseshelper Also, this requires the addition of a new non-lease entry in our custom lease database: server-duid. It is required to identify a DHCPv6 server. Now that dnsmasq doesn't maintain its own leases database, it relies on our helper program to tell it about previous leases and server duid. Thus, this patch makes our leases program honor an extra action: init, in which it sends the known info in a particular format to dnsmasq by printing it to stdout. --- src/network/bridge_driver.c | 43 +++- src/network/leaseshelper.c | 156 +--- 2 files changed, 175 insertions(+), 24 deletions(-) Ping! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On 07/11/14 02:19, Nehal J Wani wrote: This patch enables the helper
program to detect event(s) triggered when there
is a change in lease length or expiry and client-id. This transfers complete
control of leases database to libvirt and suppresses use of the lease database
file (network-name.leases). That file will not be created, read, or written.
This is achieved by adding the option --leasefile-ro to dnsmasq and applying
the symlink technique, which helps us map events related to leases with their
corresponding network bridges.
Example:
/var/lib/libvirt/dnsmasq/virbr0.helper -
/home/wani/libvirt/src/libvirt_leaseshelper
/var/lib/libvirt/dnsmasq/virbr3.helper -
/home/wani/libvirt/src/libvirt_leaseshelper
Also, this requires the addition of a new non-lease entry in our custom lease
database: server-duid. It is required to identify a DHCPv6 server.
Now that dnsmasq doesn't maintain its own leases database, it relies on our
helper program to tell it about previous leases and server duid. Thus, this
patch makes our leases program honor an extra action: init, in which it
sends
the known info in a particular format to dnsmasq by printing it to stdout.
---
src/network/bridge_driver.c | 43 +++-
src/network/leaseshelper.c | 156
+---
2 files changed, 175 insertions(+), 24 deletions(-)
Note this message is not a full review, just a few questions before I
carry on.
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6a2e760..1e1e53f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1287,9 +1303,30 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr
network,
LIBEXECDIR)))
goto cleanup;
+/* Symlink technique for dnsmasq lease file is needed so that libvirt
+ * can have complete control over the handling of leases database */
+
+/* Remove unwanted, old symlink */
+if (unlink(pseudo_leaseshelper_path) 0
+errno != ENOENT errno != ENOTDIR) {
+virReportSystemError(errno,
+ _(Failed to delete symlink '%s'),
+ pseudo_leaseshelper_path);
+goto cleanup;
+}
+
+/* Create a new symlink */
+if (symlink(leaseshelper_path, pseudo_leaseshelper_path) 0) {
+virReportSystemError(errno,
+ _(Failed to create symlink '%s' to '%s'),
+ leaseshelper_path, pseudo_leaseshelper_path);
+goto cleanup;
+}
+
cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
virCommandAddArgFormat(cmd, --conf-file=%s, configfile);
-virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path);
+virCommandAddArgFormat(cmd, --dhcp-script=%s,
pseudo_leaseshelper_path);
+virCommandAddArgFormat(cmd, --leasefile-ro);
Does dnsmasq pass through the rest of the environment we pass to it at
this point? The leaseshelper extracts quite a lot of stuff from the
environment variables that are passed by dnsmasq. In case it does we
could use an env variable to pass the interface name instead of linking
the helper and using different names.
A second issue that comes into my mind is compatibility with already
existing files. Does this work when you already have a lease file? (I
didn't try it, I'm just asking).
Peter
main(int argc, char **argv)
@@ -105,6 +113,7 @@ main(int argc, char **argv)
char *pid_file = NULL;
char *lease_entries = NULL;
char *custom_lease_file = NULL;
+char *server_duid = NULL;
const char *ip = NULL;
const char *mac = NULL;
const char *iaid = virGetEnvAllowSUID(DNSMASQ_IAID);
@@ -112,20 +121,26 @@ main(int argc, char **argv)
const char *interface = virGetEnvAllowSUID(DNSMASQ_INTERFACE);
const char *exptime_tmp = virGetEnvAllowSUID(DNSMASQ_LEASE_EXPIRES);
const char *hostname = virGetEnvAllowSUID(DNSMASQ_SUPPLIED_HOSTNAME);
+const char *server_duid_env = virGetEnvAllowSUID(DNSMASQ_SERVER_DUID);
Here we extract a lot of stuff ...
const char *leases_str = NULL;
long long currtime = 0;
long long expirytime = 0;
size_t i = 0;
+size_t count_ipv6 = 0;
+size_t count_ipv4 = 0;
int action = -1;
int pid_file_fd = -1;
int rv = EXIT_FAILURE;
int custom_lease_file_len = 0;
bool add = false;
+bool init = false;
bool delete = false;
virJSONValuePtr lease_new = NULL;
virJSONValuePtr lease_tmp = NULL;
virJSONValuePtr leases_array = NULL;
virJSONValuePtr leases_array_new = NULL;
+virJSONValuePtr *leases_ipv4 = NULL;
+virJSONValuePtr *leases_ipv6 = NULL;
virSetErrorFunc(NULL, NULL);
virSetErrorLogPriorityFunc(NULL);
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); virCommandAddArgFormat(cmd, --conf-file=%s, configfile); -virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path); +virCommandAddArgFormat(cmd, --dhcp-script=%s, pseudo_leaseshelper_path); +virCommandAddArgFormat(cmd, --leasefile-ro); Does dnsmasq pass through the rest of the environment we pass to it at this point? The leaseshelper extracts quite a lot of stuff from the environment variables that are passed by dnsmasq. In case it does we could use an env variable to pass the interface name instead of linking the helper and using different names. If I use the following line of code in the function networkBuildDhcpDaemonCommandLine ... setenv(VIR_BRIDGE_NAME, network-def-bridge, 1); .. then yes, the helper program invoked by dnsmasq does get to see this variable. (It does so in Fedora20). (Looks like I acted in haste, should've waited for more replies on RFC, sigh) So, in the next version that I will send, should I first make a wrapper by the name virSetEnvBlockSUID for setenv, just like we have virGetEnvBlockSUID for getenv? A second issue that comes into my mind is compatibility with already existing files. Does this work when you already have a lease file? (I didn't try it, I'm just asking). If we use the --leasefile-ro option, then although this method will work, but no, the lease file will *not* be read/written at all. So even if an old one exists, its of no use. But then again, the use of --leasefile-ro is mandatory, so that all events are captured by our helper program. For example, renew of a lease. Regards, Nehal J Wani -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] leaseshelper: add enhancements to support all events
On Fri, Jul 11, 2014 at 5:49 AM, Nehal J Wani nehaljw.k...@gmail.com wrote:
This patch enables the helper program to detect event(s) triggered when there
is a change in lease length or expiry and client-id. This transfers complete
control of leases database to libvirt and suppresses use of the lease database
file (network-name.leases). That file will not be created, read, or written.
This is achieved by adding the option --leasefile-ro to dnsmasq and applying
the symlink technique, which helps us map events related to leases with their
corresponding network bridges.
Example:
/var/lib/libvirt/dnsmasq/virbr0.helper -
/home/wani/libvirt/src/libvirt_leaseshelper
/var/lib/libvirt/dnsmasq/virbr3.helper -
/home/wani/libvirt/src/libvirt_leaseshelper
Also, this requires the addition of a new non-lease entry in our custom lease
database: server-duid. It is required to identify a DHCPv6 server.
Now that dnsmasq doesn't maintain its own leases database, it relies on our
helper program to tell it about previous leases and server duid. Thus, this
patch makes our leases program honor an extra action: init, in which it
sends
the known info in a particular format to dnsmasq by printing it to stdout.
---
src/network/bridge_driver.c | 43 +++-
src/network/leaseshelper.c | 156
+---
2 files changed, 175 insertions(+), 24 deletions(-)
make syntax-check reported two flaws. Following diff rectifies it:
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index 3d6c773..bdc1b77 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -30,6 +30,7 @@
#include stdlib.h
#include sys/stat.h
+#include dirname.h
#include virutil.h
#include virthread.h
#include virfile.h
@@ -54,7 +55,7 @@
* Use this when passing possibly-NULL strings to printf-a-likes.
* Required for unkown parameters during init call.
*/
-# define EMPTY_STR(s) ((s) ? (s) : *)
+#define EMPTY_STR(s) ((s) ? (s) : *)
static const char *program_name;
@@ -181,7 +182,7 @@ main(int argc, char **argv)
* expired leases. Our symlink hack provides us an alternative method to
* find it */
if (!interface) {
-interface = basename(argv[0]);
+interface = last_component(argv[0]);
*(strchr(interface, '.')) = '\0';
}
Regards,
Nehal J Wani
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] leaseshelper: add enhancements to support all events
This patch enables the helper program to detect event(s) triggered when there
is a change in lease length or expiry and client-id. This transfers complete
control of leases database to libvirt and suppresses use of the lease database
file (network-name.leases). That file will not be created, read, or written.
This is achieved by adding the option --leasefile-ro to dnsmasq and applying
the symlink technique, which helps us map events related to leases with their
corresponding network bridges.
Example:
/var/lib/libvirt/dnsmasq/virbr0.helper -
/home/wani/libvirt/src/libvirt_leaseshelper
/var/lib/libvirt/dnsmasq/virbr3.helper -
/home/wani/libvirt/src/libvirt_leaseshelper
Also, this requires the addition of a new non-lease entry in our custom lease
database: server-duid. It is required to identify a DHCPv6 server.
Now that dnsmasq doesn't maintain its own leases database, it relies on our
helper program to tell it about previous leases and server duid. Thus, this
patch makes our leases program honor an extra action: init, in which it sends
the known info in a particular format to dnsmasq by printing it to stdout.
---
src/network/bridge_driver.c | 43 +++-
src/network/leaseshelper.c | 156 +---
2 files changed, 175 insertions(+), 24 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6a2e760..1e1e53f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -229,6 +229,16 @@ networkDnsmasqLeaseFileNameCustom(const char *bridge)
}
static char *
+networkDnsmasqPseudoLeaseHelperName(const char *bridge)
+{
+char *pseudo_leasehelper;
+
+ignore_value(virAsprintf(pseudo_leasehelper, %s/%s.helper,
+ driverState-dnsmasqStateDir, bridge));
+return pseudo_leasehelper;
+}
+
+static char *
networkDnsmasqConfigFileName(const char *netname)
{
char *conffile;
@@ -1261,6 +1271,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr
network,
char *configfile = NULL;
char *configstr = NULL;
char *leaseshelper_path = NULL;
+char *pseudo_leaseshelper_path = NULL;
network-dnsmasqPid = -1;
@@ -1273,6 +1284,11 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr
network,
if (!(configfile = networkDnsmasqConfigFileName(network-def-name)))
goto cleanup;
+/* construct symlink name */
+if (!(pseudo_leaseshelper_path
+ = networkDnsmasqPseudoLeaseHelperName(network-def-bridge)))
+goto cleanup;
+
/* Write the file */
if (virFileWriteStr(configfile, configstr, 0600) 0) {
virReportSystemError(errno,
@@ -1287,9 +1303,30 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr
network,
LIBEXECDIR)))
goto cleanup;
+/* Symlink technique for dnsmasq lease file is needed so that libvirt
+ * can have complete control over the handling of leases database */
+
+/* Remove unwanted, old symlink */
+if (unlink(pseudo_leaseshelper_path) 0
+errno != ENOENT errno != ENOTDIR) {
+virReportSystemError(errno,
+ _(Failed to delete symlink '%s'),
+ pseudo_leaseshelper_path);
+goto cleanup;
+}
+
+/* Create a new symlink */
+if (symlink(leaseshelper_path, pseudo_leaseshelper_path) 0) {
+virReportSystemError(errno,
+ _(Failed to create symlink '%s' to '%s'),
+ leaseshelper_path, pseudo_leaseshelper_path);
+goto cleanup;
+}
+
cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
virCommandAddArgFormat(cmd, --conf-file=%s, configfile);
-virCommandAddArgFormat(cmd, --dhcp-script=%s, leaseshelper_path);
+virCommandAddArgFormat(cmd, --dhcp-script=%s, pseudo_leaseshelper_path);
+virCommandAddArgFormat(cmd, --leasefile-ro);
*cmdout = cmd;
ret = 0;
@@ -3432,6 +3469,10 @@ networkGetDHCPLeases(virNetworkPtr network,
goto error;
}
+/* Ignore server-duid. It's not part of a lease */
+if (virJSONValueObjectHasKey(lease_tmp, server-duid))
+continue;
+
if (!(mac_tmp = virJSONValueObjectGetString(lease_tmp,
mac-address))) {
/* leaseshelper program guarantees that lease will be stored only
if
* mac-address is known otherwise not */
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index e4b5283..3d6c773 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -50,6 +50,13 @@
*/
#define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX (32 * 1024 * 1024)
+/*
+ * Use this when passing possibly-NULL strings to printf-a-likes.
+ * Required for unkown parameters during init call.
+ */
+# define EMPTY_STR(s) ((s) ? (s) : *)
+
+
static const char *program_name;
/* Display version information. */
@@ -65,7 +72,7 @@ usage(int status)
if
