Request for Comment - Move starter proxy responsibilities into proxy set. #527

dspasojevic · 2018-04-16T04:30:50Z

Description

Removes the remote starter proxy and moves its responsibilities into a custom ProxySet.

This PR is for comment only - I don't expect it to be merged in current incarnation.

Motivation and Context

Permits closer tracking of starting, started and closing containers / proxies. Prevents creation of duplicates and orphan containers.

How Has This Been Tested?

Tested in an OpenShift environment. Requires more testing.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.

dspasojevic · 2018-04-16T05:33:01Z

@pearj do you mind taking a quick look at this change?

pearj · 2018-04-16T06:19:10Z

This relates to #441 and should probably fix most/all of the problems there.

parberge · 2018-04-16T06:26:42Z

scripts/zalenium.sh

@@ -21,6 +21,7 @@ START_TUNNEL=${START_TUNNEL:-false}
 DEBUG_ENABLED=${DEBUG_ENABLED:-false}
 KEEP_ONLY_FAILED_TESTS=${KEEP_ONLY_FAILED_TESTS:-false}
 LOG_JSON=${LOG_JSON:-false}
+NEW_SESSION_WAIT_TIMEOUT=${NEW_SESSION_WAIT_TIMEOUT:-300000}


Duplicate line of row 26

parberge · 2018-04-16T06:33:29Z

src/main/java/de/zalando/ep/zalenium/proxy/DockerSeleniumRemoteProxy.java

+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.logging.Level;
+import java.util.logging.Logger;


It think we want to use slf4j. @dennisgranath recently made changes to start using it instead of java.util.

Yeah that's true

Will change.

parberge · 2018-04-16T06:37:30Z

src/test/java/de/zalando/ep/zalenium/proxy/SauceLabsRemoteProxyTest.java

@@ -44,6 +44,7 @@

 public class SauceLabsRemoteProxyTest {

+	private DockerSeleniumRemoteProxy proxy;


Wrong indentation it seems :)

Clean up formatting and remove unused imports.

dennisgranath · 2018-04-16T08:14:47Z

When starting zalenium on docker using
docker run --rm -ti --name zalenium -p 4444:4444 -v /var/run/docker.sock:/var/run/docker.sock -v /tmp/videos:/home/seluser/videos --privileged zalenium-pr start

I get following failure when starting up the second container:

10:08:48.421 [AutoStartProxyPoolPoller] INFO d.z.e.z.proxy.AutoStartProxySet - Failed to start container. 10:08:49.502 [AutoStartProxyPoolPoller] INFO d.z.e.z.proxy.AutoStartProxySet - Autostarting container, because 1 is less than min 2 10:08:49.528 [AutoStartProxyPoolPoller] WARN d.z.e.z.c.DockerContainerClient - null Error while starting a new container com.spotify.docker.client.exceptions.DockerRequestException: Request error: POST unix://localhost:80/containers/create?name=zalenium_40000: 409, body: {"message":"Conflict. The container name \"/zalenium_40000\" is already in use by container \"7622621076a2da00a42ca7698ecb5f043ca5a73ae596cfc7df05c11b53f20c8b\". You have to remove (or rename) that container to be able to reuse that name."}

dspasojevic · 2018-04-16T08:23:07Z

Thanks @dennisgranath - I’ll run on docker and fix.

codecov-io · 2018-04-16T09:37:03Z

Codecov Report

Merging #527 into merge-527 will decrease coverage by 21.41%.
The diff coverage is 45.88%.

@@               Coverage Diff                @@
##             merge-527     #527       +/-   ##
================================================
- Coverage        80.95%   59.53%   -21.42%     
+ Complexity         585      455      -130     
================================================
  Files               32       35        +3     
  Lines             2688     2921      +233     
  Branches           233      246       +13     
================================================
- Hits              2176     1739      -437     
- Misses             372     1047      +675     
+ Partials           140      135        -5

pearj · 2018-04-16T12:35:44Z

docker/Dockerfile

@@ -318,11 +318,7 @@ RUN cd /tmp \
 # TestingBot Tunneling #
 # -----------------------#
 # https://testingbot.com/support/other/tunnel
-ENV TB_TUNNEL_URL="https://testingbot.com/tunnel/testingbot-tunnel.jar"


Probably should leave this in there. You just removed it because we doesn't work at the client site right?

Yep. I'll revert this change.

I think you could probably add: ARG testingBotEnabled=true on the line beforehand. Then you could add [ "${testingBotEnabled}" == "true" ] && \ before the wget line

