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

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