On Mon, May 09, 2011 at 09:28:47PM +0200, Michal Privoznik wrote:

API agreed on in: https://www.redhat.com/archives/libvir-list/2011-May/msg00026.html --- include/libvirt/libvirt.h.in | 7 +++++++ src/libvirt_public.syms | 7 +++++++ 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 5783303..3225294 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1309,6 +1309,13 @@ int virInterfaceDestroy (virInterfacePtr iface, int virInterfaceRef (virInterfacePtr iface); int virInterfaceFree (virInterfacePtr iface);

+int virInterfaceChangeStart (virConnectPtr conn, + unsigned int flags); +int virInterfaceChangeCommit (virConnectPtr conn, + unsigned int flags); +int virInterfaceChangeRollback(virConnectPtr conn, + unsigned int flags);

I find the combination 'Start' + 'Commit' a bit unusual. I think it would be better named 'Begin'. Apart from that ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 05/10/2011 05:38 AM, Daniel P. Berrange wrote:

On Mon, May 09, 2011 at 09:28:49PM +0200, Michal Privoznik wrote:

...

--- src/libvirt.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 129 insertions(+), 0 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index abacf85..d9b659d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -7481,6 +7481,135 @@ virInterfaceFree(virInterfacePtr iface) return 0; }

+/** + * virInterfaceChangeStart: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This functions creates a restore point to which one can return + * later by calling virInterfaceChangeRollback. This function should + * be called before any transaction with interface configuration. + * Once knowing, new configuration works, it can be commited via + * virInterfaceChangeCommit, which frees restore point. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeStart(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags& VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver&& conn->interfaceDriver->interfaceChangeStart) { + ret = conn->interfaceDriver->interfaceChangeStart(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} + +/** + * virInterfaceChangeCommit: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This commits the changes made to interfaces and frees restore point + * created by virInterfaceChangeStart. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeCommit(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags& VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver&& conn->interfaceDriver->interfaceChangeCommit) { + ret = conn->interfaceDriver->interfaceChangeCommit(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} + +/** + * virInterfaceChangeRollback: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This cancels changes made to interfaces settings by restoring previous + * state created by virInterfaceChangeStart. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeRollback(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags& VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver&& + conn->interfaceDriver->interfaceChangeRollback) { + ret = conn->interfaceDriver->interfaceChangeRollback(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +}

I think we also need to add comments to

virInterfaceDefineXML virInterfaceUndefine virInterfaceCreate virInterfaceDestroy

about the difference in their behaviour if a transaction is open, vs normal non-transactional usage.

They will behave the same at the moment they are called, but it will be possible to later undo what was done, because "opening the transaction" is really just saving off the original config state. More importantly (and probably this is the part you were talking about) if virInterfaceChangeCommit() isn't called before the next time the system is rebooted, all the changes will be automatically reverted during the boot process. (I want to make sure that nobody thinks opening a transaction means that all the changes are saved into a staging area and then later committed to the live config all at once when the transaction is committed - doing it this way would eliminate the ability to test the new config prior to committing).

On Mon, May 09, 2011 at 09:28:50PM +0200, Michal Privoznik wrote:

--- src/remote/remote_driver.c | 6 +++--- src/remote/remote_protocol.x | 18 +++++++++++++++++- src/remote_protocol-structs | 9 +++++++++ 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d56f352..a974b39 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6534,9 +6534,9 @@ static virInterfaceDriver interface_driver = { .interfaceCreate = remoteInterfaceCreate, .interfaceDestroy = remoteInterfaceDestroy, .interfaceIsActive = remoteInterfaceIsActive, - .interfaceChangeStart = NULL, - .interfaceChangeCommit = NULL, - .interfaceChangeRollback = NULL, + .interfaceChangeStart = remoteInterfaceChangeStart, + .interfaceChangeCommit = remoteInterfaceChangeCommit, + .interfaceChangeRollback = remoteInterfaceChangeRollback, };

static virStorageDriver storage_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index c706c36..95f636c 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1143,6 +1143,18 @@ struct remote_interface_destroy_args { unsigned int flags; };

+struct remote_interface_change_start_args { + unsigned int flags; +}; + +struct remote_interface_change_commit_args { + unsigned int flags; +}; + +struct remote_interface_change_rollback_args { + unsigned int flags; +}; +

/* Auth calls: */

@@ -2176,7 +2188,11 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 206, REMOTE_PROC_DOMAIN_MIGRATE_SET_MAX_SPEED = 207, REMOTE_PROC_STORAGE_VOL_UPLOAD = 208, - REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 209 + REMOTE_PROC_STORAGE_VOL_DOWNLOAD = 209, + REMOTE_PROC_INTERFACE_CHANGE_START = 210, + + REMOTE_PROC_INTERFACE_CHANGE_COMMIT = 211, + REMOTE_PROC_INTERFACE_CHANGE_ROLLBACK = 212

/* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index f904c4d..8f524f0 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -783,6 +783,15 @@ struct remote_interface_destroy_args { remote_nonnull_interface iface; u_int flags; }; +struct remote_interface_change_start_args { + unsigned int flags; +}; +struct remote_interface_change_commit_args { + unsigned int flags; +}; +struct remote_interface_change_rollback_args { + unsigned int flags; +}; struct remote_auth_list_ret { struct { u_int types_len;

ACK, modulo naming issue Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Mon, May 09, 2011 at 09:28:51PM +0200, Michal Privoznik wrote:

--- tools/virsh.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index 2b16714..60efa10 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5048,6 +5048,64 @@ cmdInterfaceDestroy(vshControl *ctl, const vshCmd *cmd) return ret; }

+/* + * "iface-change" command + */ +static const vshCmdInfo info_interface_change[] = { + {"help", N_("manage interface changes by creating a restore point, " + "rollback to previous configuration, or removing restore " + "point")}, + {"desc", N_("manage physical host interface changes")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_change[] = { + {"start", VSH_OT_BOOL, 0, N_("create a restore point")}, + {"commit", VSH_OT_BOOL, 0, N_("commit changes to interface")}, + {"rollback", VSH_OT_BOOL, 0, N_("cancel all changes and roll back to " + "known good configuration")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdInterfaceChnage(vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + bool start = vshCommandOptBool(cmd, "start"); + bool commit = vshCommandOptBool(cmd, "commit"); + bool rollback = vshCommandOptBool(cmd, "rollback"); + int num_opts = 0; + int func_ret = -1; + + if (start) + num_opts++; + if (commit) + num_opts++; + if (rollback) + num_opts++; + if (num_opts != 1) { + vshError(ctl, _("Please specify exactly one option")); + goto end; + } + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto end; + + if (start) + func_ret = virInterfaceChangeStart(ctl->conn, 0); + else if (commit) + func_ret = virInterfaceChangeCommit(ctl->conn, 0); + else if (rollback) + func_ret = virInterfaceChangeRollback(ctl->conn, 0); + else { + /* This should really never happen */ + } + + ret = func_ret == 0 ? true : false; + +end: + return ret; +}

/* * "nwfilter-define" command @@ -10826,6 +10884,7 @@ static const vshCmdDef ifaceCmds[] = { {"iface-name", cmdInterfaceName, opts_interface_name, info_interface_name}, {"iface-start", cmdInterfaceStart, opts_interface_start, info_interface_start}, {"iface-undefine", cmdInterfaceUndefine, opts_interface_undefine, info_interface_undefine}, + {"iface-change", cmdInterfaceChnage, opts_interface_change, info_interface_change}, {NULL, NULL, NULL, NULL} };

Do we really want to overload a single method with 3 different functions ? It isn't very clear that 'iface-change' is the transaction handling method. Could we justdo 'iface-begin', 'iface-commit' ' iface-rollback' or maybe even 'iface-txn-begin' 'iface-txn-commit' 'iface-txn-rollback' ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 05/09/2011 03:28 PM, Michal Privoznik wrote:

--- configure.ac | 5 +++++ src/interface/netcf_driver.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac index dcec371..041d738 100644 --- a/configure.ac +++ b/configure.ac @@ -1483,6 +1483,11 @@ if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then if test "$with_netcf" = "yes" ; then AC_DEFINE_UNQUOTED([WITH_NETCF], 1, [whether libnetcf is available to configure physical host network interfaces]) + AC_CHECK_LIB([netcf], [ncf_change_start], [new_netcf=1], [new_netcf=0])

I've changed the name, as danpb suggested, to "ncf_change_begin". Also, it might be more future-proof to use something more specific than "new_netcf". Maybe netcf_transactions, or something like that.

+ if test "$new_netcf" = "1" ; then + AC_DEFINE_UNQUOTED([HAVE_NCF_CHANGE_START], ["1"], + [we have sufficiently new version of netcf for transaction network API]) + fi fi fi AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c index fc7979c..082c4eb 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/netcf_driver.c @@ -30,6 +30,7 @@ #include "netcf_driver.h" #include "interface_conf.h" #include "memory.h" +#include "logging.h"

#define VIR_FROM_THIS VIR_FROM_INTERFACE

@@ -540,6 +541,35 @@ cleanup: return ret; }

+#ifdef HAVE_NCF_CHANGE_START +static int interfaceChangeStart(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + VIR_DEBUG0("A long time ago in a galaxy far, far away...."); + /* Nothing here yet */

Actually, you can put the function call in here even before you have a version of netcf that has it. This one will look something like this: static int interfaceChangeStart(virConnectPtr conn, unsigned int flags) { struct interface_driver *driver = conn->interfacePrivateData; int ret; virCheckFlags(0, -1); /* currently flags must be 0 */ interfaceDriverLock(driver); ret = ncf_change_begin(driver->netcf, 0); if (ret < 0) { const char *errmsg, *details; int errcode = ncf_error(driver->netcf, &errmsg, &details); interfaceReportError(netcf_to_vir_err(errcode), _("failed to begin transaction (netcf: %s - %s)"), errmsg, details ? details : ""); } interfaceDriverUnlock(driver); return ret; } The others are identical except for function name.

+ + return 0; +} + +static int interfaceChangeCommit(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + VIR_DEBUG0("I am fish"); + /* Nothing here yet */ + + return 0; +} + +static int interfaceChangeRollback(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + VIR_DEBUG0("Hello, IT. Have you tried turning it off and on again?"); + /* Nothing here yet */ + + return 0; +} +#endif /* HAVE_NCF_CHANGE_START */ + static virInterfaceDriver interfaceDriver = { "Interface", interfaceOpenInterface, /* open */ @@ -556,9 +586,15 @@ static virInterfaceDriver interfaceDriver = { interfaceCreate, /* interfaceCreate */ interfaceDestroy, /* interfaceDestroy */ interfaceIsActive, /* interfaceIsActive */ +#ifdef HAVE_NCF_CHANGE_START + interfaceChangeStart, /* interfaceChangeStart */ + interfaceChangeCommit, /* interfaceChangeCommit */ + interfaceChangeRollback, /* interfaceChangeRollback */ +#else NULL, /* interfaceChangeStart */ NULL, /* interfaceChangeCommit */ NULL, /* interfaceChangeRollback */ +#endif /* HAVE_NCF_CHANGE_START */ };

int interfaceRegister(void) {

On Mon, May 09, 2011 at 09:28:53PM +0200, Michal Privoznik wrote:

--- src/test/test_driver.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 83dcf1a..9ae6e7a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -85,6 +85,8 @@ struct _testConn { virDomainObjList domains; virNetworkObjList networks; virInterfaceObjList ifaces; + int transaction_running:1;

Or a 'bool'

+ virInterfaceObjList backupIfaces; virStoragePoolObjList pools; virNodeDeviceObjList devs; int numCells; @@ -3433,6 +3435,114 @@ cleanup: return ret; }

+static int testInterfaceChangeStart(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + unsigned int i, cnt; + + testDriverLock(privconn); + if (privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("there is another change " + "running.")); + goto cleanup; + }

I think we should refer to it as 'transaction' not 'change'.

+ + privconn->transaction_running = 1; + + cnt = privconn->ifaces.count; + for (i = 0; i < cnt; i++) { + virInterfaceDefPtr def = privconn->ifaces.objs[i]->def; + virInterfaceDefPtr backup; + virInterfaceObjPtr iface; + char *xml = virInterfaceDefFormat(def); + + if (!xml) { + virReportOOMError(); + goto no_memory; + } + + if ((backup = virInterfaceDefParseString(xml)) == NULL) { + VIR_FREE(xml); + goto cleanup; + } + + if ((iface = + virInterfaceAssignDef(&privconn->backupIfaces, backup)) == NULL) { + VIR_FREE(xml); + goto cleanup; + } + + virInterfaceObjUnlock(iface); + + conn->refs++; + + VIR_FREE(xml); + }

Perhaps in src/conf/interface_conf.c add a method virInterfaceObjListClone() to contain this code.

+ + ret = 0; +cleanup: + testDriverUnlock(privconn); + return ret; +no_memory: + virInterfaceObjListFree(&privconn->backupIfaces); + goto cleanup; +} + +static int testInterfaceChangeCommit(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + + if (!privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("no transaction running, " + "nothing to be commited.")); + goto cleanup; + } + + virInterfaceObjListFree(&privconn->backupIfaces); + privconn->transaction_running = 0; + + ret = 0; + +cleanup: + testDriverUnlock(privconn); + + return ret; +} + +static int testInterfaceChangeRollback(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + + if (!privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("no transaction running, " + "nothing to rollback.")); + goto cleanup; + } + + virInterfaceObjListFree(&privconn->ifaces); + privconn->ifaces.count = privconn->backupIfaces.count; + privconn->ifaces.objs = privconn->backupIfaces.objs; + privconn->backupIfaces.count = 0; + privconn->backupIfaces.objs = NULL; + + privconn->transaction_running = 0; + + ret = 0; + +cleanup: + testDriverUnlock(privconn); + return ret; +}

static char *testInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags ATTRIBUTE_UNUSED) @@ -5488,9 +5598,9 @@ static virInterfaceDriver testInterfaceDriver = { testInterfaceCreate, /* interfaceCreate */ testInterfaceDestroy, /* interfaceDestroy */ testInterfaceIsActive, /* interfaceIsActive */ - NULL, /* interfaceChangeStart */ - NULL, /* interfaceChangeCommit */ - NULL, /* interfaceChangeRollback */ + testInterfaceChangeStart, /* interfaceChangeStart */ + testInterfaceChangeCommit, /* interfaceChangeCommit */ + testInterfaceChangeRollback, /* interfaceChangeRollback */ };

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|