I've added a pull request to restore this bit with a build arg:
dspasojevic#3

dspasojevic · 2018-04-16T13:38:12Z

@dennisgranath I've updated the DockerContainerClient to randomise six chars in the same way that kubernetes does and implemented the isTerminated check. It spins containers up for me locally. I don't have selenium tests that I can run where I am now. It would be great if you could check whether it works for you.

dennisgranath · 2018-04-17T04:48:15Z

Thanks @dspasojevic I will run them a soon as I can. Might be able to do it later today.

…exits.

pearj · 2018-04-17T06:17:52Z

src/main/java/de/zalando/ep/zalenium/matcher/DockerSeleniumCapabilityMatcher.java

@@ -48,22 +51,6 @@ public boolean matches(Map<String, Object> nodeCapability, Map<String, Object> r
            return false;
        }

-        // We do this because the starter node does not have the browser versions when Zalenium starts


@diemol Do you know if this code is necessary to not?

Yes and no, the idea of it is to reject as fast as possible a test request with a browser version we don't provide.

We need to test what happens if I want to run a test with Chrome version 60, and we provide version 66. The request should be rejected since it should not match any nodes.

In the end, this is a minor thing since we can find ways to tweak it.

pearj · 2018-04-17T06:19:32Z

src/main/java/de/zalando/ep/zalenium/matcher/ZaleniumCapabilityMatcher.java

@@ -31,7 +32,7 @@ public boolean matches(Map<String, Object> nodeCapability, Map<String, Object> r
                nodeCapability));

        for (RemoteProxy remoteProxy : proxy.getRegistry().getAllProxies()) {
-            if ((remoteProxy instanceof DockerSeleniumStarterRemoteProxy) &&
+            if ((remoteProxy instanceof DockerSeleniumRemoteProxy) &&


@dspasojevic I presume you moved some code from the starter proxy to the regular one? That's why you have a different instance type?

Yes. But I think that the effect is different. This should be a check to see if we could hypothetically start a container to handle the request. My change has made it check what we could actually handle with the running containers.

I think this is what must be duplicated here: https://github.com/zalando/zalenium/blob/master/src/main/java/de/zalando/ep/zalenium/proxy/DockerSeleniumStarterRemoteProxy.java#L189

Stop cleanup happening for idle outside of a test session

dennisgranath · 2018-04-17T15:12:30Z

Ok now it seems to work when using plain docker however I get following exception when running my selenium tests (the selenium tests are passing though):

ded396482fb124eef3a0bd3c5f661051fa8f0c7]] INFO  d.z.e.z.c.DockerContainerClient - http://172.17.0.3:40000 [bash, -c, cleanup-container.sh]
17:03:27.519 [AutoStartProxyPoolPoller] INFO  d.z.e.z.p.DockerSeleniumRemoteProxy - [c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7] is idle.
17:03:27.519 [AutoStartProxyPoolPoller] INFO  d.z.e.z.p.DockerSeleniumRemoteProxy - c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7 Shutting down node due to proxy being idle after test.
17:03:31.262 [pool-3-thread-2] INFO  o.o.g.s.proxy.DefaultRemoteProxy - c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7 Marking the node as down because it was stopped after 1 tests.
17:03:31.262 [pool-3-thread-2] WARN  d.z.e.z.registry.ZaleniumRegistry - Cleaning up stale test sessions on the unregistered node http://172.17.0.3:40000
17:03:31.291 [pool-3-thread-2] WARN  d.z.e.z.c.DockerContainerClient - null Error while stopping the container
com.spotify.docker.client.exceptions.ContainerNotFoundException: Container not found: c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7
	at com.spotify.docker.client.DefaultDockerClient.stopContainer(DefaultDockerClient.java:772)
	at de.zalando.ep.zalenium.container.DockerContainerClient.stopContainer(DockerContainerClient.java:162)
	at de.zalando.ep.zalenium.proxy.DockeredSeleniumStarter.stopContainer(DockeredSeleniumStarter.java:303)
	at de.zalando.ep.zalenium.proxy.AutoStartProxySet.remove(AutoStartProxySet.java:199)
	at de.zalando.ep.zalenium.registry.ZaleniumRegistry.removeIfPresent(ZaleniumRegistry.java:161)
	at org.openqa.grid.selenium.proxy.DefaultRemoteProxy.onEvent(DefaultRemoteProxy.java:193)
	at org.openqa.grid.selenium.proxy.DefaultRemoteProxy.addNewEvent(DefaultRemoteProxy.java:179)
	at de.zalando.ep.zalenium.proxy.DockerSeleniumRemoteProxy.shutdownNode(DockerSeleniumRemoteProxy.java:702)
	at de.zalando.ep.zalenium.proxy.DockerSeleniumRemoteProxy$1.run(DockerSeleniumRemoteProxy.java:468)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: com.spotify.docker.client.exceptions.DockerRequestException: Request error: POST unix://localhost:80/containers/c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7/stop?t=5: 404, body: {"message":"No such container: c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7"}

	at com.spotify.docker.client.DefaultDockerClient.propagate(DefaultDockerClient.java:2702)
	at com.spotify.docker.client.DefaultDockerClient.request(DefaultDockerClient.java:2663)
	at com.spotify.docker.client.DefaultDockerClient.stopContainer(DefaultDockerClient.java:766)
	... 11 common frames omitted
Caused by: javax.ws.rs.NotFoundException: HTTP 404 Not Found
	at org.glassfish.jersey.client.JerseyInvocation.convertToException(JerseyInvocation.java:1008)
	at org.glassfish.jersey.client.JerseyInvocation.translate(JerseyInvocation.java:816)
	at org.glassfish.jersey.client.JerseyInvocation.access$700(JerseyInvocation.java:92)
	at org.glassfish.jersey.client.JerseyInvocation$5.completed(JerseyInvocation.java:773)
	at org.glassfish.jersey.client.ClientRuntime.processResponse(ClientRuntime.java:198)
	at org.glassfish.jersey.client.ClientRuntime.access$300(ClientRuntime.java:79)
	at org.glassfish.jersey.client.ClientRuntime$2.run(ClientRuntime.java:180)
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:271)
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:267)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:315)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:297)
	at org.glassfish.jersey.internal.Errors.process(Errors.java:267)
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:340)
	at org.glassfish.jersey.client.ClientRuntime$3.run(ClientRuntime.java:210)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	... 3 common frames omitted
