Proposal: deprecate "vncviewer" option in xl domain config file

Proposal: deprecate "vncviewer" option in xl domain config file

[Wei Liu]

Hi all

When I'm working on (de-)serialization of domain configurations I found
"vncviewer". I now propose to deprecate it.

What it does is that if you have it set in your domain's config file and
do "xl create cfg", xl will automatically spawn a vncviewer for you.
This option actually controls the creation process of a domain but has
nothing to do with domain configuration at all.

This option is buggy because it's also saved as part of domain state
when you do save / restore. Consider user migrates a domain to a remote
host, xl will try to auto-spawn vncviewer on the new host. This behavior
doesn't make sense at all.

Further more it becomes an obstacle for the work to (de-)serialization
domain configurations. If we want to preserve this option we then need
to create abstraction for a config file in xl or libxl. This either
introduces lots of work without much benefit (if we add out-of-band
infomation in xl) or pollute libxl public interface.

I propose to deprecate this option.

What I will do is:
1. config file still supports this option but it will prints out a
   warning about its deprecation.
2. this option is not saved as domain state when doing save / restore,
   so when you restore a domain xl will not auto-spawn vncviewer.

This may create a minor regression, but it's in no way critical. In
any case the right way to auto-spawn vncviewer is to use "-V" in "xl
create". And existing user of this config file option can also use "-V"
to work around this regression.

Comments?

Wei.

Re: Proposal: deprecate "vncviewer" option in xl domain config file

[Ian Jackson]

Wei Liu writes ("Proposal: deprecate "vncviewer" option in xl domain config file"):
> When I'm working on (de-)serialization of domain configurations I found
> "vncviewer". I now propose to deprecate it.
...
> I propose to deprecate this option.
> 
> What I will do is:
> 1. config file still supports this option but it will prints out a
>    warning about its deprecation.
> 2. this option is not saved as domain state when doing save / restore,
>    so when you restore a domain xl will not auto-spawn vncviewer.

This comes out of a conversation between Wei and me.  For the record,
I think this is the right approach.

