All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xl.conf defaults changes for Xen 4.4. (v1).
@ 2013-07-12 18:08 Konrad Rzeszutek Wilk
  2013-07-12 18:08 ` [PATCH 1/2] claim: By default enable it Konrad Rzeszutek Wilk
  2013-07-12 18:08 ` [PATCH 2/2] expert_mode: Add a new configuration option for expert users Konrad Rzeszutek Wilk
  0 siblings, 2 replies; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-12 18:08 UTC (permalink / raw)
  To: ian.jackson, xen-devel, george.dunlap

Hey George and Ian,

During the Xen 4.3 release we briefly chatted about enabling and
providing an seatbelt parameter for some of the functionality
that libxl provides. In particular these were:
 - xl vcpu-set - it has an host check such that the vCPU cannot be
                 set higher than the pCPU. But a user can set such
                 values in the guest config and launch an over-subscribed
                 guest without any warnings. (Perhaps we should add
                 a warning for that as well? Or don't launch the guest?)
 - claim_mode - The functionality here is not just specific to tmem enabled
                 guests. Any generic guest can benefit from this and it
                 allows the user to get up-to-date memory information on how
                 much free memory there really is - and how much is being
                 consumed for the guest creation. By default it is disabled.
                 This turns it on by default.

Please see the patches and enjoy reviewing them. I've asbestos jumpsuit 
ready in case this discussion gets heated.

 docs/man/xl.conf.pod.5   |   26 +++++++++++++++++++++++++-
 tools/examples/xl.conf   |    8 +++++++-
 tools/libxl/xl.c         |    4 ++++
 tools/libxl/xl.h         |    1 +
 tools/libxl/xl_cmdimpl.c |    6 ++++++
 5 files changed, 43 insertions(+), 2 deletions(-)


Konrad Rzeszutek Wilk (2):
      claim: By default enable it.
      expert_mode: Add a new configuration option for expert users.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] claim: By default enable it.
  2013-07-12 18:08 [PATCH] xl.conf defaults changes for Xen 4.4. (v1) Konrad Rzeszutek Wilk
@ 2013-07-12 18:08 ` Konrad Rzeszutek Wilk
  2013-07-16  9:03   ` Ian Campbell
  2013-07-12 18:08 ` [PATCH 2/2] expert_mode: Add a new configuration option for expert users Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-12 18:08 UTC (permalink / raw)
  To: ian.jackson, xen-devel, george.dunlap

During the Xen 4.3 release we discussed that this feature could be
turned on by default - as it benefits all of the guests - not just
tmem related.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/man/xl.conf.pod.5 |    2 +-
 tools/examples/xl.conf |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 1229c8a..125f786 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -139,7 +139,7 @@ Xen hypervisor argument and as well on the Linux kernel command line.
 Note that the claim call is not attempted if C<superpages> option is
 used in the guest config (see xl.cfg(5)).
 
-Default: C<0>
+Default: C<1>
 
 =over 4
 
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 9c037a6..a80b8e7 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -32,4 +32,4 @@
 # feedback whether the guest can be launched due to memory exhaustion
 # (which can take a long time to find out if launching huge guests).
 # see xl.conf(5) for details.
-#claim_mode=0
+claim_mode=1
-- 
1.7.7.6

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-12 18:08 [PATCH] xl.conf defaults changes for Xen 4.4. (v1) Konrad Rzeszutek Wilk
  2013-07-12 18:08 ` [PATCH 1/2] claim: By default enable it Konrad Rzeszutek Wilk
@ 2013-07-12 18:08 ` Konrad Rzeszutek Wilk
  2013-07-16  9:04   ` Ian Campbell
  1 sibling, 1 reply; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-12 18:08 UTC (permalink / raw)
  To: ian.jackson, xen-devel, george.dunlap

This could also be called 'seatbelt' option.

libxl has a variety of checks where it will fail out an operation
unless the user has provided an --force (or --ignore) parameter.
Currently one such check is for the 'vcpu-set' command which
will error out if the count of virtual cpus is greater than the
physical cpus. This parameter will ignore such checks and allow
the user to do the operations without the need for override flags.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/man/xl.conf.pod.5   |   24 ++++++++++++++++++++++++
 tools/examples/xl.conf   |    6 ++++++
 tools/libxl/xl.c         |    4 ++++
 tools/libxl/xl.h         |    1 +
 tools/libxl/xl_cmdimpl.c |    6 ++++++
 5 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 125f786..ed67472 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -158,6 +158,30 @@ massively huge guests).
 
 =back
 