17:03:31.542 [AutoStartProxyPoolPoller] INFO  d.z.e.z.c.DockerContainerClient - Container ContainerCreationStatus [isCreated=true, containerName=zalenium_40000_oDTbUA, containerId=c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7, nodePort=40000] not found - it is terminated.
17:03:31.542 [AutoStartProxyPoolPoller] INFO  d.z.e.z.proxy.AutoStartProxySet - Container ContainerCreationStatus [isCreated=true, containerName=zalenium_40000_oDTbUA, containerId=c0f7d009e72e9a8c9a2a46a27ded396482fb124eef3a0bd3c5f661051fa8f0c7, nodePort=40000] is terminated. Removing from tracked set.
17:03:51.587 [AutoStartProxyPoolPoller] INFO  d.z.e.z.proxy.AutoStartProxySet - Status:
┌────┬───────┬─────────┬─────────┬───────────┬──────────────┬──────┬───────────┬─────────────┬───────────┐
│ Id │ Proxy │ Created │ Started │ Last Used │ Last Session │ Busy │ Timed Out │ Terminating │ Tests Run │
├┬┬┬┬┼┬┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┬┬┬┬┬┼┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┬┬┬┬┼┬┬┬┬┬┬┬┬┬┬┬┤
└┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┴┘

dspasojevic · 2018-04-17T21:37:15Z

@dennisgranath thanks for testing again. It looks like I just need to handle that exception while shutting down the container. Part of the change is it to always attempt to shutdown the container when the proxy is removed to ensure that orphan, untracked containers aren't left around. I think that zalenium itself would have been fine, and would have spun up another container if necessary.

dspasojevic · 2018-04-17T21:43:03Z

@dennisgranath I've pushed up a change to specifically handle the ContainerNotFoundException. It would be great if you could try again.

dennisgranath · 2018-04-19T11:30:34Z

Yes @pearj I saw that while checking the selenium code for healtcheck. We will remove status checking and only look for 200 for now.

dennisgranath · 2018-04-19T09:25:56Z

src/main/java/de/zalando/ep/zalenium/proxy/AutoStartProxySet.java

