New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OVS events plugin #1971
OVS events plugin #1971
Conversation
|
Hi @octo , The new OVS plugin depends on YAJL library which is already integrated into collectd. The plugin uses yajl_tree.h API to easily parse JSON string. Unfortunately, some of CI builds are failed because of the following error:
Looks like the problem occurs because of old YAJL lib version pre-installed on some of CI builds. The "yajl_tree.h" API are missing in YAJL v1.x but have been added in YAJL 2.x version (~5 years ago). Failing CI builds:
Is the YAJL lib version 1.x expected for builds above? Should it be changed to use more recent version of YAJL? Thanks and Regards, |
|
Hi @vmytnykx, thank you very much for your patch! Precise and Wheezy often come with quite old library versions and should still work. In this case, the plugin should be disabled by the configure script. You can add something along these lines to Then use the Best regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vmytnykx,
thanks for fixing the configure script! I started a review, but this is quite complex and will take a while to wrap my head around, sorry. I've left some comments that I'll publish right away, should you be blocked on me.
Best regards,
—octo
| union { | ||
| struct sockaddr_in s_inet; | ||
| struct sockaddr_un s_unix; | ||
| } addr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not portable, because struct sockaddr_in may be an "incomplete type", as is the case on FreeBSD.
I recommend to replace the union with:
struct sockaddr_storage addr;That struct is guaranteed to be "large enough to accommodate all supported protocol-specific address structures".
| int sock; | ||
| int domain; | ||
| int type; | ||
| int addr_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a socklen_t for this.
| /* clang-format on */ | ||
|
|
||
| /* collectd headers */ | ||
| #include "common.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please always include "collectd.h" as the first header.
|
|
||
| /* Check if POLL thread is still running. Returns | ||
| * 1 if running otherwise 0 is returned */ | ||
| static inline int ovs_db_poll_is_running(ovs_db_t *pdb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: please remove the inline – it's premature optimization.
The inline statement lead to some confusing effects, such as function pointers no longer comparing equal. On the other hand, compilers are very good at deciding which functions to inline. So we get likely no benefit but the potential for hard to track down bugs.
|
|
||
| /* Check if POLL thread is still running. Returns | ||
| * 1 if running otherwise 0 is returned */ | ||
| static inline int ovs_db_poll_is_running(ovs_db_t *pdb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider changing the return value of this function to _Bool.
| pthread_mutex_lock(&pdb->mutex); | ||
| for (ovs_callback_t *del_cb = pdb->remote_cb; pdb->remote_cb; | ||
| del_cb = pdb->remote_cb) { | ||
| pdb->remote_cb = pdb->remote_cb->next; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At your option: I find the following more intuitive:
pdb->remote_cb = del_cb->next;| pthread_mutex_lock(&pdb->mutex); | ||
| for (ovs_callback_t *cb = pdb->remote_cb; cb != NULL; cb = cb->next) | ||
| if (cb->uid == uid) { | ||
| pthread_mutex_unlock(&pdb->mutex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think cb needs to be protected by pdb->mutex, too, otherwise another thread could call ovs_db_callback_remove_all(), freeing the memory cb points to while the caller still performs work with cb. In other words, I think the caller needs to hold pdb->mutex when calling this function.
| goto failure; | ||
|
|
||
| /* parse url depending on domain */ | ||
| if ((nexttok = strtok_r(NULL, ":", &saveptr)) != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling strtok_r() when configuring an AF_UNIX socket will break paths that contain a colon. If tmp_conn.domain == AF_UNIX, don't call strtok_r() again.
| tmp_conn.addr.s_inet.sin_family = AF_UNIX; | ||
| sstrncpy(tmp_conn.addr.s_unix.sun_path, nexttok, strlen(nexttok) + 1); | ||
| } else { | ||
| /* <IP:PORT> */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't handle IPv6.
I'd recommend to implement a config option that takes either one or two arguments. The first argument is the address, the second argument an optional port, for example:
Address "127.0.0.1" "6640"
Address "2001:DB8::c011:ec7d" "6640"
Address "2001:DB8::c011:ec7d" "service-name"
Address "unix:/path/to/socket"
You can then easily detect UNIX sockets: strncmp (arg1, "unix:", strlen("unix:")) == 0
Otherwise, call getaddrinfo() to get a struct sockaddr: getaddrinfo(arg1, arg2, hints, &res)
|
|
||
| =over 4 | ||
|
|
||
| =item B<OvsDbServerUrl> I<server> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider turning this into an option with two values, "node" and "service" (aka. "host" and "port"). That makes it much easier to support IPv6. See also my comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, it might make sense to copy what some other plugins are doing. They use four config options:
Host
Controls what goes into the value_list_t.host field.
Address
IP address or hostname to connect to.
Port
Port number or service to use when connecting.
Socket
Path for the UNIX socket. If this option is set, Address and Port are ignored.
|
Hi @octo , Thank you for reviewing the plugin implementation and your comments. I've fixed all your comments and re-implement plugin configuration based on your suggestion. Also, IPv6 support has been added (automatically) and verified. Thanks and Regards, |
| } | ||
| DEBUG("name=%s, uuid=%s, ext_iface_id=%s, ext_vm_uuid=%s", ifinfo.name, | ||
| ifinfo.uuid, ifinfo.ext_iface_id, ifinfo.ext_vm_uuid); | ||
| ovs_events_link_status_submit(&ifinfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should you not check if send nofication is enabled at this point? then either dispatch a notification or submit a value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM - this is for values from tables and not events...
|
|
||
| /* OVD DB reply callback. It parses reply, receives | ||
| * interface information and dispatches the info to | ||
| * collecd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vmytnykx,
I've made some more headway. I'm not yet 100% through, but I wanted to share this because I don't know how much bandwidth I'll have this week.
Best regards,
—octo
|
|
||
| =over 4 | ||
|
|
||
| =item B<OvsDbServerUrl> I<server> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, it might make sense to copy what some other plugins are doing. They use four config options:
Host
Controls what goes into the value_list_t.host field.
Address
IP address or hostname to connect to.
Port
Port number or service to use when connecting.
Socket
Path for the UNIX socket. If this option is set, Address and Port are ignored.
|
|
||
| /* node cannot be unset */ | ||
| if (node == NULL || strlen(node) == 0) | ||
| return (NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaks pdb. You could move this check to before the calloc().
| return pdb; | ||
|
|
||
| failure: | ||
| if (pdb->sock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you call ovs_db_destroy(pdb) here?
|
|
||
| if (cb) { | ||
| /* register result callback */ | ||
| if ((new_cb = malloc(sizeof(ovs_callback_t))) == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use:
new_cb = calloc(1, sizeof(new_cb))| if (new_cb) { | ||
| /* destroy callback */ | ||
| sem_destroy(&new_cb->result.sync); | ||
| ovs_db_callback_remove(pdb, new_cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That order seems dangerous; I'd remove the callback first before starting to freeing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this as ovs_db_callback_remove() function actually does this. It takes mutex, remove the callback, frees it and release the mutex.
| if (new_buff == NULL) | ||
| goto failure; | ||
| buff = new_buff; | ||
| ret = ssnprintf(buff + offset, buff_size, option_fmt, iface->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The available buffer size is only buff_size - offset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for that :)
| } | ||
|
|
||
| /* allocate memory for OVS DB select params */ | ||
| buff_size = offset + sizeof(params_fmt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more consistent with the above code to do:
buff_size = sizeof(params_fmt) + strlen(buff);| } | ||
|
|
||
| /* allocate memory for OVS DB select params */ | ||
| buff_size = offset + sizeof(params_fmt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of re-using variables like this. Please consider using char *params and size_t params_size instead.
| #define OVS_EVENTS_EXT_IFACE_ID_SIZE 64 | ||
| #define OVS_EVENTS_EXT_VM_UUID_SIZE 64 | ||
| #define OVS_EVENTS_PLUGIN "ovs_events" | ||
| #define OVS_EVENTS_CTX_LOCK \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever, I love it :)
| 0) | ||
| OVS_EVENTS_CONFIG_ERROR(child->key); | ||
| } else if (strcasecmp("OvsDbAddress", child->key) == 0) { | ||
| if (child->values_num < 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use cf_util_get_string_buffer() here, which will do all necessary checks and error reporting:
cf_util_get_string_buffer(child, ovs_events_ctx.config.ovs_db_node,
sizeof(ovs_events_ctx.config.ovs_db_node));|
Hi @octo, Thank you for the addition review comments. I've tried to address all of them, including
Also, I've replied to some of your comments which seems don't require any changes... Best Regards, |
| =head2 Plugin C<ovs_events> | ||
|
|
||
| The I<ovs_events> plugin monitors the link status of OVS connected interfaces, | ||
| dispatches the values to collectd and send the notification whenever the link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/send/sends/
|
|
||
| =item B<Address> I<node> | ||
|
|
||
| The address of OVS DB server JSON-RPC interface used by the plugin. To enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The address of the OVS DB server JSON-RPC interface
|
|
||
| =item B<Interfaces> [I<ifname> ...] | ||
|
|
||
| List of interface names to be monitored by this plugin. If this option is missed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not specified or is empty
| .ovs_db_serv = "6640", /* use default OVS DB service */ | ||
| .ovs_db_unix = "", /* UNIX path empty by default */ | ||
| .ifaces = NULL}, | ||
| .ovs_db_select_params = NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need to initialize static variables to their zero value.
| "\"where\":[[\"name\",\"==\",\"%s\"]]," | ||
| "\"columns\":[\"link_state\",\"external_ids\"," | ||
| "\"name\",\"_uuid\"]}"; | ||
| const char default_opt[] = ",{\"op\":\"select\",\"table\":\"Interface\"," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make these static?
| opt_buff = new_buff; | ||
| ret = ssnprintf(opt_buff + buff_off, buff_size - buff_off, option_fmt, | ||
| iface->name); | ||
| if (ret < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int ret
| } | ||
| /* if no interfaces are configured, use default params */ | ||
| if (opt_buff == NULL) | ||
| opt_buff = strdup(default_opt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check for failure.
| for (ovs_events_iface_list_t *iface = ovs_events_ctx.config.ifaces; iface; | ||
| iface = iface->next, buff_off += ret) { | ||
| /* allocate new buffer (format size + ifname len is good enough) */ | ||
| buff_size += (sizeof(option_fmt) + strlen(iface->name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe you need the outer braces.
|
|
||
| /* Dispatch OVS interface link status event to collectd */ | ||
| static void | ||
| ovs_events_dispatch_notification(const ovs_events_iface_info_t *ifinfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please combine these two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done by clang-format. I've addressed it anyway but I suppose it will be changed back during next clan-format running :)
|
|
||
| /* Dispatch OVS interface link status value to collectd */ | ||
| static void | ||
| ovs_events_link_status_submit(const ovs_events_iface_info_t *ifinfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please combine these two lines.
|
|
||
| /* register update link status event if needed */ | ||
| if (ovs_events_ctx.config.send_notification) { | ||
| ret = ovs_db_table_cb_register(pdb, tb_name, columns, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int ret.
|
Hi @rubenk, last commit addresses your comments. Thank you. |
| installed. | ||
| Detailed instructions for installing and setting up Open vSwitch, see | ||
| OVS documentation. | ||
| <http://openvswitch.org/support/dist-docs/INSTALL.md.html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, that link gives a 404. Should this point to https://github.com/openvswitch/ovs/blob/master/INSTALL.rst instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right! OvS has changed it (*.md -> *.rst, ) after the PR has been created. Thank you for discovering this :) I will fix it to the right one: http://openvswitch.org/support/dist-docs/INSTALL.rst.html.
|
Hi @octo, Do you have any farther comments regarding the changes? Sorry to ping you again but we have already implemented new plugin that rely on OVS utils API (utils_ovs.[ch]). Even, if we create new PR with the new changes based on that branch, it will also include #1971 changes, which will confuse the review process (also, rebasing will be required, etc., once current changes are merged). If you have some idea how to share the new code in this case, that would be appreciated too. FYI, new changes: https://github.com/maryamtahhan/collectd/compare/feat_ovs_link...maryamtahhan:feat_ovs_stats?expand=1 Thanks and Regards, |
This plugin consists of two parts:
- OVS link
The implementation of the plugin itself, which uses
OVS utils API to be able to monitor a link status of
OVS connected interfaces and dispatch the values
through collectd notification mechanism whenever
the link state change occurs.
- OVS utils
This module implements the OVS DB communication routine
specified by RFC7047. It includes:
- Connecting/disconnecting to/from OVS DB (via TCP/UNIX);
- Mechanism to subscribe to OVS DB table events like
init/insert/modify/delete table rows;
- API to send custom request and receive result;
- Recovery connection mechanism with OVS DB;
- Handling of ECHO request to verify the liveness
of a database connection;
- Helpers functions.
Change-Id: Icac392bd1bd40f7dd156bfd2fc4ff08d9725a22f
Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Implement read callback; - Add SendNotification option; - Update plugin documentation; - Clean-up. Change-Id: Ie645192498d234d47e1fe0272ec30e7c3d9c1774 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
Extend OVS utils to notify OVS plugin about OVS DB connection lost. If the connection is lost, OVS link plugin will dispatch notification and print error message. OVS plugin treats status of all interfaces as UNKNOWN. Change-Id: I22fe3cb0740e0f4779a5c4f6b92e78f1ad9777a3 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
Change-Id: Ic50891722074aec017b1329601a3bcbafb030ce0 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Rename files Change-Id: Ic662d5d673c1c66b2057e4b35fa3cf664e92825e Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Interface meta data are added to dispatched
values & notification.
interface metadata:
- UUID (always);
- iface-id (if exists)
- vm-uuid (if exists)
- Fix read callback to poll actual data instead of
reading cache;
- Code clean up;
Change-Id: I291cb190d31ae091c1d47ce0f5e9d439f6958d61
Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Fix Ubuntu compilation warnings. Change-Id: I7bb72ec0ca732b00c4169e0174d64407d99a2751 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
Disable clang-format for diagram in utils_ovs.c. Change-Id: Idfb1510c996f5ebbdadc96663eb032537837502a Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
Change-Id: I731b633b5f78ed63b643574a336c59717e408a78 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
If old YAJL version is installed on a system (ver < 2.0), the OVS plugin compilation will fail as it requires tree API to be supported in YAJL library. For this reason, it was decided to change 'configure' script to detect YAJL tree API also. Clean-up according updated collectd style: collectd#1931 collectd#1951 Change-Id: I90c82cdc9780ee8c0c9b794986662a39a5ab0011 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Change OVS implementation to use suggested configuration: OvsDbAddress "127.0.0.1" "6640" OvsDbAddress "2001:DB8::c011:ec7d" "6640" OvsDbAddress "2001:DB8::c011:ec7d" "service-name" OvsDbAddress "unix:/path/to/socket" - Update documentation; - Change OVS utils to use getaddrinfo(); - Clean-up compilation warnings on FreeBSD system. - Add IPv6 support; Change-Id: I60ca700c15406c783b62ee52135266d67b60393a Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
If multiple interfaces are configured, only last one will be monitored (notifications aren't sent for all others). Change-Id: Ica918ef0557747d84677db739a64698d3756a675 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Change configuration format to suggested one; - Fix init/destroy API; - Fix memory leaks; - Code-clean-up. Change-Id: I1ff94271b777c69f3d07a66f43dc10d034e71101 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
If some of interface data is not available, the garbage information may be returned by ovs_events_get_iface_info() function. Add clean-up interface information structure into the function. Change-Id: Ia3d2bdfe31d0b3db81ad7ad773eca1df5d5f6f6a Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
Clean-up OVS events plugin based on PR comments. Change-Id: Ibd18924dd2a6f936d0ea83ed4eeb3b34ff8416b5 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
Change-Id: I5bd4d7aabaf0ff66edd9c1cd75caf7f37448e00d Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- Fix OVS documentation - Code clean-up Change-Id: I84fb003aea19f73381192f31935c79b51eaba1c9 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
- the prototype of plugin_thread_create () has been changed in latest master. Change-Id: I492942442717c43cd6e6e73478248164b5083894 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
8732da7
to
f5bbbcd
Compare
The location of OVS sources in Malefile.am is `src/*` folder now. Change-Id: Iffe8a439faf531e74e0a752c3fb544369928cfb6 Signed-off-by: Mytnyk, VolodymyrX <volodymyrx.mytnyk@intel.com>
|
Hi @rubenk , I've done re-base second time because of the recent changes into build system (src/Makefile.am -> Makefile.am). So, now conflicts are resolved. Since, there are no more comments, can this be merged to avoid further re-basing ? Thanks & Regards, |
|
Hi, @rubenk , the This failure isn't related to OVS changes. Based on this comment looks like it is expected, right? Thanks, |
|
Dear Volodymyr, yes, we've dropped support for EPEL5 but still need to adjust the CI. Now merged, thanks for all your work! |
The OVS events plugin monitors the link status of OVS connected interfaces, dispatches the values to collectd and send the notification whenever the link state change occurs in OVS DB.
The plugin consists of two parts: