Community
Participate
Working Groups
Tom Watson wrote in https://bugs.eclipse.org/bugs/show_bug.cgi?id=338113#c1: > Take this all with a grain of salt, but I have some suggestions for > RegionFilter. > > In my opinion the filter of resources should be a generic as possible. Right > now RegionFilter splits shared resources into three categories. > > - allowed bundles > - java packages > - osgi services > > It may be better to use the namespace approach used by OSGi for the generic > requirements/capabilities. > > For example, there are three built-in namespaces in osgi > > osgi.wiring.package - to represent java package capabilities/requirements > > osgi.wiring.bundle - to represent bundle capability/requirement for resolving > Require-Bundle > > osgi.wiring.host - to represent host capability/requirement for resolving > Fragment-Host > > OSGi leaves all other namespaces that are not prepended by osgi.* to be generic > capabilities/requirements as specified by the Provide-Capability and > Require-Capability headers. > > What you could do is define your own namespace constants for sharing services > and allowing bundles across regions. Something like this: > > virgo.allow.bundle - namespace for allowing bundles into a region > virgo.allow.service - namespace for allowing services into a region > > Then you could replace the the six methods on RegionFilter with this: > > public void setPolicy(String namespace, Filter policy); > public Filter getPolicy(String namespace); > > Where the Map uses the namespace as the key (e.g. osgi.wiring.package etc.) and > the value is the Filter for matching the shared resource. > > The virgo.allow.service namespace would be used by the service hooks to call > Filter.match(ServiceReference) method, much like I imagine you do today. > > Filter filter = regionFilter.getPolicy("virgo.allow.service"); > boolean allow = > filter == null ? false : filter.match(serviceReference); > > > In the resolver hook you would call something like this: > > Filter filter = regionFilter.getPolicy(BundleCapability.getNamespace()); > boolean allow = > filter == null ? false : filter.matches(BundleCapability.getAttributes()); > > The tricky part is the virgo.allow.bundle namespace for bundle hooks. Here you > would have to create a Map to hold the BSN and version > > Filter filter = regionFilter.getPolicy("virgo.allow.bundle"); > Map<String, Object> bundleAttrs = new HashMap(); > bundleAttrs.put("bundle-symbolic-name", bundle.getSymbolicName()); > bundleAttrs.put("bundle-version", bundle.getVersion()); > boolean allow = > filter == null ? false : filter.matches(bundleAttrs); > > Perhaps that would pose a performance issue and you could decide to keep the > allowBundle and isBundleAllowed methods on RegionFilter.
One consideration here is that we need to serialise the region digraph, including the regions filters, when dumping OSGi state for offline analysis of resolution errors (see bug 336941).
(In reply to comment #1) > One consideration here is that we need to serialise the region digraph, > including the regions filters, when dumping OSGi state for offline analysis of > resolution errors (see bug 336941). Filter should be serialized by converting them toString(). Then deserialize using BundleContext.createFilter(). I recommend using the BundleContext to create the filter impl instead of FrameworkUtil.createFilter because it allows the framework to create an impl that takes advantage of internal optimizations. For example, this helps improve the Filter.match(ServiceReference) performance.
(In reply to comment #2) > (In reply to comment #1) > > One consideration here is that we need to serialise the region digraph, > > including the regions filters, when dumping OSGi state for offline analysis of > > resolution errors (see bug 336941). > > Filter should be serialized by converting them toString(). Then deserialize > using BundleContext.createFilter(). I recommend using the BundleContext to > create the filter impl instead of FrameworkUtil.createFilter because it allows > the framework to create an impl that takes advantage of internal optimizations. > For example, this helps improve the Filter.match(ServiceReference) > performance. That makes sense because the serial form of a digraph will then not expose the implementation types of a filter. Thanks!
I have run into an issue with expressing package import policies with a generic filter. All was going great in my investigation. I would express import package policies using the namespace defined in BundleRevision for packages. for example: (&(osgi.wiring.package=foo)(version>=1.0.0))) This represents an import of the foo package with version >= 1.0.0. But this makes it rather difficult to enforce mandatory attributes specified on export package capabilities in a generic way. I am wondering if this is important to enforce at the RegionFilter level. No bundle is going to be able to wire to an export package capability with a mandatory attribute unless they specify the mandatory attributes themselves on their Import-Package requirement. In fact the resolver is required to filter out any capabilities the importer does not match (including mandatory attrs) before invoking the resolver hook for the requirement. So the list of capabilities will never contain package capabilities with mandatory attributes which are not specified by the requirement.
(In reply to comment #4) > I have run into an issue with expressing package import policies with a generic > filter. All was going great in my investigation. I would express import > package policies using the namespace defined in BundleRevision for packages. > for example: > > (&(osgi.wiring.package=foo)(version>=1.0.0))) > > This represents an import of the foo package with version >= 1.0.0. But this > makes it rather difficult to enforce mandatory attributes specified on export > package capabilities in a generic way. I am wondering if this is important to > enforce at the RegionFilter level. No bundle is going to be able to wire to an > export package capability with a mandatory attribute unless they specify the > mandatory attributes themselves on their Import-Package requirement. In fact > the resolver is required to filter out any capabilities the importer does not > match (including mandatory attrs) before invoking the resolver hook for the > requirement. So the list of capabilities will never contain package > capabilities with mandatory attributes which are not specified by the > requirement. I agree with your analysis, but there is another case to consider (whether it really matters is something we can discuss later). Support the kernel contained a bundle which exported a package with a mandatory matching attribute and the user region properties file specified that package in packageImports but did *not* include the mandatory matching attribute in the packageImports statement. Should a bundle in the user region which imports the package with the mandatory attribute be allowed to wire to the package in the kernel region? I believe the answer for Virgo today is "no". So the philosophy here is that the packageImports statement should act like an import-package statement so far as matching goes. This is consistent with the view of a user region as a composite bundle. If we change this behaviour, then the user region properties file can no longer be modelled on a view of the user region as a composite bundle, so we'd have to find some other way of thinking about it. Perhaps users can think of it in terms of filtering instead, but that's quite a subtle change so we have to be careful. For instance, a tricky use case would be explaining to a Virgo 2.1 user how to migrate to 3.0 as (I think) it would be necessary to understand both models in the explanation.
(In reply to comment #5) > So the philosophy here is that the packageImports statement should act like an > import-package statement so far as matching goes. This is consistent with the > view of a user region as a composite bundle. OK, if this is important to solve then we have a couple of options. 1) As an implementation detail of the RegionFilter we can do some extra evaluation of the filter string to determine if it has the mandatory attrs specified. This may be costly unless we can take advantage of equinox Filter implementation which gives you access to the attribute keys of a filter. 2) Leave import package policy as a special case that allows packages import policies to be specified in a way to make it easy to evaluate mandatory attributes. I am beginning to think that my original proposal needs to be expanded to allow for a List<Filter> instead of a single Filter for each namespace. Otherwise you will have to construct large or'ed filters for big lists of packages. For example: (| (&(osgi.wiring.package=foo)(version>=1.0)) (&(osgi.wiring.package=bar)(version>=1.0)) (&(osgi.wiring.package=baz)(version>=1.0)) ) Such a filter will be hard to determine mandatory attribute matching if we go with option 1. It may be better to specify this as three different filters that are implicitly or'ed together.
Wouldn't it be better to extend the Equinox Filter implementation so we can encode mandatory attributes in the filter? I feel a bit uneasy about using List<Filter> as it implies a linear search and the list of packages may be quite long in some scenarios. Alternatively, maybe a map from package name to filter would be an efficient optimisation we could employ if and only if required.
(In reply to comment #7) > Wouldn't it be better to extend the Equinox Filter implementation so we can > encode mandatory attributes in the filter? But the mandatory attributes are specified on the provider side with a mandatory directive. I may have missed something, but could you explain how mandatory attributes could be specified on the requirement side? Are you suggesting that a new method be available on Filter to specify mandatory attributes: Filter.match(Map<String, ?> attrs, List<String> mandatoryKeys) > I feel a bit uneasy about using > List<Filter> as it implies a linear search and the list of packages may be > quite long in some scenarios. Alternatively, maybe a map from package name to > filter would be an efficient optimisation we could employ if and only if > required. Hmmm, I know the current Filter implementations are linear when dealing with OR'ed filters. The optimization you suggest would still require us to split apart the OR'ed filter into a list of filters so we could key off the osgi.wiring.package attribute key. We could make it a Collection<Filter> if you like then there is no order implied and we can evaluate them in any order we like and use what ever indexing we like at the implementation level. It is not the end of the world if we have to copy some of the FilterImpl code to do the parsing ourselves if you really want to pick apart OR'ed filters, but it seems like it may be better to simply allow a Collection of filter.
(In reply to comment #8) > (In reply to comment #7) > > Wouldn't it be better to extend the Equinox Filter implementation so we can > > encode mandatory attributes in the filter? > > But the mandatory attributes are specified on the provider side with a > mandatory directive. I may have missed something, but could you explain how > mandatory attributes could be specified on the requirement side? Are you > suggesting that a new method be available on Filter to specify mandatory > attributes: > > Filter.match(Map<String, ?> attrs, List<String> mandatoryKeys) My mistake. We just need to make sure that the filter doesn't match when there is a mandatory matching attribute which is not specified as an attribute in the filter. How far is the filter mechanism away from being able to support that? > > > I feel a bit uneasy about using > > List<Filter> as it implies a linear search and the list of packages may be > > quite long in some scenarios. Alternatively, maybe a map from package name to > > filter would be an efficient optimisation we could employ if and only if > > required. > > Hmmm, I know the current Filter implementations are linear when dealing with > OR'ed filters. The optimization you suggest would still require us to split > apart the OR'ed filter into a list of filters so we could key off the > osgi.wiring.package attribute key. We could make it a Collection<Filter> if > you like then there is no order implied and we can evaluate them in any order > we like and use what ever indexing we like at the implementation level. > > It is not the end of the world if we have to copy some of the FilterImpl code > to do the parsing ourselves if you really want to pick apart OR'ed filters, but > it seems like it may be better to simply allow a Collection of filter. I agree. I was considering the optimisation options in the case where we did decide to split apart the OR'ed filter.
(In reply to comment #9) > (In reply to comment #8) > > Filter.match(Map<String, ?> attrs, List<String> mandatoryKeys) > > My mistake. We just need to make sure that the filter doesn't match when there > is a mandatory matching attribute which is not specified as an attribute in the > filter. How far is the filter mechanism away from being able to support that? In a framework agnostic way there is nothing available and we would have to parse the filter string ourselves to determine. In equinox we have the method org.eclipse.osgi.framework.internal.core.FilterImpl.getPrimaryKeyValue(String) that can be used, but only if we split apart the OR'ed filter.
I think splitting apart the OR'ed filter is a lesser evil than having to explain a subtle change of user region model to users. If this becomes a real pain or a performance bottleneck which cannot be optimised, we can reconsider.
I have merged in Tom's changes. I have asked him to provide attachments for this bug to keep the IP log accurate after which we can close this bug.
Created attachment 191121 [details] patch including region_api branch changes. Here is a patch including the changes I made. Let me know if there are issues with this patch.
(In reply to comment #13) > Created attachment 191121 [details] > patch including region_api branch changes. > > Here is a patch including the changes I made. Let me know if there are issues > with this patch. Thanks Tom. Please would you confirm, for the record, that you authored 100% of the code, that you have your employer's permission to contribute the code, and that new files have a suitable license header.
(In reply to comment #14) > (In reply to comment #13) > > Created attachment 191121 [details] [details] > > patch including region_api branch changes. > > > > Here is a patch including the changes I made. Let me know if there are issues > > with this patch. > > Thanks Tom. Please would you confirm, for the record, that you authored 100% of > the code, that you have your employer's permission to contribute the code, and > that new files have a suitable license header. Yes I confirm: - I authored 100% of the code - I have my employer's permission to contribute the code - The new files have a suitable license header Thanks for accepting my contribution.
Thanks very much for this contribution to Virgo.
