Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[John Ferlan]

On 12/12/18 9:32 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 12.12.2018 16:28, John Ferlan wrote:
>>
>>
>> On 12/12/18 3:04 AM, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 11.12.2018 17:33, John Ferlan wrote:


 On 12/11/18 2:34 AM, Nikolay Shirokovskiy wrote:
>
>
> On 11.12.2018 01:05, John Ferlan wrote:
>>
>> $SUBJ:
>>
>> 'storage sources'
>>
>> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
>>> Every time we call all domain stats for inactive domain with
>>> unavailable source we get error message in logs. It's a bit noisy.
>>
>> Are there ones in particular?
>
> Like this one:
>
> qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No 
> such file or directory
>

 So this is the one that perhaps is bothersome to the degree of we're
 ignoring that it doesn't exist, but then again the domain is not active,
 so does it really matter.

>>>
>>> I would prefer not to see false positive messages in logs if it is possible.
>>>
>>
>>> While it's arguable whether we need such message or not for mandatory
>>> disks we would like not to see messages for optional disks. Let's
>>
>> Filtering for the 'startupPolicy = optional' for domain stats (with
>> empty/unavailable local source) would seem to be a "new use" perhaps not
>> totally in line with the original intention.
>
> But I was not going to change behaviour only to stop polluting
> logs with messages like above.
>

 Your solution requires that someone has modified their definition to
 include that startupPolicy='optional' attribute. I can be persuaded that
 this is desirable; however, it would be nice to get Peter's opinion on
 this especially since he knows the code and "rules" for backing stores
 better than I do. IOW: From a storage backing chain perspective does it
 make sense to use that.
>>>
>>> Not presicely, rather if someone already has optional disk then why logging
>>> this kind of errors.
>
>>
>>> filter at least for cases of local files. Fixing other cases would
>>> require passing flag down the stack to .backendInit of storage
>>> which is ugly.
>>
>> Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
>> from qemuDomainStorageOpenStat for
>>
>>virStorageFileBackendForType:145 : internal error: missing storage
>> backend for 'none' storage
>>virStorageFileBackendForType:141 : internal error: missing storage
>> backend for network files using iscsi protocol
>>
>> where the 'none' comes from a volume using a storage pool of iSCSI
>> disks. I chased for a bit, but yes it got messy fast.
>>
>>>
>>> Stats for active domain are fine because we either drop disks
>>> with unavailable sources or clean source which is handled
>>> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
>>>
>>> We have these logs for successful stats since 25aa7035d which
>>> in turn fixes 596a13713 which added substantial stats for offline
>>> disks.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy 
>>> ---
>>>  src/qemu/qemu_driver.c  | 5 +
>>>  src/qemu/qemu_process.c | 9 +
>>>  src/qemu/qemu_process.h | 2 ++
>>>  3 files changed, 16 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index a52e249..72e4cfe 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -20290,6 +20290,11 @@ 
>>> qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>>>  const char *backendstoragealias;
>>>  int ret = -1;
>>>  
>>> +if (!virDomainObjIsActive(dom) &&
>>> +qemuProcessMissingLocalOptionalDisk(disk))
>>> +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
>>> *recordnr,
>>> +   records, nrecords);
>>> +
>>
>> From my quick read this part would seem reasonable since the *Frontend
>> and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
>> source sizing data which for an empty source would be unobtainable.
>>
>>>  for (n = disk->src; virStorageSourceIsBacking(n); n = 
>>> n->backingStore) {
>>>  if (blockdev) {
>>>  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 9cf9718..802274e 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr 
>>> vm)
>>>  }
>>>  
>>>  
>>> +bool
>>> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
>>
>> Curious why you chose qemu_process and not qemu_domain or even

Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[Nikolay Shirokovskiy]

On 12.12.2018 16:28, John Ferlan wrote:
> 
> 
> On 12/12/18 3:04 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 11.12.2018 17:33, John Ferlan wrote:
>>>
>>>
>>> On 12/11/18 2:34 AM, Nikolay Shirokovskiy wrote:


 On 11.12.2018 01:05, John Ferlan wrote:
