Open Stack

On Thu, 2016-01-07 at 12:23 +0100, Sahid Orentino Ferdjaoui wrote:
> On Mon, Jan 04, 2016 at 09:12:06PM +0000, Mark McLoughlin wrote:
> > Hi
> > 
> > commit 8ecf93e[1] got me thinking - the live_migration_flag config
> > option unnecessarily allows operators choose arbitrary behavior of the
> > migrateToURI() libvirt call, to the extent that we allow the operator
> > to configure a behavior that can result in data loss[1].
> > 
> > I see that danpb recently said something similar:
> > 
> >   https://review.openstack.org/171098
> > 
> >   "Honestly, I wish we'd just kill off  'live_migration_flag' and
> >   'block_migration_flag' as config options. We really should not be
> >   exposing low level libvirt API flags as admin tunable settings.
> > 
> >   Nova should really be in charge of picking the correct set of flags
> >   for the current libvirt version, and the operation it needs to
> >   perform. We might need to add other more sensible config options in
> >   their place [..]"
> 
> Nova should really handle internal flags and this serie is running in
> the right way.
> 
> > ...
> 
> >   4) Add a new config option for tunneled versus native:
> > 
> >        [libvirt]
> >        live_migration_tunneled = true
> > 
> >      This enables the use of the VIR_MIGRATE_TUNNELLED flag. We have 
> >      historically defaulted to tunneled mode because it requires the 
> >      least configuration and is currently the only way to have a 
> >      secure migration channel.
> > 
> >      danpb's quote above continues with:
> > 
> >        "perhaps a "live_migration_secure_channel" to indicate that 
> >         migration must use encryption, which would imply use of 
> >         TUNNELLED flag"
> > 
> >      So we need to discuss whether the config option should express the
> >      choice of tunneled vs native, or whether it should express another
> >      choice which implies tunneled vs native.
> > 
> >        https://review.openstack.org/263434
> 
> We probably have to consider that operator does not know much about
> internal libvirt flags, so options we are exposing for him should
> reflect benefice of using them. I commented on your review we should
> at least explain benefice of using this option whatever the name is.

As predicted, plenty of discussion on this point in the review :)

You're right that we don't give the operator any guidance in the help
message about how to choose true or false for this:

  Whether to use tunneled migration, where migration data is 
  transported over the libvirtd connection. If True,
  we use the VIR_MIGRATE_TUNNELLED migration flag

libvirt's own docs on this are here:

  https://libvirt.org/migration.html#transport

which emphasizes:

  - the data copies involved in tunneling
  - the extra configuration steps required for native
  - the encryption support you get when tunneling

The discussions I've seen on this topic wrt Nova have revolved around:

  - that tunneling allows for an encrypted transport[1]
  - that qemu's NBD based drive-mirror block migration isn't supported
    using tunneled mode, and that danpb is working on fixing this
    limitation in libvirt
  - "selective" block migration[2] won't work with the fallback qemu
    block migration support, and so won't currently work in tunneled
    mode

So, the advise to operators would be:

  - You may want to choose tunneled=False for improved block migration 
    capabilities, but this limitation will go away in future.
  - You may want to choose tunneled=False if you wish to trade and
    encrypted transport for a (potentially negligible) performance
    improvement.

Does that make sense?

As for how to name the option, and as I said in the review, I think it
makes sense to be straightforward here and make it clearly about
choosing to disable libvirt's tunneled transport.

If we name it any other way, I think our explanation for operators will
immediately jump to explaining (a) that it influences the TUNNELLED
flag, and (b) the differences between the tunneled and native
transports. So, if we're going to have to talk about tunneled versus
native, why obscure that detail?

But, Pawel strongly disagrees.

One last point I'd make is this isn't about adding a *new*
configuration capability for operators. As we deprecate and remove
these configuration options, we need to be careful not to remove a
capability that operators are currently depending on for arguably
reasonable reasons.

[1] - https://review.openstack.org/#/c/171098/
[2] - https://review.openstack.org/#/c/227278


> >   5) Add a new config option for additional migration flags:
> > 
> >        [libvirt]
> >        live_migration_extra_flags = VIR_MIGRATE_COMPRESSED
> > 
> >      This allows operators to continue to experiment with libvirt behaviors
> >      in safe ways without each use case having to be accounted for.
> > 
> >        https://review.openstack.org/263435
> > 
> >      We would disallow setting the following flags via this option:
> > 
> >        VIR_MIGRATE_LIVE
> >        VIR_MIGRATE_PEER2PEER
> >        VIR_MIGRATE_TUNNELLED
> >        VIR_MIGRATE_PERSIST_DEST
> >        VIR_MIGRATE_UNDEFINE_SOURCE
> >        VIR_MIGRATE_NON_SHARED_INC
> >        VIR_MIGRATE_NON_SHARED_DISK
> > 
> >     which would allow the following currently available flags to be set:
> 
> >        VIR_MIGRATE_PAUSED
> >        VIR_MIGRATE_CHANGE_PROTECTION
> >        VIR_MIGRATE_UNSAFE
> >        VIR_MIGRATE_OFFLINE
> >        VIR_MIGRATE_COMPRESSED
> >        VIR_MIGRATE_ABORT_ON_ERROR
> >        VIR_MIGRATE_AUTO_CONVERGE
> >        VIR_MIGRATE_RDMA_PIN_ALL
> 
> We can probably consider to provide VIR_MIGRATE_PAUSED and
> VIR_MIGRATE_COMPRESSED as dedicated options too ?

Yes, I think any options we see regularly added to extra_flags by
operators, and as we understand the use cases better, then we can add
dedicated options for them.

In the review, Pawel is making a case for not allowing the operator to
enable COMPRESSED or AUTO_CONVERGE.

Thanks for the feedback,
Mark.