include/libvirt/libvirt-domain.h | 8 + src/qemu/libvirtd_qemu.aug | 6 + src/qemu/qemu.conf | 39 +++++ src/qemu/qemu_conf.c | 45 +++-- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 195 +++++++++++++-------- src/qemu/qemu_domain.h | 89 ++++++---- src/qemu/qemu_hotplug.c | 343 ++++++++++++++++++++----------------- src/qemu/qemu_hotplug.h | 24 +++ src/qemu/qemu_migration.c | 200 +++++++++++++++++++++ src/qemu/qemu_migration.h | 3 +- src/qemu/qemu_monitor.c | 11 +- src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 10 ++ src/qemu/test_libvirtd_qemu.aug.in | 4 + tools/virsh-domain.c | 7 + 16 files changed, 705 insertions(+), 287 deletions(-)
v1: http://www.redhat.com/archives/libvir-list/2017-February/msg00897.html v1 cover letter reiterated: Patches 1, 3 -> 9 are primarily quite a bit of code motion in order to allow reuse of the "core" of the chardev TLS code. Theoretically speaking of course, these patches should work - I don't have a TLS and migration environment to test with, so between following the qemu command model on Daniel's blog and prior experience with the chardev TLS would I added the saving of a flag to the private qemu domain state, although I'm not 100% sure it was necessary. At one time I created the source TLS objects during the Begin phase, but later decided to wait until just before the migration is run. I think the main reason to have the flag would be a restart of libvirtd to let 'something' know migration using TLS was configured. I think it may only be "necessary" in order to repopulate the migSecinfo after libvirtd restart, but it's not entirely clear. By the time I started thinking more about while writing this cover letter it was too late to just remove. Also rather than create the destination host TLS objects on the fly, I modified the command line generation. That model could change to adding the TLS objects once the destination is started and before the params are set for the migration. This 'model' is also going to be used for the NBD, but I figured I'd get this posted now since it was already too long of a series. v2: Changes Reorder the patches to put the reused 'chardev' code up front. Most of these patches were "ok" along the way, but only one was officially ACK'd (and that was pushed). Patch1 is new - based off code review comment to create a common New function for secinfo allocation Patch2 is adjusted to use Patch1 Patch3 is new based on review comment and having ExitMonitor outside the virSaveLastError ... virSetError Patch4 mainly follows older logic with adjustments as suggested during code review Patch5 -> Patch8 had minor changes as a result of other suggestions Patch9 just removed the _set logic Patch10 fixed the order/placement of VIR_MIGRATE_TLS Patch11 is the old patch1 w/ the fixed #undef Patch12 is the old patch2 w/o changes Patch13 Alters the server logic to create the objects on the fly rather that via command line. It also introduces 3 helpers to perform the migration TLS manipulation Patch14 similarly uses those API's AFAIU - removal of the objects would remove the migration tls-creds, tls-hostname settings. NB: I left the cfg->migrateTLS in for now - it's very simple to remove, but there would still need to be a key on something to ensure the migrateTLS environment has been properly configured since that would mean the default environment would need to be used/configured. Setting up the default environment is keyed off having the migrateTLS defined. That's all part of the qemu_conf reading logic. John Ferlan (14): qemu: Introduce qemuDomainSecretInfoNew qemu: Introduce qemuDomainSecretMigratePrepare qemu: Move exit monitor calls in failure paths qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects qemu: Refactor qemuDomainGetChardevTLSObjects to converge code qemu: Move qemuDomainSecretChardevPrepare call qemu: Move qemuDomainPrepareChardevSourceTLS call qemu: Introduce qemuDomainGetTLSObjects qemu: Add TLS params to _qemuMonitorMigrationParams Add new migration flag VIR_MIGRATE_TLS qemu: Create #define for TLS configuration setup. conf: Introduce migrate_tls_x509_cert_dir qemu: Set up the migrate TLS objects for target qemu: Set up the migration TLS objects for source include/libvirt/libvirt-domain.h | 8 + src/qemu/libvirtd_qemu.aug | 6 + src/qemu/qemu.conf | 39 +++++ src/qemu/qemu_conf.c | 45 +++-- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 195 +++++++++++++-------- src/qemu/qemu_domain.h | 89 ++++++---- src/qemu/qemu_hotplug.c | 343 ++++++++++++++++++++----------------- src/qemu/qemu_hotplug.h | 24 +++ src/qemu/qemu_migration.c | 200 +++++++++++++++++++++ src/qemu/qemu_migration.h | 3 +- src/qemu/qemu_monitor.c | 11 +- src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 10 ++ src/qemu/test_libvirtd_qemu.aug.in | 4 + tools/virsh-domain.c | 7 + 16 files changed, 705 insertions(+), 287 deletions(-) -- 2.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-02-23 at 13:42 -0500, John Ferlan wrote: > v1: http://www.redhat.com/archives/libvir-list/2017-February/msg00897.html > v1 cover letter reiterated: > > Patches 1, 3 -> 9 are primarily quite a bit of code motion in order to allow > reuse of the "core" of the chardev TLS code. > > Theoretically speaking of course, these patches should work - I don't > have a TLS and migration environment to test with, so between following > the qemu command model on Daniel's blog and prior experience with the > chardev TLS would > > I added the saving of a flag to the private qemu domain state, although > I'm not 100% sure it was necessary. At one time I created the source TLS > objects during the Begin phase, but later decided to wait until just > before the migration is run. I think the main reason to have the flag > would be a restart of libvirtd to let 'something' know migration using > TLS was configured. I think it may only be "necessary" in order to > repopulate the migSecinfo after libvirtd restart, but it's not entirely > clear. By the time I started thinking more about while writing this cover > letter it was too late to just remove. > > Also rather than create the destination host TLS objects on the fly, > I modified the command line generation. That model could change to adding > the TLS objects once the destination is started and before the params are > set for the migration. > > This 'model' is also going to be used for the NBD, but I figured I'd get > this posted now since it was already too long of a series. These changes are user-visible, and should be documented in the release notes accordingly. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/24/2017 04:01 AM, Andrea Bolognani wrote: > On Thu, 2017-02-23 at 13:42 -0500, John Ferlan wrote: >> v1: http://www.redhat.com/archives/libvir-list/2017-February/msg00897.html >> v1 cover letter reiterated: >> >> Patches 1, 3 -> 9 are primarily quite a bit of code motion in order to allow >> reuse of the "core" of the chardev TLS code. >> >> Theoretically speaking of course, these patches should work - I don't >> have a TLS and migration environment to test with, so between following >> the qemu command model on Daniel's blog and prior experience with the >> chardev TLS would >> >> I added the saving of a flag to the private qemu domain state, although >> I'm not 100% sure it was necessary. At one time I created the source TLS >> objects during the Begin phase, but later decided to wait until just >> before the migration is run. I think the main reason to have the flag >> would be a restart of libvirtd to let 'something' know migration using >> TLS was configured. I think it may only be "necessary" in order to >> repopulate the migSecinfo after libvirtd restart, but it's not entirely >> clear. By the time I started thinking more about while writing this cover >> letter it was too late to just remove. >> >> Also rather than create the destination host TLS objects on the fly, >> I modified the command line generation. That model could change to adding >> the TLS objects once the destination is started and before the params are >> set for the migration. >> >> This 'model' is also going to be used for the NBD, but I figured I'd get >> this posted now since it was already too long of a series. > > These changes are user-visible, and should be documented > in the release notes accordingly. > Yes I know - depends on "when" then get reviewed and ACK'd too. There are parts of the series that are essentially code motion - so I made conscious decision to wait. John > -- > Andrea Bolognani / Red Hat / Virtualization > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 23, 2017 at 13:42:02 -0500, John Ferlan wrote: > AFAIU - removal of the objects would remove the migration tls-creds, > tls-hostname settings. This would be a very strange behavior. > I left the cfg->migrateTLS in for now - it's very simple to remove, but > there would still need to be a key on something to ensure the migrateTLS > environment has been properly configured since that would mean the default > environment would need to be used/configured. Setting up the default > environment is keyed off having the migrateTLS defined. That's all part > of the qemu_conf reading logic. I still don't see the value in keeping migrate_tls option in qemu.conf. If we want to check the environment is setup correctly we should check the certificates are in place rather than relying on the option. The error reported when migrate_tls is set, but the environment is not prepared is: internal error: unable to execute QEMU command 'object-add': Unable to access credentials /etc/pki/qemu/ca-cert.pem: No such file or directory Not ideal but at least it names the file which is missing. > John Ferlan (14): > qemu: Introduce qemuDomainSecretInfoNew > qemu: Introduce qemuDomainSecretMigratePrepare > qemu: Move exit monitor calls in failure paths > qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects > qemu: Refactor qemuDomainGetChardevTLSObjects to converge code > qemu: Move qemuDomainSecretChardevPrepare call > qemu: Move qemuDomainPrepareChardevSourceTLS call > qemu: Introduce qemuDomainGetTLSObjects The eight patches above are mostly OK except for some small issues. But the rest of the series needs more work... > qemu: Add TLS params to _qemuMonitorMigrationParams > Add new migration flag VIR_MIGRATE_TLS > qemu: Create #define for TLS configuration setup. > conf: Introduce migrate_tls_x509_cert_dir > qemu: Set up the migrate TLS objects for target > qemu: Set up the migration TLS objects for source The code should check whether QEMU actually supports TLS migration, otherwise you will get internal error: unable to execute QEMU command 'migrate-set-parameters': Invalid parameter 'tls-creds' As I mentioned in my v1 review, we should always set the parameters if QEMU supports them to make sure they don't contain any leftovers from a previous migration. I also said we should properly clean the TLS stuff somewhere inside qemuProcessRecoverMigration*. And if we do it in addition to properly cleaning up everything in the Finish and Confirm phases, we should not need to store migrateTLS in the status XML. We can just always do the cleanup if QEMU supports TLS migration. Anyway, TLS migration doesn't work even if it is properly configured: operation failed: migration job: unexpectedly failed And the destination QEMU log contains: qemu-system-x86_64: No TLS credentials with id 'objmigrate_tls0' The reason is an interesting flow of QMP commands issued by libvirt during the Prepare phase: { "execute": "object-add", "arguments": { "qom-type": "tls-creds-x509", "id": "objmigrate_tls0", "props": { "dir": "/etc/pki/libvirt", "endpoint": "server", "verify-peer": false } }, "id": "libvirt-21" } { "execute": "migrate-set-parameters", "arguments": { "tls-creds": "objmigrate_tls0" }, "id": "libvirt-26" } { "execute": "migrate-incoming", "arguments": { "uri": "tcp:[::]:49152" }, "id": "libvirt-27" } { "execute": "object-del", "arguments": { "id": "objmigrate_tls0" }, "id": "libvirt-28" } The error reported after you deleted the objmigrate_tls0 object doesn't seem to confirm your idea about migration parameters begin unset when the object is removed. Please, test your series before you send a new version of it. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Feb 28, 2017 at 02:48:19PM +0100, Jiri Denemark wrote: > On Thu, Feb 23, 2017 at 13:42:02 -0500, John Ferlan wrote: > > AFAIU - removal of the objects would remove the migration tls-creds, > > tls-hostname settings. > > This would be a very strange behavior. > > > I left the cfg->migrateTLS in for now - it's very simple to remove, but > > there would still need to be a key on something to ensure the migrateTLS > > environment has been properly configured since that would mean the default > > environment would need to be used/configured. Setting up the default > > environment is keyed off having the migrateTLS defined. That's all part > > of the qemu_conf reading logic. > > I still don't see the value in keeping migrate_tls option in qemu.conf. > If we want to check the environment is setup correctly we should check > the certificates are in place rather than relying on the option. The > error reported when migrate_tls is set, but the environment is not > prepared is: > > internal error: unable to execute QEMU command 'object-add': Unable > to access credentials /etc/pki/qemu/ca-cert.pem: No such file or > directory > > Not ideal but at least it names the file which is missing. > > > John Ferlan (14): > > qemu: Introduce qemuDomainSecretInfoNew > > qemu: Introduce qemuDomainSecretMigratePrepare > > qemu: Move exit monitor calls in failure paths > > qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects > > qemu: Refactor qemuDomainGetChardevTLSObjects to converge code > > qemu: Move qemuDomainSecretChardevPrepare call > > qemu: Move qemuDomainPrepareChardevSourceTLS call > > qemu: Introduce qemuDomainGetTLSObjects > > The eight patches above are mostly OK except for some small issues. > > But the rest of the series needs more work... > > > qemu: Add TLS params to _qemuMonitorMigrationParams > > Add new migration flag VIR_MIGRATE_TLS > > qemu: Create #define for TLS configuration setup. > > conf: Introduce migrate_tls_x509_cert_dir > > qemu: Set up the migrate TLS objects for target > > qemu: Set up the migration TLS objects for source > > The code should check whether QEMU actually supports TLS migration, > otherwise you will get > > internal error: unable to execute QEMU command > 'migrate-set-parameters': Invalid parameter 'tls-creds' > > As I mentioned in my v1 review, we should always set the parameters if > QEMU supports them to make sure they don't contain any leftovers from a > previous migration. I also said we should properly clean the TLS stuff > somewhere inside qemuProcessRecoverMigration*. And if we do it in > addition to properly cleaning up everything in the Finish and Confirm > phases, we should not need to store migrateTLS in the status XML. We can > just always do the cleanup if QEMU supports TLS migration. > > > Anyway, TLS migration doesn't work even if it is properly configured: > > operation failed: migration job: unexpectedly failed > > And the destination QEMU log contains: > > qemu-system-x86_64: No TLS credentials with id 'objmigrate_tls0' > > The reason is an interesting flow of QMP commands issued by libvirt > during the Prepare phase: > > { > "execute": "object-add", > "arguments": { > "qom-type": "tls-creds-x509", > "id": "objmigrate_tls0", > "props": { > "dir": "/etc/pki/libvirt", > "endpoint": "server", > "verify-peer": false > } > }, > "id": "libvirt-21" > } > > { > "execute": "migrate-set-parameters", > "arguments": { > "tls-creds": "objmigrate_tls0" > }, > "id": "libvirt-26" > } > > { > "execute": "migrate-incoming", > "arguments": { > "uri": "tcp:[::]:49152" > }, > "id": "libvirt-27" > } > > { > "execute": "object-del", > "arguments": { > "id": "objmigrate_tls0" > }, > "id": "libvirt-28" > } > > The error reported after you deleted the objmigrate_tls0 object doesn't > seem to confirm your idea about migration parameters begin unset when > the object is removed. The problem here is that 'migrate-incoming' merely sets up a TCP listener, it doesn't actually initiate a connection or TLS handshake. The object-del thus deletes the TLS creds before the src QEMU has had a chance to even connect. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/28/2017 08:48 AM, Jiri Denemark wrote: > On Thu, Feb 23, 2017 at 13:42:02 -0500, John Ferlan wrote: >> AFAIU - removal of the objects would remove the migration tls-creds, >> tls-hostname settings. > > This would be a very strange behavior. > >> I left the cfg->migrateTLS in for now - it's very simple to remove, but >> there would still need to be a key on something to ensure the migrateTLS >> environment has been properly configured since that would mean the default >> environment would need to be used/configured. Setting up the default >> environment is keyed off having the migrateTLS defined. That's all part >> of the qemu_conf reading logic. > > I still don't see the value in keeping migrate_tls option in qemu.conf. > If we want to check the environment is setup correctly we should check > the certificates are in place rather than relying on the option. The > error reported when migrate_tls is set, but the environment is not > prepared is: > > internal error: unable to execute QEMU command 'object-add': Unable > to access credentials /etc/pki/qemu/ca-cert.pem: No such file or > directory > > Not ideal but at least it names the file which is missing. > In my latest branch I've removed migrate_tls. >> John Ferlan (14): >> qemu: Introduce qemuDomainSecretInfoNew >> qemu: Introduce qemuDomainSecretMigratePrepare >> qemu: Move exit monitor calls in failure paths >> qemu: Refactor hotplug to introduce qemuDomain{Add|Del}TLSObjects >> qemu: Refactor qemuDomainGetChardevTLSObjects to converge code >> qemu: Move qemuDomainSecretChardevPrepare call >> qemu: Move qemuDomainPrepareChardevSourceTLS call >> qemu: Introduce qemuDomainGetTLSObjects > > The eight patches above are mostly OK except for some small issues. OK - I will separate them out with the latest adjustments from review and repost. > > But the rest of the series needs more work... > >> qemu: Add TLS params to _qemuMonitorMigrationParams >> Add new migration flag VIR_MIGRATE_TLS >> qemu: Create #define for TLS configuration setup. >> conf: Introduce migrate_tls_x509_cert_dir >> qemu: Set up the migrate TLS objects for target >> qemu: Set up the migration TLS objects for source > > The code should check whether QEMU actually supports TLS migration, > otherwise you will get > > internal error: unable to execute QEMU command > 'migrate-set-parameters': Invalid parameter 'tls-creds' > Hmm... I see tls-creds added to migrate-set-parameters in 2.7 while they were added to ChardevSocket in 2.6... Ugh. Have to refresh my recollection of how to get the answer I need from capabilities. > As I mentioned in my v1 review, we should always set the parameters if > QEMU supports them to make sure they don't contain any leftovers from a > previous migration. I see from a quick scan of the qemu code though that it appears as if the code checks for a non null value being passed: params->has_tls_creds = !!s->parameters.tls_creds; params->has_tls_hostname = !!s->parameters.tls_hostname; So in order to "allow" clearing the tls_creds and tls_hostname, what would one do? The clearing would be necessary since a target of a migration will become a source for a migration and the tls-creds object would be different (using endpoint={server|client}). This would be the I used TLS on one migration, but not on the next type operation for the same domain. Doesn't seem we can assume that TLS will be used for every migration since the desire is to have usage controlled via API option and not host config option > I also said we should properly clean the TLS stuff > somewhere inside qemuProcessRecoverMigration*. And if we do it in > addition to properly cleaning up everything in the Finish and Confirm > phases, we should not need to store migrateTLS in the status XML. We can > just always do the cleanup if QEMU supports TLS migration. > > > Anyway, TLS migration doesn't work even if it is properly configured: > > operation failed: migration job: unexpectedly failed > > And the destination QEMU log contains: > > qemu-system-x86_64: No TLS credentials with id 'objmigrate_tls0' > > The reason is an interesting flow of QMP commands issued by libvirt > during the Prepare phase: > > { > "execute": "object-add", > "arguments": { > "qom-type": "tls-creds-x509", > "id": "objmigrate_tls0", > "props": { > "dir": "/etc/pki/libvirt", > "endpoint": "server", > "verify-peer": false > } > }, > "id": "libvirt-21" > } > > { > "execute": "migrate-set-parameters", > "arguments": { > "tls-creds": "objmigrate_tls0" > }, > "id": "libvirt-26" > } > > { > "execute": "migrate-incoming", > "arguments": { > "uri": "tcp:[::]:49152" > }, > "id": "libvirt-27" > } > > { > "execute": "object-del", > "arguments": { > "id": "objmigrate_tls0" > }, > "id": "libvirt-28" > } > > The error reported after you deleted the objmigrate_tls0 object doesn't > seem to confirm your idea about migration parameters begin unset when > the object is removed. OK - well obviously there's still quite a bit for me to understand about the migration model. > > Please, test your series before you send a new version of it. > Yep - something I noted in my cover letter - the need for a test environment... Hopefully I'll find a kind soul that will allow me to have access to an already configured environment to test with... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Feb 28, 2017 at 11:07:21 -0500, John Ferlan wrote: > On 02/28/2017 08:48 AM, Jiri Denemark wrote: > > The code should check whether QEMU actually supports TLS migration, > > otherwise you will get > > > > internal error: unable to execute QEMU command > > 'migrate-set-parameters': Invalid parameter 'tls-creds' > > > > Hmm... I see tls-creds added to migrate-set-parameters in 2.7 while > they were added to ChardevSocket in 2.6... Ugh. Have to refresh my > recollection of how to get the answer I need from capabilities. query-migrate-parameters used by qemuMonitorJSONGetMigrationParams is your friend. And that's the reason why I said you actually might need the *_set variables. > > As I mentioned in my v1 review, we should always set the parameters if > > QEMU supports them to make sure they don't contain any leftovers from a > > previous migration. > > I see from a quick scan of the qemu code though that it appears as if > the code checks for a non null value being passed: > > params->has_tls_creds = !!s->parameters.tls_creds; > params->has_tls_hostname = !!s->parameters.tls_hostname; > > So in order to "allow" clearing the tls_creds and tls_hostname, what > would one do? I was told Daniel should be the right person to ask. > > { > > "execute": "migrate-incoming", > > "arguments": { > > "uri": "tcp:[::]:49152" > > }, > > "id": "libvirt-27" > > } > > > > { > > "execute": "object-del", > > "arguments": { > > "id": "objmigrate_tls0" > > }, > > "id": "libvirt-28" > > } > > > > The error reported after you deleted the objmigrate_tls0 object doesn't > > seem to confirm your idea about migration parameters begin unset when > > the object is removed. > > OK - well obviously there's still quite a bit for me to understand about > the migration model. I think you had this part correctly in the previous version. The Prepare phase should only call qemuMigrationDelTLSObjects in case of error. The Finish phase is where all the cleanup after a migration is done. > > Please, test your series before you send a new version of it. > > Yep - something I noted in my cover letter - the need for a test > environment... Hopefully I'll find a kind soul that will allow me to > have access to an already configured environment to test with... Setting up migration environment is trivial. You just need two hosts or two VMs. The easiest way is configuring libvirt to listen on TCP with no authorization and open the port on both firewalls. Then you only need a domain to migrate. For most cases it doesn't even need a disk or you can use a live CD image stored in the same path on both hosts. It's not required to setup hostnames and make them resolvable from both hosts, but you can avoid an extra argument to virsh migrate if you do so. Setting up TLS is not hard either, you can follow https://kashyapc.fedorapeople.org/gnutls-pki-setup.txt or you can use easy-rsa. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 12:57:13 +0100, Jiri Denemark wrote: > On Tue, Feb 28, 2017 at 11:07:21 -0500, John Ferlan wrote: > > On 02/28/2017 08:48 AM, Jiri Denemark wrote: > > > The code should check whether QEMU actually supports TLS migration, > > > otherwise you will get > > > > > > internal error: unable to execute QEMU command > > > 'migrate-set-parameters': Invalid parameter 'tls-creds' > > > > > > > Hmm... I see tls-creds added to migrate-set-parameters in 2.7 while > > they were added to ChardevSocket in 2.6... Ugh. Have to refresh my > > recollection of how to get the answer I need from capabilities. > > query-migrate-parameters used by qemuMonitorJSONGetMigrationParams is > your friend. And that's the reason why I said you actually might need > the *_set variables. Oh, but you can't use query-migrate-parameters because the TLS parameters are not reported when they are not set. This looks like another thing which needs to be fixed in QEMU. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/01/2017 06:57 AM, Jiri Denemark wrote: > On Tue, Feb 28, 2017 at 11:07:21 -0500, John Ferlan wrote: >> On 02/28/2017 08:48 AM, Jiri Denemark wrote: >>> The code should check whether QEMU actually supports TLS migration, >>> otherwise you will get >>> >>> internal error: unable to execute QEMU command >>> 'migrate-set-parameters': Invalid parameter 'tls-creds' >>> >> >> Hmm... I see tls-creds added to migrate-set-parameters in 2.7 while >> they were added to ChardevSocket in 2.6... Ugh. Have to refresh my >> recollection of how to get the answer I need from capabilities. > > query-migrate-parameters used by qemuMonitorJSONGetMigrationParams is > your friend. And that's the reason why I said you actually might need > the *_set variables. > That jiggled a memory strand... It really wasn't clear from reading QEMU's qapi-schema.json description that the Get would return anything for tls-{creds,hostname}. So for determining whether the option exists or not I'm left to other options it seems. Even if code is added (in 2.9) to provide something like an empty string - that'd have to be backported to 2.8 and 2.7 and we'd have to somehow ensure/hope that was applied so that the right answer could be returned from Get... >>> As I mentioned in my v1 review, we should always set the parameters if >>> QEMU supports them to make sure they don't contain any leftovers from a >>> previous migration. >> >> I see from a quick scan of the qemu code though that it appears as if >> the code checks for a non null value being passed: >> >> params->has_tls_creds = !!s->parameters.tls_creds; >> params->has_tls_hostname = !!s->parameters.tls_hostname; >> >> So in order to "allow" clearing the tls_creds and tls_hostname, what >> would one do? > > I was told Daniel should be the right person to ask. > >>> { >>> "execute": "migrate-incoming", >>> "arguments": { >>> "uri": "tcp:[::]:49152" >>> }, >>> "id": "libvirt-27" >>> } >>> >>> { >>> "execute": "object-del", >>> "arguments": { >>> "id": "objmigrate_tls0" >>> }, >>> "id": "libvirt-28" >>> } >>> >>> The error reported after you deleted the objmigrate_tls0 object doesn't >>> seem to confirm your idea about migration parameters begin unset when >>> the object is removed. >> >> OK - well obviously there's still quite a bit for me to understand about >> the migration model. > > I think you had this part correctly in the previous version. The Prepare > phase should only call qemuMigrationDelTLSObjects in case of error. The > Finish phase is where all the cleanup after a migration is done. > I'll look at what I changed - last week was a blur and a wedding with an open bar over the weekend while the cure for many things has resulted in a few less brain cells to recall my frame of reference! >>> Please, test your series before you send a new version of it. >> >> Yep - something I noted in my cover letter - the need for a test >> environment... Hopefully I'll find a kind soul that will allow me to >> have access to an already configured environment to test with... > > Setting up migration environment is trivial. You just need two hosts or > two VMs. The easiest way is configuring libvirt to listen on TCP with no > authorization and open the port on both firewalls. Then you only need a > domain to migrate. For most cases it doesn't even need a disk or you > can use a live CD image stored in the same path on both hosts. It's not > required to setup hostnames and make them resolvable from both hosts, > but you can avoid an extra argument to virsh migrate if you do so. > > Setting up TLS is not hard either, you can follow > https://kashyapc.fedorapeople.org/gnutls-pki-setup.txt or you can use > easy-rsa. > > Jirka > Sounds easy if you've done it before many times... It's not something I do every day nor have I done once for libvirt before... ;-) I assumed one really needed two hosts - I assume configuring two guests means setting up nested virt, but that's something else I haven't done... and the whole listen, open ports on firewalls makes my head spin. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 10:14:10 -0500, John Ferlan wrote: > On 03/01/2017 06:57 AM, Jiri Denemark wrote: > That jiggled a memory strand... It really wasn't clear from reading > QEMU's qapi-schema.json description that the Get would return anything > for tls-{creds,hostname}. > > So for determining whether the option exists or not I'm left to other > options it seems. Even if code is added (in 2.9) to provide something > like an empty string - that'd have to be backported to 2.8 and 2.7 and > we'd have to somehow ensure/hope that was applied so that the right > answer could be returned from Get... Well, unless we have a way to reset the parameters we can't really use TLS migration anyway. > > I think you had this part correctly in the previous version. The Prepare > > phase should only call qemuMigrationDelTLSObjects in case of error. The > > Finish phase is where all the cleanup after a migration is done. > > I'll look at what I changed The previous version specified the objects on QEMU command line when. I think calling qemuMigrationDelTLSObjectsin the cleanup part of qemuMigrationPrepareAny only in case of error should fix the issue (I haven't tried it though). > Sounds easy if you've done it before many times... It's not something I > do every day nor have I done once for libvirt before... ;-) I assumed > one really needed two hosts - I assume configuring two guests means > setting up nested virt, but that's something else I haven't done... Not necessarily. You can start a type="qemu" domain inside the guests. Anyway, to enable nested virt, just load kvm-intel module with nested=1 (AMD should have nested enabled by default) and then you need to make sure vmx (or svm for AMD) CPU feature is enabled. For example, with the following CPU XML added to the guests' definition: <cpu> <model>Skylake-Client</model> <feature policy='require' name='vmx'/> </cpu> Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/01/2017 10:33 AM, Jiri Denemark wrote: > On Wed, Mar 01, 2017 at 10:14:10 -0500, John Ferlan wrote: >> On 03/01/2017 06:57 AM, Jiri Denemark wrote: >> That jiggled a memory strand... It really wasn't clear from reading >> QEMU's qapi-schema.json description that the Get would return anything >> for tls-{creds,hostname}. >> >> So for determining whether the option exists or not I'm left to other >> options it seems. Even if code is added (in 2.9) to provide something >> like an empty string - that'd have to be backported to 2.8 and 2.7 and >> we'd have to somehow ensure/hope that was applied so that the right >> answer could be returned from Get... > > Well, unless we have a way to reset the parameters we can't really use > TLS migration anyway. > Based on the qemu patch I see - the only way we'll know is by knowing which version the patch has been applied. We could set NULL or "", but that would seem to cause errors in versions between 2.7 and wherever the fix ends up. <sigh> While I'm thinking about these types of things... I started down the NBD path too. The server side would seem to be fairly trivial adding the tls-creds to the command line; however, the client side is a bit more tricky. Currently (if I read the code right), NBD would use 'drive-mirror' passing along the "*file" parameter as "nbd:%s:%d:exportname=%s", where the first %s is the hostname, the %d is the port, and the exportname is the diskAlias (see nbd_dest in qemuMigrationDriveMirror). If I look at the qemu end of that - it will parse that nbd: prefixed string, but the parsing code doesn't accept a tls-creds type parameter. So more research leads me to a qemu-devel conversation : http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07721.html which in the long run I think implies libvirt should be using blockdev-mirror instead. Of course that gets bogged down in a discussion about blockdev-add, but let's say blockdev-mirror was usable. How then would the tls-creds be passed? I see them added to the @BlockdevOptionsNbd in QEMU's block-core.json, but it's not very clear how they'd be provided. I assume some sort of json buffer magic, but that's purely a guess. It's certainly not as simple as providing it using the "-drive driver=nbd,host..." from as described on Daniel's TLS blog post: https://www.berrange.com/posts/2016/04/05/improving-qemu-security-part-5-tls-support-for-nbd-server-client/ Tks for any feedback - John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 12:47:18PM -0500, John Ferlan wrote: > > > On 03/01/2017 10:33 AM, Jiri Denemark wrote: > > On Wed, Mar 01, 2017 at 10:14:10 -0500, John Ferlan wrote: > >> On 03/01/2017 06:57 AM, Jiri Denemark wrote: > >> That jiggled a memory strand... It really wasn't clear from reading > >> QEMU's qapi-schema.json description that the Get would return anything > >> for tls-{creds,hostname}. > >> > >> So for determining whether the option exists or not I'm left to other > >> options it seems. Even if code is added (in 2.9) to provide something > >> like an empty string - that'd have to be backported to 2.8 and 2.7 and > >> we'd have to somehow ensure/hope that was applied so that the right > >> answer could be returned from Get... > > > > Well, unless we have a way to reset the parameters we can't really use > > TLS migration anyway. > > > > Based on the qemu patch I see - the only way we'll know is by knowing > which version the patch has been applied. > > We could set NULL or "", but that would seem to cause errors in versions > between 2.7 and wherever the fix ends up. <sigh> > > While I'm thinking about these types of things... I started down the NBD > path too. The server side would seem to be fairly trivial adding the > tls-creds to the command line; however, the client side is a bit more > tricky. > > Currently (if I read the code right), NBD would use 'drive-mirror' > passing along the "*file" parameter as "nbd:%s:%d:exportname=%s", where > the first %s is the hostname, the %d is the port, and the exportname is > the diskAlias (see nbd_dest in qemuMigrationDriveMirror). > > If I look at the qemu end of that - it will parse that nbd: prefixed > string, but the parsing code doesn't accept a tls-creds type parameter. Yep, we can't use the legacy URI syntax - we need the block dev property syntax. > So more research leads me to a qemu-devel conversation : > > http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07721.html > > which in the long run I think implies libvirt should be using > blockdev-mirror instead. Of course that gets bogged down in a discussion > about blockdev-add, but let's say blockdev-mirror was usable. How then IIRC, the conclusion was that its possible use blockdev-mirror, without using blockdev-add http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07748.html > would the tls-creds be passed? I see them added to the > @BlockdevOptionsNbd in QEMU's block-core.json, but it's not very clear > how they'd be provided. I assume some sort of json buffer magic, but > that's purely a guess. It's certainly not as simple as providing it > using the "-drive driver=nbd,host..." from as described on Daniel's TLS > blog post: The 'tls-creds' parameter in @BlockdevOptionsNbd just refers to the ID of the TLS credentials object previously created with object-add. In fact we should use the same TLS credentials for both the migration server/client and the associated NBD server/client. IIRC, in JSon, @BlockdevOptionsNbd ends up looking something like { 'driver': 'nbd', 'data': { 'server': { 'inet': { 'host': 'foobar' 'port': '9000' } }, 'tls-creds': 'tls0' } (Not entirely sure if i need another level of nesting in between the 'server' and 'inet' bit or not ). Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 04:33:36PM +0100, Jiri Denemark wrote: > On Wed, Mar 01, 2017 at 10:14:10 -0500, John Ferlan wrote: [...] > Well, unless we have a way to reset the parameters we can't really use > TLS migration anyway. > > > > I think you had this part correctly in the previous version. The Prepare > > > phase should only call qemuMigrationDelTLSObjects in case of error. The > > > Finish phase is where all the cleanup after a migration is done. > > > > I'll look at what I changed > > The previous version specified the objects on QEMU command line when. I > think calling qemuMigrationDelTLSObjectsin the cleanup part of > qemuMigrationPrepareAny only in case of error should fix the issue (I > haven't tried it though). > > > Sounds easy if you've done it before many times... It's not something I > > do every day nor have I done once for libvirt before... ;-) I assumed > > one really needed two hosts - I assume configuring two guests means > > setting up nested virt, but that's something else I haven't done... > > Not necessarily. You can start a type="qemu" domain inside the guests. > Anyway, to enable nested virt, just load kvm-intel module with nested=1 > (AMD should have nested enabled by default) and then you need to make > sure vmx (or svm for AMD) CPU feature is enabled. For example, with the > following CPU XML added to the guests' definition: > > <cpu> > <model>Skylake-Client</model> > <feature policy='require' name='vmx'/> > </cpu> Or if desired pass-through the host CPU model: $ virt-xml guest-hyp --edit --cpu host-passthrough,clearxml=yes Reboot, and don't forget to check /dev/kvm char device exists inside the L1 guest. Full notes: https://kashyapc.fedorapeople.org/virt/procedure-to-enable-nested-virt-on-intel-machines.txt -- /kashyap -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Feb 28, 2017 at 11:07:21AM -0500, John Ferlan wrote: > > As I mentioned in my v1 review, we should always set the parameters if > > QEMU supports them to make sure they don't contain any leftovers from a > > previous migration. > > I see from a quick scan of the qemu code though that it appears as if > the code checks for a non null value being passed: > > params->has_tls_creds = !!s->parameters.tls_creds; > params->has_tls_hostname = !!s->parameters.tls_hostname; That code is in the function for querying whether tls parameters are currently set. > So in order to "allow" clearing the tls_creds and tls_hostname, what > would one do? The clearing would be necessary since a target of a > migration will become a source for a migration and the tls-creds object > would be different (using endpoint={server|client}). Hmm, I see this is a limitation of the migrate-set-parameters method. You can set new parameters for tls_creds / tls_hostname, but you can't fully delete the old parameters. The tls_hostname is only set on the source host of the migration and that VM will be killed off upon successful migration. The problem only arises if you have a migration that fails, and you then try to migrate that same VM again later, *and* you don't have tls_hostname set. I don't think that'd hit libvirt, since libvirt will always need to set tls-hostname as it uses fd: migrate method. IOW, I don't see any need to be able to clear tls-hostname when used with libvirt. For TLS creds it would be a problem if we do a TLS migration and then need to migrate the target QEMU again later, but don't want TLS used, as that would require us to fully clear the tls_creds parameter. At best we can set it to empty string, which is not good enough. So seems we need a QEMU fix here. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 12:18:23 +0000, Daniel P. Berrange wrote: > Hmm, I see this is a limitation of the migrate-set-parameters method. You > can set new parameters for tls_creds / tls_hostname, but you can't fully > delete the old parameters. > > The tls_hostname is only set on the source host of the migration and that > VM will be killed off upon successful migration. The problem only arises > if you have a migration that fails, and you then try to migrate that same > VM again later, *and* you don't have tls_hostname set. I don't think that'd > hit libvirt, since libvirt will always need to set tls-hostname as it uses > fd: migrate method. IOW, I don't see any need to be able to clear > tls-hostname when used with libvirt. But we will only set it if TLS is turned on. We certainly don't want to use this parameter when saving a domain to a file, making a snapshot, or migrating without TLS. > For TLS creds it would be a problem if we do a TLS migration and then need > to migrate the target QEMU again later, but don't want TLS used, as that > would require us to fully clear the tls_creds parameter. At best we can > set it to empty string, which is not good enough. So seems we need a QEMU > fix here. Another problem (I mentioned in another email in this series) is query-migrate-parameters does not show the tls parameters unless they are set. So it looks like we need to invent a special value which can be reported when they are unset and we could use the same special value to reset them. An empty string sounds like an obvious candidate. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 01:39:18PM +0100, Jiri Denemark wrote: > On Wed, Mar 01, 2017 at 12:18:23 +0000, Daniel P. Berrange wrote: > > Hmm, I see this is a limitation of the migrate-set-parameters method. You > > can set new parameters for tls_creds / tls_hostname, but you can't fully > > delete the old parameters. > > > > The tls_hostname is only set on the source host of the migration and that > > VM will be killed off upon successful migration. The problem only arises > > if you have a migration that fails, and you then try to migrate that same > > VM again later, *and* you don't have tls_hostname set. I don't think that'd > > hit libvirt, since libvirt will always need to set tls-hostname as it uses > > fd: migrate method. IOW, I don't see any need to be able to clear > > tls-hostname when used with libvirt. > > But we will only set it if TLS is turned on. We certainly don't want to > use this parameter when saving a domain to a file, making a snapshot, or > migrating without TLS. If you don't have TLS enabled, then whether tls-hostname is set or not is irrelevant - it'll never be used. So the fact that tls-hostname may be still mistakenly set, when you later save to a file is harmless, as long as tls-creds is unset. > > For TLS creds it would be a problem if we do a TLS migration and then need > > to migrate the target QEMU again later, but don't want TLS used, as that > > would require us to fully clear the tls_creds parameter. At best we can > > set it to empty string, which is not good enough. So seems we need a QEMU > > fix here. > > Another problem (I mentioned in another email in this series) is > query-migrate-parameters does not show the tls parameters unless they > are set. So it looks like we need to invent a special value which can be > reported when they are unset and we could use the same special value to > reset them. An empty string sounds like an obvious candidate. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 01, 2017 at 12:51:32 +0000, Daniel P. Berrange wrote: > On Wed, Mar 01, 2017 at 01:39:18PM +0100, Jiri Denemark wrote: > > On Wed, Mar 01, 2017 at 12:18:23 +0000, Daniel P. Berrange wrote: > > > Hmm, I see this is a limitation of the migrate-set-parameters method. You > > > can set new parameters for tls_creds / tls_hostname, but you can't fully > > > delete the old parameters. > > > > > > The tls_hostname is only set on the source host of the migration and that > > > VM will be killed off upon successful migration. The problem only arises > > > if you have a migration that fails, and you then try to migrate that same > > > VM again later, *and* you don't have tls_hostname set. I don't think that'd > > > hit libvirt, since libvirt will always need to set tls-hostname as it uses > > > fd: migrate method. IOW, I don't see any need to be able to clear > > > tls-hostname when used with libvirt. > > > > But we will only set it if TLS is turned on. We certainly don't want to > > use this parameter when saving a domain to a file, making a snapshot, or > > migrating without TLS. > > If you don't have TLS enabled, then whether tls-hostname is set or not > is irrelevant - it'll never be used. So the fact that tls-hostname may > be still mistakenly set, when you later save to a file is harmless, as > long as tls-creds is unset. OK, makes sense. Although it also makes sense for the two parameters to provide the same behavior. > > > For TLS creds it would be a problem if we do a TLS migration and then need > > > to migrate the target QEMU again later, but don't want TLS used, as that > > > would require us to fully clear the tls_creds parameter. At best we can > > > set it to empty string, which is not good enough. So seems we need a QEMU > > > fix here. > > > > Another problem (I mentioned in another email in this series) is > > query-migrate-parameters does not show the tls parameters unless they > > are set. So it looks like we need to invent a special value which can be > > reported when they are unset and we could use the same special value to > > reset them. An empty string sounds like an obvious candidate. As Pavel mentioned offline, JSON supports null value. So it could be an option too. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.