Improve zeroconf discovery #710
Conversation
…emote.admin.shm.rpc_type property for RSA_SHM configuration type
…ove_zeroconf_discovery # Conflicts: # bundles/remote_services/rsa_spi/include/rsa_rpc_factory.h
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #710 +/- ##
==========================================
+ Coverage 88.85% 89.32% +0.47%
==========================================
Files 216 216
Lines 24293 25153 +860
==========================================
+ Hits 21585 22469 +884
+ Misses 2708 2684 -24 ☔ View full report in Codecov by Sentry. |
libs/utils/error_injector/celix_string_hash_map/include/celix_string_hash_map_ei.h
Show resolved
Hide resolved
| (void)snprintf(scope, sizeof(scope), "(&(%s=*)(%s=%s))", CELIX_FRAMEWORK_SERVICE_NAME, | ||
| OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, fwUuid); | ||
| celix_properties_set(props, OSGI_ENDPOINT_LISTENER_SCOPE, scope); | ||
| celix_properties_set(props, "DISCOVERY", "true");//Only use to avoid the discovery calls to unnecessary endpoint listener service.Endpoint should be filtered by the scope. |
There was a problem hiding this comment.
I am not sure if I get this. As also stated in the comment, I expected that the OSGI_ENDPOINT_LISTENER_SCOPE was enough. Is the "DISCOVERY" property added for additional performance optimization?
OSGi EndpointListener doc:
Endpoint Listener services can express their scope with the service property ENDPOINT_LISTENER_SCOPE. This service property is a list of filters. An Endpoint Description should only be given to a Endpoint Listener when there is at least one filter that matches the Endpoint Description properties. This filter model is quite flexible. For example, a discovery bundle is only interested in locally originating Endpoint Descriptions. The following filter ensure that it only sees local endpoints.
(org.osgi.framework.uuid=72dc5fd9-5f8f-4f8f-9821-9ebb433a5b72)
There was a problem hiding this comment.
Is the "DISCOVERY" property added for additional performance optimization?
yes, ENDPOINT_LISTENER_SCOPE is enough to ensure that the endpoint listener of discovery only sees local endpoint.
There was a problem hiding this comment.
Ok. I see this is used in discovery_common / discovery_endpointListenerAdded. I am not a fan of this approach, but IMO we can keep this for now.
| @@ -532,7 +644,8 @@ static void *discoveryZeroconfAnnouncer_refreshEndpointThread(void *data) { | |||
| discoveryZeroconfAnnouncer_handleMDNSEvent(announcer); | |||
| } | |||
| } else if (result == -1 && errno != EINTR) { | |||
| celix_logHelper_error(announcer->logHelper, "Announcer: Error Selecting event, %d.", errno); | |||
| celix_logHelper_error(announcer->logHelper, "Announcer: Error Selecting event, %d.", errno); | |||
| sleep(1);//avoid busy loop | |||
There was a problem hiding this comment.
I'm not familiar with this part yet, and thus will only leave my two cents here.
While errno == ENOMEM suggests a temporary failure, for which sleep(1) does make sense, errno == EBADF may imply that the bundle could not work any more, e.g. the eventFd or dsFd may be accidentally closed by some buggy code.
We can really do nothing locally to recover from this kind of error. This represents some long lasting exceptional system condition, which can luckily be modeled using Celix condition. Once some outside watchers get notified of this system exception, he/she can try to restart the bundle, which could fix the accidentally closed fd in this case.
I think generic system exception is a generally useful concept, which further extends error code and C++ exception. Combined with RSA, this means these exceptions can be watched remotely (cool). Event better, we don't need descriptors for Remote Celix conditions because they have no methods.
WDYT? @pnoltes @xuzhenbao
There was a problem hiding this comment.
Concerning the sleep(1) / retry on a error rc. I think this is fine, but currently the running is not updated in the else branch of if (result > 0) and this is an issue.
I think the running update can be moved to the end part of the while loop:
while (running) {
....
celixThreadMutex_lock(&announcer->mutex);
running = announcer->running;
celixThreadMutex_unlock(&announcer->mutex);
}
ensuring that running is updated every loop.
There was a problem hiding this comment.
We can really do nothing locally to recover from this kind of error. This represents some long lasting exceptional system condition, which can luckily be modeled using Celix condition. Once some outside watchers get notified of this system exception, he/she can try to restart the bundle, which could fix the accidentally closed fd in this case.
I think generic system exception is a generally useful concept, which further extends error code and C++ exception. Combined with RSA, this means these exceptions can be watched remotely (cool). Event better, we don't need descriptors for Remote Celix conditions because they have no methods.
WDYT? @pnoltes @xuzhenbao
I think remote conditions are very useful and indeed this does not need a descriptor, because conditions are marker interfaces. remote conditions combined with a dependency management component means that components can react (start/stop) based on the presence of a condition in a different framework.
Currently I think the RSA expect to find a dfi descriptor, so maybe an extensions is needed to service exported with descriptors are handled as marker interface services.
btw of course out of scope for this PR.
| if (bytes >= sizeof(filter)) { | ||
| celix_logHelper_error(logHelper,"RSA import reg: The value(%s) of %s is too long.", rsaRpcType, RSA_RPC_TYPE_KEY); | ||
| celix_logHelper_error(logHelper,"RSA import reg: The value(%s) of %s is too long.", rsaShmRpcType, RSA_RPC_TYPE_KEY); |
There was a problem hiding this comment.
nitpick: incorrect indentation
| char *savePtr = NULL; | ||
| char *token = strtok_r(ipStrListCopy, ",", &savePtr); | ||
| while (token != NULL) { | ||
| char *ip = celix_utils_trimInPlace(token); |
There was a problem hiding this comment.
Does it mean that each time we perform an RPC, we need to do these strdup strtok_r inet_pton things? It sounds bad.
There was a problem hiding this comment.
IP address is leaked from the service discovery into RSA, and discovery_zero_config is leaked into RSA. All service discoveries should hide behind a uniform interface, which ideally should be service URLs. RSA should only deal with service URL and all address resolving complexity and inherent dynamism for distributed system should be dealt within service discovery. There shouldn't be any special treatment for zero_config in RSA.
And these strdup strtok_r inet_pton things seem performance nightmare.
| @@ -36,6 +36,15 @@ | |||
| #define RSA_DFI_CONFIGURATION_TYPE "org.amdatu.remote.admin.http" | |||
| #define RSA_DFI_ENDPOINT_URL "org.amdatu.remote.admin.http.url" | |||
|
|
|||
| /** | |||
| * @brief RSA Configuration type for zeroconf http, it is synonymous(https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.remoteservices.html#i1698916) with RSA_DFI_CONFIGURATION_TYPE, they refer to the same endpoint. | |||
There was a problem hiding this comment.
I understand this different.
As I understand it, the service.exported.configs and service.imported.configs refer to remote service admin configurations and not discovery.
Different discovery mechanism should work without knowledge of the remote service admins used and vica versa.
For example
- RSA DFI adds a "org.amdatu.remote.admin.http.url" property when creating a endpoint for an export service.
- Discovery (Zeroconf, configured or etcd) "just" announces and discovers the endpoints.
- RSA DFI checks the config when importing a service through a endpoint and uses the "org.amdatu.remote.admin.http.url".
It is possible to support configuration and endpoint with a "ifname", "port", "ipdresses" and "path" next to a "url" property, but IMO this is then an extension to RSA DFI and not a ZEROCONF concern.
For example (snippet):
if (strncmp(configType, RSA_DFI_CONFIGURATION_TYPE, 1024) == 0) {
const char *url = celix_properties_get(endpoint->properties, RSA_DFI_ENDPOINT_URL, NULL);
if (url) {
return true; //RSA_DFI config with endpoint url
}
const char *ip = celix_properties_get(endpoint->properties, CELIX_RSA_DFI_ZEROCONF_ENDPOINT_IPADDRESSES, NULL);//the property is set by the discovery_zeroconf
const char *port = celix_properties_get(endpoint->properties, CELIX_RSA_DFI_ZEROCONF_ENDPOINT_PORT, NULL);
const char *path = celix_properties_get(endpoint->properties, CELIX_RSA_DFI_ZEROCONF_ENDPOINT_PATH, NULL);
if (ip != NULL || port != NULL || path != NULL) {
return true; //RSA_DFI config with a endpoint ip, port and path/
}
}
return false;There was a problem hiding this comment.
Here are some inconsistencies between RSA and ZEROCONF: RSA may not bind all (i.e. bind to a specific ip address) while ZEROCONF will advertise all addresses associated with a network interface. The interface between RSA and ZEROCONF should be carefully designed to address this issue. Otherwise, there will be performance penalty, e.g. try addresses that have no binder.
There was a problem hiding this comment.
IMO the zeroconf discovery and remote service admins are now entangled and this is not needed.
Adding the host name resolution mechanism to zeroconf_discovery does cause RSA to become entangled with zeroconf_discovery.
The main reason for adding the host name resolution mechanism to zeroconf_discovery is that if RSA binds to any address (0.0.0.0), then it does not need to configure a specific IP, and zeroconf_discovery resolves IP list from the host name, then gives the IP list to the client via the " <config_type>.ipaddresses" endpoint property. It means that if we use zeroconf_discovery, the remote service will still work even if RSA does not configure an IP.
For the above issue, should we do host name resolution in zeroconf_discovery?
If we use host name in the URL of the endpoint (e.g. http://computer.local:8080/service/example) maybe that will resolve the problem as well. (Host name resolution will be done by the RSA)
If we do host name resolution in zeroconf_discovery, we should design interfaces between RSA and zeroconf_discovery for distinguishing whether host name resolution is needed or not, because we don't need to do host name resolution for an endpoint that already specifies a specific IP. In my option, we can add a specific property(e.g. <config_type>.automatically_get_ip_by_discovery) to the endpoint description, and zeroconf_discovery will do host name resolution according to this property.
There was a problem hiding this comment.
It means that if we use zeroconf_discovery, the remote service will still work even if RSA does not configure an IP.
That is a nice feature and for now I cannot think of a way to this without some coupling between the RSA and discovery.
The option is what you mention is a good way to opt-in for discovery specific behaviour.
Maybe more generic and not coupled to zeroconf. e.g:
- If a remote endpoint has property has a property "CELIX_RSA_DISCOVERY_IP_ADRESSES" then the value will be updated to discovered ip addresses.
If I look at the RSA spec (https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.remoteserviceadmin.html#org.osgi.service.remoteserviceadmin.RemoteServiceAdmin), exportService does allow to return multiple ExportRegistration and thus multiple endpoints.
Maybe this can be combined and the RSA DFI can provide mutiple ExportRegistrations. 1 for the fixed IP and 1 for a dynamic IP adresses.
I will read the spec a bit more and check whether exporting multiple ExportRegistration (with multiple paths to the same exported service) is feasible.
There was a problem hiding this comment.
I have some new thoughts about the dynamic ip assignment resolving challenges.
First off, I think is good to come to a sound conclusion, but this does not mean that this should be directly solved in this pull request.
So if we agree on a desired solution, I will make a ticket for this and we can then decide what should flow back to this PR.
My thoughts:
a) Let RSA implementations decide if they support "dynamic IP fill-in" by providing a additional service property on the registered RSA service
b) Optionally RSA export registration that support dynamic IP fill-in should also opt-int for dynamic replacement by adding a endpoint property to the export registration properties
c) Let discovery implementations decide if the they support "interface specific" endpoints by providing a additional service property on the registered endpoint listener interface.
c) Update the Topology Manager to detect RSA services what support dynamic IP fill-in and in that case create multiple endpoints for a single export registration based on the available network interfaces and an optional network selection configuration. These endpoints are then forwarded to the discovery listener services that support network specific endpoints. The discovery implementation can pick and handle or drop endpoints based on the selected network interface.
The primary idea is that most of the boiler-plating to support dynamic IP fill-in is moved to the topology manager and kept away from the RSA and discovery implementations.
For the service/endpoint property something like the following can be used:
- RSA Dynamic IP fill-in support
- Property name: "rsa.dynamic.ip.support"
- Type boolean
- Indicates whether the RSA implementation supports dynamic IP address fill-in for service exports
- RSA Export Registration replacement opt-in
- Property name: rsa.export.dynamic.ip.optin
- Type boolean
- Description: Signals if a service export opts in for dynamic IP address replacement
- RSA Export Registration dynamic replaceable property
- Property name: rsa.ip.address
- Type string
- Description: The dynamically determined IP address for the endpoint.
- Discovery interface-specific endpoints support.
- Property Name: "discovery.interface.specific.endpoints.support
- Type boolean
- Description: Indicates if the discovery implementation supports handling of interface-specific endpoints.
I understand that this is different from the current solution, but maybe a more cleaner approach.
WDYT?
There was a problem hiding this comment.
Thank you for your reply.
First, I agree with this solution. But I still have some issues that need to confirm with you.
Update the Topology Manager to detect RSA services what support dynamic IP fill-in and in that case create multiple endpoints for a single export registration based on the available network interfaces and an optional network selection configuration. These endpoints are then forwarded to the discovery listener services that support network specific endpoints. The discovery implementation can pick and handle or drop endpoints based on the selected network interface.
Does this mean that the endpoint descriptions for dynamic IP are generated by Topology Manager when exporting service?
Should the network interface used to publish services be specified by Topology Manager or RSA?
How about dealing with it in two cases:
In the first case, if the RSA supports dynamic IP, RSA should bind its services to ANY address (0.0.0.0). Then the Topology Manager should specify which network interfaces the services are published to, and restrict access to the corresponding services from other network interfaces (e.g.: iptables -A INPUT -i eth0 -p tcp -- dport 80 -j DROP).
In the second case, if RSA does not support dynamic IP, RSA binds its network services to a specific IP, and its exported endpoint should specify which network interface the service is published to.
In the above case, we will define a generic endpoint property rsa.ifnames (Type string list). The property is used to specify which network interfaces the service is published to.
RSA Export Registration dynamic replaceable property
- Property name: rsa.ip.address
- Type string
When using dynamic IP, perhaps multiple IP are obtained, should the rsa.ip.address type be a string list?
There was a problem hiding this comment.
Thank you for your reply.
First, I agree with this solution. But I still have some issues that need to confirm with you.
Update the Topology Manager to detect RSA services what support dynamic IP fill-in and in that case create multiple endpoints for a single export registration based on the available network interfaces and an optional network selection configuration. These endpoints are then forwarded to the discovery listener services that support network specific endpoints. The discovery implementation can pick and handle or drop endpoints based on the selected network interface.
Does this mean that the endpoint descriptions for dynamic IP are generated by Topology Manager when exporting service? Should the network interface used to publish services be specified by Topology Manager or RSA?
Good point, this should indeed be the RSA.
How about dealing with it in two cases: In the first case, if the RSA supports dynamic IP, RSA should bind its services to ANY address (0.0.0.0). Then the Topology Manager should specify which network interfaces the services are published to, and restrict access to the corresponding services from other network interfaces (e.g.:
iptables -A INPUT -i eth0 -p tcp -- dport 80 -j DROP).In the second case, if RSA does not support dynamic IP, RSA binds its network services to a specific IP, and its exported endpoint should specify which network interface the service is published to.
In the above case, we will define a generic endpoint property
rsa.ifnames(Type string list). The property is used to specify which network interfaces the service is published to.
That does indeed sound better. One of my cancern was to prevent too much repeated boiler plaiting between the RSA bundles, but indeed the RSA is binding on network interface and hence it should decide and informs whether is uses a single or multiple interfaces.
RSA Export Registration dynamic replaceable property
- Property name: rsa.ip.address
- Type string
When using dynamic IP, perhaps multiple IP are obtained, should the
rsa.ip.addresstype be a string list?
List of string, but this is not yet supported. I am busy with a branch that updates properties for (among others) list of strings: https://github.com/apache/celix/tree/feature/674-improve-properties
I will make a ticket for yet another improvement on remote service and let it depend on #674
For this PR this remark can now be ignored.
| (void)snprintf(scope, sizeof(scope), "(&(%s=*)(%s=%s))", CELIX_FRAMEWORK_SERVICE_NAME, | ||
| OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, fwUuid); | ||
| celix_properties_set(props, OSGI_ENDPOINT_LISTENER_SCOPE, scope); | ||
| celix_properties_set(props, "DISCOVERY", "true");//Only use to avoid the discovery calls to unnecessary endpoint listener service.Endpoint should be filtered by the scope. |
There was a problem hiding this comment.
Ok. I see this is used in discovery_common / discovery_endpointListenerAdded. I am not a fan of this approach, but IMO we can keep this for now.
| assert(announcer->endpoints != NULL); | ||
| if (endpoints == NULL) { | ||
| celix_logHelper_logTssErrors(logHelper, CELIX_LOG_LEVEL_ERROR); | ||
| celix_logHelper_fatal(logHelper, "Announcer: Failed to create endpoints map."); |
There was a problem hiding this comment.
I think the log level should be error. This is what we currently describe in the development guide:
error: This level should be used to report issues that need immediate attention and might prevent the system from functioning correctly. These are problems that are unexpected and affect functionality, but not so severe that the process needs to stop. Examples include runtime errors, inability to connect to a service, etc.
fatal: Use this level to report severe errors that prevent the program from continuing to run. After logging a fatal error, the program will typically terminate.
Optionally we can also update the development guideline if we think situations like ENOMEM should be reported as fatal.
There was a problem hiding this comment.
IMO, ENOMEM should not be fatal.
| @@ -532,7 +644,8 @@ static void *discoveryZeroconfAnnouncer_refreshEndpointThread(void *data) { | |||
| discoveryZeroconfAnnouncer_handleMDNSEvent(announcer); | |||
| } | |||
| } else if (result == -1 && errno != EINTR) { | |||
| celix_logHelper_error(announcer->logHelper, "Announcer: Error Selecting event, %d.", errno); | |||
| celix_logHelper_error(announcer->logHelper, "Announcer: Error Selecting event, %d.", errno); | |||
| sleep(1);//avoid busy loop | |||
There was a problem hiding this comment.
Concerning the sleep(1) / retry on a error rc. I think this is fine, but currently the running is not updated in the else branch of if (result > 0) and this is an issue.
I think the running update can be moved to the end part of the while loop:
while (running) {
....
celixThreadMutex_lock(&announcer->mutex);
running = announcer->running;
celixThreadMutex_unlock(&announcer->mutex);
}
ensuring that running is updated every loop.
| @@ -532,7 +644,8 @@ static void *discoveryZeroconfAnnouncer_refreshEndpointThread(void *data) { | |||
| discoveryZeroconfAnnouncer_handleMDNSEvent(announcer); | |||
| } | |||
| } else if (result == -1 && errno != EINTR) { | |||
| celix_logHelper_error(announcer->logHelper, "Announcer: Error Selecting event, %d.", errno); | |||
| celix_logHelper_error(announcer->logHelper, "Announcer: Error Selecting event, %d.", errno); | |||
| sleep(1);//avoid busy loop | |||
There was a problem hiding this comment.
We can really do nothing locally to recover from this kind of error. This represents some long lasting exceptional system condition, which can luckily be modeled using Celix condition. Once some outside watchers get notified of this system exception, he/she can try to restart the bundle, which could fix the accidentally closed fd in this case.
I think generic system exception is a generally useful concept, which further extends error code and C++ exception. Combined with RSA, this means these exceptions can be watched remotely (cool). Event better, we don't need descriptors for Remote Celix conditions because they have no methods.
WDYT? @pnoltes @xuzhenbao
I think remote conditions are very useful and indeed this does not need a descriptor, because conditions are marker interfaces. remote conditions combined with a dependency management component means that components can react (start/stop) based on the presence of a condition in a different framework.
Currently I think the RSA expect to find a dfi descriptor, so maybe an extensions is needed to service exported with descriptors are handled as marker interface services.
btw of course out of scope for this PR.
…ove_zeroconf_discovery # Conflicts: # bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c # bundles/remote_services/rsa_rpc_json/gtest/src/RsaJsonRpcUnitTestSuite.cc
…linux-build-apt workflow
|
I have updated the PR, it includes the following changes: 1.Move the work of exporting dynamic IP endpoints to the Topology Manager, the dynamic IP endpoints contain the following specific properties.
|
Thanks for the trigger, I will try to find some time to review this in the coming days. |
| } | ||
| bool dynamicIpSupport = celix_bundleContext_getPropertyAsBool(ctx, CELIX_RSA_DFI_DYNAMIC_IP_SUPPORT, CELIX_RSA_DFI_DYNAMIC_IP_SUPPORT_DEFAULT); | ||
| if (dynamicIpSupport) { | ||
| status = celix_properties_setBool(props, CELIX_RSA_DYNAMIC_IP_SUPPORT, true); |
| } | ||
|
|
||
| int size = celix_arrayList_size(ifNames); | ||
| for (int i = 0; i < size; ++i) { |
There was a problem hiding this comment.
👍 , indeed multiple endpoints based on dynamic intf resolution
There was a problem hiding this comment.
I'm still not familiar with this part of code base, and thus will not be able to review its overall design. Instead, I had a look at the hot path (i.e. remoteServiceAdmin_send) to see how various parts fit together.
This PR could be merged once the latest issue regarding CURL is addressed.
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
Outdated
Show resolved
Hide resolved
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
Outdated
Show resolved
Hide resolved
Fix some issues of #690 and improve
zeroconf_discovery.1.Improve
zeroconf_discoveryand add hostname resolving mechanism.2.Rename remote service bundles.
3.Add zeroconf configuration type for
rsa_dfi.4.Add
remote.configs.supportedproperty forremote_service_adminservice.