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

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). >>> >>