[6/6] xl: allow specified domain id to be used for create, restore and migrate
diff mbox series

Message ID 20191224130416.3570-7-pdurrant@amazon.com
State New
Headers show
Series
  • xl/libxl: allow creation of domains with a specified domid
Related show

Commit Message

Paul Durrant Dec. 24, 2019, 1:04 p.m. UTC
This patch adds the option to use a specified domain id to be used for
the create, restore and migrate lifecycle operations and documentation
thereof.

The specified id may be numeric or, in all cases, one of two special
values. The value 'random' will cause libxl to use a randomly chosen
domain id and the value 'next' will cause Xen to automatically choose
the next available domain id (which is the default and legacy behaviour).
In the case of the migrate operation a third special value may be
specified: 'preserve'. If this value is chosen then the current id of
the domain being migrated will be used to restore the domain on the
destination host (which clearly precludes 'localhost' migrations).

NOTE: Whilst modifing xl_cmdtable.c, several formatting errors were
      corrected. Also erroneous documentation of the '-f' option in
      xl.1.pod.in was corrected (to remove the '=').

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 docs/man/xl.1.pod.in      | 34 +++++++++++++++++++++++++++----
 tools/xl/xl.h             |  1 +
 tools/xl/xl_cmdtable.c    | 22 +++++++++++++++-----
 tools/xl/xl_migrate.c     | 42 ++++++++++++++++++++++++++++++---------
 tools/xl/xl_parse.c       | 33 ++++++++++++++++++++++++++++++
 tools/xl/xl_parse.h       |  2 ++
 tools/xl/xl_saverestore.c |  9 ++++++++-
 tools/xl/xl_vmcontrol.c   | 23 +++++++++++++++------
 8 files changed, 141 insertions(+), 25 deletions(-)

Comments

Ian Jackson Jan. 2, 2020, 5:29 p.m. UTC | #1
Paul Durrant writes ("[PATCH 6/6] xl: allow specified domain id to be used for create, restore and migrate"):
> This patch adds the option to use a specified domain id to be used for
> the create, restore and migrate lifecycle operations and documentation
> thereof.

I approve of the idea that the xl user can specify the domid.  But:

Why should this be a command line argument to xl, rather than a domain
config parameter ?  Obviously there needs to be a way to override the
choice, especially to make localhost migration work, but there is
already a way to specify extra config on domain create, at least.

