On Wed, Aug 22, 2018 at 09:54:58AM +0200, Ondrej Lichtner wrote:
On Tue, Aug 21, 2018 at 03:00:01PM +0200, Jan Tluka wrote:
> Tue, Aug 21, 2018 at 02:11:06PM CEST, csfakian(a)redhat.com wrote:
> >+ m1.bond0 = BondDevice(mode="active-backup",
name="my_bond0")
> >+ m1.eth0.down()
> >+ m1.eth1.down()
> >+ m1.bond0.slave_add(m1.eth0)
> >+ m1.bond0.slave_add(m1.eth1)
>
> Could we enhance the abstraction here? Probably we could completely remove:
> m1.eth0.down()
> m1.eth1.down()
>
> and move it into BondDevice class when calling
> m1.bond0.slave_add(m1.eth0)
This is more of a generic "next" branch question... it's an interesting
idea, but I'm not 100% convinced... the slave_{add, del} methods are
shared by all devices inheriting from MasterDevice class. At the same
time the rules to adding slaves can be slightly different based on the
type of device, some types don't require first setting the slaves down,
some do, some require the master to be down.
Hiding this inside the slave_{add, del} methods could become confusing
and give testers false assumptions - e.g. adding a slave while some test
is running will end up setting all other slaves and the master down,
then add the slave and then set everything back up, whereas in the code
it would just look like a slave was added.
So I think for now I prefer this to be explicit even if it leads to a
couple more lines. But we can discuss this on an upstream meeting, see
what others think...
-Ondrej
I've thought about this a little bit more and I want to record my
thoughts somewhere so we don't forget to talk about this on an upstream
meeting.
I definitely think that LNST should be able to provide the "smallest
step possible" APIs in case they're needed for some test scenario. And
at same time provide some nice abstractions that makes writing normal
(no special scenarios) tests easy. And finally we want to have future
version API stability at some point... This kind of leads to us
supporting multiple APIs - some that do the smallest step and then some
that combine multiple steps that are commonly used.
The question then is if we want to mix these in the same place (e.g.
Device class and derivatives APIs) or if we want to separate them
somewhere else and expose them to the tester in different ways.
In case of these slave_* methods, I think I changed my mind. I think I
agree that they should take care of the up/down actions and abstract
them for the tester. The reason for that is because the most important
"smallest step possible" is really just setting device master (not
adding a slave, that's an operation in the other direction). So in this
case we already have "2" APIs that do the "same thing" - set master
is
the smallest step operation and slave_add is the abstracted operation
for the tester.
In the slave_* case it's obvious that the higher abstraction is
comfortable in the place that it's currently at, however I don't know
if that's true for all the cases where we want to provide some level of
abstraction... we should talk about that.
Thanks,
-Ondrej