[ovs-dev] [PATCH v3 2/3] ovn: Add ct_commit(ct_mark=INT, ct_label=INT); action.

[Russell Bryant]

On Tue, Mar 22, 2016 at 5:57 PM, Guru Shetty <guru at ovn.org> wrote:

>
>>>
>>> So with the LB series, I need to store the value of ct_label and ct_mark
>>> in a register and then load it from there when I finally do ct_commit.
>>> ct_label is 32 bits or one register and ct_mark is 128 bits or 4 registers
>>> for a total of 5 registers. We do not have that many registers at all. With
>>> this patch, one can set ct_mark or ct_label to any value in logical flows
>>> which means that I am forced to use 5 registers. What I am saying is that
>>> if we get rid of ct_mark from this patch and make ct_label to only be set
>>> to one value, I can confidently use a single register for all of this.
>>>
>>> I guess what you are saying is that even though this patch is general
>>> purpose, we only use one bit of ct_label for ACL. But in the code of
>>> ovn-controller, it no longer will be general purpose and I will only have
>>> to read one bit from the set logical flow for ct_label and ignore ct_mark.
>>> That would look odd, no?
>>>
>>
>> 1) General capabilities of the logical flow syntax.  That's what this
>> patch adds.  It doesn't really say anything about how it's used.  I *think*
>> you're saying it looks like this could be used in a way that would be
>> painful for implementing multiple stateful services.  It's great to
>> consider the needs of ovn-northd today, but the syntax is a more general
>> capability.
>>
>
> Correct.
>
>
>> Someone could throw ovn-northd away and write their own logical flows,
>> perhaps.
>>
>
> Right. Or 2 weeks from now, some other developer can implement a feature
> that sets a logical flow as ct_commit (ct_label = 0x3002, ct_mark = 0x3333)
> in ovn-northd. So my suggestion was don't make it general purpose, but only
> limit setting ct_label to one specific value as that is all is what is
> needed right now.
>
>
>> 2) How we structure our logical flows in ovn-northd.  I think that's what
>> you're most concerned with.
>>
>> Right now the next patch in this series makes use of this logical flow
>> capability to do:
>>
>>     ct_commit(ct_label=1)
>>     ... or ...
>>     ct_commit(ct_label=0)
>>
>> I believe you were splitting stateful services up to share a ct_commit in
>> a later table, so we could change that to something like:
>>
>>     reg0[0] = 1;
>>
>> and in the next table:
>>
>>     ct_commit(ct_label=reg0[0])
>>
>
> Correct.
>
>>
>> We could even give "reg0[0]" a nicer name by adding a new symbol like
>> "ct_drop_connection".
>>
>
> Yes.
>
>
>>
>> This is all in logical flows, not code in ovn-controller.  I think this
>> current patch is all that's needed for ovn-controller, so it knows how to
>> handle this action in logical flows.
>>
>
> If we go by symbols, then ovn-controller will read the symbol, check
> whether it is a valid symbol and then convert it to a integer and then do
> openflow action.
>
>>
>> If I'm still missing something, maybe we can hop on the phone or
>> something tomorrow and recap the conversation on the mailing list?
>>
> I think we are close to be on the same page. But, if my answers in this
> mail confused this more, or if you think of a better idea or a different
> perspective, I am happy to jump on a call.
>

We chatted a bit on IRC.  I think we're on the same page.  We discussed
some different ideas, but I think we landed on this not requiring any
changes for now.  Is that right?

I'm happy to work with you on rebasing the LB series to work with this, as
well, if that's helpful.

-- 
Russell Bryant