Deployment cleanup & modifications

[alesj]

An exception I got yesterday while testing new MC releases
gave me plenty to think about in the evening.

WRT:
- http://www.jboss.com/index.html?module=bb&op=viewtopic&t=148563
Doing just DeploymentContext::cleanup is not enough.

Why?
Since we have a notion of modifying a deployment (X):
- http://www.jboss.com/index.html?module=bb&op=viewtopic&t=149077
What does this actually do?
If using 'temp' directive we create a temporary copy of deployment's file,
hence the actual Deployment::root != DeploymentContext::root.
In other words:
* Deployment == client view
* DeploymentContext == server view

But while we're trying to recognize the structure, to create a DeploymentContext instance,
we already potentially do some temp unpacking, ...
That's why a need to do cleanup also on client side Deployment.

How to do this?
1) Deployment::cleanup - add new method
2) DeploymentFactory::destroyDeployment - add new method
3) usage specific, e.g. in Profile Service code: VFSDeployment::getRoot::cleanup
Which one to use/impl?

Also a few observations/issues about modification usage.
There might be more, but this are the one's I scrapped together.

(a) When to replace a handler when doing a modification?

I actually forgot about this issue and changed how this behaved in VFS. :-(
In my opinion 'temp' and 'explode' should not replace handler.
As this changes client behavior.
e.g. server/deploy/myapp.ear --> server/temp/some-uuid/myapp.ear
If I did a replacement on a parent (= deploy/), then deploy/ would actually see both deployments at update.
Only unpack should replace it. As it only applies to nested resoruces,
hence doesn't change client view that much. At least not in a confusing fashion.

(b) direct URL usage

Although we use modification, the URL's are still the same as non modified.
So, if you use direct URL (any of its ops), the vfs cache is gonna navigate you to non-temp view.

Meaning even though you're using things on server side,
it's gonna drive you off into client side.
It's still gonna be somehow the same, as modifications don't modify the resource itself,
but it's just conceptually wrong, since you did modifications for a reason.

(c) Even if you use modifications, all undeploy (see (X)) issues don't go away

e.g. if you use a direct VFS URL, at undeploy, it's gonna go to VFS cache,
and try to navigate to previously deleted file (aka client view).
Since VFS cache has no idea that you actually created a temp version of your deployment.

If you use other mechanisms, e.g. classloaders, deployers,
they are gonna work, as they transparently know about the temp.

(d) tmp and VFSCache

How to apply vfs cache to tmp directory so that we don't hit vfs-nested.tmp dir,
which will be massive memory drawback.
How to actually make usage of it, as temp should be impl detail/hidden.
Really uncharted territory ...

[alesj]

"alesj" wrote:

How to do this?
1) Deployment::cleanup - add new method
2) DeploymentFactory::destroyDeployment - add new method
3) usage specific, e.g. in Profile Service code: VFSDeployment::getRoot::cleanup
Which one to use/impl?

I used (2), as it seems the best choice.
- https://jira.jboss.org/jira/browse/JBDEPLOY-156

[starksm64]

Not that this is relevant for this release, but how much of the copying is required due to jar file native locking issues? Can we do away with the need for copying by getting a java zip file implementation, or is this an issue of concurrent access to virtual files that should have a valid view even after being deleted?

[starksm64]

For reference, some java implementations of zip file handling I found:

http://www.jcraft.com/jzlib/index.html
http://jazzlib.sourceforge.net/
https://truezip.dev.java.net/

[dmlloyd]

For what it's worth, I intend to make use of the JCraft implementation for XNIO-SSH.

[alesj]

Not that this is relevant for this release, but how much of the copying is required due to jar file native locking issues?

I'm not aware of any issue that would force use to use copy to resolve native locking issues.
We do copy so we can handle nested jars w/o in-memory usage.

or is this an issue of concurrent access to virtual files that should have a valid view even after being deleted?

Yes, temp is for undeploy resources lookups.
Although like (c) item states, it won't always work.

How was this dealt in previous 4.x version?
I guess temp resources produced 'temp' urls.

[alesj]

For what it's worth, I intend to make use of the JCraft implementation for XNIO-SSH.

To do what?

What about if you integrated that impl into VFS?
As a new (3rd attempt) jar handling impl. ;-)
While you'll be already working on the vfs-remote. :-)

[alesj]

"alesj" wrote:

But while we're trying to recognize the structure, to create a DeploymentContext instance,
we already potentially do some temp unpacking, ...
That's why a need to do cleanup also on client side Deployment.

And even this is not enough.
As we might not even know how to cleanup all that we've temped.

e.g.
VFSDeployment only has an access to top root.

But if our deployment looks like this
* myapp.ear (exploded)
** mywep-ui.war (packed)
*** WEB-INF/lib
**** some-potential-persistence.jar
***** META-INF/persiostence.xml

In this case our WarStructure would temp that some-potential-persistence.jar
leaving us with no handle to clean it up.

This might work if Deployment had a knowledge of structure,
as we would be able to call cleanup on sub-deployment (the war),
but this again means it's messing with sever-side info.

Since it's only temps I wouldn't loose too much sleep over it
if there is no quick/non-dirty solution.

