From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Fabian Ebner <f.ebner@proxmox.com>, pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage
Date: Mon, 22 Mar 2021 11:01:33 +0100 [thread overview]
Message-ID: <1616406650.sq238n1ivv.astroid@nora.none> (raw)
In-Reply-To: <ff7b850d-8350-20ed-64fd-bf384de9553a@proxmox.com>
On March 22, 2021 9:56 am, Fabian Ebner wrote:
> Am 19.03.21 um 15:16 schrieb Fabian Grünbichler:
>> On March 19, 2021 2:49 pm, Fabian Ebner wrote:
>>> it's cheap and saves code.
>>
>> but also changes behaviour in a non-backwards-compatible fashion.
>>
>> previously, if a disk was already on a storage that does not have images
>> configured, and the migration leaves it on that storage, this config
>> mismatch was ignored (hence the "grandfather in existing mismatches"
>> comment). note that users might be able to migrate, but not able to
>> change storage.cfg to fix this "misconfiguration".
>>
>
> What about the recent change [0] in pve-storage then, i.e. not listing
> VM disks for storages without an appropriate content type?
I'd have to think more about that (I mean, it obviously makes sense, the
question is whether it breaks something ;))
>
> I'd argue that unreferenced images that lie on storages without an
> 'image' content type should not be picked up in the first place. If we
> don't agree on this, then [0] needs to be reverted...
but this is not just about unreferenced disks? I mean, this code runs
for ALL disks returned by pve-storage. if pve-storage now filters, then
this does not apply for the '"old == new" storage which has wrong content
type' scenario, since this part of the code thinks there are no volumes
there, so what I originally thought is broken is not,
> If we do agree on this, we should double down on [0] and actually be
> precise about the content type in vdisk_list by either:
> A) adding a content/guest type parameter to vdisk_list.
> B) adapting the call sites to filter storages.
> Afterwards, this patch could be applied with the appropriate dependency
> bump ;)
>
> Note that referenced unused images will still be picked up by the
> PVE::QemuServer::foreach_volid iteration, no matter what the content
> type of their storage is.
but this would then mean that for referenced images, '"old != new"
storage where both old and new storage have wrong content type' now
suddenly works where it previously didn't? not because of this patch,
but because of vdisk_list not returning those, thus skipping the check
that is changed in this patch? oh, and shared storages with wrong
content type also just work, but since we don't really touch volumes
there anyway with migration that's probably fine..
IFF we want to say "wrong content type" is wrong and should make the VM
unmigratable, we should probably move the check into test_volid so that
it catches ALL volumes always..
>
> [0]:
> https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d
>
>>>
>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>>> ---
>>> PVE/QemuMigrate.pm | 7 ++-----
>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>>> index 3597cc9..44cecce 100644
>>> --- a/PVE/QemuMigrate.pm
>>> +++ b/PVE/QemuMigrate.pm
>>> @@ -410,11 +410,8 @@ sub sync_disks {
>>> $log_error->("storage '$targetsid' is not available on node '$self->{node}'")
>>> if !$target_scfg;
>>>
>>> - # grandfather in existing mismatches
>>> - if ($targetsid ne $storeid && $target_scfg) {
>>> - $log_error->("content type 'images' is not available on storage '$targetsid'")
>>> - if !$target_scfg->{content}->{images};
>>> - }
>>> + $log_error->("content type 'images' is not available on storage '$targetsid'")
>>> + if $target_scfg && !$target_scfg->{content}->{images};
>>>
>>> PVE::Storage::foreach_volid($dl, sub {
>>> my ($volid, $sid, $volinfo) = @_;
>>> --
>>> 2.20.1
>>>
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
next prev parent reply other threads:[~2021-03-22 10:02 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 13:49 [pve-devel] [PATCH qemu-server 1/3] migrate: improve early error messages Fabian Ebner
2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage Fabian Ebner
2021-03-19 14:16 ` Fabian Grünbichler
2021-03-22 8:56 ` Fabian Ebner
2021-03-22 10:01 ` Fabian Grünbichler [this message]
2021-03-22 11:08 ` Fabian Ebner
2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 3/3] code cleanup: migrate: avoid post-ifs Fabian Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1616406650.sq238n1ivv.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox