11 Replies Latest reply on Feb 17, 2009 7:04 AM by Adrian Brock

    Redeploy is done wrong

    Ales Justin Master

      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.


        • 1. Re: Redeploy is done wrong
          Scott Stark Master

          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?

          • 2. Re: Redeploy is done wrong
            Ales Justin Master

             

            "scott.stark@jboss.org" wrote:
            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?

            • 3. Re: Redeploy is done wrong
              Scott Stark Master

              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.

              • 4. Re: Redeploy is done wrong
                Ales Justin Master

                How would you pull this?

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

                • 5. Re: Redeploy is done wrong
                  Scott Stark Master

                  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?

                  • 6. Re: Redeploy is done wrong
                    Ales Justin Master

                    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.

                    • 7. Re: Redeploy is done wrong
                      Adrian Brock Master

                       

                      "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.

                      • 8. Re: Redeploy is done wrong
                        Ales Justin Master

                         

                        "adrian@jboss.org" 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?

                        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?


                        • 9. Re: Redeploy is done wrong
                          Ales Justin Master

                           

                          "alesj" wrote:
                          "adrian@jboss.org" 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?

                          • 10. Re: Redeploy is done wrong
                            Ales Justin Master

                             

                            "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.

                            • 11. Re: Redeploy is done wrong
                              Adrian Brock Master

                               

                              "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().