On Wed, Jun 27, 2018 at 2:09 PM, Mark Michelson <[email protected]> wrote:
> On 06/26/2018 10:22 PM, Han Zhou wrote: > >> On Wed, Jun 20, 2018 at 11:04 PM, Ben Pfaff <[email protected]> wrote: >> >>> >>> To make ovn-controller recompute incrementally, we need accurate >>> dependencies for each function that reads or writes a table. It's >>> currently difficult to be sure about these dependencies, and certainly >>> difficult to maintain them over time, because there's no way to actually >>> enforce them. >>> >>> This commit experiments with an approach that allows for fairly >>> fine-grained access control within ovn-controller to tables and columns. >>> It's based on generating a new version of the IDL data structures for >>> each >>> case where we want different access control. All of these data >>> structures >>> have the same format, but the columns that a given piece of code is not >>> supposed to touch are renamed to discourage programmers from using them, >>> e.g. they're given names suffixed with "__noaccess" or "__writeonly". >>> (This means that there is no runtime overhead to the access control since >>> it only requires a cast to convert between the regular and restricted >>> versions.) In addition, when a columns is supposed to be read-only, >>> functions for modifying the column are not supplied. >>> >>> Type-safe indexes are supplied in the same way. >>> >>> Comments? >>> >>> >> Hi Ben, >> >> Thanks for the great work and sorry for slow response. >> >> Regarding the idl generation, the .def files define how each field of a >> record can be accessed: no access, writeonly, etc, but it didn't specify >> whether intersions or deletions are allowed on the table. Apparently >> insertions and deletions should be considered as write operations to the >> table. The insert/delete interfaces should not be generated for RO tables. >> >> Another general comment is, this patch defines views of tables at module >> level. In the incremental processing, we probably want to define different >> engine nodes (like my earlier patches do), which may not be exact 1-1 >> match >> to the modules. Each node should have its own view of the data, so I am >> not >> sure if this module level view satisfies our goal of incremental >> processing. Do you have any thoughts on this? Or are you suggesting making >> engine nodes 1-1 mapping with modules? >> > > I interpreted the module level views to be what fits with the current > ovn-controller. You can think of this as an example to follow rather than a > permanent fixture. > > With incremental processing, I think you would remove some of the existing > module-level views and replace them with views that represent engine nodes. > Once incremental processing is fully implemented, I think you'd see a 1-1 > mapping of views to engine nodes, rather than the current 1-1 mapping of > views to modules. > Makes sense. > > >> Please find my other comments inlined. >> > P.S. I removed the contents of the original patch below since it is big and may hide my inlined comments. And I also corrected a typo in my previous comments (sorry for that) to avoid confusion. >> Thanks, >> Han >> >> >>> >>> - struct sbrec_port_binding *target = >>> >> sbrec_port_binding_index_init_row( >> >>> - sbrec_port_binding_by_datapath); >>> + struct bfd_sbrec_port_binding *target >>> + = bfd_sbrec_port_binding_by_datapath_new_row( >>> >> >> Would it be better to follow same naming convention as the original >> interface, i.e. ...init_row instead of ...new_row? >> >> + sbrec_port_binding_by_datapath); >>> >>> /* Go through whole graph to figure out all chassis which may >>> deliver >>> * packets to gateway. */ >>> @@ -140,7 +135,8 @@ bfd_travel_gw_related_chassis( >>> dp = dp_binding->dp; >>> free(dp_binding); >>> for (size_t i = 0; i < dp->n_peer_dps; i++) { >>> - const struct sbrec_datapath_binding *pdp = dp->peer_dps[i]; >>> + const struct bfd_sbrec_datapath_binding *pdp >>> + = bfd_sbrec_datapath_binding_get(dp->peer_dps[i]); >>> >> >> I have a concern for this kind of conversion for DB records that is >> contained in other data structures. There are 3 problems: >> >> 1. Since local_datapaths is just a hmap, how to link it to the structure >> of >> the hmap node when generating the dependency remains unclear to me. >> >> 2. From the input of this function, it depends on local_datapaths, which >> contains the original version of datapath record, so the function should >> be >> considered as potentially writing the datapath record, no matter if the >> conversion happens here or not, while in fact this function only reads the >> record with the restricted view. So the real dependency is not reflected >> correctly from the interface of the function. >> >> 3. Looking at the how the data structure is generated, it is in >> binding_run(), where a different view of the table is used as input >> (binding_sbrec_datapath_binding), and it is then type converted to the >> generic type sbrec_datapath_binding. This also loses track of the real >> dependency. >> >> Maybe you already have idea how to address these problems in the >> dependency >> auto-generation, but here is my rough idea: >> >> When node B depends on node A, and A has ovsdb table record T as its >> output >> while B take T as input, it requires some coversion between B's view of T >> (TB) and A's view of T. (TA) This coversion can be performed in a generic >> way in the incremental processing engine framework, so that output of B is >> converted as input of A according to the definition of the input and the >> output data structure of engine nodes themselves. >> >> Basically, each node has its own version of data structure, even if they >> in >> fact contains same data (e.g. node B's output has TB in a structure S-B, >> while node A's input has TA in a structure S-A, and S-A is converted from >> S-B. It is simply type convertion without runtime cost. This can be >> achieved with some json *template* similar as the .def files to define >> dependencies explicitely with output <-> input conversion mappings. >> (instead of auto-generate the dependency, we can simplify it with >> auto-validation, but probably you have better idea, like you always did >> :)) >> >> Regarding the problem 1), what I think we can do is to make sure naming >> convention is followed for parameters with type hmap, smap, etc. For >> example, struct hmap *local_datapaths indicates the type of the node is >> struct local_datapath. This doesn't prevent someone from using struct X to >> access a hmap node supposed to be struct Y, but I think it is generic >> problem even without considering the incremental processing. >> >> There is much more details to be figured out, but it leads to the >> discussion of the implementation of dependency auto-generation mechanism. >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