Thanks,
Ian.
Paul Durrant Jan. 3, 2020, 9:23 a.m. UTC | #2
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 02 January 2020 17:30
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 6/6] xl: allow specified domain id to be used for
> create, restore and migrate
> 
> Paul Durrant writes ("[PATCH 6/6] xl: allow specified domain id to be used
> for create, restore and migrate"):
> > This patch adds the option to use a specified domain id to be used for
> > the create, restore and migrate lifecycle operations and documentation
> > thereof.
> 
> I approve of the idea that the xl user can specify the domid.  But:
> 
> Why should this be a command line argument to xl, rather than a domain
> config parameter ?  Obviously there needs to be a way to override the
> choice, especially to make localhost migration work, but there is
> already a way to specify extra config on domain create, at least.
> 

I debated which was best and came down on the side of a command line parameter becase I thought that chances of an admin wanting to tie a cfg to specified domid was small. But I guess the cfg can already specify a name, which needs to be overridden on a per-creation basis if an admin wants to use a common 'template' cfg... so maybe that option is indeed more consistent.

  Paul

> Thanks,
> Ian.

Patch
diff mbox series

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index d4b5e8e362..2def32664b 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -122,11 +122,19 @@  B<OPTIONS>
 
 =over 4
 
+=item B<-D DOMID>, B<--domid=DOMID>
+
+If DOMID is numeric then create the new domain with this domain id. If
+DOMID is I<random> then use a randomly generated value for domain id
+otherwise, if DOMID is I<next> (the default value for this option) then
+use the next available domain id following on from the previous domain to
+be created, restored or migrated in.
+
 =item B<-q>, B<--quiet>
 
 No console output.
 
-=item B<-f=FILE>, B<--defconfig=FILE>
+=item B<-f FILE>, B<--defconfig=FILE>
 
 Use the given configuration file.
 
@@ -205,8 +213,7 @@  B<OPTIONS>
 
 =over 4
 
-=item B<-f=FILE>, B<--defconfig=FILE>
-
+=item B<-f FILE>, B<--defconfig=FILE>
 Use the given configuration file.
 
 =item B<key=value>
@@ -467,6 +474,17 @@  B<OPTIONS>
 
 =over 4
 
+=item B<-D DOMID>, B<--domid=DOMID>
+
+If DOMID is numeric then create the migrated domain with this domain id. If
+DOMID is I<preserve> then use the same numeric domain id as the domain
+being migrated has on the current host. Note that migration will fail in
+the case that a specified or preserved domain id is already in use on the
+destination host. If DOMID is I<random> then use a randomly generated
+value for domain id otherwise, if DOMID is I<next> (the default value for
+this option) then use the next available domain id following on from the
+previous domain to be created, restored or migrated in.
+
 =item B<-s> I<sshcommand>
 
 Use <sshcommand> instead of ssh.  String will be passed to sh. If empty, run
@@ -648,6 +666,14 @@  B<OPTIONS>
 
 =over 4
 
+=item B<-D DOMID>, B<--domid=DOMID>
+
+If DOMID is numeric then create the restored domain with this domain id. If
+DOMID is I<random> then use a randomly generated value for domain id
+otherwise, if DOMID is I<next> (the default value for this option) then
+use the next available domain id following on from the previous domain to
+be created, restored or migrated in.
+
 =item B<-p>
 
 Do not unpause the domain after restoring it.
@@ -1287,7 +1313,7 @@  B<OPTIONS>
 
 =over 4
 
-=item B<-f=FILE>, B<--defconfig=FILE>
+=item B<-f FILE>, B<--defconfig=FILE>
 
 Use the given configuration file.
 
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 60bdad8ffb..f2500f36e0 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -31,6 +31,7 @@  struct cmd_spec {
 };
 
 struct domain_create {
+    int domid;
     int debug;
     int daemonize;
     int monitor; /* handle guest reboots etc */
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 5baa6023aa..b244b6a243 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -26,17 +26,22 @@  struct cmd_spec cmd_table[] = {
       "-h                      Print this help.\n"
       "-p                      Leave the domain paused after it is created.\n"
       "-c                      Connect to the console after the domain is created.\n"
-      "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
+      "-f FILE, --defconfig=FILE\n"
+      "                        Use the given configuration file.\n"
       "-q, --quiet             Quiet.\n"
       "-n, --dryrun            Dry run - prints the resulting configuration\n"
-      "                         (deprecated in favour of global -N option).\n"
+      "                        (deprecated in favour of global -N option).\n"
       "-d                      Enable debug messages.\n"
       "-F                      Run in foreground until death of the domain.\n"
       "-e                      Do not wait in the background for the death of the domain.\n"
       "-V, --vncviewer         Connect to the VNC display after the domain is created.\n"
       "-A, --vncviewer-autopass\n"
       "                        Pass VNC password to viewer via stdin.\n"
-      "--ignore-global-affinity-masks Ignore global masks in xl.conf."
+      "--ignore-global-affinity-masks\n"
+      "                        Ignore global masks in xl.conf.\n"
+      "-D, --domid=DOMID|next|random\n"
+      "                        Use the specified domain id, the next available (default)\n"
+      "                        or choose one at random."
     },
     { "config-update",
       &main_config_update, 1, 1,
@@ -167,7 +172,11 @@  struct cmd_spec cmd_table[] = {
       "-e              Do not wait in the background (on <host>) for the death\n"
       "                of the domain.\n"
       "--debug         Print huge (!) amount of debug during the migration process.\n"
-      "-p              Do not unpause domain after migrating it."
+      "-p              Do not unpause domain after migrating it.\n"
+      "-D, --domid=DOMID|next|random|preserve\n"
+      "                Use the specified domain id, the next available (default),\n"
+      "                choose one at random, or preserve the existing domain's id\n"
+      "                (hence precluding localhost migrate)."
     },
     { "restore",
       &main_restore, 0, 1,
@@ -178,7 +187,10 @@  struct cmd_spec cmd_table[] = {
       "-e                       Do not wait in the background for the death of the domain.\n"
       "-d                       Enable debug messages.\n"
       "-V, --vncviewer          Connect to the VNC display after the domain is created.\n"
-      "-A, --vncviewer-autopass Pass VNC password to viewer via stdin."
+      "-A, --vncviewer-autopass Pass VNC password to viewer via stdin.\n"
+      "-D, --domid=DOMID|next|random\n"
+      "                         Use the specified domain id, the next available (default)\n"
+      "                         or choose one at random."
     },
     { "migrate-receive",
       &main_migrate_receive, 0, 1,
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index 22f0429b84..b0d8f12d95 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -315,14 +315,13 @@  static void migrate_domain(uint32_t domid, const char *rune, int debug,
     exit(EXIT_FAILURE);
 }
 
-static void migrate_receive(int debug, int daemonize, int monitor,
-                            int pause_after_migration,
+static void migrate_receive(int domid, int debug, int daemonize,
+                            int monitor, int pause_after_migration,
                             int send_fd, int recv_fd,
                             libxl_checkpointed_stream checkpointed,
                             char *colo_proxy_script,
                             bool userspace_colo_proxy)
 {
-    uint32_t domid;
     int rc, rc2;
     char rc_buf;
     char *migration_domname;
@@ -339,6 +338,7 @@  static void migrate_receive(int debug, int daemonize, int monitor,
                      "migration ack stream", "banner") );
 
     memset(&dom_info, 0, sizeof(dom_info));
+    dom_info.domid = domid;
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
@@ -477,6 +477,7 @@  static void migrate_receive(int debug, int daemonize, int monitor,
 
 int main_migrate_receive(int argc, char **argv)
 {
+    const char *domid = NULL;
     int debug = 0, daemonize = 1, monitor = 1, pause_after_migration = 0;
     libxl_checkpointed_stream checkpointed = LIBXL_CHECKPOINTED_STREAM_NONE;
     int opt;
@@ -490,7 +491,10 @@  int main_migrate_receive(int argc, char **argv)
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "Fedrp", opts, "migrate-receive", 0) {
+    SWITCH_FOREACH_OPT(opt, "D:Fedrp", opts, "migrate-receive", 0) {
+    case 'D':
+        domid = optarg;
+        break;
     case 'F':
         daemonize = 0;
         break;
@@ -522,7 +526,9 @@  int main_migrate_receive(int argc, char **argv)
         help("migrate-receive");
         return EXIT_FAILURE;
     }
-    migrate_receive(debug, daemonize, monitor, pause_after_migration,
+
+    migrate_receive(parse_domid(domid), debug, daemonize,
+                    monitor, pause_after_migration,
                     STDOUT_FILENO, STDIN_FILENO,
                     checkpointed, script, userspace_colo_proxy);
 
@@ -531,7 +537,8 @@  int main_migrate_receive(int argc, char **argv)
 
 int main_migrate(int argc, char **argv)
 {
-    uint32_t domid;
+    uint32_t src_domid;
+    const char *dst_domid = NULL;
     const char *config_filename = NULL;
     const char *ssh_command = "ssh";
     char *rune = NULL;
@@ -540,10 +547,14 @@  int main_migrate(int argc, char **argv)
     static struct option opts[] = {
         {"debug", 0, 0, 0x100},
         {"live", 0, 0, 0x200},
+        {"domid", 1, 0, 'D'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "FC:s:ep", opts, "migrate", 2) {
+    SWITCH_FOREACH_OPT(opt, "D:FC:s:ep", opts, "migrate", 2) {
+    case 'D':
+        dst_domid = optarg;
+        break;
     case 'C':
         config_filename = optarg;
         break;
@@ -568,7 +579,7 @@  int main_migrate(int argc, char **argv)
         break;
     }
 
-    domid = find_domain(argv[optind]);
+    src_domid = find_domain(argv[optind]);
     host = argv[optind + 1];
 
     bool pass_tty_arg = progress_use_cr || (isatty(2) > 0);
@@ -578,6 +589,8 @@  int main_migrate(int argc, char **argv)
     } else {
         char verbose_buf[minmsglevel_default+3];
         int verbose_len;
+        char *extra = NULL;
+
         verbose_buf[0] = ' ';
         verbose_buf[1] = '-';
         memset(verbose_buf+2, 'v', minmsglevel_default);
@@ -594,9 +607,20 @@  int main_migrate(int argc, char **argv)
                   daemonize ? "" : " -e",
                   debug ? " -d" : "",
                   pause_after_migration ? " -p" : "");
+
+        if (dst_domid) {
+            if (!strcmp(dst_domid, "preserve"))
+                xasprintf(&extra, " -D %u", src_domid);
+            else
+                xasprintf(&extra, " -D %s", dst_domid);
+        }
+        if (extra) {
+            string_realloc_append(&rune, extra);
+            free(extra);
+        }
     }
 
-    migrate_domain(domid, rune, debug, config_filename);
+    migrate_domain(src_domid, rune, debug, config_filename);
     return EXIT_SUCCESS;
 }
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b881184804..58b1aeea8c 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -399,6 +399,30 @@  static unsigned long parse_ulong(const char *str)
     return val;
 }
 
+static unsigned long parse_long(const char *str)
+{
+    char *endptr;
+    long val;
+
+    val = strtol(str, &endptr, 10);
+    if (endptr == str || val == LONG_MIN || val == LONG_MAX) {
+        fprintf(stderr, "xl: failed to convert \"%s\" to number\n", str);
+        exit(EXIT_FAILURE);
+    }
+    return val;
+}
+
+static int parse_int(const char *str)
+{
+    long val = parse_long(str);
+
+    if (val < INT_MIN || val > INT_MAX) {
+        fprintf(stderr, "xl: \"%s\" is out of range\n", str);
+        exit(EXIT_FAILURE);
+    }
+    return val;
+}
+
 void replace_string(char **str, const char *val)
 {
     free(*str);
@@ -2865,6 +2889,15 @@  out:
     return rc;
 }
 
+int parse_domid(const char *arg)
+{
+    if (!arg || !strcmp(arg, "next"))
+        return INVALID_DOMID; /* Xen will use the next available */
+    else if (!strcmp(arg, "random"))
+        return RANDOM_DOMID; /* libxl will choose a random value */
+
+    return parse_int(arg);
+}
 
 /*
  * Local variables:
diff --git a/tools/xl/xl_parse.h b/tools/xl/xl_parse.h
index bab2861f8c..3a82722f92 100644
--- a/tools/xl/xl_parse.h
+++ b/tools/xl/xl_parse.h
@@ -56,6 +56,8 @@  void trim(char_predicate_t predicate, const char *input, char **output);
 
 const char *get_action_on_shutdown_name(libxl_action_on_shutdown a);
 
+int parse_domid(const char *str);
+
 #endif	/* XL_PARSE_H */
 
 /*
diff --git a/tools/xl/xl_saverestore.c b/tools/xl/xl_saverestore.c
index 9be033fe65..4be0afa04c 100644
--- a/tools/xl/xl_saverestore.c
+++ b/tools/xl/xl_saverestore.c
@@ -164,17 +164,22 @@  int main_restore(int argc, char **argv)
 {
     const char *checkpoint_file = NULL;
     const char *config_file = NULL;
+    const char *domid = NULL;
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, monitor = 1,
         console_autoconnect = 0, vnc = 0, vncautopass = 0;
     int opt, rc;
     static struct option opts[] = {
+        {"domid", 1, 0, 'D'},
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "FcpdeVA", opts, "restore", 1) {
+    SWITCH_FOREACH_OPT(opt, "D:FcpdeVA", opts, "restore", 1) {
+    case 'D':
+        domid = optarg;
+        break;
     case 'c':
         console_autoconnect = 1;
         break;
@@ -210,6 +215,8 @@  int main_restore(int argc, char **argv)
     }
 
     memset(&dom_info, 0, sizeof(dom_info));
+
+    dom_info.domid = parse_domid(domid);
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index e520b1da79..d81a629a5a 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -641,7 +641,7 @@  static void autoconnect_console(libxl_ctx *ctx_ignored,
 
 int create_domain(struct domain_create *dom_info)
 {
-    uint32_t domid = INVALID_DOMID;
+    uint32_t domid = dom_info->domid;
 
     libxl_domain_config d_config;
 
@@ -660,6 +660,7 @@  int create_domain(struct domain_create *dom_info)
 
     int i;
     int need_daemon = daemonize;
+    const char *op;
     int ret, rc;
     libxl_evgen_domain_death *deathw = NULL;
     libxl_evgen_disk_eject **diskws = NULL; /* one per disk */
@@ -872,8 +873,6 @@  int create_domain(struct domain_create *dom_info)
         goto out;
 
 start:
-    assert(domid == INVALID_DOMID);
-
     rc = acquire_lock();
     if (rc < 0)
         goto error_out;
@@ -911,6 +910,7 @@  start:
         libxl_defbool_set(&params.userspace_colo_proxy,
                           dom_info->userspace_colo_proxy);
 
+        op = "restore";
         ret = libxl_domain_create_restore(ctx, &d_config,
                                           &domid, restore_fd,
                                           send_back_fd, &params,
@@ -925,16 +925,21 @@  start:
         restoring = 0;
     } else if (domid_soft_reset != INVALID_DOMID) {
         /* Do soft reset. */
+        op = "soft reset";
         ret = libxl_domain_soft_reset(ctx, &d_config, domid_soft_reset,
                                       0, autoconnect_console_how);
         domid = domid_soft_reset;
         domid_soft_reset = INVALID_DOMID;
     } else {
+        op = "create";
         ret = libxl_domain_create_new(ctx, &d_config, &domid,
                                       0, autoconnect_console_how);
     }
-    if ( ret )
+    if (ret) {
+        fprintf(stderr, "%s operation failed: %s\n", op,
+                libxl_error_to_string(ret));
         goto error_out;
+    }
 
     release_lock();
 
@@ -1111,7 +1116,7 @@  start:
 
 error_out:
     release_lock();
-    if (libxl_domid_valid_guest(domid)) {
+    if (ret != ERROR_DEVICE_EXISTS && libxl_domid_valid_guest(domid)) {
         libxl_domain_destroy(ctx, domid, 0);
         domid = INVALID_DOMID;
     }
@@ -1153,11 +1158,13 @@  out:
 int main_create(int argc, char **argv)
 {
     const char *filename = NULL;
+    const char *domid = NULL;
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
         quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0;
     int opt, rc;
     static struct option opts[] = {
+        {"domid", 1, 0, 'D'},
         {"dryrun", 0, 0, 'n'},
         {"quiet", 0, 0, 'q'},
         {"defconfig", 1, 0, 'f'},
@@ -1174,7 +1181,10 @@  int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVAi", opts, "create", 0) {
+    SWITCH_FOREACH_OPT(opt, "D:Fnqf:pcdeVAi", opts, "create", 0) {
+    case 'D':
+        domid = optarg;
+        break;
     case 'f':
         filename = optarg;
         break;
@@ -1226,6 +1236,7 @@  int main_create(int argc, char **argv)
         }
     }
 
+    dom_info.domid = parse_domid(domid);
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;