[adrian.brock]

Why are you trying to "destroy" the Deployment object?

That is a client side object. That is supposed to be under the caller's control you shouldn't
be modifying it or giving it some "stateful" representation.

All the book keeping for temp data of a VFS file should be inside the VFS itself.
I said this before WRT to the horrible explicit VFS Cache you did, but I was ignored.

The "simplist" solution to this problem should be that when somebody creates a root
VFSContext it is against that object that the "temp" information should be stored
or at least linked.

That root VFSContext should always be "live" while somebody holds a
link to the root VirtualFile.

Then when somebody asks to cleanup a VirtualFile within that context, it can workout
which parts of the temp information need to be removed.

The context should still be usable, its just removed the temp data which would need to be
reconstructed. Which is why close() or destroy() is a bad name/semantic.

[adrian.brock]

The "simplist" solution to this problem should be that when somebody creates a root
VFSContext it is against that object that the "temp" information should be stored
or at least linked.

That root VFSContext should always be "live" while somebody holds a
link to the root VirtualFile.

Then when somebody asks to cleanup a VirtualFile within that context, it can workout
which parts of the temp information need to be removed.

The context should still be usable, its just removed the temp data which would need to be
reconstructed. Which is why close() or destroy() is a bad name/semantic.

By way of a more concrete example using your example:

deploy/
* myapp.ear (exploded)
** myweb-ui.war (packed)
*** WEB-INF/lib
**** some-persistence.jar
***** META-INF/persistence.xml

Typically the root VFS context is going to be the deploy folder.

When you want to unpack myweb-ui.war or some-persistence.jar you tell the
root context what you've done.

(As usual this is pseudo code)

VFSContext ctx = getRootContext();
ctx.addCacheInfo(getPath(), origName, new TempJarInfo(...));

public class TempJarInfo implements VFSCacheInfo
{
...

 public void cleanup() { ... }
}

This also trivially allows you to avoid unpacking them again, since you can ask
this cache.

// Already unpacked?
VFSContext ctx = getRootContext();
tempJar = ctx.getCacheInfo(getPath(), origName);

Then when the deployment says you can cleanup "deploy/myapp.war"
you can iterate over these cached objects that being with that VFS path and clean
them up.

VFSContext ctx = getRootContext();
ctx.cleanupCache(getPath());

i.e.

for (Map.Entry<String, VFSCacheInfo> entry : allCacheItems)
{
 if (entry.getKey().startsWith(path))
 getValue().cleanup();
}

[alesj]

Why are you trying to "destroy" the Deployment object?

I'm not or I'm only destroying it if I created it.
As does the ProfileService.

All the book keeping for temp data of a VFS file should be inside the VFS itself.

It is.
As much as it can be, since temp/modifications
were completely left out from initial/original design.

I said this before WRT to the horrible explicit VFS Cache you did, but I was ignored.

Where was this?
I never (or I must really missed it) heard any idea/feedback about vfs cache.
When things were horribly slow, I thought of something.
And even then there was no feedback.

Good, bad, horrible ... whatever ... it still only has a few lines to change if you wanna ditch that.

The "simplist" solution to this problem should be that when somebody creates a root
VFSContext it is against that object that the "temp" information should be stored or at least linked.

That root VFSContext should always be "live" while somebody holds a
link to the root VirtualFile.

Then when somebody asks to cleanup a VirtualFile within that context, it can workout which parts of the temp information need to be removed.

The context should still be usable, its just removed the temp data which would need to be
reconstructed. Which is why close() or destroy() is a bad name/semantic.

This makes sense.
But it should have been there 2 years ago. :-)

All these problems and your suggestions make
me wonder if we wouldn't be better off rewriting the whole stuff.
Perhaps even moving jar/zip handling to what DML plans to use.

[alesj]

This avoids also trivially allows you to avoid unpacking them again, since you can ask this cache.

This looks really good :-) since it solves (b), (c) and (d).
I'll try to hack some stuff based on this ...

[starksm64]

"alesj" wrote:
lthough like (c) item states, it won't always work.
How was this dealt in previous 4.x version?
I guess temp resources produced 'temp' urls.

Yes, all nested elements were unpacked and had temp urls.

[adrian.brock]

"alesj" wrote:

I said this before WRT to the horrible explicit VFS Cache you did, but I was ignored.

Where was this?
I never (or I must really missed it) heard any idea/feedback about vfs cache.
When things were horribly slow, I thought of something.
And even then there was no feedback.

Good, bad, horrible ... whatever ... it still only has a few lines to change if you wanna ditch that.

It was on some thread where you described the VFS cache api.
I don't want you to ditch it, I just want it to be used transparently within the VFS.

The programmers that write VFS code shouldn't have to be aware of the caching
and code in a different way to get it. It should be configurable in an "aspectized" way.

[alesj]

The programmers that write VFS code shouldn't have to be aware of the caching
and code in a different way to get it. It should be configurable in an "aspectized" way.

This is done in the latest VFS release.
I deprecated the VFS::getCachedFile api.

It's just missing more detailed cache type.
e.g. your VFSCacheInfo for temp handling