Add dp_session_query function #8
Conversation
|
Thanks for splitting this into a PR with the dataplane enhancements and moving the plugin and control plane daemon to their own repos/packages. |
|
Related Jira ticket: https://danosproject.atlassian.net/browse/DAN-147 |
|
Please bear with us in reviewing this, some of us need to refresh our memories on the dp session infra and its mechanisms. On the face of it, you're making use of a facility which the original author added, but to date has not been used - the session watch mechanism. Since the code mentions there can only be one watcher per type, we'll need to review the original design docs for why that mechanism was added in the first place. I suspect @pjaitken and/or @gavin-shr should also be looking at these three PRs. |
dungmv56
force-pushed
the
dp_query
branch
2 times, most recently
from
9716106 to
7d537f0
Compare
|
@dungmv56 Are you also interesting in migrating your https://github.com/dungmv56/vyatta-service-flowstat and https://github.com/dungmv56/vyatta-flowstat-plugin repositories into the github.com/danos project and having the feature included as part of the default DANOS community image? |
| return -1; | ||
|
|
||
| info->se_id = s->se_id; | ||
|
|
There was a problem hiding this comment.
If info was populated with all available information, then query wouldn't be necessary which would make it easier to add new information in future.
| /** | ||
| * Session attribute. | ||
| */ | ||
| enum dp_session_attr { |
There was a problem hiding this comment.
Is the need to request specific attrs due to the plugin directly outputting the returned JSON?
The plugin should retrieve the necessary attrs from the JSON and format them into a suitable table.
Therefore it should not matter if additional attrs are returned.
The dataplane could return all of the attrs, and the plugin only output the relevant values.
There was a problem hiding this comment.
The plugin registers a session_watch to hook when session state or stats changed. This was called from dataplane forwarding path and I try to keep plugin not affect forwarding performance as much as possible.
I see that returning all attrs from dataplane make easier for adding new attributes but I afraid it could affect performance. That why I added a query as parameter.
include/dp_session.h
Outdated
| const char *se_app_type; | ||
|
|
||
| // misc | ||
| time_t timestamp; |
There was a problem hiding this comment.
Please use tabs in lines 106-118 so the indentation aligns with the rest of the struct.
There was a problem hiding this comment.
Please add your copyright message to the modified files.
Fixed in 278e9bf
|
Please add your copyright message to the modified files. |
Yes, very happy if it was a part of DANOS community image. |
|
Please squash the fix commits into the original commit. |
That's great to hear. Once these changes to the dataplane are reviewed, and if merged, we can then look at bringing dungmv56/vyatta-service-flowstat and dungmv56/vyatta-flowstat-plugin into DANOS. This will likely consist of creating empty (Couple of initial comments after a quick look at those repos. It would be good to extend the readme in the vyatta-service-flowstat repo with those parts from the readme in vyatta-flowstat-plugin that are concerned with config/op commands. Contributions to the DANOS project need to use the Apache License 2.0. More detailed review of the code and data-model will take place on initial submission by others) Follow-up: I think calling the repos and source packages 'dataplane-flowstat-plugin' and 'danos-service-flowstat' would be appropriate, to avoid re-using the 'vyatta' name. |
|
danos/dataplane-flowstat-plugin and danos/danos-service-flowstat have been created. Please submit PRs with your code. You'll need to update your debian package names to reflect these new repo names. |
|
For the dataplane plugin repos, lintian reports some errors and warnings: |
|
The control plane daemon and data-model repo lintian reports some errors and warnings: |
I have a question about the license. Currently two repos use some modified code from DANOS which has some licenses like LGPL, GPL, BSD. And that result to that repos will be licensed in LGPL/GPL. So, is it allow to license repos as LGPL and make a pull request? |
Yes, if existing LGPL/GPL/BSD code is being copied/modified then that license continues to apply. It's worth mentioning that in the PR. Only new code need use the Apache 2 license. |
In which case, is it worth splitting the stuff with difference license conditions in to distinct files? |
Yes, when building plugin debian package, it used LTO. |
|
These together form the full feature: |
Add support query session information from pipeline plugin. Signed-off-by: Dung Man <[email protected]>
se_sen field in struct session may be NULL in two cases for newly
created sessions not yet added to sentry hash list, or after sesssion
removed from sentry hash list during reclaim in session gc.
This causes the following segmentaion fault infrequently.
[Current thread is 1 (Thread 0x7ff5c3fff700 (LWP 30235))]
#0 0x000055ce31a8978d in csync_get_session_from_init_sentry (
cse=<synthetic pointer>, cs=<synthetic pointer>, sp=0x7ff5f004ef36)
at ../src/npf/csync/csync_session_unpack.c:38
#1 csync_session_unpack_update (csu=0x7ff5f004ef2e)
at ../src/npf/csync/csync_session_unpack.c:71
#2 csync_unpack_session (size=<optimized out>, msg=0x7ff5f004ef26)
at ../src/npf/csync/csync_session_unpack.c:421
#3 csync_recv_session_update (frame=<optimized out>)
at ../src/npf/csync/csync_session_unpack.c:501
#4 0x000055ce31b2d57d in csync_restore_sessions (n=<optimized out>,
flist=<optimized out>) at ../src/csync/csync_transfer.c:218
#5 csync_pull_batch (info=0x7ff58400b880) at
../src/csync/csync_transfer.c:506
#6 csync_xfer_backup (pipe=0x7ff58400b8e0, arg=0x7ff58400b880)
at ../src/csync/csync_transfer.c:301
#7 0x00007ff6629618d3 in ?? () from
/usr/lib/x86_64-linux-gnu/libczmq.so.4
#8 0x00007ff6611ad4a4 in start_thread ()
from /lib/x86_64-linux-gnu/libpthread.so.0
danos#9 0x00007ff660eefd0f in clone () from /lib/x86_64-linux-gnu/libc.so.6
Fixed by doing a safe derefernce and checking for NULL.
VRVDR-54586
Currently, dataplane support a limited info session for pipeline plugins. This adding support query session information from pipeline plugin.
Example:
Additional, this pull request was made to support functionality of flowstat plugin.
vyatta-flowstat-plugin
vyatta-service-flowstat