>
> $SUBJ:
>
> 'storage sources'
>
> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
>> Every time we call all domain stats for inactive domain with
>> unavailable source we get error message in logs. It's a bit noisy.
>
> Are there ones in particular?

 Like this one:

 qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No such 
 file or directory

>>>
>>> So this is the one that perhaps is bothersome to the degree of we're
>>> ignoring that it doesn't exist, but then again the domain is not active,
>>> so does it really matter.
>>>
>>
>> I would prefer not to see false positive messages in logs if it is possible.
>>
>
>> While it's arguable whether we need such message or not for mandatory
>> disks we would like not to see messages for optional disks. Let's
>
> Filtering for the 'startupPolicy = optional' for domain stats (with
> empty/unavailable local source) would seem to be a "new use" perhaps not
> totally in line with the original intention.

 But I was not going to change behaviour only to stop polluting
 logs with messages like above.

>>>
>>> Your solution requires that someone has modified their definition to
>>> include that startupPolicy='optional' attribute. I can be persuaded that
>>> this is desirable; however, it would be nice to get Peter's opinion on
>>> this especially since he knows the code and "rules" for backing stores
>>> better than I do. IOW: From a storage backing chain perspective does it
>>> make sense to use that.
>>
>> Not presicely, rather if someone already has optional disk then why logging
>> this kind of errors.

>
>> filter at least for cases of local files. Fixing other cases would
>> require passing flag down the stack to .backendInit of storage
>> which is ugly.
>
> Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
> from qemuDomainStorageOpenStat for
>
>virStorageFileBackendForType:145 : internal error: missing storage
> backend for 'none' storage
>virStorageFileBackendForType:141 : internal error: missing storage
> backend for network files using iscsi protocol
>
> where the 'none' comes from a volume using a storage pool of iSCSI
> disks. I chased for a bit, but yes it got messy fast.
>
>>
>> Stats for active domain are fine because we either drop disks
>> with unavailable sources or clean source which is handled
>> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
>>
>> We have these logs for successful stats since 25aa7035d which
>> in turn fixes 596a13713 which added substantial stats for offline
>> disks.
>>
>> Signed-off-by: Nikolay Shirokovskiy 
>> ---
>>  src/qemu/qemu_driver.c  | 5 +
>>  src/qemu/qemu_process.c | 9 +
>>  src/qemu/qemu_process.h | 2 ++
>>  3 files changed, 16 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a52e249..72e4cfe 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -20290,6 +20290,11 @@ 
>> qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>>  const char *backendstoragealias;
>>  int ret = -1;
>>  
>> +if (!virDomainObjIsActive(dom) &&
>> +qemuProcessMissingLocalOptionalDisk(disk))
>> +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
>> *recordnr,
>> +   records, nrecords);
>> +
>
> From my quick read this part would seem reasonable since the *Frontend
> and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
> source sizing data which for an empty source would be unobtainable.
>
>>  for (n = disk->src; virStorageSourceIsBacking(n); n = 
>> n->backingStore) {
>>  if (blockdev) {
>>  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 9cf9718..802274e 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr 
>> vm)
>>  }
>>  
>>  
>> +bool
>> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
>
> Curious why you chose qemu_process and not qemu_domain or even
> virstoragefile?  This has nothing to do with whether the qemu process is
> running or not.

 Yeah, we have nothing qemu specific here. Just not sure is this useful

Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[John Ferlan]

On 12/12/18 3:04 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 11.12.2018 17:33, John Ferlan wrote:
>>
>>
>> On 12/11/18 2:34 AM, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 11.12.2018 01:05, John Ferlan wrote:

 $SUBJ:

 'storage sources'

 On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
> Every time we call all domain stats for inactive domain with
> unavailable source we get error message in logs. It's a bit noisy.

 Are there ones in particular?
