Now that we have the auto-fill code in place, and with proper documentation
to let the user know that (1) we will auto-fill the NUMA cpus up to the
number to maximum VCPUs number if QEMU supports it and (2) the user
is advised to always supply a complete NUMA topology, this warning
is unneeded.

This reverts commit 38d2e033686b5cc274f8f55075ce1985b71e329a.

CC: Maxiwell S. Garcia <maxiwell@linux.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/qemu/qemu_validate.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 584d1375b8..14d614934d 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -895,24 +895,15 @@ qemuValidateDomainDef(const virDomainDef *def,
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
         unsigned int topologycpus;
         unsigned int granularity;
-        unsigned int numacpus;
 
         /* Starting from QEMU 2.5, max vCPU count and overall vCPU topology
          * must agree. We only actually enforce this with QEMU 2.7+, due
          * to the capability check above */
-        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0) {
-            if (topologycpus != virDomainDefGetVcpusMax(def)) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                               _("CPU topology doesn't match maximum vcpu count"));
-                return -1;
-            }
-
-            numacpus = virDomainNumaGetCPUCountTotal(def->numa);
-            if ((numacpus != 0) && (topologycpus != numacpus)) {
-                VIR_WARN("CPU topology doesn't match numa CPU count; "
-                         "partial NUMA mapping is obsoleted and will "
-                         "be removed in future");
-            }
+        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
+            topologycpus != virDomainDefGetVcpusMax(def)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("CPU topology doesn't match maximum vcpu count"));
+            return -1;
         }
 
         /* vCPU hotplug granularity must be respected */
-- 
2.26.2

On Mon, Jun 01, 2020 at 14:50:41 -0300, Daniel Henrique Barboza wrote:
> Now that we have the auto-fill code in place, and with proper documentation
> to let the user know that (1) we will auto-fill the NUMA cpus up to the
> number to maximum VCPUs number if QEMU supports it and (2) the user
> is advised to always supply a complete NUMA topology, this warning
> is unneeded.
> 
> This reverts commit 38d2e033686b5cc274f8f55075ce1985b71e329a.

Since we already have the validation in place for some time now I think
we should just keep it. The auto-filling would be a useful hack to work
around if config breaks, but judged by itself it's of questionable
benefit.

Specifically users might end up with a topology which they didn't
expect. Reasoning is basically the same as with qemu. Any default
behaviour here is a policy decision and it might not suit all uses.

On 6/1/20 4:40 PM, Peter Krempa wrote:
> On Mon, Jun 01, 2020 at 14:50:41 -0300, Daniel Henrique Barboza wrote:
>> Now that we have the auto-fill code in place, and with proper documentation
>> to let the user know that (1) we will auto-fill the NUMA cpus up to the
>> number to maximum VCPUs number if QEMU supports it and (2) the user
>> is advised to always supply a complete NUMA topology, this warning
>> is unneeded.
>>
>> This reverts commit 38d2e033686b5cc274f8f55075ce1985b71e329a.
> 
> Since we already have the validation in place for some time now I think
> we should just keep it. The auto-filling would be a useful hack to work
> around if config breaks, but judged by itself it's of questionable
> benefit.

That's a good point. I agree that removing the message after being in place
for this long is more trouble than it's worth.

> 
> Specifically users might end up with a topology which they didn't
> expect. Reasoning is basically the same as with qemu. Any default
> behaviour here is a policy decision and it might not suit all uses.
>

  
An ideal situation would be QEMU to never accept incomplete NUMA topologies
in the first place.

Given that this wasn't the case and now there might be a plethora of guests
running with goofy topologies all around, the already existing warning
message + this auto-fill hack + documentation mentioning that users should
avoid these topologies is a fine solution from Libvirt side, in my
estimation.


Thanks,


DHB

On Mon, 1 Jun 2020 17:14:20 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 6/1/20 4:40 PM, Peter Krempa wrote:
> > On Mon, Jun 01, 2020 at 14:50:41 -0300, Daniel Henrique Barboza wrote:  
> >> Now that we have the auto-fill code in place, and with proper documentation
> >> to let the user know that (1) we will auto-fill the NUMA cpus up to the
> >> number to maximum VCPUs number if QEMU supports it and (2) the user
> >> is advised to always supply a complete NUMA topology, this warning
> >> is unneeded.
> >>
> >> This reverts commit 38d2e033686b5cc274f8f55075ce1985b71e329a.  
> > 
> > Since we already have the validation in place for some time now I think
> > we should just keep it. The auto-filling would be a useful hack to work
> > around if config breaks, but judged by itself it's of questionable
> > benefit.  
> 
> That's a good point. I agree that removing the message after being in place
> for this long is more trouble than it's worth.
> 
> > 
> > Specifically users might end up with a topology which they didn't
> > expect. Reasoning is basically the same as with qemu. Any default
> > behaviour here is a policy decision and it might not suit all uses.
> >  
> 
>   
> An ideal situation would be QEMU to never accept incomplete NUMA topologies
> in the first place.
At least with your series I can safely drop deprecated incomplete NUMA topologies
on QEMU side (which were producing warnings for a while)

> 
> Given that this wasn't the case and now there might be a plethora of guests
> running with goofy topologies all around, the already existing warning
> message + this auto-fill hack + documentation mentioning that users should
> avoid these topologies is a fine solution from Libvirt side, in my
> estimation.
> 
> 
> Thanks,
> 
> 
> DHB
> 

On 6/2/20 5:53 AM, Igor Mammedov wrote:
> On Mon, 1 Jun 2020 17:14:20 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>

  
[...]

>> An ideal situation would be QEMU to never accept incomplete NUMA topologies
>> in the first place.
> At least with your series I can safely drop deprecated incomplete NUMA topologies
> on QEMU side (which were producing warnings for a while)

Keep in mind that I'm not enabling this auto-fill feature for all guests across
the board. The code is relying on the availability of QEMU_CAPS_NUMA.

Granted, there are not many 6 year old guests running Libvirt 1.2.6/QEMU 2.2.0
around, and I'm not even sure if we care about supporting this scenario, but I
feel obliged to mention this for the record :)


Thanks,


DHB