+    }
+
+    private void dumpStatus() {
+        if (LOGGER.isInfoEnabled()) {


We are logging into json and sending logs to an ELK stack. The ascii table is pretty neat for debugging but screws up things in kibana. Can we log this on debug or trace perhaps?

or feature hide it somehow?

Yep. Though might move to be a separate log category.

Added a separate category for the table. It is off by default in prod, on when running tests.

pearj · 2018-04-23T02:34:13Z

👍

pearj · 2018-04-23T02:37:31Z

src/test/java/de/zalando/ep/zalenium/proxy/DockerSeleniumStarterRemoteProxyTest.java

@@ -1,459 +1,414 @@
 package de.zalando.ep.zalenium.proxy;


@dspasojevic Any reason why this isn't deleted altogether yet?

Was waiting to see if there was something that could be salvaged from here. I'll look at replacing with higher value integration tests.

pearj · 2018-04-23T02:39:50Z

src/test/java/de/zalando/ep/zalenium/servlet/LiveNodeServletTest.java

-        new JMXHelper().unregister(objectName);
-        ContainerFactory.setContainerClientGenerator(originalContainerClient);
-    }
+//    private GridRegistry registry;


@dspasojevic What's the plan for this test?

I will see whether I can rebirth this test.

I think that this test was written before the registry implementation could be swapped out. I'll see if I can rewrite to use a simpler grid impl so the docker container client doesn't have to be mocked.

Does that sound sensible to you @diemol ?

Re-enabled.

pearj · 2018-04-23T02:40:24Z

src/test/java/de/zalando/ep/zalenium/servlet/VncAuthenticationServletTest.java

-
-        assertThat(statusCaptor.getValue(), equalTo(403));
-    }
+//    private GridRegistry registry;


@dspasojevic and this one?

Will check.

Re-enabled.

pearj · 2018-04-23T09:34:51Z

👍

dennisgranath · 2018-04-23T11:09:23Z

src/main/resources/logback.xml

@@ -17,6 +17,9 @@
    <!-- ALL TRACE DEBUG INFO WARN ERROR OFF -->
    <logger name="com.spotify.docker" level="WARN" />
    <logger name="org.seleniumhq.jetty9" level="WARN" />
+
+    <!-- SET TO DEBUG TO GET STATUS LOGGING -->
+    <logger name="de.zalando.ep.zalenium.proxy.AutoStartProxySet.Status" level="INFO" />


Do we need to set log level for this explicitly? The log level is set to logback.loglevel (defaulted to info) and can be set to debug using --debugEnabled flag to zalenium.

It isn't necessary. In the past, I've explicitly (though unnecessarily) set debug levels to make it obvious to others how to turn on specific types of logging.

Happy to remove if that is the consensus :)

I wonder when this logging is most useful? For people new to zalenium? Which makes me think that it should be opt-out maybe? So default it to on, but have an environment variable that can turn it off for people who don't want it? Or maybe it's just too much noise to have it on by default?

I don't know 😉

When JSON logging is isn't enabled this should be on as default is my opinion

pearj · 2018-04-23T11:45:24Z

👍

* Also make it possible to disable testing bot via a buildArg, this is to allow builds on sites where testing bot is blocked due to organisational Internet policies.

Restore testing bot

diemol · 2018-04-23T19:12:57Z

src/main/java/de/zalando/ep/zalenium/proxy/AutoStartProxySet.java

@@ -0,0 +1,525 @@
+
+// Licensed to the Software Freedom Conservancy (SFC) under one


This needs to be removed, since Zalenium is not licensed to the SFC.

diemol · 2018-04-23T19:19:40Z

src/main/java/de/zalando/ep/zalenium/proxy/DockerSeleniumRemoteProxy.java