>>>
>>> Like this one:
>>>
>>> qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No such 
>>> file or directory
>>>
>>
>> So this is the one that perhaps is bothersome to the degree of we're
>> ignoring that it doesn't exist, but then again the domain is not active,
>> so does it really matter.
>>
> 
> I would prefer not to see false positive messages in logs if it is possible.
> 

> While it's arguable whether we need such message or not for mandatory
> disks we would like not to see messages for optional disks. Let's

 Filtering for the 'startupPolicy = optional' for domain stats (with
 empty/unavailable local source) would seem to be a "new use" perhaps not
 totally in line with the original intention.
>>>
>>> But I was not going to change behaviour only to stop polluting
>>> logs with messages like above.
>>>
>>
>> Your solution requires that someone has modified their definition to
>> include that startupPolicy='optional' attribute. I can be persuaded that
>> this is desirable; however, it would be nice to get Peter's opinion on
>> this especially since he knows the code and "rules" for backing stores
>> better than I do. IOW: From a storage backing chain perspective does it
>> make sense to use that.
> 
> Not presicely, rather if someone already has optional disk then why logging
> this kind of errors.
> >>

> filter at least for cases of local files. Fixing other cases would
> require passing flag down the stack to .backendInit of storage
> which is ugly.

 Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
 from qemuDomainStorageOpenStat for

virStorageFileBackendForType:145 : internal error: missing storage
 backend for 'none' storage
virStorageFileBackendForType:141 : internal error: missing storage
 backend for network files using iscsi protocol

 where the 'none' comes from a volume using a storage pool of iSCSI
 disks. I chased for a bit, but yes it got messy fast.

>
> Stats for active domain are fine because we either drop disks
> with unavailable sources or clean source which is handled
> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
>
> We have these logs for successful stats since 25aa7035d which
> in turn fixes 596a13713 which added substantial stats for offline
> disks.
>
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_driver.c  | 5 +
>  src/qemu/qemu_process.c | 9 +
>  src/qemu/qemu_process.h | 2 ++
>  3 files changed, 16 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a52e249..72e4cfe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20290,6 +20290,11 @@ 
> qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>  const char *backendstoragealias;
>  int ret = -1;
>  
> +if (!virDomainObjIsActive(dom) &&
> +qemuProcessMissingLocalOptionalDisk(disk))
> +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
> *recordnr,
> +   records, nrecords);
> +

 From my quick read this part would seem reasonable since the *Frontend
 and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
 source sizing data which for an empty source would be unobtainable.

>  for (n = disk->src; virStorageSourceIsBacking(n); n = 
> n->backingStore) {
>  if (blockdev) {
>  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 9cf9718..802274e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
>  }
>  
>  
> +bool
> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)

 Curious why you chose qemu_process and not qemu_domain or even
 virstoragefile?  This has nothing to do with whether the qemu process is
 running or not.
>>>
>>> Yeah, we have nothing qemu specific here. Just not sure is this useful
>>> for other purpuses or not.
>>>
>>
>> Since your chosen filter is "domain startup policy", I think qemu_domain
>> is where it belongs.
>>