+=item B<expert_mode=BOOLEAN>
+
+Do not act on host performed checks that might lead to performance
+degradations. Currently checks are made for following operations:
+ - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
+   physical count the operation will error out.
+
+
+Default: C<1>
+
+=over 4
+
+=item C<0>
+
+The checks are active and the operation will fail if the checks
+are triggered.
+
+=item C<1>
+
+The checks are active but will be ignored and the operations
+will commence.
+
+=back
+
 =back
 
 =head1 SEE ALSO
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index a80b8e7..6f27d67 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -33,3 +33,9 @@
 # (which can take a long time to find out if launching huge guests).
 # see xl.conf(5) for details.
 claim_mode=1
+
+# The user knows what to do. Currently enabling this option will mean that:
+#  vcpu-set won't check the physical CPU count - which means the guest can
+#           over-subscribe (more vCPUS than pCPUS).
+# see xl.conf(5) for details.
+expert_mode=1
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 1ce820c..4063b7c 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -47,6 +47,7 @@ char *default_bridge = NULL;
 char *default_gatewaydev = NULL;
 enum output_format default_output_format = OUTPUT_FORMAT_JSON;
 int claim_mode = 0;
+int expert_mode = 0;
 
 static xentoollog_level minmsglevel = XTL_PROGRESS;
 
@@ -173,6 +174,9 @@ static void parse_global_config(const char *configfile,
     if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
         claim_mode = l;
 
+    if (!xlu_cfg_get_long (config, "expert_mode", &l, 0))
+        expert_mode = l;
+
     xlu_cfg_destroy(config);
 }
 
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 5ad3e17..51b7008 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -147,6 +147,7 @@ extern int autoballoon;
 extern int run_hotplug_scripts;
 extern int dryrun_only;
 extern int claim_mode;
+extern int expert_mode;
 extern char *lockfile;
 extern char *default_vifscript;
 extern char *default_bridge;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8a478ba..a55c66a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4560,6 +4560,12 @@ int main_vcpuset(int argc, char **argv)
         break;
     }
 
+    /*
+     * No seatbelts for the user.
+     */
+    if (expert_mode)
+        check_host = 0;
+
     vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
     return 0;
 }
-- 
1.7.7.6

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] claim: By default enable it.
  2013-07-12 18:08 ` [PATCH 1/2] claim: By default enable it Konrad Rzeszutek Wilk
@ 2013-07-16  9:03   ` Ian Campbell
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2013-07-16  9:03 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: george.dunlap, xen-devel, ian.jackson

On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
> During the Xen 4.3 release we discussed that this feature could be
> turned on by default - as it benefits all of the guests - not just
> tmem related.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  docs/man/xl.conf.pod.5 |    2 +-
>  tools/examples/xl.conf |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
> index 1229c8a..125f786 100644
> --- a/docs/man/xl.conf.pod.5
> +++ b/docs/man/xl.conf.pod.5
> @@ -139,7 +139,7 @@ Xen hypervisor argument and as well on the Linux kernel command line.
>  Note that the claim call is not attempted if C<superpages> option is
>  used in the guest config (see xl.cfg(5)).
>  
> -Default: C<0>
> +Default: C<1>
>  
>  =over 4
>  
> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
> index 9c037a6..a80b8e7 100644
> --- a/tools/examples/xl.conf
> +++ b/tools/examples/xl.conf
> @@ -32,4 +32,4 @@
>  # feedback whether the guest can be launched due to memory exhaustion
>  # (which can take a long time to find out if launching huge guests).
>  # see xl.conf(5) for details.
> -#claim_mode=0
> +claim_mode=1

We should change the default in the C code and maker this a commented
out claim_mode=1.

Ian.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-12 18:08 ` [PATCH 2/2] expert_mode: Add a new configuration option for expert users Konrad Rzeszutek Wilk
@ 2013-07-16  9:04   ` Ian Campbell
  2013-07-16 15:21     ` Konrad Rzeszutek Wilk
  2013-07-17 10:56     ` Ian Jackson
  0 siblings, 2 replies; 12+ messages in thread
