On Mon, Oct 08, 2018 at 10:23:40AM +0000, Petr Machata wrote:
Ondrej Lichtner <olichtne(a)redhat.com> writes:
> On Tue, Oct 02, 2018 at 03:06:40PM +0000, Petr Machata wrote:
>> @@ -27,9 +30,11 @@ class RandomValuePicker:
>> # support static threshold only for now
>> return "static"
>>
>> - def _get_th(self):
>> - # support dynamic threshold only for now
>> - return random.randint(3,16)
>> + def _get_th(self, pool):
>> + th = random.randint(3,16)
>> + if pool == 8: # Pool 8 has static quotas.
>> + th *= CELL_SIZE
>> + return th
>>
>> def _get_pool(self, direction):
>> arr = self._pools[direction]
>> @@ -40,14 +45,17 @@ class RandomValuePicker:
>> return (self._get_size(), self._get_thtype())
>> if isinstance(objid, TcBind):
>> pool = self._get_pool(objid["type"])
>> - th = self._get_th()
>> + th = self._get_th(pool)
>> return (pool, th)
>> if isinstance(objid, PortPool):
>> - return (self._get_th(),)
>> + return (self._get_th(objid["pool"]),)
>>
>> class RecordValuePickerException(Exception):
>> pass
>>
>> +class SkipTest(Exception):
>> + pass
>> +
>> class RecordValuePicker:
>> def __init__(self, objlist):
>> self._recs = []
>> @@ -55,6 +63,9 @@ class RecordValuePicker:
>> self._recs.append({"objid": item, "value":
item.var_tuple()})
>>
>> def get_value(self, objid):
>> + if isinstance(objid, Pool) and objid["pool"] == 8:
>> + # Pool 8 is reported with infinite size, which can't be reset.
>> + raise SkipTest()
>> for rec in self._recs:
>> if rec["objid"].weak_eq(objid):
>> return rec["value"]
>
> "8" seems like a weird magic number constant. Does it make sense to use
> it in the context of an upstream repository where we want to track
> generic recipes?
Definitely not, this is specific to mlxsw. What would be a good way to
make this e.g. configurable, or somehow generic? An alias?
Thanks,
Petr
Well, considering that the recipe is switchdev specific, if mlxsw is the
only expected implementation of it, it's probably fine to have it this
way, though it might be a good idea to have the "8" explained. Note,
this may already be the case - the commit message has some description
and the code has some additional specifics where it's used.
I'm not really familiar with switchdev so I just wanted to ask if this
is sufficient and makes sense. If it does it's ok and I can apply the
patch.
On the other hand, if you expect this switchdev recipe to run in a
non-mlxsw environment then i see two options:
* you expect that the changed pool (and therefore recipe) behaviour will
always be present, but maybe under a different constant for different
drivers/vendors. In this case, using an alias as a test parameter
might make sense. This could also cover the case where the specific
functionality isn't implemented - don't specify the alias, the
specific recipe functionality is also skipped.
* the changed pool behaviour is mlxsw specific and will forever stay
that way - e.g. other drivers/vendors will have different solutions to
this and might instead want to have a different special handling of
those (maybe even using the value "8"). In that case it would maybe
make more sense to have multiple subclasses with the specific
behaviour, and you could choose between them with either an alias, or
if applicable/possible you could also detect the driver used.
I might not be explaining myself well thoug... if that's the case it
might be easier to discuss interactively over irc, feel free to ping me :).
-Ondrej