@@ -147,7 +170,17 @@ public HtmlRenderer getHtmlRender() {
        Incrementing the number of tests that will be executed when the session is assigned.
     */
    @Override
-    public TestSession getNewSession(Map<String, Object> requestedCapability) {
+    public synchronized TestSession getNewSession(Map<String, Object> requestedCapability) {


I am not sure if synchronized is needed, since each proxy should get its own class instance. Perhaps I am missing something? @dspasojevic

We need to ensure that the thread marking instances as timed out (which will start the container shutdown process) doesn't conflict with the thread assigning sessions to the proxy. We could make the locking finer grained - e.g. use a lock when checking and updating the timed out and last command flags, but the throughput here didn't seem to warrant it.

diemol · 2018-04-23T19:22:42Z

src/main/java/de/zalando/ep/zalenium/proxy/DockerSeleniumRemoteProxy.java

    }

    /*
        Method to terminate an idle session via the registry, the code works because each one has only one slot
        We use BROWSER_TIMEOUT as a reason, but this could be changed in the future to show a more clear reason
     */
    @VisibleForTesting
-    protected synchronized void terminateIdleTest() {
+    protected void terminateIdleTest() {


Exactly, this was not really needed since each proxy has its own thread and only one test runs per proxy. 👍

This change removes that thread per proxy, there is a fixed threadpool of size 5 that handles terminations now:

See:
https://github.com/zalando/zalenium/pull/527/files#diff-cdb17f721a30ede405aeccaa422c3006R93

diemol · 2018-04-23T20:11:19Z

src/main/java/de/zalando/ep/zalenium/proxy/DockerSeleniumRemoteProxy.java

                LOGGER.info(String.format("Test session started with internal key %s and external key %s assigned to remote %s.",
                              session.getInternalKey(),
-                              externalKey.getKey(),
+                              session.getExternalKey().getKey(),


I had it like that initially, but sometimes the browser does not start for some reason and the whole thing crashes. In those cases session.getExternalKey() is null and the real exception is not sent back to the client.

As a matter of fact, I just bumped into it

22:04:16.497 [Terminate Test Session int id: [dccfea44-3436-4b26-ab35-dd9faf2311c6] ext id: [No external key was assigned] container: [f85d9cc697a6a4ff528d5930dbfed124fe856f9998295a03126831a60293c949]] INFO d.z.e.z.p.DockerSeleniumRemoteProxy - f85d9cc697a6a4ff528d5930dbfed124fe856f9998295a03126831a60293c949 AFTER_SESSION command received. Node should shutdown soon... 22:04:16.500 [qtp1046665075-39] INFO o.o.g.w.s.handler.RequestHandler - Error forwarding the new session null java.lang.NullPointerException: null at de.zalando.ep.zalenium.proxy.DockerSeleniumRemoteProxy.afterCommand(DockerSeleniumRemoteProxy.java:320) at org.openqa.grid.internal.TestSession.forward(TestSession.java:281) at org.openqa.grid.web.servlet.handler.RequestHandler.forwardNewSessionRequestAndUpdateRegistry(RequestHandler.java:91) at org.openqa.grid.web.servlet.handler.RequestHandler.process(RequestHandler.java:114) at org.openqa.grid.web.servlet.DriverServlet.process(DriverServlet.java:86) at org.openqa.grid.web.servlet.DriverServlet.doPost(DriverServlet.java:70) at javax.servlet.http.HttpServlet.service(HttpServlet.java:707) at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)

Could you please revert it?

diemol · 2018-04-23T20:19:19Z

src/main/java/de/zalando/ep/zalenium/container/DockerContainerClient.java

    private String generateContainerName(String zaleniumContainerName,
                                         String nodePort) {
-        return String.format("%s_%s", zaleniumContainerName, nodePort);
+        final String suffix = RandomStringUtils.randomAlphanumeric(6);
+        return String.format("%s_%s_%s", zaleniumContainerName, nodePort, suffix);


This PR is always using port 40000 (which makes sense since we are not mapping that to the host anymore), and then a random string.

I think we should just remove the 40000 from the name and leave the random part.

Actually it would be cool to have a random name instead of the random string, to simplify debugging and checking logs, but that's just a nice to have - we can do that in the future.

Done.

https://github.com/igr/nomen-est-omen looks like a simple drop in for a subsequent PR.

diemol · 2018-04-23T20:30:48Z

Thanks @dspasojevic for this PR!

Also thanks to @pearj, @dennisgranath and @peerster for the review and testing!

Overall, I really like what the PR is doing and goes in line with what we have discussed, I think it'll help a lot and solve other created issues.

We would really appreciate a couple of things:

Please check the Codacy comments to see if something makes sense for you to fix.
Would be cool to reenable most of the tests. I know that coverage doesn't guarantee working code always, but in the past those tests have saved us from screwing up a few things.

So, the proposal is the following:

Please check my comments and let me know if it is ok to make those changes.
Let's merge this to a protected branch, so we can run Travis with the Cloud providers as well.
We review again how things look like, do some more testing and if more changes are needed, we can send a PR to that protected branch.

After merging and releasing this, I guess some users will create issues since we are changing slightly the behaviour of some parts (like the desiredContainers part). So it would be cool if you guys help us to answer those issues and also to update the docs (things that you already do often).

Let's target to release this in 1 or 2 weeks tops (hopefully earlier), what do you think?

dspasojevic · 2018-04-23T22:42:09Z

Hi @diemol. Thanks for the feedback, I've actioned most of the changes and commented on the ones that I haven't. Codacy seems happy again.

There are two tests disabled at the moment. I'll see if I can fix them. I do wonder if more integration tests would be better - most of the unit tests that I've looked at seem to use a lot of mocking which (sometimes?) means that the tests are not as effective as they could be. wdyt?

diemol · 2018-04-25T06:16:04Z

Let's merge to a protected branch and we can take it from there, like that we can run Travis whole CI and get a better idea.

Move starter proxy responsibilities into proxy set.

121e327

pearj mentioned this pull request Apr 16, 2018

Tests fail because OpenShift container vanishes underneath #518

Closed

parberge reviewed Apr 16, 2018

View reviewed changes

Combine auto start proxy set and pool.

2806d20

Clean up formatting and remove unused imports.

Temporarily ignore two failing tests.

19bb904

pearj reviewed Apr 16, 2018

View reviewed changes

dspasojevic added 2 commits April 16, 2018 23:11

Generate unique docker container names

3bb05c3

Check for terminated docker containers.

bed45f3

Rename StarterRemoteProxy to ProxyConfiguration.

f98e40f

Rationalise config and starter. Remove proxy from set when container …

c772827

…exits.

pearj reviewed Apr 17, 2018

View reviewed changes

Daniel Spasojevic and others added 5 commits April 17, 2018 17:19

Start adding tests for pool.

37604a0

Add first pool tests.

db5a8d1

Stop cleanup happening for idle outside of a test session

119921c

Fix premature shutdown of container in test at max sessions

20c836c

Merge pull request #1 from pearj/dan-rewrite

5d376d7

Stop cleanup happening for idle outside of a test session

Handle missing docker container when shutting down gracefully.

5fbb757

dennisgranath reviewed Apr 19, 2018

View reviewed changes

pearj reviewed Apr 23, 2018

View reviewed changes

Daniel Spasojevic added 2 commits April 23, 2018 15:13

Rebirth servlet tests

8498288

Revert brain dead debug logging.

8753658

Use different category to log table output.

85c070b

dennisgranath reviewed Apr 23, 2018

View reviewed changes

pearj and others added 2 commits April 23, 2018 22:55

Restore testing bot

46756e5

* Also make it possible to disable testing bot via a buildArg, this is to allow builds on sites where testing bot is blocked due to organisational Internet policies.

Merge pull request #3 from pearj/dan-rewrite

4c872c4

Restore testing bot

diemol reviewed Apr 23, 2018

View reviewed changes

diemol mentioned this pull request Apr 23, 2018

Add initial Prometheus Support #533

Merged

5 tasks

dspasojevic added 2 commits April 24, 2018 08:22

Code review changes.

08ccd84

Codacy issues.

25e8924

pearj mentioned this pull request Apr 24, 2018

Adding Prometheus Support #534

Merged

diemol changed the base branch from master to merge-527 April 25, 2018 06:14

diemol merged commit be5e1c9 into zalando:merge-527 Apr 25, 2018

This was referenced Apr 25, 2018

Move starter proxy responsibilities into proxy set. #537

Merged

Teardown re-registered proxies so that their threads are stopped gc'd. #548

Merged

		@@ -44,6 +44,7 @@

		public class SauceLabsRemoteProxyTest {

		private DockerSeleniumRemoteProxy proxy;

		@@ -0,0 +1,525 @@

		// Licensed to the Software Freedom Conservancy (SFC) under one

Request for Comment - Move starter proxy responsibilities into proxy set. #527

Request for Comment - Move starter proxy responsibilities into proxy set. #527

Conversation

dspasojevic commented Apr 16, 2018 • edited by diemol Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

dspasojevic commented Apr 16, 2018

pearj commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisgranath commented Apr 16, 2018

dspasojevic commented Apr 16, 2018

codecov-io commented Apr 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dspasojevic commented Apr 16, 2018

dennisgranath commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisgranath commented Apr 17, 2018 • edited Loading

dspasojevic commented Apr 17, 2018

dspasojevic commented Apr 17, 2018

dennisgranath commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearj commented Apr 23, 2018

pearj Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearj commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearj commented Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diemol commented Apr 23, 2018

dspasojevic commented Apr 23, 2018

diemol commented Apr 25, 2018

dspasojevic commented Apr 16, 2018 •

edited by diemol

Loading

codecov-io commented Apr 16, 2018 •

edited

Loading

dennisgranath commented Apr 17, 2018 •

edited

Loading

pearj Apr 23, 2018 •

edited

Loading

pearj commented Apr 23, 2018 •

edited

Loading