From: Ian Campbell @ 2013-07-16  9:04 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: george.dunlap, xen-devel, ian.jackson

On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
> This could also be called 'seatbelt' option.
> 
> libxl has a variety of checks where it will fail out an operation
> unless the user has provided an --force (or --ignore) parameter.
> Currently one such check is for the 'vcpu-set' command which
> will error out if the count of virtual cpus is greater than the
> physical cpus. This parameter will ignore such checks and allow
> the user to do the operations without the need for override flags.

Does this overlap  somewhat with various commands which individually
take a -f(orce) option?

> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  docs/man/xl.conf.pod.5   |   24 ++++++++++++++++++++++++
>  tools/examples/xl.conf   |    6 ++++++
>  tools/libxl/xl.c         |    4 ++++
>  tools/libxl/xl.h         |    1 +
>  tools/libxl/xl_cmdimpl.c |    6 ++++++
>  5 files changed, 41 insertions(+), 0 deletions(-)
> 
> diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
> index 125f786..ed67472 100644
> --- a/docs/man/xl.conf.pod.5
> +++ b/docs/man/xl.conf.pod.5
> @@ -158,6 +158,30 @@ massively huge guests).
>  
>  =back
>  
> +=item B<expert_mode=BOOLEAN>
> +
> +Do not act on host performed checks that might lead to performance
> +degradations. Currently checks are made for following operations:
> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
> +   physical count the operation will error out.
> +
> +
> +Default: C<1>
> +
> +=over 4
> +
> +=item C<0>
> +
> +The checks are active and the operation will fail if the checks
> +are triggered.
> +
> +=item C<1>
> +
> +The checks are active but will be ignored and the operations
> +will commence.
> +
> +=back
> +
>  =back
>  
>  =head1 SEE ALSO
> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
> index a80b8e7..6f27d67 100644
> --- a/tools/examples/xl.conf
> +++ b/tools/examples/xl.conf
> @@ -33,3 +33,9 @@
>  # (which can take a long time to find out if launching huge guests).
>  # see xl.conf(5) for details.
>  claim_mode=1
> +
> +# The user knows what to do. Currently enabling this option will mean that:
> +#  vcpu-set won't check the physical CPU count - which means the guest can
> +#           over-subscribe (more vCPUS than pCPUS).
> +# see xl.conf(5) for details.
> +expert_mode=1
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 1ce820c..4063b7c 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -47,6 +47,7 @@ char *default_bridge = NULL;
>  char *default_gatewaydev = NULL;
>  enum output_format default_output_format = OUTPUT_FORMAT_JSON;
>  int claim_mode = 0;
> +int expert_mode = 0;
>  
>  static xentoollog_level minmsglevel = XTL_PROGRESS;
>  
> @@ -173,6 +174,9 @@ static void parse_global_config(const char *configfile,
>      if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
>          claim_mode = l;
>  
> +    if (!xlu_cfg_get_long (config, "expert_mode", &l, 0))
> +        expert_mode = l;
> +
>      xlu_cfg_destroy(config);
>  }
>  
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 5ad3e17..51b7008 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -147,6 +147,7 @@ extern int autoballoon;
>  extern int run_hotplug_scripts;
>  extern int dryrun_only;
>  extern int claim_mode;
> +extern int expert_mode;
>  extern char *lockfile;
>  extern char *default_vifscript;
>  extern char *default_bridge;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a478ba..a55c66a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4560,6 +4560,12 @@ int main_vcpuset(int argc, char **argv)
>          break;
>      }
>  
> +    /*
> +     * No seatbelts for the user.
> +     */
> +    if (expert_mode)
> +        check_host = 0;
> +
>      vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
>      return 0;
>  }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-16  9:04   ` Ian Campbell
@ 2013-07-16 15:21     ` Konrad Rzeszutek Wilk
  2013-07-17 10:56     ` Ian Jackson
  1 sibling, 0 replies; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-16 15:21 UTC (permalink / raw)
  To: Ian Campbell; +Cc: george.dunlap, Konrad Rzeszutek Wilk, xen-devel, ian.jackson

On Tue, Jul 16, 2013 at 10:04:15AM +0100, Ian Campbell wrote:
> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
> > This could also be called 'seatbelt' option.
> > 
> > libxl has a variety of checks where it will fail out an operation
> > unless the user has provided an --force (or --ignore) parameter.
> > Currently one such check is for the 'vcpu-set' command which
> > will error out if the count of virtual cpus is greater than the
> > physical cpus. This parameter will ignore such checks and allow
> > the user to do the operations without the need for override flags.
> 
> Does this overlap  somewhat with various commands which individually
> take a -f(orce) option?

Not exactly. This particular issue was more of the variety of - we
add some checks that earlier version of toolstacks did not have. And
as such we fail to do the operation _unless_ a force is given.

But all of this kind of falls under a seatbelt function - if the user
knows what he or she is doing could be bad, well - if they know what
they are doing then they can go ahead.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-16  9:04   ` Ian Campbell
  2013-07-16 15:21     ` Konrad Rzeszutek Wilk
@ 2013-07-17 10:56     ` Ian Jackson
  2013-07-17 11:03       ` George Dunlap
  2013-07-17 17:17       ` Konrad Rzeszutek Wilk
  1 sibling, 2 replies; 12+ messages in thread
