> On March 28, 2014, 9:36 p.m., Chi Zhang wrote:
> > src/linux/routing.hpp, line 159
> > <https://reviews.apache.org/r/19702/diff/2/?file=537899#file537899line159>
> >
> > We do need this, but only because libnl doesn't have a way to remove
> > one action from the multiple actions on a filter, so we work around the
> > issue to do 'replace with all the other remaining actions'. But really we
> > wouldn't want this if libnl was a little bit better.
> >
> > That's why I wish we don't have to do this for append.
>
> Jie Yu wrote:
> As far as I know, there is no simple way to let you atomically add an
> action to a filter. You'll still have to do "get+update". That's the reason I
> don't want to introduce an interface "append" to make the users feel that the
> "append" is atomic, but in fact it is not.
>
> Chi Zhang wrote:
> Cong should weigh in if I am making a mistake here. I think it's atomic
> inside the kernel: whatever we do to prepare the 'package' in the user space
> won't do any changes to the kernel until we call `rtnl_cls_change`, when the
> package gets delivered down to kernel space to do a swap, so we get whatever
> atomicity the kernel provides, which I'd assume is good enough.
>
> I am a little bit against the new Mirror and ::add combo for two reasons:
>
> - At the end of the day, people are going to map classifier into _cls and
> actions into _act and this Mirror abstraction is quite different from the
> common understanding of an action in this area. I agree we should make users
> of this interface care as little as possible, but it's going to be people who
> know networking who'd look at this code, and straying away from the core
> concept won't really help them understand better; people who don't know would
> learn core concepts first and find it mismatch with our abstraction here.
>
> - We only have one replace/update in ::cleanup, but we have a _lot_ more
> add/append for arp/icmp in the isolator. In ::cleanup where
> replaceFilterAction is used, you can see we need to do loops to prepare the
> 'vector package'. While we have to in this case, we don't for the append
> cases. It feels superfluous to me to do this every time for a reason we don't
> have. (We'd actually have to query kernel to get the information, not because
> we need it, but because the API needs it, every time we do an append.)
>
>
>
> Jie Yu wrote:
> > I think it's atomic inside the kernel: whatever we do to prepare the
> 'package' in
> > the user space won't do any changes to the kernel until we call
> `rtnl_cls_change`,
> > when the package gets delivered down to kernel space to do a swap, so
> we get
> > whatever atomicity the kernel provides, which I'd assume is good enough.
>
> The 'rtnl_cls_change' is atomic as it will just send a RTM_NEWTFILTER
> netlink message with flag NLM_F_REPLACE. But we don't have a way to
> atomically add a single action to a filter.
>
> To do an append, you need to get the current list of actions, prepare the
> new actions in the user space, and push the changed filter using
> rtnl_cls_change mentioned above. You don't have atomicity here since two
> netlink messages are involved.
>
> You can always write a helper function to combine a "get" (atomic) and an
> "update" (atomic) in this API to mimic an "append" (not atomic) if you want.
>
> > In ::cleanup where replaceFilterAction is used, you can see we need to
> do loops to
> > prepare the 'vector package'.
>
> That's exactly why I want to make sure only one action is associated with
> a filter so that you don't need to iterate over all actions in a given filter.
>
>
>
> Chi Zhang wrote:
> yeah I was suspecting we weren't talking about the same atomicity issue.
> I meant to say that traffic wouldn't be interrupted.
>
> You were talking about two users updating at the same time. This issue is
> definitely here, unless we do active control in the interface. Can we ignore
> this issue for now in general until we have that use case?
>
> Maybe could you give an example how to use ::add for the 2 scenarios?
>
> also ::add returns false if the given classifer couldn't be found, then
> how do we add the first classifier with just ::add?
>
> Cong Wang wrote:
> Jie, what's the problem with getting a filter with a list action and
> appending a new action then pushing it into kernel? IOW, who cares about the
> new action until we push it? Since we use actions attached to a filter,
> modifying an action _is_ modifying the filter itself too, in other word,
> essentially we are just replacing a filter here.
Cong, I don't quite follow your comments. I guess there are two problems here:
1) To represent mirror (to a set of links), should we use a single Mirror
action, or we use a set of Mirror actions?
2) Problems with the add/update interfaces.
For 1), I still think enforcing exact 1 action per filter is a nice semantics
to the user, and easier to understand the reason about.
For 2), in my case, adding filter uses flag: NLM_F_CREATE | NLM_F_EXCL, meaning
that if there exist a matching filter, return immediately without touching the
filter. Updating filter uses flag: NLM_F_REPLACE such that if the matching
filter does not exist, return immediately. To add an action, you can do is:
if (/* filter not exist */) {
/* add filter */
} else {
/* get the filter info */
/* update the action in user level */
/* update filter */
}
You can create a helper function that wraps the above logic.
What do you think?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review38947
-----------------------------------------------------------
On March 29, 2014, 5:59 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated March 29, 2014, 5:59 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod
> Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> UPDATE:
>
> 1) adjusted a few interfaces per review comments.
> 2) added impl. (including tests) for managing links.
>
> I'll be adding impl. for managing filters soon (currently, they return
> Error("Unimplemented").)
>
>
> ------
>
> Hey guys, I send this review in order to get an idea about the interface
> design.
>
> Feel free to jump in to express your thoughts, suggestions, concerns, etc.
>
> Thanks!
>
>
> Diffs
> -----
>
> configure.ac 5404dc2
> src/Makefile.am 47d03b3
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