> +{
> +return 

Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[Nikolay Shirokovskiy]

On 11.12.2018 17:33, John Ferlan wrote:
> 
> 
> On 12/11/18 2:34 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 11.12.2018 01:05, John Ferlan wrote:
>>>
>>> $SUBJ:
>>>
>>> 'storage sources'
>>>
>>> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
 Every time we call all domain stats for inactive domain with
 unavailable source we get error message in logs. It's a bit noisy.
>>>
>>> Are there ones in particular?
>>
>> Like this one:
>>
>> qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No such 
>> file or directory
>>
> 
> So this is the one that perhaps is bothersome to the degree of we're
> ignoring that it doesn't exist, but then again the domain is not active,
> so does it really matter.
> 

I would prefer not to see false positive messages in logs if it is possible.

>>>
 While it's arguable whether we need such message or not for mandatory
 disks we would like not to see messages for optional disks. Let's
>>>
>>> Filtering for the 'startupPolicy = optional' for domain stats (with
>>> empty/unavailable local source) would seem to be a "new use" perhaps not
>>> totally in line with the original intention.
>>
>> But I was not going to change behaviour only to stop polluting
>> logs with messages like above.
>>
> 
> Your solution requires that someone has modified their definition to
> include that startupPolicy='optional' attribute. I can be persuaded that
> this is desirable; however, it would be nice to get Peter's opinion on
> this especially since he knows the code and "rules" for backing stores
> better than I do. IOW: From a storage backing chain perspective does it
> make sense to use that.

Not presicely, rather if someone already has optional disk then why logging
this kind of errors.

> 
>>>
 filter at least for cases of local files. Fixing other cases would
 require passing flag down the stack to .backendInit of storage
 which is ugly.
>>>
>>> Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
>>> from qemuDomainStorageOpenStat for
>>>
>>>virStorageFileBackendForType:145 : internal error: missing storage
>>> backend for 'none' storage
>>>virStorageFileBackendForType:141 : internal error: missing storage
>>> backend for network files using iscsi protocol
>>>
>>> where the 'none' comes from a volume using a storage pool of iSCSI
>>> disks. I chased for a bit, but yes it got messy fast.
>>>

 Stats for active domain are fine because we either drop disks
 with unavailable sources or clean source which is handled
 by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.

 We have these logs for successful stats since 25aa7035d which
 in turn fixes 596a13713 which added substantial stats for offline
 disks.

 Signed-off-by: Nikolay Shirokovskiy 
 ---
  src/qemu/qemu_driver.c  | 5 +
  src/qemu/qemu_process.c | 9 +
  src/qemu/qemu_process.h | 2 ++
  3 files changed, 16 insertions(+)

 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index a52e249..72e4cfe 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -20290,6 +20290,11 @@ 
 qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
  const char *backendstoragealias;
  int ret = -1;
  
 +if (!virDomainObjIsActive(dom) &&
 +qemuProcessMissingLocalOptionalDisk(disk))
 +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
 *recordnr,
 +   records, nrecords);
 +
