On 10.08.20 13:04, Vladimir Sementsov-Ogievskiy wrote:
> 10.08.2020 11:12, Max Reitz wrote:
>> On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote:

[...]

>>> But, with our proposed way (freeze only chain up to base_overlay
>>> inclusively, and use backing(base_overlay) as final backing), all will
>>> work as expected, and two parallel jobs will work..
>>
>> I don’t think it will work as expected because users can no longer
>> specify which node should be the base node after streaming.  And the
>> QAPI schema says that base-node is to become the backing file of the top
>> node after streaming.
> 
> But this will never work with either way: base node may disappear during
> stream. Even with you way, they only stable thing is "above-base", which
> backing child may be completely another node at stream finish.

Yeah, but after c624b015bf, that’s just how it is.  I thought the best
would be an approach where if there are no graph changes during the job,
you would indeed end up with @base as the backing file afterwards.

[...]

>> Well, that still leaves the problem that users should be able to specify
>> which node is to become the base after streaming, and that that node
>> maybe shouldn’t be restricted to immediate children of COW images.
> 
> And again, this is impossible even with your way. I have an idea:
> 
> What about making the whole thing explicit?
> 
> We add an optional parameter to stream-job: bottom-node, which is
> mutally exclusive with specifying base.
> 
> Then, if user specified base node, we freeze base as well, so it can't
> disappear. User will not be able to start parallel stream with this base
> node as top (because new stream can not insert a filter into frozen
> chain), but for sure it's rare case, used only in iotest 30 :)).
> Benefit: user have guarantee of what would be final backing node.

Sounds very nice to me, but...

> Otherwise, if user specified bottom-node, we use the way of this patch.
> So user can run parallel streams (iotest 30 will have to use bottom-node
> argument). No guarantee of final base-node, it would be backing of
> bottom-node at job finish.
> 
> But, this is incompatible change, and we probably should wait for 3
> releases for deprecation of old behavior..

...yeah.  Hm.  What a pain, but right, we can just deprecate it.

Unfortunately, I don’t think there’s any way we could issue a warning
(we’d want to deprecate using the @base node in something outside of the
stream job, and we can’t really detect this case to issue a warning).
So it would be a deprecation found only in the deprecation notes and the
QAPI spec...

> Anyway, I feel now, that you convinced me. I'm not sure that we will not
> have to change it make filter work, but not reason to change something
> now. Andrey, could you try to rebase your series on top of this and fix
> iotest 30 by just specifying  exact node-names in it?..
> 
> 
> Hmmm. My thought goes further. Seems, that in this way, introducing
> explicit filter would be incompatible change anyway: it will break
> scenario with parallel stream jobs, when user specifies filenames, not
> node names (user will have to specify filter-node name as base for
> another stream job, as you said). So, it's incompatible anyway.
> 
> What do you think of it? Could we break this scenario in one release
> without deprecation and don't care?

I don’t know, but I’m afraid I don’t think we can.

> Than I think my idea about base vs
> bottom-node arguments for stream job may be applied. Or what to do?
> 
> If we can't break this scenario without a deprecation, we'll have to
> implement "implicit" filter, like for mirror, when filter-node-name is
> not specified. And for this implicit filter we'll need additional logic
> (closer to what I've proposed in a previous mail). Or, try to keep
> stream without a filter (not insert it at all and behave the old way),
> when filter-node-name is not specified. Than new features based on
> filter will be available only when filter-node-name is specified, but
> this is OK. The latter seems better for me.

If that works...


OK.  So what I think we can do is first just take this patch as part of
this series.  Then, we add @bottom-node separately and deprecate @base
not being frozen.

If it’s feasible to not add a stream filter node until the deprecation
period is over, then that should work.

Max