| Message ID | 20171030112112.6952-6-quintela@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote: > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.c | 25 ++++++++++++++++++------- > migration/migration.h | 2 ++ > migration/socket.c | 7 +++++++ > 3 files changed, 27 insertions(+), 7 deletions(-) > diff --git a/migration/socket.c b/migration/socket.c > index 3a8232dd2d..c3ab81d1fb 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp) > Error *err = NULL; > SocketAddress *saddr = tcp_build_address(host_port, &err); > if (!err) { > + char *new_uri; > socket_start_incoming_migration(saddr, &err); > + if (!err) { > + new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host, > + saddr->u.inet.port); This is bad as it is throwing away data that the original URI had. In particular you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the original URI for later, then why not just keep the 'host_port' parameter that was passed into this function instead of trying to reverse engineeer the URI ? Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> wrote: > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote: >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/migration.c | 25 ++++++++++++++++++------- >> migration/migration.h | 2 ++ >> migration/socket.c | 7 +++++++ >> 3 files changed, 27 insertions(+), 7 deletions(-) > > >> diff --git a/migration/socket.c b/migration/socket.c >> index 3a8232dd2d..c3ab81d1fb 100644 >> --- a/migration/socket.c >> +++ b/migration/socket.c >> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp) >> Error *err = NULL; >> SocketAddress *saddr = tcp_build_address(host_port, &err); >> if (!err) { >> + char *new_uri; >> socket_start_incoming_migration(saddr, &err); >> + if (!err) { >> + new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host, >> + saddr->u.inet.port); > > This is bad as it is throwing away data that the original URI had. In particular > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the > original URI for later, then why not just keep the 'host_port' parameter that > was passed into this function instead of trying to reverse engineeer the URI ? I don't need the original uri anymore, this is the incoming side of migration, and we can only set that once, if migration fails, we need to restart qemu anymore. I changed it to this: new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host, address->u.inet.port, iaddr->has_ipv4 ? ",ipv4" : "", iaddr->has_ipv6 ? ",ipv6" : ""); Clearly, we don't care about the to= parameter. The whole point of this exercise is that in destination, we use -incoming tcp:0:0 and with the output of info migrate_parameters (uri) we are able to migrate to that place. For rest of migration posibilities, there is no changes at all. Thanks, Juan.
On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote: > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote: > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> migration/migration.c | 25 ++++++++++++++++++------- > >> migration/migration.h | 2 ++ > >> migration/socket.c | 7 +++++++ > >> 3 files changed, 27 insertions(+), 7 deletions(-) > > > > > >> diff --git a/migration/socket.c b/migration/socket.c > >> index 3a8232dd2d..c3ab81d1fb 100644 > >> --- a/migration/socket.c > >> +++ b/migration/socket.c > >> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp) > >> Error *err = NULL; > >> SocketAddress *saddr = tcp_build_address(host_port, &err); > >> if (!err) { > >> + char *new_uri; > >> socket_start_incoming_migration(saddr, &err); > >> + if (!err) { > >> + new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host, > >> + saddr->u.inet.port); > > > > This is bad as it is throwing away data that the original URI had. In particular > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the > > original URI for later, then why not just keep the 'host_port' parameter that > > was passed into this function instead of trying to reverse engineeer the URI ? > > I don't need the original uri anymore, this is the incoming side of > migration, and we can only set that once, if migration fails, we need to > restart qemu anymore. > > I changed it to this: > > new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host, > address->u.inet.port, > iaddr->has_ipv4 ? ",ipv4" : "", > iaddr->has_ipv6 ? ",ipv6" : ""); > > > Clearly, we don't care about the to= parameter. > > The whole point of this exercise is that in destination, we use > > -incoming tcp:0:0 > > and with the output of info migrate_parameters (uri) we are able to > migrate to that place. For rest of migration posibilities, there is no > changes at all. That doesn't work typically. When the incoming QEMU listens for connections, by default it will listen on 0.0.0.0, or ::, so that's what the address you're creating will contain. You can't use 0.0.0.0 or :: in a connect() call on the other side as that's a wildcard address that refers to "any valid IP address on the current host". When you connect to the listening QEMU you need the actual IP address of one (of the possibly many) NICs on the target host. IOW, the only time the listen address is useful is if you have told QEMU to listen on a specific NIC's IP address, instead of the wildcard address. This is the core reason why libvirt calls 'gethostname()' on the target host to identify the primary IP address of the host, and uses that on the source host to establish the connection. Regards, Daniel
On Wed, Nov 22, 2017 at 12:54:58PM +0000, Daniel P. Berrange wrote: > On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote: > > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote: > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > >> --- > > >> migration/migration.c | 25 ++++++++++++++++++------- > > >> migration/migration.h | 2 ++ > > >> migration/socket.c | 7 +++++++ > > >> 3 files changed, 27 insertions(+), 7 deletions(-) > > > > > > > > >> diff --git a/migration/socket.c b/migration/socket.c > > >> index 3a8232dd2d..c3ab81d1fb 100644 > > >> --- a/migration/socket.c > > >> +++ b/migration/socket.c > > >> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp) > > >> Error *err = NULL; > > >> SocketAddress *saddr = tcp_build_address(host_port, &err); > > >> if (!err) { > > >> + char *new_uri; > > >> socket_start_incoming_migration(saddr, &err); > > >> + if (!err) { > > >> + new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host, > > >> + saddr->u.inet.port); > > > > > > This is bad as it is throwing away data that the original URI had. In particular > > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the > > > original URI for later, then why not just keep the 'host_port' parameter that > > > was passed into this function instead of trying to reverse engineeer the URI ? > > > > I don't need the original uri anymore, this is the incoming side of > > migration, and we can only set that once, if migration fails, we need to > > restart qemu anymore. > > > > I changed it to this: > > > > new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host, > > address->u.inet.port, > > iaddr->has_ipv4 ? ",ipv4" : "", > > iaddr->has_ipv6 ? ",ipv6" : ""); > > > > > > Clearly, we don't care about the to= parameter. > > > > The whole point of this exercise is that in destination, we use > > > > -incoming tcp:0:0 > > > > and with the output of info migrate_parameters (uri) we are able to > > migrate to that place. For rest of migration posibilities, there is no > > changes at all. > > That doesn't work typically. When the incoming QEMU listens for connections, > by default it will listen on 0.0.0.0, or ::, so that's what the address > you're creating will contain. You can't use 0.0.0.0 or :: in a connect() > call on the other side as that's a wildcard address that refers to "any > valid IP address on the current host". > > When you connect to the listening QEMU you need the actual IP address > of one (of the possibly many) NICs on the target host. IOW, the only > time the listen address is useful is if you have told QEMU to listen on > a specific NIC's IP address, instead of the wildcard address. > > This is the core reason why libvirt calls 'gethostname()' on the target > host to identify the primary IP address of the host, and uses that on the > source host to establish the connection. I should have said the port number info will still be useful (if you're using the dynamic port allocation), it is just the IP address that's not usable in general. Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> wrote: > On Wed, Nov 22, 2017 at 12:54:58PM +0000, Daniel P. Berrange wrote: >> On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote: >> > "Daniel P. Berrange" <berrange@redhat.com> wrote: >> > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote: > > > This is bad as it is throwing away data that the original URI >> > > had. In particular >> > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the >> > > original URI for later, then why not just keep the 'host_port' parameter that >> > > was passed into this function instead of trying to reverse >> > > engineeer the URI ? >> > >> > I don't need the original uri anymore, this is the incoming side of >> > migration, and we can only set that once, if migration fails, we need to >> > restart qemu anymore. >> > >> > I changed it to this: >> > >> > new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host, >> > address->u.inet.port, >> > iaddr->has_ipv4 ? ",ipv4" : "", >> > iaddr->has_ipv6 ? ",ipv6" : ""); >> > >> > >> > Clearly, we don't care about the to= parameter. >> > >> > The whole point of this exercise is that in destination, we use >> > >> > -incoming tcp:0:0 >> > >> > and with the output of info migrate_parameters (uri) we are able to >> > migrate to that place. For rest of migration posibilities, there is no >> > changes at all. >> >> That doesn't work typically. When the incoming QEMU listens for connections, >> by default it will listen on 0.0.0.0, or ::, so that's what the address >> you're creating will contain. You can't use 0.0.0.0 or :: in a connect() >> call on the other side as that's a wildcard address that refers to "any >> valid IP address on the current host". >> >> When you connect to the listening QEMU you need the actual IP address >> of one (of the possibly many) NICs on the target host. IOW, the only >> time the listen address is useful is if you have told QEMU to listen on >> a specific NIC's IP address, instead of the wildcard address. >> >> This is the core reason why libvirt calls 'gethostname()' on the target >> host to identify the primary IP address of the host, and uses that on the >> source host to establish the connection. > > I should have said the port number info will still be useful (if you're > using the dynamic port allocation), it is just the IP address that's not > usable in general. I gave up O:-) I introduced tcp_port parameter, that is really what I wanted to know. The test use case (the one that I am interested right now) is that I do: qemu-system-x86_64 ..... --incomping tcp:127.0.0.1:0 And I want to know the port that the destination gets to connect to it. For migration, it don't really matters if we _also_ set the address/ip/whatever it gets translated to, because it is not possible right now to restart the migration incoming side (you need to restart qemu for long and boring historic reasons). So, I ended just adding: +# +# @tcp-port: Only used for tcp, to know what is the real port +# (Since 2.12) # And all the boilerplate code to access/setup this one. BTW, I am not sure how well the current code will work with a range of ports, because we don't have a way to tell the source which one the kernel/qemu decided to use O:-) Later, Juan.
On Wed, Nov 29, 2017 at 05:43:35PM +0100, Juan Quintela wrote: > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > On Wed, Nov 22, 2017 at 12:54:58PM +0000, Daniel P. Berrange wrote: > >> On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote: > >> > "Daniel P. Berrange" <berrange@redhat.com> wrote: > >> > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote: > > > > > This is bad as it is throwing away data that the original URI > >> > > had. In particular > >> > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the > >> > > original URI for later, then why not just keep the 'host_port' parameter that > >> > > was passed into this function instead of trying to reverse > >> > > engineeer the URI ? > >> > > >> > I don't need the original uri anymore, this is the incoming side of > >> > migration, and we can only set that once, if migration fails, we need to > >> > restart qemu anymore. > >> > > >> > I changed it to this: > >> > > >> > new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host, > >> > address->u.inet.port, > >> > iaddr->has_ipv4 ? ",ipv4" : "", > >> > iaddr->has_ipv6 ? ",ipv6" : ""); > >> > > >> > > >> > Clearly, we don't care about the to= parameter. > >> > > >> > The whole point of this exercise is that in destination, we use > >> > > >> > -incoming tcp:0:0 > >> > > >> > and with the output of info migrate_parameters (uri) we are able to > >> > migrate to that place. For rest of migration posibilities, there is no > >> > changes at all. > >> > >> That doesn't work typically. When the incoming QEMU listens for connections, > >> by default it will listen on 0.0.0.0, or ::, so that's what the address > >> you're creating will contain. You can't use 0.0.0.0 or :: in a connect() > >> call on the other side as that's a wildcard address that refers to "any > >> valid IP address on the current host". > >> > >> When you connect to the listening QEMU you need the actual IP address > >> of one (of the possibly many) NICs on the target host. IOW, the only > >> time the listen address is useful is if you have told QEMU to listen on > >> a specific NIC's IP address, instead of the wildcard address. > >> > >> This is the core reason why libvirt calls 'gethostname()' on the target > >> host to identify the primary IP address of the host, and uses that on the > >> source host to establish the connection. > > > > I should have said the port number info will still be useful (if you're > > using the dynamic port allocation), it is just the IP address that's not > > usable in general. > > I gave up O:-) > > I introduced tcp_port parameter, that is really what I wanted to know. > The test use case (the one that I am interested right now) is that I > do: > > qemu-system-x86_64 ..... --incomping tcp:127.0.0.1:0 > > And I want to know the port that the destination gets to connect to it. > For migration, it don't really matters if we _also_ set the > address/ip/whatever it gets translated to, because it is not possible > right now to restart the migration incoming side (you need to restart > qemu for long and boring historic reasons). > > So, I ended just adding: > > +# > +# @tcp-port: Only used for tcp, to know what is the real port > +# (Since 2.12) > # > > And all the boilerplate code to access/setup this one. > > BTW, I am not sure how well the current code will work with a range of > ports, because we don't have a way to tell the source which one the > kernel/qemu decided to use O:-) Ultimately I think we need to be able to report a full list of SocketAddress structs representing the listening addresses, because we will eventually be listening on multiple distinct sockets. Currently if a hostname resolves to multiple IP addrs, we only listen on the first successful IP, but I have patches that fix this so we listen on multiple IPs.... Regards, Daniel
diff --git a/migration/migration.c b/migration/migration.c index 3e48d37880..507226907b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -240,10 +240,21 @@ void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, } } +void migrate_set_uri(const char *uri, Error **errp) +{ + MigrateSetParameters p = { + .has_uri = true, + .uri = (char *)uri, + }; + + qmp_migrate_set_parameters(&p, errp); +} + void qemu_start_incoming_migration(const char *uri, Error **errp) { const char *p; + migrate_set_uri(uri, errp); qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); if (!strcmp(uri, "defer")) { deferred_incoming_migration(errp); @@ -1363,7 +1374,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, error_setg(errp, "Guest is waiting for an incoming migration"); return; } - + migrate_set_uri(uri, errp); if (migration_is_blocked(errp)) { return; } @@ -1388,20 +1399,20 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, s = migrate_init(); - if (strstart(uri, "tcp:", &p)) { + if (strstart(s->parameters.uri, "tcp:", &p)) { tcp_start_outgoing_migration(s, p, &local_err); #ifdef CONFIG_RDMA - } else if (strstart(uri, "rdma:", &p)) { + } else if (strstart(s->parameters.uri, "rdma:", &p)) { rdma_start_outgoing_migration(s, p, &local_err); #endif - } else if (strstart(uri, "exec:", &p)) { + } else if (strstart(s->parameters.uri, "exec:", &p)) { exec_start_outgoing_migration(s, p, &local_err); - } else if (strstart(uri, "unix:", &p)) { + } else if (strstart(s->parameters.uri, "unix:", &p)) { unix_start_outgoing_migration(s, p, &local_err); - } else if (strstart(uri, "fd:", &p)) { + } else if (strstart(s->parameters.uri, "fd:", &p)) { fd_start_outgoing_migration(s, p, &local_err); } else { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "s->parameters.uri", "a valid migration protocol"); migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED); diff --git a/migration/migration.h b/migration/migration.h index 663415fe48..cb0ab4807a 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -210,4 +210,6 @@ void migrate_send_rp_pong(MigrationIncomingState *mis, void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname, ram_addr_t start, size_t len); +void migrate_set_uri(const char *uri, Error **errp); + #endif diff --git a/migration/socket.c b/migration/socket.c index 3a8232dd2d..c3ab81d1fb 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp) Error *err = NULL; SocketAddress *saddr = tcp_build_address(host_port, &err); if (!err) { + char *new_uri; socket_start_incoming_migration(saddr, &err); + if (!err) { + new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host, + saddr->u.inet.port); + migrate_set_uri(new_uri, &err); + g_free(new_uri); + } } qapi_free_SocketAddress(saddr); error_propagate(errp, err);
Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/migration.c | 25 ++++++++++++++++++------- migration/migration.h | 2 ++ migration/socket.c | 7 +++++++ 3 files changed, 27 insertions(+), 7 deletions(-)