Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 325735 - As a user I'd like to be able to deploy config properties file for ManagedServiceFactory configuration
Summary: As a user I'd like to be able to deploy config properties file for ManagedSer...
Status: REOPENED
Alias: None
Product: Virgo
Classification: RT
Component: unknown (show other bugs)
Version: unspecified   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 enhancement with 1 vote (vote)
Target Milestone: 3.8.0.M02   Edit
Assignee: daniel marthaler CLA
QA Contact:
URL: https://issuetracker.springsource.com...
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 07:07 EDT by Glyn Normington CLA
Modified: 2017-11-01 08:30 EDT (History)
8 users (show)

See Also:


Attachments
Fixes to the eclipse/bundlor meta-data (33.70 KB, patch)
2010-10-05 15:27 EDT, Dmitry Sklyut CLA
no flags Details | Diff
Initial Patch (20.25 KB, patch)
2010-10-05 15:28 EDT, Dmitry Sklyut CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Glyn Normington CLA 2010-09-20 07:07:30 EDT
Currently Virgo can deploy properties files for ConfigAdmin consumption, but there is no support for ManagedServiceFactory, i.e. multiple configurations of the same service.

This would be very useful when configuration multiple datasources for example.

See for discussion in he context of dm Server.
Comment 1 Glyn Normington CLA 2010-09-20 07:08:19 EDT
See http://forum.springsource.org/showthread.php?t=74840 for discussion in the context of dm Server.
Comment 2 Dmitry Sklyut CLA 2010-10-04 16:52:47 EDT
I stated looking into a patch for this issue.

The one area that I am looking at is org.eclipse.virgo.kernel.install.artifact.internal.ConfigLifecycleEngine.
I really hope that all of the changes can be localized there.

Here is a set of changes:
1. Add a new property to the configuration (org.eclipse.virgo.kernel.install.artifact) that points to the file name or URI of the ArtifactFS.
2. Change search for the config to filter based on that new property, i.e.
   return this.configurationAdmin.listConfigurations(filter)
3. If configuration is null - based on the "service.pid" or "service.factoryPid" property in the incoming config file create proper configuration (createFactoryConfiguration vs. getConfiguration).
     If neither of those properties exist in the config file - use ArtifactIdentity.getName().

That should take care of start() in ConfigLifecycleEngine.

Few questions:
1. refresh - In case refresh is called but there is no Configuration found 
    Should this be an error condition or fall back to start lifecycle?
2.  stop - Should stop delete all of the factory configs or just the one instance?

Finally - is there any guaranties on ArtifactFS in *Engine classes?  That is are they always non null with ability to get and InputStream to the backing config file?

Thanks!
Comment 3 Dmitry Sklyut CLA 2010-10-05 15:27:52 EDT
Created attachment 180277 [details]
Fixes to the eclipse/bundlor meta-data