From: Ian Jackson @ 2013-07-17 10:56 UTC (permalink / raw)
  To: Ian Campbell; +Cc: george.dunlap, Konrad Rzeszutek Wilk, xen-devel

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):
> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
> > This could also be called 'seatbelt' option.
> > 
> > libxl has a variety of checks where it will fail out an operation
> > unless the user has provided an --force (or --ignore) parameter.
> > Currently one such check is for the 'vcpu-set' command which
> > will error out if the count of virtual cpus is greater than the
> > physical cpus. This parameter will ignore such checks and allow
> > the user to do the operations without the need for override flags.
> 
> Does this overlap  somewhat with various commands which individually
> take a -f(orce) option?

Clearly it should disable all of those -f's too.

> > +=item B<expert_mode=BOOLEAN>
> > +
> > +Do not act on host performed checks that might lead to performance
> > +degradations. Currently checks are made for following operations:
> > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
> > +   physical count the operation will error out.

I don't think this is a very coherent specification.  Surely it should
override "all -f options" or something similar.

I still don't see why you would want such a thing.

Ian.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-17 10:56     ` Ian Jackson
@ 2013-07-17 11:03       ` George Dunlap
  2013-07-17 11:12         ` Ian Campbell
  2013-07-17 17:17       ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 12+ messages in thread
From: George Dunlap @ 2013-07-17 11:03 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Konrad Rzeszutek Wilk, xen-devel, Ian Campbell

On 17/07/13 11:56, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):
>> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
>>> This could also be called 'seatbelt' option.
>>>
>>> libxl has a variety of checks where it will fail out an operation
>>> unless the user has provided an --force (or --ignore) parameter.
>>> Currently one such check is for the 'vcpu-set' command which
>>> will error out if the count of virtual cpus is greater than the
>>> physical cpus. This parameter will ignore such checks and allow
>>> the user to do the operations without the need for override flags.
>> Does this overlap  somewhat with various commands which individually
>> take a -f(orce) option?
> Clearly it should disable all of those -f's too.
>
>>> +=item B<expert_mode=BOOLEAN>
>>> +
>>> +Do not act on host performed checks that might lead to performance
>>> +degradations. Currently checks are made for following operations:
>>> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
>>> +   physical count the operation will error out.
> I don't think this is a very coherent specification.  Surely it should
> override "all -f options" or something similar.

I think that makes sense for some but not all.  For example, for "xl 
shutdown", -f means "send an ACPI event if no PV drivers are detected".  
I don't think we'd want to change that behavior with such a switch.  
(Arguably -f was the wrong option to use for that in the first place, 
but it's a bit late for that now.)

  -George.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-17 11:03       ` George Dunlap
@ 2013-07-17 11:12         ` Ian Campbell
  2013-07-17 11:13           ` George Dunlap
  0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2013-07-17 11:12 UTC (permalink / raw)
  To: George Dunlap; +Cc: Konrad Rzeszutek Wilk, xen-devel, Ian Jackson

