Re: [libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
On 11.12.2018 01:05, John Ferlan wrote: > $SUBJ > > "storage sources" > > On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote: >> Because missing optional source is not error. The patch >> address only local files. Fixing other cases is a bit ugly. >> >> Signed-off-by: Nikolay Shirokovskiy >> --- >> src/qemu/qemu_process.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index 802274e..5e04bf9 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, >> if (!blockdev) >> virStorageSourceBackingStoreClear(disk->src); >> >> -if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) >> +if (!qemuProcessMissingLocalOptionalDisk(disk) && >> +qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) > > Although it makes more sense for this path to use the startupPolicy, I > will point out that the first thing qemuDomainDetermineDiskChain does is > filter on virStorageSourceIsEmpty, so regardless of whether the > startupPolicy is optional or not, not much is happening for empty > sources. So in essence unnecessary at least from my read. The issue is not with empty (no source file is specified) sources but rather with missing ones (source file is specified but file itself does not exist). In this case virStorageSourceIsEmpty does not help and we get this log notice: error: virStorageFileReportBrokenChain:427 : Cannot access storage file '/path/to/missing/optional/disk': No such file or directory Which is not fatal for starting a domain because source is optional. Sorry, I should put error messages in the original letters so you can easier understand what's going on. Nikolay > > John > >> continue; >> >> if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= >> 0) >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats
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 argua
Re: [libvirt] [PATCH] secret: Add check/validation for correct usage when LookupByUUID
ping?
Thanks,
John
On 12/4/18 3:32 PM, John Ferlan wrote:
> If virSecretGetSecretString is using by secretLookupByUUID,
> then it's possible the found sec->usageType doesn't match the
> desired @secretUsageType. If this occurs for the encrypted
> volume creation processing and a subsequent pool refresh is
> executed, then the secret used to create the volume will not
> be found by the storageBackendLoadDefaultSecrets which expects
> to find secrets by VIR_SECRET_USAGE_TYPE_VOLUME.
>
> Add a check to virSecretGetSecretString to avoid the possibility
> along with an error indicating the incorrect matched types.
>
> Signed-off-by: John Ferlan
> ---
>
> If someone has an idea regarding how the usage could be filled
> in "properly" for the qemuxml2argvtest, I'm willing to give it
> a shot. However, fair warning trying to "mock" for tls, volume,
> iscsi, and ceph could be rather painful. Thus the NONE was the
> well, easiest way to go since the stored secret (ahem) shouldn't
> be of usageType "none" (famous last words).
>
> src/secret/secret_util.c | 17 +
> tests/qemuxml2argvtest.c | 4 +++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c
> index 16e43ab2cc..27e164a425 100644
> --- a/src/secret/secret_util.c
> +++ b/src/secret/secret_util.c
> @@ -71,6 +71,23 @@ virSecretGetSecretString(virConnectPtr conn,
> if (!sec)
> goto cleanup;
>
> +/* NB: NONE is a byproduct of the qemuxml2argvtest test mocking
> + * for UUID lookups. Normal secret XML processing would fail if
> + * the usage type was NONE and since we have no way to set the
> + * expected usage in that environment, let's just accept NONE */
> +if (sec->usageType != VIR_SECRET_USAGE_TYPE_NONE &&
> +sec->usageType != secretUsageType) {
> +char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +virUUIDFormat(seclookupdef->u.uuid, uuidstr);
> +virReportError(VIR_ERR_INVALID_ARG,
> + _("secret with uuid %s is of type '%s' not "
> + "expected '%s' type"),
> + uuidstr, virSecretUsageTypeToString(sec->usageType),
> + virSecretUsageTypeToString(secretUsageType));
> +goto cleanup;
> +}
> +
> *secret = conn->secretDriver->secretGetValue(sec, secret_size, 0,
>
> VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e17709e7e1..700868ca0b 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -77,7 +77,9 @@ static virSecretPtr
> fakeSecretLookupByUUID(virConnectPtr conn,
> const unsigned char *uuid)
> {
> -return virGetSecret(conn, uuid, 0, "");
> +/* NB: This mocked value could be "tls" or "volume" depending on
> + * which test is being run, we'll leave at NONE (or 0) */
> +return virGetSecret(conn, uuid, VIR_SECRET_USAGE_TYPE_NONE, "");
> }
>
> static virSecretDriver fakeSecretDriver = {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/8] Add tests for Storage Pool startup command line generation
ping? Thanks, John On 12/4/18 11:47 AM, John Ferlan wrote: > Similar to qemuxml2argv and storagevolxml2argv, add storagepoolxml2argvtest > in order to check the command line creation for the pool 'Start' commands. > > Only applicable for pool types with a "@startPool" function - that is disk, > fs, iscsi, logical, scsi, and vstorage and further restricted if the pool > doesn't use virCommandPtr type processing. > > This initial series only addresses fs and logical so that it's not "too > many patches to review". > > The iscsi and vstorage pools could have their own tests, with the iscsi > ones being more challenging to write. > > The disk pool does not have the normal command line processing to start > something - rather the command line processing is used to validate that > the pool about to be started is of a valid type based on the label seen > at startup compared to the pool XML. This processing is not easily mocked. > > The scsi (for NPIV) doesn't use the virCommandPtr style interfaces, but at > least that processing is tested in other ways using testCreateVport from > test_driver.c. > > John Ferlan (8): > storage: Extract out mount command creation for FS Backend > storage: Move FS backend mount creation command helper > storage: Move virStorageBackendFileSystemGetPoolSource > tests: Introduce tests for storage pool xml to argv checks > tests: Add storagepool xml test for netfs-auto > storage: Rework virStorageBackendFileSystemMountCmd > logical: Fix @on argument type > storage: Add tests for logical backend startup > > src/storage/storage_backend_fs.c | 77 +--- > src/storage/storage_backend_logical.c | 12 +- > src/storage/storage_util.c| 122 > src/storage/storage_util.h| 11 ++ > tests/Makefile.am | 12 ++ > tests/storagepoolxml2argvdata/pool-fs.argv| 1 + > .../pool-logical-create.argv | 1 + > .../pool-logical-noname.argv | 1 + > .../pool-logical-nopath.argv | 1 + > .../storagepoolxml2argvdata/pool-logical.argv | 1 + > .../pool-netfs-auto.argv | 1 + > .../pool-netfs-cifs.argv | 1 + > .../pool-netfs-gluster.argv | 1 + > tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + > tests/storagepoolxml2argvtest.c | 175 ++ > .../storagepoolxml2xmlin/pool-netfs-auto.xml | 19 ++ > .../storagepoolxml2xmlout/pool-netfs-auto.xml | 20 ++ > tests/storagepoolxml2xmltest.c| 1 + > 18 files changed, 375 insertions(+), 83 deletions(-) > create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv > create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv > create mode 100644 tests/storagepoolxml2argvtest.c > create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-auto.xml > create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-auto.xml > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't log error for missing optional sources on start
$SUBJ "storage sources" On 11/12/18 7:58 AM, Nikolay Shirokovskiy wrote: > Because missing optional source is not error. The patch > address only local files. Fixing other cases is a bit ugly. > > Signed-off-by: Nikolay Shirokovskiy > --- > src/qemu/qemu_process.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 802274e..5e04bf9 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -6100,7 +6100,8 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver, > if (!blockdev) > virStorageSourceBackingStoreClear(disk->src); > > -if (qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) > +if (!qemuProcessMissingLocalOptionalDisk(disk) && > +qemuDomainDetermineDiskChain(driver, vm, disk, true) >= 0) Although it makes more sense for this path to use the startupPolicy, I will point out that the first thing qemuDomainDetermineDiskChain does is filter on virStorageSourceIsEmpty, so regardless of whether the startupPolicy is optional or not, not much is happening for empty sources. So in essence unnecessary at least from my read. John > continue; > > if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) >= > 0) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: don't log error for missing optional sources on stats
$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
Re: [libvirt] [PATCH 2/2] libxl: handle external domain destroy
On 12/7/18 7:46 PM, Marek Marczykowski-Górecki wrote:
If domain is killed with `xl destroy`, libvirt will not notice it and
still report the domain as running. Also trying to destroy the domain
through libvirt will fail. The only way to recover from such a situation
is to restart libvirt daemon. The problem is that even though libxl
report LIBXL_EVENT_TYPE_DOMAIN_DEATH, libvirt ignore it as all the
domain cleanup is done in a function actually destroying the domain. If
destroy is done outside of libvirt, there is no place where it would be
handled.
Fix this by doing domain cleanup in LIBXL_EVENT_TYPE_DOMAIN_DEATH too.
To avoid doing it twice, add a ignoreDeathEvent flag
libxlDomainObjPrivate, set when the domain death is triggered by libvirt
itself.
Signed-off-by: Marek Marczykowski-Górecki
---
src/libxl/libxl_domain.c | 71 ++--
src/libxl/libxl_domain.h | 3 ++
2 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 5fe3f44fbe..6d1e15b14c 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -609,6 +609,54 @@ libxlDomainShutdownThread(void *opaque)
virObjectUnref(cfg);
}
+static void
+libxlDomainDeathThread(void *opaque)
+{
+struct libxlShutdownThreadInfo *shutdown_info = opaque;
+virDomainObjPtr vm = NULL;
+libxl_event *ev = shutdown_info->event;
+libxlDriverPrivatePtr driver = shutdown_info->driver;
+virObjectEventPtr dom_event = NULL;
+libxlDriverConfigPtr cfg;
+libxlDomainObjPrivatePtr priv;
+
+cfg = libxlDriverConfigGet(driver);
+
+vm = virDomainObjListFindByID(driver->domains, ev->domid);
+if (!vm) {
+/* vm->def->id already cleared, means the death was handled by the
+ * driver already */
+goto cleanup;
+}
+
+priv = vm->privateData;
+
+if (priv->ignoreDeathEvent) {
+priv->ignoreDeathEvent = false;
+goto cleanup;
+}
+
+if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+goto cleanup;
+
+virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_DESTROYED);
+dom_event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
+libxlDomainCleanup(driver, vm);
+if (!vm->persistent)
+virDomainObjListRemove(driver->domains, vm);
+libxlDomainObjEndJob(driver, vm);
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+virObjectEventStateQueue(driver->domainEventState, dom_event);
+libxl_event_free(cfg->ctx, ev);
+VIR_FREE(shutdown_info);
+virObjectUnref(cfg);
+}
Thanks for adding death handling in a new function. libxlDomainShutdownThread()
is getting a bit unwieldy.
+
+
/*
* Handle previously registered domain event notification from libxenlight.
*/
@@ -619,8 +667,10 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST
libxl_event *event)
struct libxlShutdownThreadInfo *shutdown_info = NULL;
virThread thread;
libxlDriverConfigPtr cfg;
+int ret = -1;
-if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
+if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN &&
+event->type != LIBXL_EVENT_TYPE_DOMAIN_DEATH) {
VIR_INFO("Unhandled event type %d", event->type);
goto error;
}
@@ -634,8 +684,14 @@ libxlDomainEventHandler(void *data, VIR_LIBXL_EVENT_CONST
libxl_event *event)
shutdown_info->driver = driver;
shutdown_info->event = (libxl_event *)event;
-if (virThreadCreate(&thread, false, libxlDomainShutdownThread,
-shutdown_info) < 0) {
+if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN)
+ret = virThreadCreate(&thread, false, libxlDomainShutdownThread,
+ shutdown_info);
+else if (event->type == LIBXL_EVENT_TYPE_DOMAIN_DEATH)
+ret = virThreadCreate(&thread, false, libxlDomainDeathThread,
+ shutdown_info);
+
+if (ret < 0) {
/*
* Not much we can do on error here except log it.
*/
@@ -751,14 +807,21 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver,
virDomainObjPtr vm)
{
libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+libxlDomainObjPrivatePtr priv = vm->privateData;
int ret = -1;
+/* Ignore next LIBXL_EVENT_TYPE_DOMAIN_DEATH as the caller will handle
+ * domain death appropriately already (having more info, like the reason).
+ */
+priv->ignoreDeathEvent = true;
/* Unlock virDomainObj during destroy, which can take considerable
* time on large memory domains.
*/
virObjectUnlock(vm);
ret = libxl_domain_destroy(cfg->ctx, vm->def->id, NULL);
virObjectLock(vm);
+if (ret)
+priv->ignoreDeathEvent =
Re: [libvirt] [PATCH 1/2] libxl: add missing cleanup on error path in libxlDomainPMWakeup
On 12/7/18 7:45 PM, Marek Marczykowski-Górecki wrote: Since domain was suspended before and on failed wakeup is destroyed, send an event. Also, add missing libxlDomainCleanup. Signed-off-by: Marek Marczykowski-Górecki --- src/libxl/libxl_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5aa68a7c43..eb719345e8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1527,6 +1527,9 @@ libxlDomainPMWakeup(virDomainPtr dom, unsigned int flags) libxlDomainDestroyInternal(driver, vm); vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); +event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FAILED); +libxlDomainCleanup(driver, vm); endjob: libxlDomainObjEndJob(driver, vm); Reviewed-by: Jim Fehlig Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] On Xen, domains may be destroyed without libvirt being involved
On 12/9/18 11:01 AM, Demi M. Obenour wrote:
This can happen via `xl destroy`, for example. When this happens,
libvirt is stuck in an inconsistent state: libvirt believes the domain
is still running, but attempts to use libvirt’s APIs to shutdown the
domain fail. The only way out of this situation is to restart libvirt.
Marek asked about this last Friday
https://www.redhat.com/archives/libvir-list/2018-December/msg00196.html
and then spent the day creating a patch :-)
https://www.redhat.com/archives/libvir-list/2018-December/msg00212.html
I'll review/test his patches today, but thank you for taking a stab at this
problem!
Regards,
Jim
To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as
well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not
already begun to destroy the domain.
Signed-off-by: Demi Obenour
---
src/conf/domain_conf.h | 4
src/libxl/libxl_domain.c | 24 +++-
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b24e6ec3de..d3520bde15 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2620,6 +2620,10 @@ struct _virDomainObj {
unsigned int updated : 1;
unsigned int removing : 1;
+ /* Only used by the Xen backend */
+ unsigned int being_destroyed_by_libvirt : 1;
+ unsigned int already_destroyed : 1;
+
virDomainDefPtr def; /* The current definition */
virDomainDefPtr newDef; /* New definition to activate at shutdown */
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 5fe3f44fbe..680e5f209f 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -482,9 +482,21 @@ libxlDomainShutdownThread(void *opaque)
goto cleanup;
}
+ VIR_INFO("Domain %d died", event->domid);
+
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto cleanup;
+ if (LIBXL_EVENT_TYPE_DOMAIN_DEATH == ev->type) {
+ if (vm->being_destroyed_by_libvirt) {
+ VIR_INFO("VM %d already being destroyed by libvirt",
event->domid);
+ goto cleanup;
+ }
+
+ VIR_INFO("Marking VM %d as already destroyed", event->domid);
+ vm->already_destroyed = true;
+ }
+
if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) {
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,
VIR_DOMAIN_SHUTOFF_SHUTDOWN);
@@ -620,7 +632,8 @@ libxlDomainEventHandler(void *data,
VIR_LIBXL_EVENT_CONST libxl_event *event)
virThread thread;
libxlDriverConfigPtr cfg;
- if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
+ if (LIBXL_EVENT_TYPE_DOMAIN_DEATH != event->type &&
+ LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN != event->type) {
VIR_INFO("Unhandled event type %d", event->type);
goto error;
}
@@ -629,18 +642,16 @@ libxlDomainEventHandler(void *data,
VIR_LIBXL_EVENT_CONST libxl_event *event)
* Start a thread to handle shutdown. We don't want to be tying up
* libxl's event machinery by doing a potentially lengthy shutdown.
*/
- if (VIR_ALLOC(shutdown_info) < 0)
- goto error;
+ while (VIR_ALLOC(shutdown_info) < 0) {}
shutdown_info->driver = driver;
shutdown_info->event = (libxl_event *)event;
- if (virThreadCreate(&thread, false, libxlDomainShutdownThread,
+ while (virThreadCreate(&thread, false, libxlDomainShutdownThread,
shutdown_info) < 0) {
/*
* Not much we can do on error here except log it.
*/
VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
- goto error;
}
/*
@@ -752,6 +763,9 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver,
{
libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
int ret = -1;
+ if (vm->already_destroyed)
+ return -1;
+ vm->being_destroyed_by_libvirt = true;
/* Unlock virDomainObj during destroy, which can take considerable
* time on large memory domains.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: disable external snapshot of readonly disk
On 11/9/18 3:00 AM, Nikolay Shirokovskiy wrote: > Disable external snapshot of readonly disk for inactive domains > as this operation is not very useful. As to active domains > such snapshot was not possible before already but error message was > not helpful so now it will be better. > > Signed-off-by: Nikolay Shirokovskiy > --- > > Diff from v1 [1] > > > - move check to qemuDomainSnapshotPrepareDiskExternal > - disable such snapshot for inactive domain as well > > > [1] [PATCH] qemu: snapshot: better error for active external readonly disk > https://www.redhat.com/archives/libvir-list/2018-October/msg01322.html > continues in > https://www.redhat.com/archives/libvir-list/2018-November/msg00265.html > > > src/qemu/qemu_driver.c | 7 +++ > 1 file changed, 7 insertions(+) > Do you mind if I "merge" some details from the first patch on this to generate the following commit message?: Disable external snapshot of a readonly disk for domains as this operation is not very useful. Such a snapshot is not possible for active domains but the error message from QEMU is more cryptic: error: internal error: unable to execute QEMU command 'transaction': Could not create file: Permission denied This error at least makes the error more understandable for active domains and disallows for inactive domains as well. Reviewed-by: John Ferlan John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/4] qemu: support metadata-cache-size for blockdev
On 11/8/18 8:02 AM, Nikolay Shirokovskiy wrote:
> Just set l2-cache-size to INT64_MAX for all format nodes of
> qcow2 type in block node graph.
>
> -drive configuration is not supported because we can not
> set l2 cache size down the backing chain in this case.
>
> Note that imlementation sets l2-cache-size and not cache-size.
implementation
> Unfortunately at time of this patch setting cache-size to INT64_MAX
> fails and as guest performance depends only on l2 cache size
> and not refcount cache size (which is documented in recent qemu)
> we can set l2 directly.
More fodder for the let's not take the all or nothing approach. Say
nothing of introducing cache-size and refcount-cache-size terminology in
a commit message when I believe it'd be better in code...
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_block.c | 5 -
> src/qemu/qemu_command.c | 23 +++
> src/qemu/qemu_domain.c| 2 ++
> src/util/virstoragefile.c | 1 +
> src/util/virstoragefile.h | 1 +
> 5 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 5321dda..8771cc1 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -1322,7 +1322,6 @@
> qemuBlockStorageSourceGetFormatQcow2Props(virStorageSourcePtr src,
> * 'pass-discard-snapshot'
> * 'pass-discard-other'
> * 'overlap-check'
> - * 'l2-cache-size'
> * 'l2-cache-entry-size'
> * 'refcount-cache-size'
> * 'cache-clean-interval'
> @@ -1331,6 +1330,10 @@
> qemuBlockStorageSourceGetFormatQcow2Props(virStorageSourcePtr src,
> if (qemuBlockStorageSourceGetFormatQcowGenericProps(src, "qcow2", props)
> < 0)
> return -1;
>
I think this is where you indicate why l2-cache-size is only being used
and what "downside" there is to adding 'cache-size' and/or
'refcount-cache-size'. If I'm reading code, it's a lot "nicer" to find
that information when reading rather than having to go find the commit
where this was added and find the comment about why something wasn't added.
> +if (src->metadata_cache_size ==
> VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM &&> +
> virJSONValueObjectAdd(props, "I:l2-cache-size", INT64_MAX,
NULL) < 0)
QEMU uses "INT_MAX" which is different than INT64_MAX by a few
magnitudes - although the math in the code may help in this case.
As for "I"... Maybe "Z" or "Y" would be better choices... or "U"... The
json schema can accept an 'int' although read_cache_sizes seems to allow
a uint64_t although perhaps limited (I didn't have the energy to follow
the math).
The rest of the changes could be different based on the patch1 adjustments.
John
> +return -1;
> +> return 0;
> }
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f59cbf5..12b2c8d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1330,6 +1330,20 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
> return -1;
> }
>
> +if (disk->metadata_cache_size) {
> +if (disk->src->format != VIR_STORAGE_FILE_QCOW2) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata_cache_size can only be set for qcow2
> disks"));
> +return -1;
> +}
> +
> +if (disk->metadata_cache_size !=
> VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata_cache_size can only be set to
> 'maximum'"));
> +return -1;
> +}
> +}
> +
> if (qemuCaps) {
> if (disk->serial &&
> disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
> @@ -1353,6 +1367,15 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
> _("detect_zeroes is not supported by this QEMU
> binary"));
> return -1;
> }
> +
> +if (disk->metadata_cache_size &&
> +!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV) &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QCOW2_L2_CACHE_SIZE))) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("setting metadata_cache_size is not supported
> by "
> + "this QEMU binary"));
> +return -1;
> +}
> }
>
> if (disk->serial &&
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 045a7b4..23d9348 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9074,6 +9074,7 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> /* "snapshot" is a libvirt internal field and thus can be changed */
> /* startupPolicy is allowed to be updated. Therefore not checked here. */
> CHECK_EQ(transient, "transient", true);
> +CHECK_EQ(metadata_cache_size, "metadata_cache_size", true);
>
> /* Note: For some address types the address auto ge
Re: [libvirt] [PATCH v2 2/4] qemu: caps: add QEMU_CAPS_QCOW2_L2_CACHE_SIZE
On 11/8/18 8:02 AM, Nikolay Shirokovskiy wrote:
> For qemu capable of setting l2-cache-size for qcow2 images
> to INT64_MAX and semantics of upper limit on l2 cache
> size. We can only check this by qemu version (3.1.0) now.
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_capabilities.c | 5 +
> src/qemu/qemu_capabilities.h | 1 +
> 2 files changed, 6 insertions(+)
>
This patch needs to be updated to top of tree.
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2ca5af3..49a3b60 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -509,6 +509,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"vfio-pci.display",
>"blockdev",
>"vfio-ap",
> + "qcow2.l2-cache-size",
When you do update, you will need to fix the comma-less entry for
"egl-headless.rendernode" as well, unless someone else gets to it first.
> );
>
>
> @@ -4117,6 +4118,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT);
> }
>
> +/* l2-cache-size before 3001000 does not accept INT64_MAX */
> +if (qemuCaps->version >= 3001000)
> +virQEMUCapsSet(qemuCaps, QEMU_CAPS_QCOW2_L2_CACHE_SIZE);
> +
Not a fan of this.
The 'l2-cache-size' was supported since QEMU 2.2. A "newer" algorithm
for cache adjustment has been supported since QEMU 2.12 (and there's
l2-cache-entry-size that "could be used" to know that). Then there's
some unreferenced commit indicating usage of INT64_MAX. Tracking that
down, I find qemu.git commit 6c1c8d5d from Max which enforces MAX_INT.
Still does that really matter? Let QEMU pick their own max and don't
have libvirt be the arbiter of that (like I noted in my 1/4 review). My
reasoning is that there's been quite a few "adjustments" to the
algorithms along the way. Those adjustments are one of the concerns
voiced in the past why making any "semi-permanent" change to the value
in some libvirt XML format has been NACKed historically. THus by placing
boundaries that are true today we limit ourselves for the future.
BTW: If 3.1 was the "base" from which you want to work, then adjustments
to the tests/qemucapabilitiesdata/*3_1*{.replies|.xml } files would be
required as evidenced by your patch 4. The *.xml file would need to have
the correct 3001000 setting which should allow patch4
to be merged into patch3.
John
> if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
> goto cleanup;
>
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 6bb9a2c..bb2ac17 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -493,6 +493,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
> syntax-check */
> QEMU_CAPS_VFIO_PCI_DISPLAY, /* -device vfio-pci.display */
> QEMU_CAPS_BLOCKDEV, /* -blockdev and blockdev-add are supported */
> QEMU_CAPS_DEVICE_VFIO_AP, /* -device vfio-ap */
> +QEMU_CAPS_QCOW2_L2_CACHE_SIZE, /* -blockdev supports l2-cache-size with
> INT64_MAX value */
>
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] xml: add disk driver metadata_cache_size option
On 11/8/18 8:02 AM, Nikolay Shirokovskiy wrote:
> Signed-off-by: Nikolay Shirokovskiy
> ---
> docs/formatdomain.html.in | 8
> docs/schemas/domaincommon.rng | 11 +
> src/conf/domain_conf.c | 17
> src/conf/domain_conf.h | 9
> .../qemuxml2argvdata/disk-metadata_cache_size.xml | 42 +++
> .../disk-metadata_cache_size.xml | 48
> ++
> tests/qemuxml2xmltest.c| 2 +
> 7 files changed, 137 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.xml
> create mode 100644 tests/qemuxml2xmloutdata/disk-metadata_cache_size.xml
>
looks like a forgotten thread... It seems reviewer bandwidth is
limited and won't get much better during the last couple weeks of the
month when many if not most Red Hat employees are out to due company
shutdown period.
You need to add a few words to the commit message to describe what's
being changed. Oh and you'll also need a "last" patch to docs/news.xml
to describe the new feature.
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 269741a..1d186ab 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3556,6 +3556,14 @@
> virt queues for virtio-blk. (Since
> 3.9.0)
>
>
> +The optional metadata_cache_size attribute specifies
> +metadata cache size policy, possible values are "default" and
> "maximum".
s/policy, possible/policy. Possible/
> +"default" leaves setting cache size to hypervisor, "maximum"
> makes
s/"default"/Using "default"/
s/to hypervisor,/to the hypervisor./
s/"maximum"/Using "maximum"
> +cache size large enough to keep all metadata, this will help if
> workload
"Using maximum assigns the largest value possible for the cache size.
This ensures the entire disk cache remains in memory for faster I/O at
the expense of utilizing more memory."
[Editorial comment: it's really not 100% clear what all the tradeoffs
someone is making here. The phrase "large enough" just sticks out, but
since you use INT64_MAX in patch 3, I suppose that *is* the maximum.
Still in some way indicating that this allows QEMU to grow the cache and
keep everything in memory, but has the side effect that disks configured
this way will cause guest memory requirements to grow albeit limited by
the QEMU algorithms. It seems from my read the max is 32MB, so perhaps
not a huge deal, but could be useful to note. Whether QEMU shrinks the
cache when not in use wasn't 100% clear to me.]
It's too bad it's not possible to utilize some dynamic value via
qcow2_update_options_prepare. If dynamic adjustment were possible, then
saving the value in the XML wouldn't be necessary - we could just allow
dynamic adjustment similar to what I did for of a couple of IOThread
params (see commit 11ceedcda and the patches before it).
> +needs access to whole disk all the time. ( class="since">Since
> +4.10.0, QEMU 3.1)
This will be at least 5.0.0 now.
> +
> +
>For virtio disks,
>Virtio-specific options can also be
>set. (Since 3.5.0)
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index b9ac5df..3e406fc 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1990,6 +1990,9 @@
>
>
>
> +
> +
> +
>
>
>
> @@ -2090,6 +2093,14 @@
>
>
>
> +
> +
> +
> +default
> +maximum
I didn't go back and read the previous reviews nor was I present for the
KVM Forum discussion on this, but default really means "minimum" I
think. Even that just doesn't feel "right". There's 3 QEMU knobs, but
this changes 1 knob allowing one direction. And yes, changing 3 knobs is
also confusing. I "assume" that it's been determined that this one knob
has the greatest affect on I/O performance...
Reading https://github.com/qemu/qemu/blob/master/docs/qcow2-cache.txt
some phrases stick out:
1. "setting the right cache sizes is not a straightforward operation".
2. "In order to choose the cache sizes we need to know how they relate
to the amount of allocated space."
3. The limit of the l2 cache size is "32 MB by default on Linux
platforms (enough for full coverage of 256 GB images, with the default
cluster size)".
4. "QEMU will not use more memory than needed to hold all of the
image's L2 tables, regardless of this max. value."
So at least providing INT_MAX won't hurt, although given the math in
qcow2_update_options_prepare:
l2_cache_size /= l2_cache_entry_size;
if (l2_cache_size < MIN_L2_CACHE_SIZE) {
l2_cache_size = MIN_L2_CACHE_SIZE;
}
if (l2_cache_size > INT_MAX) {
wouldn't
[libvirt] [PATCH] tools: relax x509 Subject regexes to allow numbers and more
The virt-pki-validate tool is extracting components in the x509 certificate Subject field. Unfortunately the regex it is is using is far too strict, and so truncating valid data. It needs to consider ',' as a field separator, and if that's not there take all data until the EOL. With the broken regex: $ echo " Subject: O=Test,CN=guestHyp1ver" | sed 's+.*CN=\(.[a-zA-Z \._-]*\).*+\1+' guestHyp And with the fixed regex $ echo "Subject: O=Test,CN=guestHyp1ver" | sed 's+.*CN=\([^,]*\).*+\1+' guestHyp1ver Reported-by: Kashyap Chamarthy Signed-off-by: Daniel P. Berrangé --- tools/virt-pki-validate.in | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/virt-pki-validate.in b/tools/virt-pki-validate.in index b04680ddef..c3fadbba64 100755 --- a/tools/virt-pki-validate.in +++ b/tools/virt-pki-validate.in @@ -201,14 +201,14 @@ then echo Client certificate $LIBVIRT/clientcert.pem should be world readable echo "as root do: chown root:root $LIBVIRT/clientcert.pem ; chmod 644 $LIBVIRT/clientcert.pem" else -S_ORG=`"$CERTOOL" -i --infile "$LIBVIRT/clientcert.pem" | grep Subject: | sed 's+.*O=\([a-zA-Z \._-]*\).*+\1+'` +S_ORG=`"$CERTOOL" -i --infile "$LIBVIRT/clientcert.pem" | grep Subject: | sed 's+.*O=\([^,]*\).*+\1+'` if [ "$ORG" != "$S_ORG" ] then echo The CA certificate and the client certificate do not match echo CA organization: $ORG echo Client organization: $S_ORG fi -CLIENT=`"$CERTOOL" -i --infile "$LIBVIRT/clientcert.pem" | grep Subject: | sed 's+.*CN=\(.[a-zA-Z \._-]*\).*+\1+'` +CLIENT=`"$CERTOOL" -i --infile "$LIBVIRT/clientcert.pem" | grep Subject: | sed 's+.*CN=\(.[^,]*\).*+\1+'` echo Found client certificate $LIBVIRT/clientcert.pem for $CLIENT if [ ! -e "$LIBVIRTP/clientkey.pem" ] then @@ -248,14 +248,14 @@ then echo Server certificate $LIBVIRT/servercert.pem should be world readable echo "as root do: chown root:root $LIBVIRT/servercert.pem ; chmod 644 $LIBVIRT/servercert.pem" else -S_ORG=`"$CERTOOL" -i --infile "$LIBVIRT/servercert.pem" | grep Subject: | sed 's+.*O=\([a-zA-Z\. _-]*\).*+\1+'` +S_ORG=`"$CERTOOL" -i --infile "$LIBVIRT/servercert.pem" | grep Subject: | sed 's+.*O=\([^,]*\).*+\1+'` if [ "$ORG" != "$S_ORG" ] then echo The CA certificate and the server certificate do not match echo CA organization: $ORG echo Server organization: $S_ORG fi -S_HOST=`"$CERTOOL" -i --infile "$LIBVIRT/servercert.pem" | grep Subject: | sed 's+.*CN=\(.[a-zA-Z \._-]*\).*+\1+'` +S_HOST=`"$CERTOOL" -i --infile "$LIBVIRT/servercert.pem" | grep Subject: | sed 's+.*CN=\([^,]*\).*+\1+'` if test "$S_HOST" != "`hostname -s`" && test "$S_HOST" != "`hostname`" then echo The server certificate does not seem to match the host name -- 2.19.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] On Xen, domains may be destroyed without libvirt being involved
On Sun, Dec 09, 2018 at 01:01:27PM -0500, Demi M. Obenour wrote:
> This can happen via `xl destroy`, for example. When this happens,
> libvirt is stuck in an inconsistent state: libvirt believes the domain
> is still running, but attempts to use libvirt’s APIs to shutdown the
> domain fail. The only way out of this situation is to restart libvirt.
>
> To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as
> well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not
> already begun to destroy the domain.
>
> Signed-off-by: Demi Obenour
> ---
> src/conf/domain_conf.h | 4
> src/libxl/libxl_domain.c | 24 +++-
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b24e6ec3de..d3520bde15 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2620,6 +2620,10 @@ struct _virDomainObj {
> unsigned int updated : 1;
> unsigned int removing : 1;
>
> + /* Only used by the Xen backend */
> + unsigned int being_destroyed_by_libvirt : 1;
> + unsigned int already_destroyed : 1;
This ought to go in the _libxlDomainObjPrivate struct instead
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 5fe3f44fbe..680e5f209f 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -482,9 +482,21 @@ libxlDomainShutdownThread(void *opaque)
> goto cleanup;
> }
>
> + VIR_INFO("Domain %d died", event->domid);
> +
> if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> goto cleanup;
>
> + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH == ev->type) {
Normal style is to have variable name first, constant second
> + if (vm->being_destroyed_by_libvirt) {
> + VIR_INFO("VM %d already being destroyed by libvirt",
> event->domid);
The patch got mangled by your mail client i'm afraid.
> + goto cleanup;
> + }
> +
> + VIR_INFO("Marking VM %d as already destroyed", event->domid);
> + vm->already_destroyed = true;
Using 'true' for an "unsigned int :1' bitfield is not valid.
You would nede to declare the variable as a bool, or use an
integer value here.
> + }
> +
> if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) {
> virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,
> VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> @@ -620,7 +632,8 @@ libxlDomainEventHandler(void *data,
> VIR_LIBXL_EVENT_CONST libxl_event *event)
> virThread thread;
> libxlDriverConfigPtr cfg;
>
> - if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
> + if (LIBXL_EVENT_TYPE_DOMAIN_DEATH != event->type &&
> + LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN != event->type) {
Again, this is contrary to normal libvirt style, so please put it
back to variable name first.
> VIR_INFO("Unhandled event type %d", event->type);
> goto error;
> }
> @@ -629,18 +642,16 @@ libxlDomainEventHandler(void *data,
> VIR_LIBXL_EVENT_CONST libxl_event *event)
> * Start a thread to handle shutdown. We don't want to be tying up
> * libxl's event machinery by doing a potentially lengthy shutdown.
> */
> - if (VIR_ALLOC(shutdown_info) < 0)
> - goto error;
> + while (VIR_ALLOC(shutdown_info) < 0) {}
Huh ? This is busy-waiting when getting OOM which is definitely
not what we want.
>
> shutdown_info->driver = driver;
> shutdown_info->event = (libxl_event *)event;
> - if (virThreadCreate(&thread, false, libxlDomainShutdownThread,
> + while (virThreadCreate(&thread, false, libxlDomainShutdownThread,
> shutdown_info) < 0) {
Again, busy-waiting when failin to spawn threads.
> /*
> * Not much we can do on error here except log it.
> */
> VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
> - goto error;
> }
>
> /*
> @@ -752,6 +763,9 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver,
> {
> libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> int ret = -1;
> + if (vm->already_destroyed)
> + return -1;
> + vm->being_destroyed_by_libvirt = true;
Again, using a bool "true" with an unsigned int bitfied.
>
> /* Unlock virDomainObj during destroy, which can take considerable
> * time on large memory domains.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] On Xen, domains may be destroyed without libvirt being involved
On Sun, Dec 09, 2018 at 13:01:27 -0500, Demi M. Obenour wrote:
> This can happen via `xl destroy`, for example. When this happens,
> libvirt is stuck in an inconsistent state: libvirt believes the domain
> is still running, but attempts to use libvirt’s APIs to shutdown the
> domain fail. The only way out of this situation is to restart libvirt.
>
> To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as
> well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not
> already begun to destroy the domain.
>
> Signed-off-by: Demi Obenour
> ---
> src/conf/domain_conf.h | 4
> src/libxl/libxl_domain.c | 24 +++-
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b24e6ec3de..d3520bde15 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2620,6 +2620,10 @@ struct _virDomainObj {
> unsigned int updated : 1;
> unsigned int removing : 1;
>
> + /* Only used by the Xen backend */
> + unsigned int being_destroyed_by_libvirt : 1;
> + unsigned int already_destroyed : 1;
Please put this into 'struct _libxlDomainObjPrivate' if it's going to be
only used by the libxl driver.
Also the 'bool' type should be used.
> +
> virDomainDefPtr def; /* The current definition */
> virDomainDefPtr newDef; /* New definition to activate at shutdown */
>
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt-csharp for .NET Standard/Core
On Sun, Dec 09, 2018 at 11:27:35AM +0100, Brian Wengel wrote: > Anyone out there manage to port *libvirt-csharp* to *.NET Standard* (or > Core)? > Or perhaps it could be in the future? > > I think .Core is getting more mature on Linux and I would love to have more > direct access to libvirt in my .Core code :-) FWIW, the libvirt csharp bindings haven't had any work done on them for many years now. So we'd welcome anyone who is interested & able to contribute to their future development. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Adding MTU DHCP option when specified
On Fri, Dec 07, 2018 at 04:29:18PM +0100, Casey Callendrello wrote: > Hi there, > I was working on some MTU testing for OpenShift, and I realized that, while > libvirt does allow specifying the MTU of the bridge interface, it doesn't > plumb it through the DHCP server. > > This is really easy - it's just an extra line in the dnsmasq.conf file, > e.g.: > > dhcp-option-force=option:mtu,9000 > > Any interest in this as a patch? And, apologies if this has been discussed > to death (though I searched around and didn't find anything). I don't recall this being discussed before. We set the MTU on the bridge in the host. This in turn means we set the MTU on the TAP devices attached to the bridge. The guest OS though still uses the default MTU size. IIUC, this DHCP option is intended to tell the guest OS to configure its NIC with the different MTU size, completing the end-to-end config. If so, this makese sense as something todo. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] On Xen, domains may be destroyed without libvirt being involved
This can happen via `xl destroy`, for example. When this happens,
libvirt is stuck in an inconsistent state: libvirt believes the domain
is still running, but attempts to use libvirt’s APIs to shutdown the
domain fail. The only way out of this situation is to restart libvirt.
To prevent this from happening, process LIBXL_EVENT_TYPE_DOMAIN_DEATH as
well as LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN, but only if libvirt has not
already begun to destroy the domain.
Signed-off-by: Demi Obenour
---
src/conf/domain_conf.h | 4
src/libxl/libxl_domain.c | 24 +++-
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b24e6ec3de..d3520bde15 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2620,6 +2620,10 @@ struct _virDomainObj {
unsigned int updated : 1;
unsigned int removing : 1;
+ /* Only used by the Xen backend */
+ unsigned int being_destroyed_by_libvirt : 1;
+ unsigned int already_destroyed : 1;
+
virDomainDefPtr def; /* The current definition */
virDomainDefPtr newDef; /* New definition to activate at shutdown */
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 5fe3f44fbe..680e5f209f 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -482,9 +482,21 @@ libxlDomainShutdownThread(void *opaque)
goto cleanup;
}
+ VIR_INFO("Domain %d died", event->domid);
+
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
goto cleanup;
+ if (LIBXL_EVENT_TYPE_DOMAIN_DEATH == ev->type) {
+ if (vm->being_destroyed_by_libvirt) {
+ VIR_INFO("VM %d already being destroyed by libvirt",
event->domid);
+ goto cleanup;
+ }
+
+ VIR_INFO("Marking VM %d as already destroyed", event->domid);
+ vm->already_destroyed = true;
+ }
+
if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) {
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,
VIR_DOMAIN_SHUTOFF_SHUTDOWN);
@@ -620,7 +632,8 @@ libxlDomainEventHandler(void *data,
VIR_LIBXL_EVENT_CONST libxl_event *event)
virThread thread;
libxlDriverConfigPtr cfg;
- if (event->type != LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) {
+ if (LIBXL_EVENT_TYPE_DOMAIN_DEATH != event->type &&
+ LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN != event->type) {
VIR_INFO("Unhandled event type %d", event->type);
goto error;
}
@@ -629,18 +642,16 @@ libxlDomainEventHandler(void *data,
VIR_LIBXL_EVENT_CONST libxl_event *event)
* Start a thread to handle shutdown. We don't want to be tying up
* libxl's event machinery by doing a potentially lengthy shutdown.
*/
- if (VIR_ALLOC(shutdown_info) < 0)
- goto error;
+ while (VIR_ALLOC(shutdown_info) < 0) {}
shutdown_info->driver = driver;
shutdown_info->event = (libxl_event *)event;
- if (virThreadCreate(&thread, false, libxlDomainShutdownThread,
+ while (virThreadCreate(&thread, false, libxlDomainShutdownThread,
shutdown_info) < 0) {
/*
* Not much we can do on error here except log it.
*/
VIR_ERROR(_("Failed to create thread to handle domain shutdown"));
- goto error;
}
/*
@@ -752,6 +763,9 @@ libxlDomainDestroyInternal(libxlDriverPrivatePtr driver,
{
libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
int ret = -1;
+ if (vm->already_destroyed)
+ return -1;
+ vm->being_destroyed_by_libvirt = true;
/* Unlock virDomainObj during destroy, which can take considerable
* time on large memory domains.
--
2.17.2
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Adding MTU DHCP option when specified
Hi there, I was working on some MTU testing for OpenShift, and I realized that, while libvirt does allow specifying the MTU of the bridge interface, it doesn't plumb it through the DHCP server. This is really easy - it's just an extra line in the dnsmasq.conf file, e.g.: dhcp-option-force=option:mtu,9000 Any interest in this as a patch? And, apologies if this has been discussed to death (though I searched around and didn't find anything). Cheers, -- Casey C. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] libvirt-csharp for .NET Standard/Core
Anyone out there manage to port *libvirt-csharp* to *.NET Standard* (or Core)? Or perhaps it could be in the future? I think .Core is getting more mature on Linux and I would love to have more direct access to libvirt in my .Core code :-) Best regards Brian Wengel Denmark -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [bug report] Requested operation is not valid: migration with shmem device is not supported
On Mon, Dec 10, 2018 at 08:39:37 +, 吴 雨霖 wrote: > 1.Description of problem: > > It report use migration with shmem device is not supported , when I use > the command "virsh migrate --live" to migrate a VM to another host. Migration with shmem was specifically forbidden by commit: commit d17fab69be7a73336d388b805c282037ffb29647 Author: Martin Kletzander Date: Tue Sep 20 11:24:49 2016 +0200 qemu: Disable migration with ivshmem It was never safe anyway and as such shouldn't have been enabled in the first place. Future patches will allow hot-(un)pluging of some ivshmem devices as a workaround. Signed-off-by: Martin Kletzander That means that there are probably technical reasons which don't allow it to be migrated. Unfortunately the commit message does not go in depth enough here, but you'll need to prove that it's safe now if you ever want to add the support back. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [bug report] Requested operation is not valid: migration with shmem device is not supported
1.Description of problem:
It report use migration with shmem device is not supported , when I use
the command "virsh migrate --live" to migrate a VM to another host.
2.Version-Release number of selected component (if applicable):
libvirt 3.9.0
qemu-kvm 2.8.1
3.Steps to Reproduce:
1) Add serveral lines below to guest configuration
4
2) virsh start guest-1
3) virsh migrate --live guest-1 qemu+ssh://target/system
Actual results:
error: Requested operation is not valid: migration with shmem device is
not supported
4.Code Review
When using the command “virsh create guset-1.xml,it produce corresponding
command below.
“qemu-kvm -device ivshmem-plain,
id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0xa”
I read qemu-doc.texi in qemu-2.8.1 source code, which have a explain of
migrating ivshmem below
“With device property @option{master=on}, the guest will copy the shared
memory on migration to the destination host. With @option{master=off},
the guest will not be able to migrate with the device attached.”
But libvirt library can not recognize the property “master=on”. When I directly
used command "qemu-kvm -device
ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,master=on,bus=pci.0,addr=0xa” to
launch a guest, it can migrate in live.
I review the code about qemu code with branch 2.8.1 and master from
https://github.com/libvirt/libvirt, it have no configuration definition for
“master=on”
The below is the part of source code
domain_conf.h
struct _virDomainShmemDef {
char *name;
unsigned long long size;
int model; /* enum virDomainShmemModel */
struct {
bool enabled;
virDomainChrSourceDef chr;
} server;
struct {
bool enabled;
unsigned vectors;
virTristateSwitch ioeventfd;
} msi;
virDomainDeviceInfo info;
};
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] 转发: [bug confirm] Requested operation is not valid: migration with shmem device is not supported
1.Description of problem:
It report use migration with shmem device is not supported , when I use
the command "virsh migrate --live" to migrate a VM to another host.
2.Version-Release number of selected component (if applicable):
libvirt 3.9.0
qemu-kvm 2.8.1
3.Steps to Reproduce:
1) Add serveral lines below to guest configuration
4
2) virsh start guest-1
3) virsh migrate --live guest-1 qemu+ssh://target/system
Actual results:
error: Requested operation is not valid: migration with shmem device is
not supported
4.Code Review
When using the command “virsh create guset-1.xml,it produce corresponding
command below.
“qemu-kvm -device ivshmem-plain,
id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0xa”
I read qemu-doc.texi in qemu-2.8.1 source code, which have a explain of
migrating ivshmem below
“With device property @option{master=on}, the guest will copy the shared
memory on migration to the destination host. With @option{master=off},
the guest will not be able to migrate with the device attached.”
But libvirt library can not recognize the property “master=on”. When I directly
used command "qemu-kvm -device
ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,master=on,bus=pci.0,addr=0xa” to
launch a guest, it can migrate in live.
I review the code about qemu code with branch 2.8.1 and master from
https://github.com/libvirt/libvirt, it have no configuration definition for
“master=on”
The below is the part of source code
domain_conf.h
struct _virDomainShmemDef {
char *name;
unsigned long long size;
int model; /* enum virDomainShmemModel */
struct {
bool enabled;
virDomainChrSourceDef chr;
} server;
struct {
bool enabled;
unsigned vectors;
virTristateSwitch ioeventfd;
} msi;
virDomainDeviceInfo info;
};
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