Always saw errors during builds in STS.  Narrowed it down to bundlor not having enough info.
${version} value is in build.properties not in build.versions.  Added build.versions to the bundlor config.
Updated template.mf to 2.2.x vs. 2.1.x
Comment 4 Dmitry Sklyut CLA 2010-10-05 15:28:44 EDT
Created attachment 180278 [details]
Initial Patch
Comment 5 Steve Powell CLA 2010-10-14 11:05:31 EDT
(In reply to comment #3)
This issue arises only in the most recent versions of STS which has changed the Bundlor execution defaults. It might therefore be an STS problem -- which I shall raise with them.

The patch you provide is a suitable workaround for now, although it does, disappointingly, involve project-specific settings for every sub-project of the kernel (and whatever other projects assume build.properties available to Bundlor).

Ant builds provide Bundlor with both these property files.

I have yet to decide whether to put this patch up; if we did we might want to revert it when/if STS changes.
Comment 6 Steve Powell CLA 2010-10-14 11:38:49 EDT
Your Config Admin patch is quite a good starting point, thank you.

We have been thinking about the design of the Configuration Artifact lifecycle (including FactoryConfigurations) and we have reached the following conclusions -- can you comment, please?

1.  A configuration artifact filename is not necessarily the configuration pid.
2.  A configuration pid is the artifact id Name (as in Type x Name x Version).
3.  A configuration artifact file can specify either a service.factoryPid or a service.pid as a property.  If a service.factoryPid is specified then any service.pid property is ignored and the configuration pid (equal to the artifact id (Name)) is generated at install time; otherwise if a service.pid is specified this is taken to be the configuration pid and the artifact id (Name) of the artifact; otherwise (if neither is specified) the file name (without the .properties extension) is taken as the artifact id (Name) of the artifact and the configuration pid. 
4.  Start of a configuration means UPDATING the configuration dictionary for the first time.
5.  Stop of a configuration means UPDATING the configuration dictionary back to the original (empty) state (which is indistinguishable from non-existence).
6.  Refresh of a configuration artifact means replacing the configuration dictionary with the new artifact contents in its entirety -- regardless of any subsequent updates that may have been made to the Configuration object.
7.  Refresh (and restart) of a configuration artifact may involve a change to its artifact id (Name) -- if it is a factory configuration.
8.  There is no need to insert any reference to the artifact file in the configuration dictionary, but there is a need to remember the factory pid in the InstallArtifact for refresh/restart purposes.
9.  Applications that want to find all configurations for a factory pid can filter on the service.factoryPid property.
10. Applications that wish to manage configurations (collectively or individually) can register the manager service with the service.pid or the factory manager service with the service.factoryPid as now.

We will not try to implement versioning of configurations until we fully understand how the Config Admin spec evolves.

Accordingly, I will be extending the implementation of the ConfigLifecycleEngine along these lines, which is similar to your patch (apart from the InstallArtifact factory pid property and refresh algorithm).

I am extremely interested in your reaction to this design -- especially whether it will meet your particular requirements.
Comment 7 Dmitry Sklyut CLA 2010-10-14 13:04:34 EDT
(In reply to comment #5)

> I have yet to decide whether to put this patch up; if we did we might want to
> revert it when/if STS changes.

I should have put this into another issue but ...  This STS error was bugging me something fierce.  

> disappointingly, involve project-specific settings for every sub-project
Not sure what you mean project-specific.  Each project already had a .settings/com.springsource.server.ide.bundlor.core.prefs file that pointed to "../build.versions"

I would really love to see that addressed as STS fix or move of ${version} from build.properties to build.versions.  I think both of those are read in every time anyways (could be wrong).
Comment 8 Dmitry Sklyut CLA 2010-10-14 13:28:34 EDT
(In reply to comment #6)
Just to get it straight:

1. Take name of the file - .properties as ArtifactId and service.pid when:
1.1. No service.pid in file
1.2. No service.factoryPid in file

2. Take service.pid as ArtifactId and service.pid when:
2.1. No service.factoryPid in file

3. Generate ArtifactId when
3.1 service.factoryPid is present in file

I am somewhat confused on #3.  i.d.

> If a service.factoryPid is specified then any service.pid property is ignored and the configuration pid
> (equal to the artifact id (Name)) is generated at install time;

Does this generated id corresponds to configAdmin.createFactoryConfiguration(String factoryPid).getPid()?
I did not follow how ArtifactId corresponds to service.pid in ConfigurationAdmin fully.
Are they the same?  Including for individual factory configurations?  Is that is the case - this design works better than my patch, no extra attributes in configuration.

One concern, what if there is another property file is deployed that specifies exact same service.pid but for a different file name.  How will that case be handled?  Will ArtifactId be computed in the bridge vs. ConfigLifecycleEngine and repository checked for already deployed?  But that will mean refresh right?  What happens to the old properties file of different name but with same service.pid?

I fully agree that versioning should be skipped at this stage.  

One thing that might be an interesting change is to handle configurations in scoped plans/pars.  Rewrite to scope+service.pid maybe somehow.  This way multiple apps with same components can live side by side in virgo.
Comment 9 Steve Powell CLA 2010-10-15 12:36:34 EDT
(In reply to comment #7)
Well you are right about the project-specific settings -- I hadn't spotted that.  However, the build.properties file used to be included (for bundlor) by STS without us having to put it into the prefs.

I think this is an STS regression.

build.properties version is necessary and relied upon for many things -- the manipulation of the build.properties and build.versions files occurs elsewhere in our process; a useful separation of function. We will not be merging them.

In any case, it is not a ConfigAdmin issue -- so please raise another bugzilla.

Incidentally, you didn't say what version of STS you are using. I'd like to reproduce this when raising the STS issue.
Comment 10 Steve Powell CLA 2010-10-15 13:15:04 EDT
(In reply to comment #8)
Yes you have it right.  The service.pid is the same as the one generated by the factoryConfiguration create method.

This will be done at install time; and the update (which triggers the registered factory service) done at start time.

The issue with files and service.pids is dealt with in exactly the same was as bundle/jars with the same artifact id within them.  If the same type,name,version of the artifact has already been deployed we reject the second deploy.

This also indicates what happens on refresh/restart.  It is possible today for the artifact id of a bundle to change on these actions and we treat this change to the service.pid as a change of the artifact id.  The InstallArtifact copes in the same way.

We have thought about scoping -- but the scoping of configurations is not going to be easy -- we'd need to intercept the registrations and have some sort of naming convention. In any case, we think this is orthogonal to this change -- and may very well change when the Config Admin specs are updated.
Comment 11 Dmitry Sklyut CLA 2010-10-19 15:13:43 EDT
(In reply to comment #9)

> Incidentally, you didn't say what version of STS you are using. I'd like to
> reproduce this when raising the STS issue.

I was running snapshots of STS on 3.6 based version from update site
Comment 12 Dmitry Sklyut CLA 2010-10-19 15:15:37 EDT
(In reply to comment #10)

Would't this mean that for factory config - you will need to generate an empty configuration at the time of InstallArtifact creation in a bridge?  That would be the only way to get a "proper" service.pid.
Comment 13 Steve Powell CLA 2010-10-19 16:40:11 EDT
(In reply to comment #11)
I assume you mean an e3.6 update site.  I'm afraid this doesn't tell me what STS base you are working from.  If you are updating plugins too, then you might have a mix of versions that few people have.
Comment 14 Steve Powell CLA 2010-10-19 16:44:41 EDT
(In reply to comment #12)
Yes, you are right -- we need to create the Configuration object to get the pid (and therefore the artifact id). Provided the ConfigAdmin implementation doesn't publish the configuration we are OK.  No-one else is likely to look for it -- and if they do, they'll see an empty one. We will 'update()' the dictionary in the configuration when our artifact is 'started', and then any registered service factory managers will see it.

We may not do it in the Bridge -- we may do it before that (we have to examine an artifact for its type, name and version earlier than the Bridge code).
Comment 15 Alexander Shutyaev CLA 2010-11-16 08:55:19 EST
what is the current status of this issue? Do I correctly understand the current workaround?

1. Apply 'Initial Patch' to Virgo 1.0.0.RELEASE
2. Adding line 'service.factoryPid=myfactory' in file 'sample.properties' causes Virgo to register a configuration with service.pid=sample & service.factoryPid=myfactory.
Comment 16 Alexander Shutyaev CLA 2010-11-16 09:42:47 EST
Sorry, I meant 2.1.0.RELEASE
Comment 17 Steve Powell CLA 2010-11-17 04:13:26 EST
(In reply to comment #15)
Alexander,

There is no 'workaround' for this issue -- this is a proposed enhancement.  The actions you suggest will have no effect in the current release.  Although this is assigned, we have no immediate plans to implement this.  Anyone can propose an enhancement and submit it as a patch here.

Do you have a specific requirement -- if so, this piece of work may have an increased priority.

Currently we are making detailed plans for the next six months or so of Virgo development and this is one of the (easier) items under consideration.
Comment 18 Dmitry Sklyut CLA 2011-03-04 12:52:02 EST
committed with ebeb2f75923102e95b3d304054ab66d670f241c6
Comment 19 Steve Powell CLA 2011-03-07 05:59:51 EST
Dmitry, 
Nice changes, but a few trivial changes, please:
would you check the file comment header for ConfigurationTestUtils.java for project conformance, and insert a correct one into FactoryConfigurationDeploymentTests.java?

(Guidelines are here: http://wiki.eclipse.org/Virgo/Committers/Coding and you may notice they advise using the code_template, formatter and clean_up Eclipse profiles supplied -- it would have helped you to correct headers.)

Also, thanks for putting your commit sha1 here -- please identify the repository it goes in for future reference -- this is a good example to follow.
Comment 20 Dmitry Sklyut CLA 2011-03-07 11:34:59 EST
(In reply to comment #19)
> Dmitry, 
> Nice changes, but a few trivial changes, please:
> would you check the file comment header for ConfigurationTestUtils.java for
> project conformance, and insert a correct one into
> FactoryConfigurationDeploymentTests.java?
> 
> (Guidelines are here: http://wiki.eclipse.org/Virgo/Committers/Coding and you
> may notice they advise using the code_template, formatter and clean_up Eclipse
> profiles supplied -- it would have helped you to correct headers.)

Thanks for spotting that.  Will do.
Comment 21 Dmitry Sklyut CLA 2011-03-07 14:49:51 EST
I just found an interesting class: org.eclipse.virgo.kernel.config.internal.UserConfigurationPropertiesSource

This class is basically creates ConfigurationAdmin entries from all .properties files in config folder.

It also fully bypasses all of the deployment pipeline.  Any changes that I have made to support factory config are ignored.

So the question is:

1.  Should this class be modified to support deployment pipeline
2.  Duplicate the code to support factory configuration
3.  Document that factory configuration will not be supported in config folder
Comment 22 Dmitry Sklyut CLA 2011-03-07 14:52:56 EST
(In reply to comment #21)
Class mentioned in comment 21 does collection of properties.  Actual deployment is done in org.eclipse.virgo.kernel.config.internal.ConfigurationPublisher
Comment 23 Dmitry Sklyut CLA 2011-03-07 17:12:56 EST
(In reply to comment #22)

I pushed commit 3ddd434fd5eb1343f08d2ffbd6773996139bc96d to kernel branch bug325735-manager-service-factory.

Please let me know if I am on the right track here.  Logic is very similar to deployer changes done prior.
Comment 24 Glyn Normington CLA 2011-07-25 05:54:38 EDT
This missed the feature freeze for 3.0, so deferring to the next release.
Comment 25 Alexander Shutyaev CLA 2013-01-21 07:02:03 EST
What is the current state of this issue? What works and what doesn't? Are there any plans to complete it?
Comment 26 Dmitry Sklyut CLA 2013-01-23 21:39:13 EST
(In reply to comment #25)
> What is the current state of this issue? What works and what doesn't? Are
> there any plans to complete it?

I see 1be791f220cb768e60c94adafc9b4e6e7435fb9a commit in kernel git repo.  I will verify and reply back once I verify if it made its way into release.

Thanks
Dmitry
Comment 27 Dmitry Sklyut CLA 2013-01-24 00:03:13 EST
After some further git archaeology and checking current state of code base, I see that code supporting factory configuration is all in.  I will work on adding proper docs over the next few days.

Currently only thing in programmer docs:
"Factory Configuration is supported by specifying the service.factoryPid property. In this case the actual PID will be created by Configuration Admin (see section 104.6 in the Compendium Specification)."

Thanks
Dmitry
Comment 28 Dmitry Sklyut CLA 2013-02-05 07:53:21 EST
This functionality is currently broken in user region but works in Kernel.

UserRegionConfigurationDeployer in kernel.userregion does not consider factory based configurations.  There is also a bit of a thought that needs to go into generating artifact name for factory configurations in PropertiesBridge during deployment.

Factory configurations are more like a container with 1->n configurations, but PropertiesBridge tries to fit it into a single artifact -> single deployment model.

I will follow up on the dev list with some ideas, but I think there is a need for a container Artifact type specifically for Factory configurations and potentially changes on the Web Admin ui to handle it.

Regards,
Dmitry