On Wed, 2013-07-17 at 12:03 +0100, George Dunlap wrote:
> On 17/07/13 11:56, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):
> >> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
> >>> This could also be called 'seatbelt' option.
> >>>
> >>> libxl has a variety of checks where it will fail out an operation
> >>> unless the user has provided an --force (or --ignore) parameter.
> >>> Currently one such check is for the 'vcpu-set' command which
> >>> will error out if the count of virtual cpus is greater than the
> >>> physical cpus. This parameter will ignore such checks and allow
> >>> the user to do the operations without the need for override flags.
> >> Does this overlap  somewhat with various commands which individually
> >> take a -f(orce) option?
> > Clearly it should disable all of those -f's too.
> >
> >>> +=item B<expert_mode=BOOLEAN>
> >>> +
> >>> +Do not act on host performed checks that might lead to performance
> >>> +degradations. Currently checks are made for following operations:
> >>> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
> >>> +   physical count the operation will error out.
> > I don't think this is a very coherent specification.  Surely it should
> > override "all -f options" or something similar.
> 
> I think that makes sense for some but not all.  For example, for "xl 
> shutdown", -f means "send an ACPI event if no PV drivers are detected".  
> I don't think we'd want to change that behavior with such a switch.  
> (Arguably -f was the wrong option to use for that in the first place, 
> but it's a bit late for that now.)

That's -F (for fallback not force), deliberately avoiding -f for the
reasons you might think.

If "xl shutdown -f" were to do anything it would probably be to act as
"xl destroy" (nb I don't think we should actually do that...)

Ian.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-17 11:12         ` Ian Campbell
@ 2013-07-17 11:13           ` George Dunlap
  0 siblings, 0 replies; 12+ messages in thread
From: George Dunlap @ 2013-07-17 11:13 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Konrad Rzeszutek Wilk, xen-devel, Ian Jackson

On 17/07/13 12:12, Ian Campbell wrote:
> On Wed, 2013-07-17 at 12:03 +0100, George Dunlap wrote:
>> On 17/07/13 11:56, Ian Jackson wrote:
>>> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):
>>>> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
>>>>> This could also be called 'seatbelt' option.
>>>>>
>>>>> libxl has a variety of checks where it will fail out an operation
>>>>> unless the user has provided an --force (or --ignore) parameter.
>>>>> Currently one such check is for the 'vcpu-set' command which
>>>>> will error out if the count of virtual cpus is greater than the
>>>>> physical cpus. This parameter will ignore such checks and allow
>>>>> the user to do the operations without the need for override flags.
>>>> Does this overlap  somewhat with various commands which individually
>>>> take a -f(orce) option?
>>> Clearly it should disable all of those -f's too.
>>>
>>>>> +=item B<expert_mode=BOOLEAN>
>>>>> +
>>>>> +Do not act on host performed checks that might lead to performance
>>>>> +degradations. Currently checks are made for following operations:
>>>>> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
>>>>> +   physical count the operation will error out.
>>> I don't think this is a very coherent specification.  Surely it should
>>> override "all -f options" or something similar.
>> I think that makes sense for some but not all.  For example, for "xl
>> shutdown", -f means "send an ACPI event if no PV drivers are detected".
>> I don't think we'd want to change that behavior with such a switch.
>> (Arguably -f was the wrong option to use for that in the first place,
>> but it's a bit late for that now.)
> That's -F (for fallback not force), deliberately avoiding -f for the
> reasons you might think.
>
> If "xl shutdown -f" were to do anything it would probably be to act as
> "xl destroy" (nb I don't think we should actually do that...)

Ah, right.  That makes sense.

  -G

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-17 10:56     ` Ian Jackson
  2013-07-17 11:03       ` George Dunlap
@ 2013-07-17 17:17       ` Konrad Rzeszutek Wilk
  2013-07-18 11:08         ` George Dunlap
  1 sibling, 1 reply; 12+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-07-17 17:17 UTC (permalink / raw)
  To: Ian Jackson; +Cc: george.dunlap, Konrad Rzeszutek Wilk, xen-devel, Ian Campbell

On Wed, Jul 17, 2013 at 11:56:24AM +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):
> > On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
> > > This could also be called 'seatbelt' option.
> > > 
> > > libxl has a variety of checks where it will fail out an operation
> > > unless the user has provided an --force (or --ignore) parameter.
> > > Currently one such check is for the 'vcpu-set' command which
> > > will error out if the count of virtual cpus is greater than the
> > > physical cpus. This parameter will ignore such checks and allow
> > > the user to do the operations without the need for override flags.
> > 
> > Does this overlap  somewhat with various commands which individually
> > take a -f(orce) option?
> 
> Clearly it should disable all of those -f's too.
> 
> > > +=item B<expert_mode=BOOLEAN>
> > > +
> > > +Do not act on host performed checks that might lead to performance
> > > +degradations. Currently checks are made for following operations:
> > > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
> > > +   physical count the operation will error out.
> 
> I don't think this is a very coherent specification.  Surely it should
> override "all -f options" or something similar.

Sure, I hadn't looked at the other ones.
> 
> I still don't see why you would want such a thing.

We discussed it during Xen 4.3 release that certain changes, like this one
are inconsistent (for example you can launch an guest with more vCPUs
than pCPU, but you if you lower the amount of vCPUs you can't increase
it until you use -f flag). 

But you could consider such inconsistent behavior to be a failsafe
mechanism so that the user does not do something silly. But if they
are an expert... well

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
  2013-07-17 17:17       ` Konrad Rzeszutek Wilk
@ 2013-07-18 11:08         ` George Dunlap
  0 siblings, 0 replies; 12+ messages in thread
From: George Dunlap @ 2013-07-18 11:08 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Konrad Rzeszutek Wilk, xen-devel, Ian Jackson, Ian Campbell

On Wed, Jul 17, 2013 at 6:17 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Jul 17, 2013 at 11:56:24AM +0100, Ian Jackson wrote:
>> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):
>> > On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:
>> > > This could also be called 'seatbelt' option.
>> > >
>> > > libxl has a variety of checks where it will fail out an operation
>> > > unless the user has provided an --force (or --ignore) parameter.
>> > > Currently one such check is for the 'vcpu-set' command which
>> > > will error out if the count of virtual cpus is greater than the
>> > > physical cpus. This parameter will ignore such checks and allow
>> > > the user to do the operations without the need for override flags.
>> >
>> > Does this overlap  somewhat with various commands which individually
>> > take a -f(orce) option?
>>
>> Clearly it should disable all of those -f's too.
>>
>> > > +=item B<expert_mode=BOOLEAN>
>> > > +
>> > > +Do not act on host performed checks that might lead to performance
>> > > +degradations. Currently checks are made for following operations:
>> > > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the
>> > > +   physical count the operation will error out.
>>
>> I don't think this is a very coherent specification.  Surely it should
>> override "all -f options" or something similar.
>
> Sure, I hadn't looked at the other ones.
>>
>> I still don't see why you would want such a thing.
>
> We discussed it during Xen 4.3 release that certain changes, like this one
> are inconsistent (for example you can launch an guest with more vCPUs
> than pCPU, but you if you lower the amount of vCPUs you can't increase
> it until you use -f flag).
>
> But you could consider such inconsistent behavior to be a failsafe
> mechanism so that the user does not do something silly. But if they
> are an expert... well

Is it really such a burden for "experts" to type '-f' occasionally
when doing strange configurations?

The problem with this in general is one of unpredictability.  You may
want to disable this check by default, but do you really want to
disable *all* checks?  That doesn't sound like a good idea... but then
if not, which checks *are* disabled?  It's not clear from the name,
and it's likely that we'll want to add or remove things, which means
having safety checks suddenly disappear... it's a UI nightmare.

I suppose I could accept a "force_all" option.  You'd have to be
basically daft to use it outside a testing environment, but it would
certainly be simple to define.  I still mostly think typing '-f' is a
better option.

 -George

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-07-18 11:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-12 18:08 [PATCH] xl.conf defaults changes for Xen 4.4. (v1) Konrad Rzeszutek Wilk
2013-07-12 18:08 ` [PATCH 1/2] claim: By default enable it Konrad Rzeszutek Wilk
2013-07-16  9:03   ` Ian Campbell
2013-07-12 18:08 ` [PATCH 2/2] expert_mode: Add a new configuration option for expert users Konrad Rzeszutek Wilk
2013-07-16  9:04   ` Ian Campbell
2013-07-16 15:21     ` Konrad Rzeszutek Wilk
2013-07-17 10:56     ` Ian Jackson
2013-07-17 11:03       ` George Dunlap
2013-07-17 11:12         ` Ian Campbell
2013-07-17 11:13           ` George Dunlap
2013-07-17 17:17       ` Konrad Rzeszutek Wilk
2013-07-18 11:08         ` George Dunlap

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.