>>>
>>> From my quick read this part would seem reasonable since the *Frontend
>>> and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
>>> source sizing data which for an empty source would be unobtainable.
>>>
  for (n = disk->src; virStorageSourceIsBacking(n); n = 
 n->backingStore) {
  if (blockdev) {
  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 9cf9718..802274e 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
  }
  
  
 +bool
 +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
>>>
>>> Curious why you chose qemu_process and not qemu_domain or even
>>> virstoragefile?  This has nothing to do with whether the qemu process is
>>> running or not.
>>
>> Yeah, we have nothing qemu specific here. Just not sure is this useful
>> for other purpuses or not.
>>
> 
> Since your chosen filter is "domain startup policy", I think qemu_domain
> is where it belongs.
> 
>>>
 +{
 +return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&
>>>
>>> While I agree in principle about optional disks; however, since
>>> startupPolicy is optional it would seem this particular 

Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[John Ferlan]

On 12/11/18 2:34 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 11.12.2018 01:05, John Ferlan wrote:
>>
>> $SUBJ:
>>
>> 'storage sources'
>>
>> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
>>> Every time we call all domain stats for inactive domain with
>>> unavailable source we get error message in logs. It's a bit noisy.
>>
>> Are there ones in particular?
> 
> Like this one:
> 
> qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No such 
> file or directory
> 

So this is the one that perhaps is bothersome to the degree of we're
ignoring that it doesn't exist, but then again the domain is not active,
so does it really matter.

>>
>>> While it's arguable whether we need such message or not for mandatory
>>> disks we would like not to see messages for optional disks. Let's
>>
>> Filtering for the 'startupPolicy = optional' for domain stats (with
>> empty/unavailable local source) would seem to be a "new use" perhaps not
>> totally in line with the original intention.
> 
> But I was not going to change behaviour only to stop polluting
> logs with messages like above.
> 

Your solution requires that someone has modified their definition to
include that startupPolicy='optional' attribute. I can be persuaded that
this is desirable; however, it would be nice to get Peter's opinion on
this especially since he knows the code and "rules" for backing stores
better than I do. IOW: From a storage backing chain perspective does it
make sense to use that.

>>
>>> filter at least for cases of local files. Fixing other cases would
>>> require passing flag down the stack to .backendInit of storage
>>> which is ugly.
>>
>> Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
>> from qemuDomainStorageOpenStat for
>>
>>virStorageFileBackendForType:145 : internal error: missing storage
>> backend for 'none' storage
>>virStorageFileBackendForType:141 : internal error: missing storage
>> backend for network files using iscsi protocol
>>
>> where the 'none' comes from a volume using a storage pool of iSCSI
>> disks. I chased for a bit, but yes it got messy fast.
>>
>>>
>>> Stats for active domain are fine because we either drop disks
>>> with unavailable sources or clean source which is handled
>>> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
>>>
>>> We have these logs for successful stats since 25aa7035d which
>>> in turn fixes 596a13713 which added substantial stats for offline
>>> disks.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy 
>>> ---
>>>  src/qemu/qemu_driver.c  | 5 +
>>>  src/qemu/qemu_process.c | 9 +
>>>  src/qemu/qemu_process.h | 2 ++
>>>  3 files changed, 16 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index a52e249..72e4cfe 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -20290,6 +20290,11 @@ 
>>> qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>>>  const char *backendstoragealias;
>>>  int ret = -1;
>>>  
>>> +if (!virDomainObjIsActive(dom) &&
>>> +qemuProcessMissingLocalOptionalDisk(disk))
>>> +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
>>> *recordnr,
>>> +   records, nrecords);
>>> +
>>
>> From my quick read this part would seem reasonable since the *Frontend
>> and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
>> source sizing data which for an empty source would be unobtainable.
>>
>>>  for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) 
>>> {
>>>  if (blockdev) {
>>>  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index 9cf9718..802274e 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
>>>  }
>>>  
>>>  
>>> +bool
>>> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
>>
>> Curious why you chose qemu_process and not qemu_domain or even
>> virstoragefile?  This has nothing to do with whether the qemu process is
>> running or not.
> 
> Yeah, we have nothing qemu specific here. Just not sure is this useful
> for other purpuses or not.
> 

Since your chosen filter is "domain startup policy", I think qemu_domain
is where it belongs.

>>
>>> +{
>>> +return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&
>>
>> While I agree in principle about optional disks; however, since
>> startupPolicy is optional it would seem this particular check is chasing
>> a very specific condition. Would it be reasonable to use a flag instead?
>> Something passed on the command line to essentially say to only collect
>> the name/format the name of the local empty sources.
> 
> Not sure I understand you. This patch does not change anything in respect
> to collected stats so I can not see why we need extra 

Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[Nikolay Shirokovskiy]

On 11.12.2018 01:05, John Ferlan wrote:
> 
> $SUBJ:
> 
> 'storage sources'
> 
> On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
>> Every time we call all domain stats for inactive domain with
>> unavailable source we get error message in logs. It's a bit noisy.
> 
> Are there ones in particular?

Like this one:

qemuOpenFileAs:3324 : Failed to open file '/path/to/optinal/disk': No such file 
or directory

> 
>> While it's arguable whether we need such message or not for mandatory
>> disks we would like not to see messages for optional disks. Let's
> 
> Filtering for the 'startupPolicy = optional' for domain stats (with
> empty/unavailable local source) would seem to be a "new use" perhaps not
> totally in line with the original intention.

But I was not going to change behaviour only to stop polluting
logs with messages like above.

> 
>> filter at least for cases of local files. Fixing other cases would
>> require passing flag down the stack to .backendInit of storage
>> which is ugly.
> 
> Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
> from qemuDomainStorageOpenStat for
> 
>virStorageFileBackendForType:145 : internal error: missing storage
> backend for 'none' storage
>virStorageFileBackendForType:141 : internal error: missing storage
> backend for network files using iscsi protocol
> 
> where the 'none' comes from a volume using a storage pool of iSCSI
> disks. I chased for a bit, but yes it got messy fast.
> 
>>
>> Stats for active domain are fine because we either drop disks
>> with unavailable sources or clean source which is handled
>> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
>>
>> We have these logs for successful stats since 25aa7035d which
>> in turn fixes 596a13713 which added substantial stats for offline
>> disks.
>>
>> Signed-off-by: Nikolay Shirokovskiy 
>> ---
>>  src/qemu/qemu_driver.c  | 5 +
>>  src/qemu/qemu_process.c | 9 +
>>  src/qemu/qemu_process.h | 2 ++
>>  3 files changed, 16 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a52e249..72e4cfe 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -20290,6 +20290,11 @@ 
>> qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>>  const char *backendstoragealias;
>>  int ret = -1;
>>  
>> +if (!virDomainObjIsActive(dom) &&
>> +qemuProcessMissingLocalOptionalDisk(disk))
>> +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
>> *recordnr,
>> +   records, nrecords);
>> +
> 
> From my quick read this part would seem reasonable since the *Frontend
> and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
> source sizing data which for an empty source would be unobtainable.
> 
>>  for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
>>  if (blockdev) {
>>  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 9cf9718..802274e 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
>>  }
>>  
>>  
>> +bool
>> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
> 
> Curious why you chose qemu_process and not qemu_domain or even
> virstoragefile?  This has nothing to do with whether the qemu process is
> running or not.

Yeah, we have nothing qemu specific here. Just not sure is this useful
for other purpuses or not.

> 
>> +{
>> +return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&
> 
> While I agree in principle about optional disks; however, since
> startupPolicy is optional it would seem this particular check is chasing
> a very specific condition. Would it be reasonable to use a flag instead?
> Something passed on the command line to essentially say to only collect
> the name/format the name of the local empty sources.

Not sure I understand you. This patch does not change anything in respect
to collected stats so I can not see why we need extra flags.

> 
> That way we're not reusing something that has other uses. Although I'm
> open to other opinions.
> 
>> +   virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&
> 
> Use virStorageSourceIsEmpty instead.

But this is a bit different, say I use !virStorageSourceIsEmpty(src) instead of 
the line
above, then I can not sure disk->src->path even makes sense in the below check.

> 
>> +   !virFileExists(disk->src->path);



> 
> And while I believe I understand why you chose this, it would seem to me
> that it might be useful to know if an optional local disk had a path
> disappear.  IOW: If all the other conditions are met, I'd like to know
> that the source path is missing. That might be a good thing to know if
> I'm getting stats for a domain. Maybe that's just me.

Yeah, its 

Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[John Ferlan]

$SUBJ:

'storage sources'

On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote:
> Every time we call all domain stats for inactive domain with
> unavailable source we get error message in logs. It's a bit noisy.

Are there ones in particular?

> While it's arguable whether we need such message or not for mandatory
> disks we would like not to see messages for optional disks. Let's

Filtering for the 'startupPolicy = optional' for domain stats (with
empty/unavailable local source) would seem to be a "new use" perhaps not
totally in line with the original intention.

> filter at least for cases of local files. Fixing other cases would
> require passing flag down the stack to .backendInit of storage
> which is ugly.

Yeah, I remember chasing down into backendInit (virStorageFileInitAs)
from qemuDomainStorageOpenStat for

   virStorageFileBackendForType:145 : internal error: missing storage
backend for 'none' storage
   virStorageFileBackendForType:141 : internal error: missing storage
backend for network files using iscsi protocol

where the 'none' comes from a volume using a storage pool of iSCSI
disks. I chased for a bit, but yes it got messy fast.

> 
> Stats for active domain are fine because we either drop disks
> with unavailable sources or clean source which is handled
> by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.
> 
> We have these logs for successful stats since 25aa7035d which
> in turn fixes 596a13713 which added substantial stats for offline
> disks.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_driver.c  | 5 +
>  src/qemu/qemu_process.c | 9 +
>  src/qemu/qemu_process.h | 2 ++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a52e249..72e4cfe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20290,6 +20290,11 @@ 
> qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr disk,
>  const char *backendstoragealias;
>  int ret = -1;
>  
> +if (!virDomainObjIsActive(dom) &&
> +qemuProcessMissingLocalOptionalDisk(disk))
> +return qemuDomainGetStatsBlockExportHeader(disk, disk->src, 
> *recordnr,
> +   records, nrecords);
> +

>From my quick read this part would seem reasonable since the *Frontend
and *Backend wouldn't have valid data and *GetStatsOneBlock is fetching
source sizing data which for an empty source would be unobtainable.

>  for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
>  if (blockdev) {
>  frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 9cf9718..802274e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
>  }
>  
>  
> +bool
> +qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)

Curious why you chose qemu_process and not qemu_domain or even
virstoragefile?  This has nothing to do with whether the qemu process is
running or not.

> +{
> +return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&

While I agree in principle about optional disks; however, since
startupPolicy is optional it would seem this particular check is chasing
a very specific condition. Would it be reasonable to use a flag instead?
Something passed on the command line to essentially say to only collect
the name/format the name of the local empty sources.

That way we're not reusing something that has other uses. Although I'm
open to other opinions.

> +   virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&

Use virStorageSourceIsEmpty instead.

> +   !virFileExists(disk->src->path);

And while I believe I understand why you chose this, it would seem to me
that it might be useful to know if an optional local disk had a path
disappear.  IOW: If all the other conditions are met, I'd like to know
that the source path is missing. That might be a good thing to know if
I'm getting stats for a domain. Maybe that's just me.

BTW: I fixed a bug in the domstats path recently where the "last error"
was lost (see commit e1fc7ec08).  Although I don't think that's what
you're chasing here.

John


> +}
> +
> +
>  static int
>  qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 2037467..52d396d 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -214,4 +214,6 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);
>  
>  void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);
>  
> +bool qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk);
> +
>  #endif /* __QEMU_PROCESS_H__ */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats

[Nikolay Shirokovskiy]

Every time we call all domain stats for inactive domain with
unavailable source we get error message in logs. It's a bit noisy.
While it's arguable whether we need such message or not for mandatory
disks we would like not to see messages for optional disks. Let's
filter at least for cases of local files. Fixing other cases would
require passing flag down the stack to .backendInit of storage
which is ugly.

Stats for active domain are fine because we either drop disks
with unavailable sources or clean source which is handled
by virStorageSourceIsEmpty in qemuDomainGetStatsOneBlockFallback.

We have these logs for successful stats since 25aa7035d which
in turn fixes 596a13713 which added substantial stats for offline
disks.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_driver.c  | 5 +
 src/qemu/qemu_process.c | 9 +
 src/qemu/qemu_process.h | 2 ++
 3 files changed, 16 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e249..72e4cfe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20290,6 +20290,11 @@ qemuDomainGetStatsBlockExportDisk(virDomainDiskDefPtr 
disk,
 const char *backendstoragealias;
 int ret = -1;
 
+if (!virDomainObjIsActive(dom) &&
+qemuProcessMissingLocalOptionalDisk(disk))
+return qemuDomainGetStatsBlockExportHeader(disk, disk->src, *recordnr,
+   records, nrecords);
+
 for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
 if (blockdev) {
 frontendalias = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9cf9718..802274e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6070,6 +6070,15 @@ qemuProcessPrepareSEVGuestInput(virDomainObjPtr vm)
 }
 
 
+bool
+qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk)
+{
+return disk->startupPolicy == VIR_DOMAIN_STARTUP_POLICY_OPTIONAL &&
+   virStorageSourceIsLocalStorage(disk->src) && disk->src->path &&
+   !virFileExists(disk->src->path);
+}
+
+
 static int
 qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 2037467..52d396d 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -214,4 +214,6 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);
 
 void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);
 
+bool qemuProcessMissingLocalOptionalDisk(virDomainDiskDefPtr disk);
+
 #endif /* __QEMU_PROCESS_H__ */
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list