[cinder] Review of tiny patch to add Ceph RBD fast-diff to cinder-backup
Hey everyone, I wrote a tiny patch to add the Ceph RDB feature of fast-diff to backups created by cinder-backup: * https://review.opendev.org/c/openstack/cinder/+/766856/ Could someone please take a peek and let me know of this is sufficient to be merged? Thanks and with kind regards, Christian
On 1/13/21 4:37 AM, Christian Rohmann wrote:
Hey everyone,
I wrote a tiny patch to add the Ceph RDB feature of fast-diff to backups created by cinder-backup:
* https://review.opendev.org/c/openstack/cinder/+/766856/
Could someone please take a peek and let me know of this is sufficient to be merged?
Thanks for raising awareness of your patch. Right now, the cinder team is prioritizing new driver reviews in light of the impending merge deadline at wallaby milestone-2 next week: http://lists.openstack.org/pipermail/openstack-discuss/2021-January/019720.h... So there may be a slight delay to your patch being reviewed. If you have some time, you can always help things along by reviewing some small patches by other authors. cheers, brian
Thanks and with kind regards,
Christian
I apologise if this is not the place to ask, but this question was actually bugging me for a long time now. Is there any particular reason fast-diff was disabled for backup images in the first place? Like, any actual technical limitation, i.e. "don't enable it or it will break your backups"? Or is it only because earlier versions of CEPH did not support fast-diff, so it was disabled for compatibility reasons? ср, 13 янв. 2021 г. в 16:40, Brian Rosmaita <rosmaita.fossdev@gmail.com>:
On 1/13/21 4:37 AM, Christian Rohmann wrote:
Hey everyone,
I wrote a tiny patch to add the Ceph RDB feature of fast-diff to backups created by cinder-backup:
* https://review.opendev.org/c/openstack/cinder/+/766856/
Could someone please take a peek and let me know of this is sufficient to be merged?
Thanks for raising awareness of your patch. Right now, the cinder team is prioritizing new driver reviews in light of the impending merge deadline at wallaby milestone-2 next week:
http://lists.openstack.org/pipermail/openstack-discuss/2021-January/019720.h...
So there may be a slight delay to your patch being reviewed. If you have some time, you can always help things along by reviewing some small patches by other authors.
cheers, brian
Thanks and with kind regards,
Christian
Hello again Brian, all, On 13/01/2021 14:27, Brian Rosmaita wrote:
On 1/13/21 4:37 AM, Christian Rohmann wrote:
Hey everyone,
I wrote a tiny patch to add the Ceph RDB feature of fast-diff to backups created by cinder-backup:
* https://review.opendev.org/c/openstack/cinder/+/766856/
Could someone please take a peek and let me know of this is sufficient to be merged?
Thanks for raising awareness of your patch. Right now, the cinder team is prioritizing new driver reviews in light of the impending merge deadline at wallaby milestone-2 next week:
http://lists.openstack.org/pipermail/openstack-discuss/2021-January/019720.h...
So there may be a slight delay to your patch being reviewed. If you have some time, you can always help things along by reviewing some small patches by other authors.
After some initial feedback, which I hopefully all fixed in my updated patchset, the review of (https://review.opendev.org/c/openstack/cinder/+/766856/) has stalled. Is there anything I would need to do to ask for this patch to be considered for a merge? Regards Christian
Hey Jay, all, On 18/02/2021 11:43, Christian Rohmann wrote:
After some initial feedback, which I hopefully all fixed in my updated patchset, the review of (https://review.opendev.org/c/openstack/cinder/+/766856/) has stalled.
Is there anything I would need to do to ask for this patch to be considered for a merge?
Thanks for giving my patchset a +2. I am now wondering what else is required for this to be merged to the next release? Do I need to find another reviewer, do I need to add it to some list or will this get merged automatically? Regards Christian
On 2/23/21 12:08 PM, Christian Rohmann wrote:
Hey Jay, all,
On 18/02/2021 11:43, Christian Rohmann wrote:
After some initial feedback, which I hopefully all fixed in my updated patchset, the review of (https://review.opendev.org/c/openstack/cinder/+/766856/) has stalled.
Is there anything I would need to do to ask for this patch to be considered for a merge?
Thanks for giving my patchset a +2.
I am now wondering what else is required for this to be merged to the next release? Do I need to find another reviewer, do I need to add it to some list or will this get merged automatically?
We'll discuss this at tomorrow's (wednesday 24 Feb) cinder meeting. There is support for your patch, but maybe we shouldn't add a config option but should just do the fast diff by default if the array supports it and fall back to current behavior otherwise. But to answer your question, you would need another +2 on the patch before it can be approved; once it's approved, it merges automatically (more or less).
Regards
Christian
Hey Brian, On 23/02/2021 22:21, Brian Rosmaita wrote:
On 2/23/21 12:08 PM, Christian Rohmann wrote:
On 18/02/2021 11:43, Christian Rohmann wrote:
After some initial feedback, which I hopefully all fixed in my updated patchset, the review of (https://review.opendev.org/c/openstack/cinder/+/766856/) has stalled.
Is there anything I would need to do to ask for this patch to be considered for a merge?
Thanks for giving my patchset a +2.
I am now wondering what else is required for this to be merged to the next release? Do I need to find another reviewer, do I need to add it to some list or will this get merged automatically?
We'll discuss this at tomorrow's (wednesday 24 Feb) cinder meeting. There is support for your patch, but maybe we shouldn't add a config option but should just do the fast diff by default if the array supports it and fall back to current behavior otherwise.
But to answer your question, you would need another +2 on the patch before it can be approved; once it's approved, it merges automatically (more or less).
I uploaded a new Patchset now ... with the changes to enable fast-diff if available and I also added "backup" to the commit message. Please kindly let me know if there is anything required to get this merged. Regards Christian
On 2/25/21 8:35 AM, Christian Rohmann wrote:
Hey Brian,
On 23/02/2021 22:21, Brian Rosmaita wrote:
On 2/23/21 12:08 PM, Christian Rohmann wrote:
On 18/02/2021 11:43, Christian Rohmann wrote:
After some initial feedback, which I hopefully all fixed in my updated patchset, the review of (https://review.opendev.org/c/openstack/cinder/+/766856/) has stalled.
Is there anything I would need to do to ask for this patch to be considered for a merge?
Thanks for giving my patchset a +2.
I am now wondering what else is required for this to be merged to the next release? Do I need to find another reviewer, do I need to add it to some list or will this get merged automatically?
We'll discuss this at tomorrow's (wednesday 24 Feb) cinder meeting. There is support for your patch, but maybe we shouldn't add a config option but should just do the fast diff by default if the array supports it and fall back to current behavior otherwise.
But to answer your question, you would need another +2 on the patch before it can be approved; once it's approved, it merges automatically (more or less).
I uploaded a new Patchset now ... with the changes to enable fast-diff if available and I also added "backup" to the commit message.
Thanks for the quick response!
Please kindly let me know if there is anything required to get this merged.
We have to release wallaby os-brick next week, so highest priority right now are os-brick reviews, but we'll get you some feedback on your patch as soon as we can.
Regards
Christian
Hey Brian, On 25/02/2021 19:05, Brian Rosmaita wrote:
Please kindly let me know if there is anything required to get this merged.
We have to release wallaby os-brick next week, so highest priority right now are os-brick reviews, but we'll get you some feedback on your patch as soon as we can.
There is one +1 from Sofia on the change now. Let me know if there is anything else missing or needs changing. Is this something that still could go into Wallaby BTW? Regards Christian
On 3/11/21 8:21 AM, Christian Rohmann wrote:
Hey Brian,
On 25/02/2021 19:05, Brian Rosmaita wrote:
Please kindly let me know if there is anything required to get this merged.
We have to release wallaby os-brick next week, so highest priority right now are os-brick reviews, but we'll get you some feedback on your patch as soon as we can.
There is one +1 from Sofia on the change now. Let me know if there is anything else missing or needs changing.
Commit message needs an update to reflect the current direction of the patch.
Is this something that still could go into Wallaby BTW?
Yes indeed! Thanks for your patience.
Regards
Christian
On 12/03/2021 23:31, Brian Rosmaita wrote:
There is one +1 from Sofia on the change now. Let me know if there is anything else missing or needs changing.
Commit message needs an update to reflect the current direction of the patch.
Argh, stupid mistake - I fixed it. Regards Christian
On 13/01/2021 10:37, Christian Rohmann wrote:
I wrote a tiny patch to add the Ceph RDB feature of fast-diff to backups created by cinder-backup:
* https://review.opendev.org/c/openstack/cinder/+/766856/
Could someone please take a peek and let me know of this is sufficient to be merged?
This change was already merged to master and I now created cherry-picks / backports to victoria (https://review.opendev.org/c/openstack/cinder/+/782917) and ussuri (https://review.opendev.org/c/openstack/cinder/+/782929). Also Andrey Bolgov did create yet another backport of this feaure down to stable/train (https://review.opendev.org/c/openstack/cinder/+/784041). While the cherry-pick onto the stable/victoria branch does verify fine with Zuul (only need review to be merged), the cinder-plugin-ceph-tempest tests fail for ussuri and also train.
Stdout: 'RBD image feature set mismatch. You can disable features unsupported by the kernel with "rbd feature disable volumes/volume-081e9c22-21f3-4585-a2fe-1caed098052b object-map fast-diff".\nIn some cases useful info is found in syslog - try "dmesg | tail".\n' Stderr: 'rbd: sysfs write failed\nrbd: map failed: (6) No such device or address\n'
Could anybody give me a hint on why this might be? Also is there any other process to follow for backports than to create a cherry-pick from the following release down and wait for review? Regards Christian
On 4/7/21 7:53 AM, Christian Rohmann wrote:
On 13/01/2021 10:37, Christian Rohmann wrote:
I wrote a tiny patch to add the Ceph RDB feature of fast-diff to backups created by cinder-backup:
* https://review.opendev.org/c/openstack/cinder/+/766856/
Could someone please take a peek and let me know of this is sufficient to be merged?
This change was already merged to master and I now created cherry-picks / backports to victoria (https://review.opendev.org/c/openstack/cinder/+/782917) and ussuri (https://review.opendev.org/c/openstack/cinder/+/782929). Also Andrey Bolgov did create yet another backport of this feaure down to stable/train (https://review.opendev.org/c/openstack/cinder/+/784041).
While the cherry-pick onto the stable/victoria branch does verify fine with Zuul (only need review to be merged), the cinder-plugin-ceph-tempest tests fail for ussuri and also train.
Stdout: 'RBD image feature set mismatch. You can disable features unsupported by the kernel with "rbd feature disable volumes/volume-081e9c22-21f3-4585-a2fe-1caed098052b object-map fast-diff".\nIn some cases useful info is found in syslog - try "dmesg | tail".\n' Stderr: 'rbd: sysfs write failed\nrbd: map failed: (6) No such device or address\n'
Could anybody give me a hint on why this might be?
You have hit https://bugs.launchpad.net/devstack-plugin-ceph/+bug/1921897 Eric has a patch up addressing this: https://review.opendev.org/c/openstack/devstack-plugin-ceph/+/783880
Also is there any other process to follow for backports than to create a cherry-pick from the following release down and wait for review?
You're following the correct procedure. One thing I noticed, though, is that your pick to stable/ussuri should have the cherry pick info for both the cherry-pick from master (which was the wallaby development branch at the time) to stable/victoria and also from victoria to stable/ussuri.
Regards
Christian
participants (3)
-
Brian Rosmaita
-
Christian Rohmann
-
Vladimir Prokofev