The existing behaviour, where "xl restore" might start a vncviewer if
the previously saved domain config had specified vncviewer=1, is IMO
insane (and I doubt intentional on anyone's part).

Thanks,
Ian.

Re: Proposal: deprecate "vncviewer" option in xl domain config file

[Ian Campbell]

On Tue, 2014-04-22 at 16:19 +0100, Wei Liu wrote:
> Hi all
> 
> When I'm working on (de-)serialization of domain configurations I found
> "vncviewer". I now propose to deprecate it.
> 
> What it does is that if you have it set in your domain's config file and
> do "xl create cfg", xl will automatically spawn a vncviewer for you.
> This option actually controls the creation process of a domain but has
> nothing to do with domain configuration at all.
> 
> This option is buggy because it's also saved as part of domain state
> when you do save / restore.

Where is it saved?

>  Consider user migrates a domain to a remote
> host, xl will try to auto-spawn vncviewer on the new host. This behavior
> doesn't make sense at all.
> 
> Further more it becomes an obstacle for the work to (de-)serialization
> domain configurations. If we want to preserve this option we then need
> to create abstraction for a config file in xl or libxl. This either
> introduces lots of work without much benefit (if we add out-of-band
> infomation in xl) or pollute libxl public interface.
> 
> I propose to deprecate this option.
> 
> What I will do is:
> 1. config file still supports this option but it will prints out a
>    warning about its deprecation.
> 2. this option is not saved as domain state when doing save / restore,
>    so when you restore a domain xl will not auto-spawn vncviewer.
> 
> This may create a minor regression, but it's in no way critical. In
> any case the right way to auto-spawn vncviewer is to use "-V" in "xl
> create". And existing user of this config file option can also use "-V"
> to work around this regression.
> 
> Comments?

Rather than throwing the baby out with the bathwater can't we just say
that this option is only obeyed for the initial domain creation and not
for any subsequent migration or restore? What would avoid the need to
propagate it along with the save/migrate image.

Ian.

Re: Proposal: deprecate "vncviewer" option in xl domain config file

[Wei Liu]

On Tue, Apr 22, 2014 at 04:50:56PM +0100, Ian Campbell wrote:
> On Tue, 2014-04-22 at 16:19 +0100, Wei Liu wrote:
> > Hi all
> > 
> > When I'm working on (de-)serialization of domain configurations I found
> > "vncviewer". I now propose to deprecate it.
> > 
> > What it does is that if you have it set in your domain's config file and
> > do "xl create cfg", xl will automatically spawn a vncviewer for you.
> > This option actually controls the creation process of a domain but has
> > nothing to do with domain configuration at all.
> > 
> > This option is buggy because it's also saved as part of domain state
> > when you do save / restore.
> 
> Where is it saved?
> 

The domain config file is saved, then used when restoring. Restoring
process involves re-parsing that config file.

> >  Consider user migrates a domain to a remote
> > host, xl will try to auto-spawn vncviewer on the new host. This behavior
> > doesn't make sense at all.
> > 
> > Further more it becomes an obstacle for the work to (de-)serialization
> > domain configurations. If we want to preserve this option we then need
> > to create abstraction for a config file in xl or libxl. This either
> > introduces lots of work without much benefit (if we add out-of-band
> > infomation in xl) or pollute libxl public interface.
> > 
> > I propose to deprecate this option.
> > 
> > What I will do is:
> > 1. config file still supports this option but it will prints out a
> >    warning about its deprecation.
> > 2. this option is not saved as domain state when doing save / restore,
> >    so when you restore a domain xl will not auto-spawn vncviewer.
> > 
> > This may create a minor regression, but it's in no way critical. In
> > any case the right way to auto-spawn vncviewer is to use "-V" in "xl
> > create". And existing user of this config file option can also use "-V"
> > to work around this regression.
> > 
> > Comments?
> 
> Rather than throwing the baby out with the bathwater can't we just say
> that this option is only obeyed for the initial domain creation and not
> for any subsequent migration or restore? What would avoid the need to
> propagate it along with the save/migrate image.
> 

I think this is just wording issue. My "xl-json" format patch does this
already.  I'm OK with any approach as long as I don't need to propogate
it. :-P

Wei.

> Ian.
> 

Re: Proposal: deprecate "vncviewer" option in xl domain config file

[Ian Jackson]

Wei Liu writes ("Re: Proposal: deprecate "vncviewer" option in xl domain config file"):
> On Tue, Apr 22, 2014 at 04:50:56PM +0100, Ian Campbell wrote:
> > Where is it saved?
> 
> The domain config file is saved, then used when restoring. Restoring
> process involves re-parsing that config file.

Ie the domain configuration file provided to xl create is saved in the
libxl userdata.  During xl save, that domain configuration file is
recorded in the save image.  During xl restore, it is extracted from
the save image and reparsed.  If the original configuration file
contained "vncviewer=1", this will result in xl restore running the
vncviewer, regardless of xl restore's command line arguments.

> > Rather than throwing the baby out with the bathwater can't we just say
> > that this option is only obeyed for the initial domain creation and not
> > for any subsequent migration or restore? What would avoid the need to
> > propagate it along with the save/migrate image.
> 
> I think this is just wording issue. My "xl-json" format patch does this
> already.  I'm OK with any approach as long as I don't need to propogate
> it. :-P

Precisely.

Although, I would go further and say that this kind of thing shouldn't
be in the domain config file.  The same config file should be able to
start a domain both with and without automatically running vncviewer.

After all we don't have an xl domain config file option for
automatically running xenconsole - we rely, only, on the -c option for
that.

Ian.

Re: Proposal: deprecate "vncviewer" option in xl domain config file

[Ian Campbell]

On Tue, 2014-04-22 at 17:32 +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: Proposal: deprecate "vncviewer" option in xl domain config file"):
> > On Tue, Apr 22, 2014 at 04:50:56PM +0100, Ian Campbell wrote:
> > > Where is it saved?
> > 
> > The domain config file is saved, then used when restoring. Restoring
> > process involves re-parsing that config file.
> 
> Ie the domain configuration file provided to xl create is saved in the
> libxl userdata.  During xl save, that domain configuration file is
> recorded in the save image.  During xl restore, it is extracted from
> the save image and reparsed.  If the original configuration file
> contained "vncviewer=1", this will result in xl restore running the
> vncviewer, regardless of xl restore's command line arguments.

Right, but Wei is removing this reparsing of the vncviewer.

> > > Rather than throwing the baby out with the bathwater can't we just say
> > > that this option is only obeyed for the initial domain creation and not
> > > for any subsequent migration or restore? What would avoid the need to
> > > propagate it along with the save/migrate image.
> > 
> > I think this is just wording issue. My "xl-json" format patch does this
> > already.  I'm OK with any approach as long as I don't need to propogate
> > it. :-P
> 
> Precisely.

Yes, I think it was unclear whether the intention was only to deprecate
the option on restore/migration or entirely. The former is unequivocally
fine IMHO.

> Although, I would go further and say that this kind of thing shouldn't
> be in the domain config file.  The same config file should be able to
> start a domain both with and without automatically running vncviewer.
> 
> After all we don't have an xl domain config file option for
> automatically running xenconsole - we rely, only, on the -c option for
> that.

I mostly agree, but this is the sort of thing which xend users might
think was a functional regression, not that this should be a blocker if
we really think this behaviour is intolerably bad/strange.

Ian.

Re: Proposal: deprecate "vncviewer" option in xl domain config file

[Wei Liu]

On Wed, Apr 23, 2014 at 09:38:42AM +0100, Ian Campbell wrote:
> On Tue, 2014-04-22 at 17:32 +0100, Ian Jackson wrote:
> > Wei Liu writes ("Re: Proposal: deprecate "vncviewer" option in xl domain config file"):
> > > On Tue, Apr 22, 2014 at 04:50:56PM +0100, Ian Campbell wrote:
> > > > Where is it saved?
> > > 
> > > The domain config file is saved, then used when restoring. Restoring
> > > process involves re-parsing that config file.
> > 
> > Ie the domain configuration file provided to xl create is saved in the
> > libxl userdata.  During xl save, that domain configuration file is
> > recorded in the save image.  During xl restore, it is extracted from
> > the save image and reparsed.  If the original configuration file
> > contained "vncviewer=1", this will result in xl restore running the
> > vncviewer, regardless of xl restore's command line arguments.
> 
> Right, but Wei is removing this reparsing of the vncviewer.
> 
> > > > Rather than throwing the baby out with the bathwater can't we just say
> > > > that this option is only obeyed for the initial domain creation and not
> > > > for any subsequent migration or restore? What would avoid the need to
> > > > propagate it along with the save/migrate image.
> > > 
> > > I think this is just wording issue. My "xl-json" format patch does this
> > > already.  I'm OK with any approach as long as I don't need to propogate
> > > it. :-P
> > 
> > Precisely.
> 
> Yes, I think it was unclear whether the intention was only to deprecate
> the option on restore/migration or entirely. The former is unequivocally
> fine IMHO.
> 
> > Although, I would go further and say that this kind of thing shouldn't
> > be in the domain config file.  The same config file should be able to
> > start a domain both with and without automatically running vncviewer.
> > 
> > After all we don't have an xl domain config file option for
> > automatically running xenconsole - we rely, only, on the -c option for
> > that.
> 
> I mostly agree, but this is the sort of thing which xend users might
> think was a functional regression, not that this should be a blocker if
> we really think this behaviour is intolerably bad/strange.
> 

OK. So the plan can be:
1. retain parsing "vncviewer" for config file in this release and print
   a warning to users this option may be removed in future release
2. don't save this option in save image

Wei.

> Ian.