You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2017/02/16 09:34:40 UTC
[GitHub] brooklyn-server pull request #560: Attempted non-determinite test fixes
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/560
Attempted non-determinite test fixes
Prompted by the following (from https://builds.apache.org/job/brooklyn-server-pull-requests/1730/ ):
```
Test Result (3 failures / +3)
org.apache.brooklyn.launcher.CleanOrphanedLocationsIntegrationTest.testCleanedCopiedPersistedState
org.apache.brooklyn.launcher.CleanOrphanedLocationsIntegrationTest.testSelectionWithOrphanedLocationsInData
org.apache.brooklyn.launcher.BrooklynLauncherTest.testErrorsCaughtByApiAndRestApiWorks
```
The first two have been observed in several places, and might be fixed by the first commit here. The second one is less frequent but the second commit here might help and should improve logging.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server orphan-test-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/560.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #560
----
commit d07c2af901d50f7a2c2767f1175bf990c327489c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-02-16T09:09:33Z
attempt to fix non-det failures by forcing a persist
commit 08608b1132bc42319a1f216c02159d2e0d76e31c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-02-16T09:32:01Z
a retry and extra logging when bind exception happens
to fix non-det test failures on server, due to bind conflicts
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/560
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
also, this line
```
2017-02-18 02:00:01,345 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Persistence mode AUTO, directory /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations, no previous state
```
appears only in the failed run, and code confirms it should be logged iff the dir is empty / non-existant. so how is the dir being wiped?!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
+1 go ahead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinite test fixes
Posted by ahgittin <gi...@git.apache.org>.
GitHub user ahgittin reopened a pull request:
https://github.com/apache/brooklyn-server/pull/560
Attempted non-determinite test fixes
Prompted by the following (from https://builds.apache.org/job/brooklyn-server-pull-requests/1730/ ):
```
Test Result (3 failures / +3)
org.apache.brooklyn.launcher.CleanOrphanedLocationsIntegrationTest.testCleanedCopiedPersistedState
org.apache.brooklyn.launcher.CleanOrphanedLocationsIntegrationTest.testSelectionWithOrphanedLocationsInData
org.apache.brooklyn.launcher.BrooklynLauncherTest.testErrorsCaughtByApiAndRestApiWorks
```
The first two have been observed in several places, and might be fixed by the first commit here. The second one is less frequent but the second commit here might help and should improve logging.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server orphan-test-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/560.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #560
----
commit d07c2af901d50f7a2c2767f1175bf990c327489c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-02-16T09:09:33Z
attempt to fix non-det failures by forcing a persist
commit 08608b1132bc42319a1f216c02159d2e0d76e31c
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-02-16T09:32:01Z
a retry and extra logging when bind exception happens
to fix non-det test failures on server, due to bind conflicts
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Good idea about the listener. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
still seems not merged @neykov
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r103746212
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/CleanOrphanedLocationsIntegrationTest.java ---
@@ -66,13 +72,26 @@
private Set<ManagementContext> mgmts;
private ManagementContext mgmt;
+ private String copyResourcePathToTempPath(String resourcePath) {
+ BundleMaker bm = new BundleMaker(ResourceUtils.create(this));
+ bm.setDefaultClassForLoading(CleanOrphanedLocationsIntegrationTest.class);
+ File jar = bm.createJarFromClasspathDir(resourcePath);
+ File output = Os.newTempDir("brooklyn-test-resouce-from-"+resourcePath);
+ try {
+ ArchiveUtils.extractZip(new ZipFile(jar), output.getAbsolutePath());
+ return output.getAbsolutePath();
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+ }
+
@BeforeMethod(alwaysRun = true)
@Override
public void setUp() throws Exception {
super.setUp();
- persistenceDirWithOrphanedLocations = getClass().getResource(PERSISTED_STATE_PATH_WITH_ORPHANED_LOCATIONS).getFile();
- persistenceDirWithoutOrphanedLocations = getClass().getResource(PERSISTED_STATE_PATH_WITHOUT_ORPHANED_LOCATIONS).getFile();
- persistenceDirWithMultipleLocationsOccurrence = getClass().getResource(PERSISTED_STATE_PATH_WITH_MULTIPLE_LOCATIONS_OCCURRENCE).getFile();
+ persistenceDirWithOrphanedLocations = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITH_ORPHANED_LOCATIONS);
+ persistenceDirWithoutOrphanedLocations = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITHOUT_ORPHANED_LOCATIONS);
+ persistenceDirWithMultipleLocationsOccurrence = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITH_MULTIPLE_LOCATIONS_OCCURRENCE);
--- End diff --
really? javadoc says `File(String pathname)`, no indication that it should accept a URI. but i haven't tried.
in any case i think the extract is a better pattern than assuming resources are in `classes/` dir on filesystem -- wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
the debug log was still around, so we get a bit more info:
```
2017-02-18 02:00:01,331 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: File-based objectStore will use directory /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations
2017-02-18 02:00:01,332 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,345 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Persistence mode AUTO, directory /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations, no previous state
2017-02-18 02:00:01,349 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/entities for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,367 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/locations for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,367 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/policies for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,367 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/enrichers for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,368 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/feeds for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,368 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/catalog for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,368 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/plane for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,373 DEBUG o.a.b.c.m.p.FileBasedObjectStore [main]: Created dir /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/nodes for FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,373 DEBUG o.a.b.c.m.h.ManagementPlaneSyncRecordPersisterToObjectStore [main]: ManagementPlaneMemento-persister will use store FileBasedObjectStore{basedir=/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-18 02:00:01,374 DEBUG o.a.b.c.m.p.BrooklynMementoPersisterToObjectStore [main]: Loaded rebind lists; took 0ms: 0 entities, 0 locations, 0 policies, 0 enrichers, 0 feeds, 0 catalog items; from /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations
2017-02-18 02:00:01,374 DEBUG o.a.b.c.m.p.BrooklynMementoPersisterToObjectStore [main]: Loaded rebind raw data; took 0ms; 0 entities, 0 locations, 0 policies, 0 enrichers, 0 feeds, 0 catalog items, from /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations
```
seems the directory `/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests%402/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations` did not exist
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
that comment comes immediately after the `File[] subPathDirFiles = subPathDir.listFiles(fileFilter);` so does seems like the dir is empty when the failing test runs (as opposed to some other cause)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
(elsewhere in the log it refers to path `/home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests@2/launcher/target/surefire/surefirebooter8075241968007199755.jar!/META-INF/MANIFEST.MF` so `%40` is hex for `@`)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
a healthy run also shows the `Creating directory` message, but the key difference seems to be:
```
2017-02-14 16:16:13,575 DEBUG o.a.b.c.m.h.ManagementPlaneSyncRecordPersisterToObjectStore [main]: ManagementPlaneMemento-persister will use store FileBasedObjectStore{basedir=/Users/alex/Data/cloudsoft/dev/gits/brooklyn/brooklyn-server/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations}
2017-02-14 16:16:13,575 DEBUG o.a.b.c.m.p.BrooklynMementoPersisterToObjectStore [main]: Loaded rebind lists; took 0ms: 2 entities, 5 locations, 0 policies, 0 enrichers, 0 feeds, 0 catalog items; from /Users/alex/Data/cloudsoft/dev/gits/brooklyn/brooklyn-server/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r101577936
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,15 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // port discovery routines may take some time to clear, e.g. 250ms for SO_TIMEOUT
+ // tests fail because of this; see if adding a delay improves things
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
--- End diff --
https://github.com/apache/brooklyn-server/pull/563 - will let it run a few times to see how it behaves on the salves. Obviously not failing doesn't mean anything.
While experimenting I found that `SO_REUSEADDR` is activated **by default** on `ServerSocket`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r101578521
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,15 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // port discovery routines may take some time to clear, e.g. 250ms for SO_TIMEOUT
+ // tests fail because of this; see if adding a delay improves things
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
--- End diff --
One thing was clear though - `TIME_WAIT` doesn't fail binding the `ServerSocket`. It's something else running that's bugging the test. Could be another Brooklyn job that manages to steal the port or something unrelated doing funny things with the interfaces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r103716892
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/CleanOrphanedLocationsIntegrationTest.java ---
@@ -66,13 +72,26 @@
private Set<ManagementContext> mgmts;
private ManagementContext mgmt;
+ private String copyResourcePathToTempPath(String resourcePath) {
+ BundleMaker bm = new BundleMaker(ResourceUtils.create(this));
+ bm.setDefaultClassForLoading(CleanOrphanedLocationsIntegrationTest.class);
+ File jar = bm.createJarFromClasspathDir(resourcePath);
+ File output = Os.newTempDir("brooklyn-test-resouce-from-"+resourcePath);
+ try {
+ ArchiveUtils.extractZip(new ZipFile(jar), output.getAbsolutePath());
+ return output.getAbsolutePath();
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+ }
+
@BeforeMethod(alwaysRun = true)
@Override
public void setUp() throws Exception {
super.setUp();
- persistenceDirWithOrphanedLocations = getClass().getResource(PERSISTED_STATE_PATH_WITH_ORPHANED_LOCATIONS).getFile();
- persistenceDirWithoutOrphanedLocations = getClass().getResource(PERSISTED_STATE_PATH_WITHOUT_ORPHANED_LOCATIONS).getFile();
- persistenceDirWithMultipleLocationsOccurrence = getClass().getResource(PERSISTED_STATE_PATH_WITH_MULTIPLE_LOCATIONS_OCCURRENCE).getFile();
+ persistenceDirWithOrphanedLocations = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITH_ORPHANED_LOCATIONS);
+ persistenceDirWithoutOrphanedLocations = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITHOUT_ORPHANED_LOCATIONS);
+ persistenceDirWithMultipleLocationsOccurrence = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITH_MULTIPLE_LOCATIONS_OCCURRENCE);
--- End diff --
The proper way to escape the URL is `new File(url.toURI())`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Thanks @ahgittin, got distracted and left it at `git push`.
Merged before seeing @aledsage's comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Wow, I wasn't expecting to see this set of commits of file-edits based on the name of the PR and the description!
It would be great to review + merge your improvements for non-deterministic test failures independent of trying to review + merge the `BundleMaker` changes. We can then get it merged quickly, and get the benefits immediately.
Can you separate those out into their own PR, rather than building on the `BundleMaker` PR (and thus blocking the merging of those changes until that other PR is dealt with)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinite test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Aha nvm Jenkins is now running, it just took a little while.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinite test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Based on one run this improves things. I've tried closing and reopening but that no longer triggers a rebuild. Damn server-side smartness!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinite test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r101513076
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,15 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // port discovery routines may take some time to clear, e.g. 250ms for SO_TIMEOUT
+ // tests fail because of this; see if adding a delay improves things
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
--- End diff --
@neykov Why would `setReuseAddress(true)` improve false positives? As long as we make the same call when we start the server it should be identical? (Or are you thinking that the root cause is that the server-start call isn't using that, and that is what you'd fix?)
I agree finding and fixing the root cause (and recognising and logging better when we encounter it?) is even better, but I don't know what it is!
I don't think we do parallel tests so I suspect that isn't the issue. Letting sockets pick their own ports is a big change so I'd rather not go down that line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
working on the basis that the `getResource(..).getFile()` call might do URL-escaping when we say `getResource()` and then not properly unescape it when we do `getFile()`. that technique isn't guaranteed to work (definitely not if tests are run from JARs), this is just another failure case we hadn't encountered.
will rewrite to extract that entire folder.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r101522786
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,15 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // port discovery routines may take some time to clear, e.g. 250ms for SO_TIMEOUT
+ // tests fail because of this; see if adding a delay improves things
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
--- End diff --
Another use case which would cause bind to fail is described in the comments of [isPortAvailable](https://github.com/apache/brooklyn-server/blob/master/utils/common/src/main/java/org/apache/brooklyn/util/net/Networking.java#L96-L98). Again the solution is to remove `SO_REUSEADDR`.
Will prepare a PR which does the change - let's see if it will go well with the PR build.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r101520554
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,15 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // port discovery routines may take some time to clear, e.g. 250ms for SO_TIMEOUT
+ // tests fail because of this; see if adding a delay improves things
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
--- End diff --
> Why would setReuseAddress(true) improve false positives? As long as we make the same call when we start the server it should be identical? (Or are you thinking that the root cause is that the server-start call isn't using that, and that is what you'd fix?)
That's right - `setReuseAddress(true)` is used in `isPortAvailable`, but not in the actual server bind call (for web or jmx). `SO_REUSEADDR` allows you to bind to a port in `TIME_WAIT` state. So the availability check will succeed, but the server bind will fail.
I wouldn't add `setReuseAddress(true)` to the server, but will instead remove it from `isPortAvailable`.
We have parallel tests not from the same maven run, but from parallel jenkins jobs (another process).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinite test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin closed the pull request at:
https://github.com/apache/brooklyn-server/pull/560
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r103718993
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,14 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // retry once just in case it was some fluke or race
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
+ }
--- End diff --
Wdyt about catching the second `BindException` in the test that used to fail (in case the backoff is not good enough) and dump some [troubleshooting details](https://github.com/apache/incubator-brooklyn/pull/657/files#diff-2219dcab34c5379452f6ac98854d8dd0R88)?
Seems the problem is rare and it's hard to get to the root cause just by re-running jobs on the slaves.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
This now extracts classpath folders to a temp folder on the filespace. Builds on #485 (using an intermediate JAR) for simplicity. Assuming the `@` escaping was the cause of orphans this should fix it -- in any case it improves the hacky `getFile()` done on a classpath resource URL.
Note #485 needs review first now. Personally I think that PR should be simpler than it has been as it's a pretty clear improvement and further PRs can improve it further -- but if it's contentious I can refactor this to avoid the intermediate archive and thus the dependency (though personally not convinced it's worth it).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r103887396
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/CleanOrphanedLocationsIntegrationTest.java ---
@@ -66,13 +72,26 @@
private Set<ManagementContext> mgmts;
private ManagementContext mgmt;
+ private String copyResourcePathToTempPath(String resourcePath) {
+ BundleMaker bm = new BundleMaker(ResourceUtils.create(this));
+ bm.setDefaultClassForLoading(CleanOrphanedLocationsIntegrationTest.class);
+ File jar = bm.createJarFromClasspathDir(resourcePath);
+ File output = Os.newTempDir("brooklyn-test-resouce-from-"+resourcePath);
+ try {
+ ArchiveUtils.extractZip(new ZipFile(jar), output.getAbsolutePath());
+ return output.getAbsolutePath();
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+ }
+
@BeforeMethod(alwaysRun = true)
@Override
public void setUp() throws Exception {
super.setUp();
- persistenceDirWithOrphanedLocations = getClass().getResource(PERSISTED_STATE_PATH_WITH_ORPHANED_LOCATIONS).getFile();
- persistenceDirWithoutOrphanedLocations = getClass().getResource(PERSISTED_STATE_PATH_WITHOUT_ORPHANED_LOCATIONS).getFile();
- persistenceDirWithMultipleLocationsOccurrence = getClass().getResource(PERSISTED_STATE_PATH_WITH_MULTIPLE_LOCATIONS_OCCURRENCE).getFile();
+ persistenceDirWithOrphanedLocations = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITH_ORPHANED_LOCATIONS);
+ persistenceDirWithoutOrphanedLocations = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITHOUT_ORPHANED_LOCATIONS);
+ persistenceDirWithMultipleLocationsOccurrence = copyResourcePathToTempPath(PERSISTED_STATE_PATH_WITH_MULTIPLE_LOCATIONS_OCCURRENCE);
--- End diff --
There's the [File(URI)](https://docs.oracle.com/javase/7/docs/api/java/io/File.html#File(java.net.URI)) constructor. That's tests so it's pretty safe to assume they execute from the file system.
Since what you wrote works correctly I'm happy to leave it as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r103746683
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,14 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // retry once just in case it was some fluke or race
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
+ }
--- End diff --
good idea for the future. i maybe wouldn't do it here (in `/src/main` code), but instead attach it as a failure listener for all/selected tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #560: Attempted non-determinite test fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/560#discussion_r101483274
--- Diff: launcher/src/main/java/org/apache/brooklyn/launcher/BrooklynWebServer.java ---
@@ -447,7 +450,15 @@ public synchronized void start() throws Exception {
rootContext.setTempDirectory(Os.mkdirs(new File(webappTempDir, "war-root")));
server.setHandler(handlers);
- server.start();
+ try {
+ server.start();
+ } catch (BindException e) {
+ // port discovery routines may take some time to clear, e.g. 250ms for SO_TIMEOUT
+ // tests fail because of this; see if adding a delay improves things
+ log.warn("Initial server start-up failed binding (retrying after a delay): "+e);
+ Time.sleep(Duration.millis(500));
+ server.start();
--- End diff --
Just binding to a socket won't result in a `TIME_WAIT` state (the `isPortAvailable` check). `TIME_WAIT` is the result of a previous closed connection (previous test) or a client trying to connect in the small window the probe socket is open.
One thing that will improve false positive tests is removing [`setReuseAddress(true)`](https://github.com/apache/brooklyn-server/blob/master/utils/common/src/main/java/org/apache/brooklyn/util/net/Networking.java#L105). The `SO_TIMEOUT=250` in the same method isn't used at all since it applies to reads which we don't do here. So it can go away as well. `TIME_WAIT` timeout could be in the minutes range.
Instead of papering over the problem suggest we go for the root cause directly and the Apache servers are an excellent test bed for it. I can do the networking changes if you prefer?
LGTM other than this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
LGTM as well; merging now :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
@aledsage I didn't pull in #485 for fun. The simplest fix for some of the failures involves building an intermediate bundle thus
> Builds on #485 (using an intermediate JAR) for simplicity.
I'll split out the REST portion of #485 and look at your comments after which this will be simpler to review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Applied @neykov 's comments. If all tests pass then good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
i wonder if the problem is that the `@` is being escaped when it shouldn't. would explain why it fails non-deterministically, probably sometimes it runs in the non-`@2` directory. and can't tell why it is getting escaped at all ... we do `Class.getResource(..).getFile()` and that's what we use. on my box it doesn't seem to be escaped if i mess it up to run in a dir containing an `@`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
Curious. I'll amend comments here and merge if that's okay. Happy to remove
the setReuseAddr or not depending who gets to it first.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
@aledsage I like how you split out #485; this has been rebased and conflicts merged. Should be easy to review now!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
interesting comments @neykov -- but think this can be merged without those (once you've finished)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
nope, last failure shows this didn't fix the orphaned state failures either :(
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #560: Attempted non-determinate test fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/560
at the moment the contents look healthy in https://builds.apache.org/job/brooklyn-server-pull-requests/ws/launcher/target/test-classes/orphaned-locations-test-data/data-with-orphaned-locations/locations/ ... i guess either there is something wrong with the path (need to confirm `%402` but assuming `%40` is octal for a space that seems reasonable) or something else is clobbering the files on disk (but I'd expect a parallel build in the same space to wipe out the `target/` directory and thus be very damaging).
probably good practise to read the files from the classpath and save them somewhere else, rather than assuming an unpacked directory. that would fix the problem. but i'd like to to understand it in case it's causes other issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---