commit 1de0e7cccba81a7da1501e228a5d3e355e3ef819 Author: Thomas Draebing Date: Wed Aug 5 16:25:05 2020 +0200 Limit graceful shutdown to SSH sessions serving git requests Not all SSH sessions should be shut down gracefully. E.g. sessions serving the EventStream command should rather be closed immediately, since it would otherwise keep the shutdown waiting until the timeout is reached. This change adds a counter to the SshSession that tracks the number of threads running tasks for this session that require a graceful shutdown. A graceful shutdown of the session is only done, if the counter is zero. Each command has to take care itself of incrementing or decrementing the counter. This change activates graceful shutdown for git commands, but does not enable it for other commands yet. Change-Id: I31ac3fbfe791fc3dc4aacbae9b1226549a1e8f49 diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 6bacc1a..f05c598 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -364,6 +364,15 @@ public abstract class AbstractDaemonTest { initSsh(); } + protected void restart() throws Exception { + server = GerritServer.restart(server, createModule(), createSshModule()); + server.getTestInjector().injectMembers(this); + if (resetter != null) { + server.getTestInjector().injectMembers(resetter); + } + initSsh(); + } + protected void evictAndReindexAccount(Account.Id accountId) { accountCache.evict(accountId); accountIndexer.index(accountId); @@ -399,13 +408,16 @@ public abstract class AbstractDaemonTest { baseConfig.setInt("receive", null, "changeUpdateThreads", 4); Module module = createModule(); + Module sshModule = createSshModule(); if (classDesc.equals(methodDesc) && !classDesc.sandboxed() && !methodDesc.sandboxed()) { if (commonServer == null) { - commonServer = GerritServer.initAndStart(temporaryFolder, classDesc, baseConfig, module); + commonServer = + GerritServer.initAndStart(temporaryFolder, classDesc, baseConfig, module, sshModule); } server = commonServer; } else { - server = GerritServer.initAndStart(temporaryFolder, methodDesc, baseConfig, module); + server = + GerritServer.initAndStart(temporaryFolder, methodDesc, baseConfig, module, sshModule); } server.getTestInjector().injectMembers(this); @@ -445,6 +457,11 @@ public abstract class AbstractDaemonTest { return null; } + /** Override to bind an additional Guice module for SSH injector */ + public Module createSshModule() { + return null; + } + protected void initSsh() throws Exception { if (testRequiresSsh && SshMode.useSsh() diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD index f7c0aee..25d8df1 100644 --- a/java/com/google/gerrit/acceptance/BUILD +++ b/java/com/google/gerrit/acceptance/BUILD @@ -84,6 +84,7 @@ java_library2( "//lib:jimfs", "//lib/auto:auto-value", "//lib/auto:auto-value-annotations", + "//lib/flogger:api", "//lib/httpcomponents:fluent-hc", "//lib/httpcomponents:httpclient", "//lib/httpcomponents:httpcore", diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 893106b..bb828af 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -273,6 +273,7 @@ public class GerritServer implements AutoCloseable { * @param desc server description. * @param baseConfig default config values; merged with config from {@code desc}. * @param testSysModule additional Guice module to use. + * @param testSshModule additional Guice module to use. * @return started server. * @throws Exception */ @@ -280,14 +281,15 @@ public class GerritServer implements AutoCloseable { TemporaryFolder temporaryFolder, Description desc, Config baseConfig, - @Nullable Module testSysModule) + @Nullable Module testSysModule, + @Nullable Module testSshModule) throws Exception { Path site = temporaryFolder.newFolder().toPath(); try { if (!desc.memory()) { init(desc, baseConfig, site); } - return start(desc, baseConfig, site, testSysModule, null); + return start(desc, baseConfig, site, testSysModule, testSshModule, null); } catch (Exception e) { throw e; } @@ -303,6 +305,7 @@ public class GerritServer implements AutoCloseable { * initialize this directory. Can be retrieved from the returned instance via {@link * #getSitePath()}. * @param testSysModule optional additional module to add to the system injector. + * @param testSshModule optional additional module to add to the ssh injector. * @param inMemoryRepoManager {@link InMemoryRepositoryManager} that should be used if the site is * started in memory * @param additionalArgs additional command-line arguments for the daemon program; only allowed if @@ -315,6 +318,7 @@ public class GerritServer implements AutoCloseable { Config baseConfig, Path site, @Nullable Module testSysModule, + @Nullable Module testSshModule, @Nullable InMemoryRepositoryManager inMemoryRepoManager, String... additionalArgs) throws Exception { @@ -337,6 +341,9 @@ public class GerritServer implements AutoCloseable { if (testSysModule != null) { daemon.addAdditionalSysModuleForTesting(testSysModule); } + if (testSshModule != null) { + daemon.addAdditionalSshModuleForTesting(testSshModule); + } daemon.setEnableSshd(desc.useSsh()); if (desc.memory()) { @@ -553,7 +560,24 @@ public class GerritServer implements AutoCloseable { server.close(); server.daemon.stop(); - return start(server.desc, cfg, site, null, inMemoryRepoManager); + return start(server.desc, cfg, site, null, null, inMemoryRepoManager); + } + + public static GerritServer restart( + GerritServer server, @Nullable Module testSysModule, @Nullable Module testSshModule) + throws Exception { + checkState(server.desc.sandboxed(), "restarting as slave requires @Sandboxed"); + Config cfg = server.testInjector.getInstance(Key.get(Config.class, GerritServerConfig.class)); + Path site = server.testInjector.getInstance(Key.get(Path.class, SitePath.class)); + + InMemoryRepositoryManager inMemoryRepoManager = null; + if (hasBinding(server.testInjector, InMemoryRepositoryManager.class)) { + inMemoryRepoManager = server.testInjector.getInstance(InMemoryRepositoryManager.class); + } + + server.close(); + server.daemon.stop(); + return start(server.desc, cfg, site, testSysModule, testSshModule, inMemoryRepoManager); } private static boolean hasBinding(Injector injector, Class clazz) { diff --git a/java/com/google/gerrit/acceptance/SshSession.java b/java/com/google/gerrit/acceptance/SshSession.java index fa0bc90..fd60d16 100644 --- a/java/com/google/gerrit/acceptance/SshSession.java +++ b/java/com/google/gerrit/acceptance/SshSession.java @@ -66,6 +66,22 @@ public class SshSession { } } + @SuppressWarnings("resource") + public int execAndReturnStatus(String command) throws Exception { + ChannelExec channel = (ChannelExec) getSession().openChannel("exec"); + try { + channel.setCommand(command); + InputStream err = channel.getErrStream(); + channel.connect(); + + Scanner s = new Scanner(err, UTF_8.name()).useDelimiter("\\A"); + error = s.hasNext() ? s.next() : null; + return channel.getExitStatus(); + } finally { + channel.disconnect(); + } + } + public InputStream exec2(String command, InputStream opt) throws Exception { ChannelExec channel = (ChannelExec) getSession().openChannel("exec"); channel.setCommand(command); diff --git a/java/com/google/gerrit/acceptance/StandaloneSiteTest.java b/java/com/google/gerrit/acceptance/StandaloneSiteTest.java index 53f1ce9..d20124a 100644 --- a/java/com/google/gerrit/acceptance/StandaloneSiteTest.java +++ b/java/com/google/gerrit/acceptance/StandaloneSiteTest.java @@ -207,7 +207,7 @@ public abstract class StandaloneSiteTest { private GerritServer startImpl(@Nullable Module testSysModule, String... additionalArgs) throws Exception { return GerritServer.start( - serverDesc, baseConfig, sitePaths.site_path, testSysModule, null, additionalArgs); + serverDesc, baseConfig, sitePaths.site_path, testSysModule, null, null, additionalArgs); } protected static void runGerrit(String... args) throws Exception { diff --git a/java/com/google/gerrit/acceptance/ssh/GracefulCommand.java b/java/com/google/gerrit/acceptance/ssh/GracefulCommand.java new file mode 100644 index 0000000..ddaf341 --- /dev/null +++ b/java/com/google/gerrit/acceptance/ssh/GracefulCommand.java @@ -0,0 +1,31 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE; + +import com.google.gerrit.sshd.CommandMetaData; + +@CommandMetaData( + name = "graceful", + description = "Test command for graceful shutdown", + runsAt = MASTER_OR_SLAVE) +public class GracefulCommand extends TestCommand { + + @Override + boolean isGraceful() { + return true; + } +} diff --git a/java/com/google/gerrit/acceptance/ssh/NonGracefulCommand.java b/java/com/google/gerrit/acceptance/ssh/NonGracefulCommand.java new file mode 100644 index 0000000..ed635c8 --- /dev/null +++ b/java/com/google/gerrit/acceptance/ssh/NonGracefulCommand.java @@ -0,0 +1,31 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE; + +import com.google.gerrit.sshd.CommandMetaData; + +@CommandMetaData( + name = "non-graceful", + description = "Test command for immediate shutdown", + runsAt = MASTER_OR_SLAVE) +public class NonGracefulCommand extends TestCommand { + + @Override + boolean isGraceful() { + return false; + } +} diff --git a/java/com/google/gerrit/acceptance/ssh/TestCommand.java b/java/com/google/gerrit/acceptance/ssh/TestCommand.java new file mode 100644 index 0000000..7839578 --- /dev/null +++ b/java/com/google/gerrit/acceptance/ssh/TestCommand.java @@ -0,0 +1,49 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.sshd.SshCommand; +import java.util.concurrent.CyclicBarrier; +import org.kohsuke.args4j.Option; + +public abstract class TestCommand extends SshCommand { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + public static final CyclicBarrier syncPoint = new CyclicBarrier(2); + + @Option( + name = "--duration", + aliases = {"-d"}, + required = true, + usage = "Duration of the command execution in seconds") + private int duration; + + @Override + protected void run() throws UnloggedFailure, Failure, Exception { + logger.atFine().log("Starting command."); + if (isGraceful()) { + enableGracefulStop(); + } + try { + syncPoint.await(); + Thread.sleep(duration * 1000); + logger.atFine().log("Stopping command."); + } catch (Exception e) { + throw die("Command ended prematurely.", e); + } + } + + abstract boolean isGraceful(); +} diff --git a/java/com/google/gerrit/acceptance/ssh/TestSshCommandModule.java b/java/com/google/gerrit/acceptance/ssh/TestSshCommandModule.java new file mode 100644 index 0000000..626092b --- /dev/null +++ b/java/com/google/gerrit/acceptance/ssh/TestSshCommandModule.java @@ -0,0 +1,25 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +import com.google.gerrit.sshd.CommandModule; + +public class TestSshCommandModule extends CommandModule { + @Override + protected void configure() { + command("graceful").to(GracefulCommand.class); + command("non-graceful").to(NonGracefulCommand.class); + } +} diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index d2371e8..a5c6dac 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -189,6 +189,7 @@ public class Daemon extends SiteProgram { private AbstractModule luceneModule; private Module emailModule; private List testSysModules = new ArrayList<>(); + private List testSshModules = new ArrayList<>(); private Module auditEventModule; private Runnable serverStarted; @@ -320,6 +321,11 @@ public class Daemon extends SiteProgram { } @VisibleForTesting + public void addAdditionalSshModuleForTesting(@Nullable Module... modules) { + testSshModules.addAll(Arrays.asList(modules)); + } + + @VisibleForTesting public void start() throws IOException { if (dbInjector == null) { dbInjector = createDbInjector(true /* enableMetrics */); @@ -523,6 +529,8 @@ public class Daemon extends SiteProgram { slave, sysInjector.getInstance(DownloadConfig.class), sysInjector.getInstance(LfsPluginAuthCommand.Module.class))); + + modules.addAll(testSshModules); if (!slave) { modules.add(new IndexCommandsModule(sysInjector)); } diff --git a/java/com/google/gerrit/sshd/AbstractGitCommand.java b/java/com/google/gerrit/sshd/AbstractGitCommand.java index f617ebb..90f0c43 100644 --- a/java/com/google/gerrit/sshd/AbstractGitCommand.java +++ b/java/com/google/gerrit/sshd/AbstractGitCommand.java @@ -48,6 +48,7 @@ public abstract class AbstractGitCommand extends BaseCommand { @Override public void start(Environment env) { + enableGracefulStop(); Context ctx = context.subContext(newSession(), context.getCommandLine()); final Context old = sshScope.set(ctx); try { diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java index 7c77a2c..1d9635f 100644 --- a/java/com/google/gerrit/sshd/BaseCommand.java +++ b/java/com/google/gerrit/sshd/BaseCommand.java @@ -401,6 +401,10 @@ public abstract class BaseCommand implements Command { } } + protected void enableGracefulStop() { + context.getSession().setGracefulStop(true); + } + protected String getTaskDescription() { String[] ta = getTrimmedArguments(); if (ta != null) { diff --git a/java/com/google/gerrit/sshd/SshDaemon.java b/java/com/google/gerrit/sshd/SshDaemon.java index b254873..8265413 100644 --- a/java/com/google/gerrit/sshd/SshDaemon.java +++ b/java/com/google/gerrit/sshd/SshDaemon.java @@ -88,6 +88,7 @@ import org.apache.sshd.common.mac.Mac; import org.apache.sshd.common.random.Random; import org.apache.sshd.common.random.SingletonRandomFactory; import org.apache.sshd.common.session.Session; +import org.apache.sshd.common.session.helpers.AbstractSession; import org.apache.sshd.common.session.helpers.DefaultUnknownChannelReferenceHandler; import org.apache.sshd.common.util.buffer.Buffer; import org.apache.sshd.common.util.buffer.ByteArrayBuffer; @@ -366,14 +367,24 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { Collection ioSessions = daemonAcceptor.getManagedSessions().values(); CountDownLatch allSessionsClosed = new CountDownLatch(ioSessions.size()); for (IoSession io : ioSessions) { - logger.atFine().log("Waiting for session %s to stop.", io.getId()); - io.addCloseFutureListener( - new SshFutureListener() { - @Override - public void operationComplete(CloseFuture future) { - allSessionsClosed.countDown(); - } - }); + AbstractSession serverSession = AbstractSession.getSession(io, true); + SshSession sshSession = + serverSession != null ? serverSession.getAttribute(SshSession.KEY) : null; + if (sshSession != null && sshSession.requiresGracefulStop()) { + logger.atFine().log("Waiting for session %s to stop.", io.getId()); + io.addCloseFutureListener( + new SshFutureListener() { + @Override + public void operationComplete(CloseFuture future) { + logger.atFine().log("Session %s was stopped.", io.getId()); + allSessionsClosed.countDown(); + } + }); + } else { + logger.atFine().log("Stopping session %s immediately.", io.getId()); + io.close(true); + allSessionsClosed.countDown(); + } } try { if (!allSessionsClosed.await(gracefulStopTimeout, TimeUnit.SECONDS)) { diff --git a/java/com/google/gerrit/sshd/SshSession.java b/java/com/google/gerrit/sshd/SshSession.java index 1a60a20..a2c3d93 100644 --- a/java/com/google/gerrit/sshd/SshSession.java +++ b/java/com/google/gerrit/sshd/SshSession.java @@ -35,6 +35,8 @@ public class SshSession { private volatile String authError; private volatile String peerAgent; + private volatile boolean gracefulStop = false; + SshSession(int sessionId, SocketAddress peer) { this.sessionId = sessionId; this.remoteAddress = peer; @@ -58,6 +60,14 @@ public class SshSession { return sessionId; } + public boolean requiresGracefulStop() { + return gracefulStop; + } + + public void setGracefulStop(boolean gracefulStop) { + this.gracefulStop = gracefulStop; + } + /** Identity of the authenticated user account on the socket. */ public CurrentUser getUser() { return identity; diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshDaemonIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshDaemonIT.java new file mode 100644 index 0000000..827c192 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/ssh/SshDaemonIT.java @@ -0,0 +1,100 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.ssh; + +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.Sandboxed; +import com.google.gerrit.acceptance.UseSsh; +import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.restapi.config.ListTasks; +import com.google.gerrit.testing.ConfigSuite; +import com.google.inject.Inject; +import com.google.inject.Module; +import java.time.LocalDateTime; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.lib.Config; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; + +@NoHttpd +@UseSsh +@Sandboxed +@RunWith(ConfigSuite.class) +@SuppressWarnings("unused") +public class SshDaemonIT extends AbstractDaemonTest { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + @Inject private ListTasks listTasks; + @Inject private SitePaths gerritSitePath; + + @ConfigSuite.Parameter protected Config config; + + @ConfigSuite.Config + public static Config gracefulConfig() { + Config config = new Config(); + config.setString("sshd", null, "gracefulStopTimeout", "10s"); + return config; + } + + @Override + public Module createSshModule() { + return new TestSshCommandModule(); + } + + public Future startCommand(String command) throws Exception { + Callable gracefulSession = + () -> { + int returnCode = -1; + logger.atFine().log("Before Command"); + returnCode = userSshSession.execAndReturnStatus(command); + logger.atFine().log("After Command"); + return returnCode; + }; + + ExecutorService executor = Executors.newFixedThreadPool(1); + Future future = executor.submit(gracefulSession); + + LocalDateTime timeout = LocalDateTime.now().plusSeconds(10); + + TestCommand.syncPoint.await(); + + return future; + } + + @Test + public void NonGracefulCommandIsStoppedImmediately() throws Exception { + Future future = startCommand("non-graceful -d 5"); + restart(); + Assert.assertTrue(future.get() == -1); + } + + @Test + public void GracefulCommandIsStoppedGracefully() throws Exception { + Future future = startCommand("graceful -d 5"); + restart(); + if (cfg.getTimeUnit("sshd", null, "gracefulStopTimeout", 0, TimeUnit.SECONDS) == 0) { + Assert.assertTrue(future.get() == -1); + } else { + Assert.assertTrue(future.get() == 0); + } + } +}