Hi, I've pushed another set with your suggestions. Please continue reviewing.
Note: 2 of the patches failed CI, it is some random error it seems, not the ones Nir fixed. I'll take a better look tomorrow and deal with those on separate email threads. Thanks a lot. On Wed, Nov 28, 2018 at 4:15 PM Germano Veit Michel <[email protected]> wrote: > Thank you for all the reviews! > > I'm working on some of the changes you suggested to improve the code. Plan > to push an updated series tomorrow. > > On Tue, Nov 13, 2018 at 4:30 PM Germano Veit Michel <[email protected]> > wrote: > >> >> >> On Mon, Nov 12, 2018 at 3:20 PM Germano Veit Michel <[email protected]> >> 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 <[email protected]> >>> wrote: >>> >>>> All passing CI and rebased to latest master. >>>> Reviews are welcome. >>>> >>>> On Tue, Sep 25, 2018 at 8:01 AM Germano Veit Michel <[email protected]> >>>> wrote: >>>> >>>>> >>>>> >>>>> On Tue, Sep 25, 2018 at 7:59 AM Dan Kenigsberg <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Tue, Sep 25, 2018 at 12:49 AM Germano Veit Michel < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Sep 24, 2018 at 11:56 PM Dan Kenigsberg <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> On Mon, Sep 24, 2018 at 9:24 AM Germano Veit Michel < >>>>>>>> [email protected]> 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:merged) >>>>>>>> > 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 >> >
_______________________________________________ Devel mailing list -- [email protected] To unsubscribe send an email to [email protected] Privacy Statement: https://www.ovirt.org/site/privacy-policy/ oVirt Code of Conduct: https://www.ovirt.org/community/about/community-guidelines/ List Archives: https://lists.ovirt.org/archives/list/[email protected]/message/JTIQUW3MUNB2TW5DM7OWWOGBNWO2SFHR/
