Redeploy is done wrong

[alesj]

or VFS needs a re-deploy notion.

WRT:
- https://jira.jboss.org/jira/browse/JBAS-6504

What goes on and where does it go wrong?
* some ProfileService part finds modified deployment
* it returns list of Modifications to HDScanner
* HDScanner for MODIFIED deployment does this:

 mainDeployer::addDeployment(deployment)

* MainDeployer::addDeployment <-- (W)
* HDScanned invokes mainDeployer::process()
* MainDeployer --> DeployersImpl::process(undeploy, deploy)
* DeployerImpl::process --> undeploys modifed deployment, (re-)deploys modifed deployment

The wrong part is this:

In (W) we find existing deployment context, hence this means that addition is actually a re-deploy.
We add existing deployment context to undeploy, and then we create/recognize new one to be deployed.
And this is what's wrong --> we are actually doing new structure recognition on a deployment that's about to be undeployed --> its temp are gonna be cleared.
This already reads some VirtualFile refs into the DeploymentContext.
This refs can point to ZipEntryHandlers (VirtualFileHandler impl) whos underlying temp file is about to be deleted.
But the actual temp deletion only happens in DeployersImpl:process, its undeploy part.
So, we end up with parts from old VirtualFile refs and a few new ones.
e.g. from my test, the classpath was and old ref

In VFS on VirtualFile::clear I delete possible temps and reset jar VirtualFileHandlers (actually it's its contexts).
This reset enables that owning jar handlers are gonna re-initialize its contents, hence re-creating nested temp jars.

That this clearly works I added the following change to HDScanner:

 case MODIFIED:
 mainDeployer.removeDeployment(ctx); // change
 mainDeployer.process(); // change
 mainDeployer.addDeployment(ctx);
 break;

But this is not the right solution, since it's MainDeployer that should transparently handle re-deploys.

What would be the right way to do this?
We're probably have to break existing 2 phase deployment for re-deploys.

[starksm64]

The problem is that we can't make a clean transition from the old temp files to the new one. Can we force the reset prior to doing the structure scan and have the undeploy processing delete the now unreferenced old temp files?

[alesj]

Can we force the reset prior to doing the structure scan and have the undeploy processing delete the now unreferenced old temp files?

Sure.

But this then requires re-deploy or prepare clean notion on both sides
- deployers and vfs.

I would need to add some prepareClean to DeploymentContext
and perhaps two phases to VFS clean method.
In the first one I would just reset zip context,
where the deletion would take place in real cleanup.
It sounds doable in theory. :-)

Or is this too invasive/intrusive for either vfs or deployers?

[starksm64]

Can we just pull the delete out of the DeployersImpl into a separate call from the MainDeployerImpl to the vfs?

Ideally this would just be handled automatically by the vfs, but I don't see a problem with having an extra method that forces the cleanup to occur at a known point.

[alesj]

How would you pull this?

IMO it needs to be on DeploymentContex,
as that's what knows about vfs and old root.

[starksm64]

Yes, your right, the MainDeployerImpl does not know about the vfs. So can the existing DeploymentContext cleanup be leveraged for this, or would this drop the metadata repository info too soon?

[alesj]

Actually dreaming about it some more :-), this is not good either.
It would be just the other way around then, the old one seeing the new contents.

e.g.
* we see it's a redeploy --> DeploymentContext::prepareClean --> reset existing zip contexts
* do structure recognition on the same root, same zip contexts --> re-initializing entries
* undeploy on the same root/zip context == it sees the new enitres <---- X

In most cases it would probably work, as you mostly change metadata,
but if someone actually changed underlying jar file and
then tried to load some newly non-existing resources at undeploy, it would break.

I guess this could all be solved by having a temp copy all the time,
but that's just the opposite of why we have vfs.

[adrian.brock]

"alesj" wrote:

>> ps: seen then redeploy issue? :-(
>
> Yes, but what's the point in repeating the "hack to a hack" comment. :-(

What's the hack there?

"alesj" wrote:

The wrong part is this:

In (W) we find existing deployment context, hence this means that addition is actually a re-deploy.
We add existing deployment context to undeploy, and then we create/recognize new one to be deployed.
And this is what's wrong --> we are actually doing new structure recognition on a deployment that's about to be undeployed --> its temp are gonna be cleared.
This already reads some VirtualFile refs into the DeploymentContext.

"alesj" wrote:

That this clearly works I added the following change to HDScanner:

 case MODIFIED:
 mainDeployer.removeDeployment(ctx); // change
 mainDeployer.process(); // change
 mainDeployer.addDeployment(ctx);
 break;

But this is not the right solution, since it's MainDeployer that should transparently handle re-deploys.

What would be the right way to do this?

Let's fix it so the two have the same semantics and stop trying to hack around
problems when you already know the correct solution.

i.e. the structure determination is not done until the "deploy" stage of process()
after the "undeploy" stage has been done.

I know why you don't want to do that, but its obviously the correct solution to
the problem identified and will always be cleaner than trying to workaround the
issue at a level (the vfs) that doesn't understand the deployment lifecycle.

[alesj]

i.e. the structure determination is not done until the "deploy" stage of process() after the "undeploy" stage has been done.

Just for re-deployed deployments or for all?

This would mean we have to move all the maps that track failing deployments
from MainDeployerImpl into DeployersImpl.

How does this relate with method signature changes?

[alesj]

"alesj" wrote:

i.e. the structure determination is not done until the "deploy" stage of process() after the "undeploy" stage has been done.

Just for re-deployed deployments or for all?

I'll do it for all. ;-)

"alesj" wrote:

This would mean we have to move all the maps that track failing deployments
from MainDeployerImpl into DeployersImpl.

How does this relate with method signature changes?

This means I have to move everything (map, comparators, ...) to DeployersImpl.
As there is no point in hacking again, to have some ugly callbacks to MainDeployersImpl.

I'll keep MainDeployerImpls's interfaces, which will only delegate to DeployersImpl, OK?

I guess this means we'll have to do Deployers 2.1?

[alesj]

"alesj" wrote:

This means I have to move everything (map, comparators, ...) to DeployersImpl.
As there is no point in hacking again, to have some ugly callbacks to MainDeployersImpl.

I'll actually create some middle class,
sort of a deployment(context) registry.

MainDeployer will only read from it,
Deployers will write to it.

[adrian.brock]

"alesj" wrote:

"alesj" wrote:

This would mean we have to move all the maps that track failing deployments
from MainDeployerImpl into DeployersImpl.

How does this relate with method signature changes?

This means I have to move everything (map, comparators, ...) to DeployersImpl.
As there is no point in hacking again, to have some ugly callbacks to MainDeployersImpl.

I'll keep MainDeployerImpls's interfaces, which will only delegate to DeployersImpl, OK?

I guess this means we'll have to do Deployers 2.1?

Why can't you just change MainDeployersImpl.process() to be?

Deployers.process(toUndeploy, Collections.emptyList());
determineStructureForToDeploy();
Deployers.process(Collection.emptyList(), toDeploy);

That doesn't involve any changes to the external api.

The thing that changes is that you can't get the DeploymentContext/Unit from the
MainDeployer until you've invoked process() whereas previously
you could get it after addDeployment().