[WFCORE-6255] Deployment scanner did not undeploy failed archives #5642
Conversation
github-actions
bot
added
the
deps-ok
There was a problem hiding this comment.
Thanks @khermano -- the approach seems reasonable, although I mention a scenario that needs to be covered, plus this needs specific testing.
| @@ -931,6 +931,7 @@ private void scanDirectory(final File directory, final String relativePath, fina | |||
| final String deploymentName = fileName.substring(0, fileName.length() - FAILED_DEPLOY.length()); | |||
| scanContext.toRemove.remove(deploymentName); | |||
| if (!deployed.containsKey(deploymentName) && !(new File(child.getParent(), deploymentName).exists())) { | |||
| scanContext.scannerTasks.add(new UndeployTask(deploymentName, deploymentDir, child.lastModified(), true)); | |||
There was a problem hiding this comment.
I believe child.lastModified() should be scanContext.scanStartTime. If this actually does an undeploy, the undeploy didn't happen when the FAILED_DEPLOY marker was written.
It doesn't really matter because this param ends up unused since you pass 'true' as the forcedUndeploy param, but maybe after some future change it will matter.
There was a problem hiding this comment.
A more substantive problem is there needs to be a check that this isn't going to undeploy something not associated with this scanner; e.g. a scanned deployment failed, and admin removed the file, and then the admin used the CLI to force-deploy.
I believe scanContext.persistentDeployments is meant to contain the set of deployments not associated with this scanner, so that could be how to check.
Tests of this change should cover this scenario in addition to the basic one described in the JIRA.
| @@ -495,6 +495,7 @@ public void testCleanSpuriousMarkers() throws Exception { | |||
| f1 = createFile("spurious" + FileSystemDeploymentService.DEPLOYED); | |||
| f2 = createFile(new File(tmpDir, "nested"), "nested" + FileSystemDeploymentService.DEPLOYED); | |||
|
|
|||
| ts.controller.addCompositeSuccessResponse(1); | |||
There was a problem hiding this comment.
I would prefer this test being left as is and one or two next test methods specifically targeting this JIRA be added. It's too hard to reason about how this test change means the fix is being tested.
If you had to do this just to get the existing test to pass, a code comment explaining why it's needed would be good.
There was a problem hiding this comment.
Hello @bstansberry. Thank you very much for the review and detailed description of the possible problems.
Yes, I just tried to get the existing test pass.
If I understand it correctly, the test was failing because I added UndeploymentTask in FileSystemDeploymentService which wasn't considered by the mocked controller. I guess the addCompositeSuccessResponse method only adds one success response to MockServerController.responses to use it in the processOp method somehow as a counter of requests.
Should I also try to create some other test?
For now I will force-push my changes.
There was a problem hiding this comment.
Hi @khermano -- yes, please add another test method or two, e.g. one that specifically covers the bug and another that validates that a deployment not associated with the scanner being tested isn't undeployed just because a cruft FAILED_DEPLOY marker file is left in the scanner's directory. I think the test infrastructure used by FileSystemDeploymentServiceTestCase can handle that scenario, or at least is close to being able to.
It sounds like the line I commented on here needs to remain, which is fine, but preceding it with a brief comment would be good as it's not obvious why it's there.
There was a problem hiding this comment.
Hello @bstansberry ,
so I added the comment and the test for the bug you recommended, but I am unsure about the other one.
I tried a couple of things to deploy the failing kitchensink.war through CLI and console, but none of them create any content in the scanned standalone/deployments directory, not even deploy ~/../kitchensink.war --force.
And I think that maybe creating markers like ".failed", ".deployed" and others is only a deployment-scanner thing, or am I wrong about that?
There was a problem hiding this comment.
@khermano For the 2nd one I was looking for another test method in FileSystemDeploymentServiceUnitTestCase. I can't say precisely how to do it but have a look at the various testIgnoreExternal...Deployment methods and their use of the ExternalDeployment class. The purpose of that class was to represent a deployment not under the control of the scanner being tested. I don't know if the test utilities in FileSystemDeploymentServiceUnitTestCase are sufficient to cover your scenario, but if they are not they can probably be tweaked a bit.
There was a problem hiding this comment.
Hi @bstansberry ,
I am sorry for the delay. I got stuck with this and needed to work on something else, but I think I finally have it.
So again, I'm really sorry for the inconvenience. Hopefully, it looks okay.
khermano
force-pushed
the
WFCORE-6255_from_main
branch
from
30edd3a
to
70802e0
Compare
khermano
force-pushed
the
WFCORE-6255_from_main
branch
from
70802e0
to
db56c3a
Compare
khermano
force-pushed
the
WFCORE-6255_from_main
branch
from
db56c3a
to
43a9b17
Compare
There was a problem hiding this comment.
Hi @khermano
The tests look good. Converting scanContext to an instance field brings some maintenance risk related to thread safety, so I've requested a few changes related to reducing that risk. But the basic fix looks fine.
| @@ -152,6 +153,11 @@ class FileSystemDeploymentService implements DeploymentScanner, NotificationHand | |||
| @SuppressWarnings("deprecation") | |||
| private final DeploymentTransformer deploymentTransformer; | |||
|
|
|||
| private ScanContext scanContext; | |||
There was a problem hiding this comment.
| @@ -152,6 +153,11 @@ class FileSystemDeploymentService implements DeploymentScanner, NotificationHand | |||
| @SuppressWarnings("deprecation") | |||
| private final DeploymentTransformer deploymentTransformer; | |||
|
|
|||
| private ScanContext scanContext; | |||
|
|
|||
| public Map<String,Boolean> getScanContextRegisteredDeployments() { | |||
There was a problem hiding this comment.
This should be package-protected and moved down in the file to a less prominent position. Please add method javadoc stating the method only exists for testing. The test is in the same package, so package-protected works. That kind of thing is why it's in the same package. :)
| private ScanContext scanContext; | ||
|
|
||
| public Map<String,Boolean> getScanContextRegisteredDeployments() { | ||
| return Collections.unmodifiableMap(scanContext.registeredDeployments); |
There was a problem hiding this comment.
Per my comment above about the new scanContext field, please wrap this in acquireScanLock() try { ... } finally { releaseScanLock(); }
an 'assert scanContext != null' would also be good.
Doing all this doesn't really matter for your test but it reduces the risk of future misuse of this method by having the test code follow the concurrency rules that we create for the production code.
khermano
force-pushed
the
WFCORE-6255_from_main
branch
from
43a9b17
to
9418b98
Compare
|
Core -> Full Integration Build 12991 outcome was FAILURE using a merge of 9418b98 Failed tests |
|
Thanks @khermano |
Issue: https://issues.redhat.com/browse/WFCORE-6255