[ovirt-devel] Re: Request for reviews - vdsm-tool update-volume

On Mon, Nov 12, 2018 at 3:20 PM Germano Veit Michel <germano(a)redhat.com&gt; wrote: > Hi, > > I've finished developing the tools we would like to have to fix snapshot > related issues. > > Piotr did an initial review in one of the patches, but there is still a > lot of reviewing that needs to be done. Could you please review this > series? https://gerrit.ovirt.org/#/q/topic:snapshot-tools > I'm specially concerned about 94366, I'm not sure that is the right way > do to that. > > Note: there are some random CI failures on F28 that we are investigating > in a different mail thread* , so with every rebase 1 to 3 patches fail. > * [VDSM] TestMount.testSymlinkMount failing > > Thanks > > > > On Mon, Oct 8, 2018 at 4:43 PM Germano Veit Michel <germano(a)redhat.com&gt; > wrote: > >> All passing CI and rebased to latest master. >> Reviews are welcome. >> >> On Tue, Sep 25, 2018 at 8:01 AM Germano Veit Michel <germano(a)redhat.com&gt; >> wrote: >> >>> >>> >>> On Tue, Sep 25, 2018 at 7:59 AM Dan Kenigsberg <danken(a)redhat.com&gt; >>> wrote: >>> >>>> >>>> >>>> On Tue, Sep 25, 2018 at 12:49 AM Germano Veit Michel < >>>> germano(a)redhat.com&gt; wrote: >>>> >>>>> >>>>> >>>>> On Mon, Sep 24, 2018 at 11:56 PM Dan Kenigsberg <danken(a)redhat.com&gt; >>>>> wrote: >>>>> >>>>>> On Mon, Sep 24, 2018 at 9:24 AM Germano Veit Michel < >>>>>> germano(a)redhat.com&gt; wrote: >>>>>> > >>>>>> > Hi, >>>>>> > >>>>>> > I'm working an a tool (vdsm-tool update-volume) to make modifying >>>>>> SD metadata easier and more importantly, safer. This is very useful to >>>>>> recover from failed LSMs or snapshot issues. >>>>>> > >>>>>> > The plan is to use the VDSM API (modified by some of these >>>>>> patches) and add a tool (vdsm-tool) that talks to the API and modifies the >>>>>> volumes metadata as required by the user. Currently this is done manually, >>>>>> i.e.: looking at MD_XXX tags, doing dd, sed and then dd back to the >>>>>> storage. Any wrong argument (like a skip in place of a seek) can ruin the >>>>>> entire metadata, so this tool can be quite handy. >>>>>> > >>>>>> > The code is not necessarily 100% finished yet, but I've been >>>>>> testing this for some time and it seems ok from a functional point of view. >>>>>> I'm just not sure everything I did (especially inside VDSM, example 94366) >>>>>> is correct. Your comments on what can/should be improved are very welcome >>>>>> at this point. Please see this series and help reviewing it. >>>>>> > >>>>>> > >>>>>> https://gerrit.ovirt.org/#/q/topic:update-volume+(status:open+OR+status:m... >>>>>> > https://gerrit.ovirt.org/#/c/93258/ >>>>>> >>>>>> I am not a maintainer of vdsm.storage, but I can say that I'm missing >>>>>> a high-level description of the change that you are suggesting, and >>>>>> its motivation. When do you see a need to manually change the >>>>>> metadata >>>>>> of volumes? Shouldn't we fix the bug that causes this need? >>>>>> >>>>> >>>>> Ohh, sorry. I thought it was obvious. This is for snapshot related >>>>> issues. These bugs have been present for a while, they are fixed but new >>>>> ones come up. In the end downstream support is constantly manually >>>>> repairing chains. This is why this tool is needed. Both to make those >>>>> changes safer and to save time. >>>>> >>>>> >>>>>> I personally have a deep resentment to a "force" flags - not just >>>>>> here, but everywhere. It is never clear what is being forced. Some >>>>>> things cannot or should not be forced. >>>>>> >>>>> >>>>> Nir already suggested to remove the force flag. I don't mind, the >>>>> idea was just to keep the old behavior the same if the force flag is not >>>>> used. >>>>> >>>>> >>>>>> >>>>>> One last note: a CI+1 gives a positive psychological vibe to your >>>>>> reviewer. >>>>>> >>>>> Yes, I know. Most of them have CI+1. They keep randomly failing on >>>>> FC28 (but EL7 succeeds) on every push. >>>>> >>>> >>>> We used to have a regression in iproute on Fedora few weeks ago. a >>>> rebase on master may help. It is your responsibility to find why fc28 >>>> fails, as we cannot merge a patch that does not pass there. >>>> >>> >>> Thanks, I'll rebase them. Hopefully thats it. >>> >>> >>>> >>>> And there is just one that is constantly failing, I even sent an email >>>>> to this list ("Jenkins help") to try to better understand why, but >>>>> no one replied yet. It seems to be complaining about an object not having a >>>>> member, but the member is created at runtime (based on api schema). >>>>> >>>> Today was my lucky day and CI didn't fail with those random errors on umount. So all patches passed, please review: https://gerrit.ovirt.org/#/q/topic:snapshot-tools