Merge branch 'stable-3.1' into stable-3.2 * stable-3.1: e2e-tests: Use now available cluster_port Change-Id: I5cd01e4fac9c4c7f82139acb07d5f17271534fbc
diff --git a/BUILD b/BUILD index c3f5a11..3b896ff 100644 --- a/BUILD +++ b/BUILD
@@ -25,6 +25,7 @@ junit_tests( name = "high-availability_tests", srcs = glob(["src/test/java/**/*.java"]), + javacopts = ["-Xep:DoNotMock:OFF"], resources = glob(["src/test/resources/**/*"]), tags = [ "high-availability",
diff --git a/WORKSPACE b/WORKSPACE index 6969594..acca707 100644 --- a/WORKSPACE +++ b/WORKSPACE
@@ -3,7 +3,7 @@ load("//:bazlets.bzl", "load_bazlets") load_bazlets( - commit = "87fd5f0d0a89d01df13deaf2d21a4bdb3bc03cfd", + commit = "8dc0767541f16b35d2136eccebffd9ebe2b81133", #local_path = "/home/<user>/projects/bazlets", )
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java index 093628e..af1c4c3 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/Configuration.java
@@ -17,6 +17,7 @@ import static java.util.concurrent.TimeUnit.HOURS; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CharMatcher; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; @@ -77,8 +78,9 @@ STATIC } + @VisibleForTesting @Inject - Configuration( + public Configuration( PluginConfigFactory pluginConfigFactory, @PluginName String pluginName, SitePaths site) { Config cfg = pluginConfigFactory.getGlobalPluginConfig(pluginName); main = new Main(site, cfg);
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java index 561bb19..62d9c10 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/IndexTs.java
@@ -133,12 +133,12 @@ currCtx.onlyWithContext( (ctx) -> { try { - ChangeNotes changeNotes = changeFinder.findOne(projectName + "~" + id); + Optional<ChangeNotes> changeNotes = changeFinder.findOne(projectName + "~" + id); update( IndexName.CHANGE, - changeNotes == null + !changeNotes.isPresent() ? LocalDateTime.now() - : changeNotes.getChange().getLastUpdatedOn().toLocalDateTime()); + : changeNotes.get().getChange().getLastUpdatedOn().toLocalDateTime()); } catch (Exception e) { log.atWarning().withCause(e).log("Unable to update the latest TS for change %d", id); }
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java index 0f43eaf..baf512f 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcher.java
@@ -26,7 +26,7 @@ @Singleton class CachePatternMatcher { private static final List<String> DEFAULT_PATTERNS = - ImmutableList.of("accounts", "^groups.*", "ldap_usernames", "projects", "sshkeys"); + ImmutableList.of("^groups.*", "ldap_usernames", "projects", "sshkeys"); private final Pattern pattern;
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java index f156da4..fb651b7 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBroker.java
@@ -14,6 +14,7 @@ package com.ericsson.gerrit.plugins.highavailability.forwarder; +import com.google.gerrit.server.config.GerritInstanceId; import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.EventBroker; import com.google.gerrit.server.events.EventListener; @@ -23,6 +24,8 @@ import com.google.gerrit.server.plugincontext.PluginSetContext; import com.google.gerrit.server.project.ProjectCache; import com.google.inject.Inject; +import java.util.Objects; +import javax.annotation.Nullable; class ForwardedAwareEventBroker extends EventBroker { @@ -32,12 +35,30 @@ PluginSetContext<EventListener> unrestrictedListeners, PermissionBackend permissionBackend, ProjectCache projectCache, - Factory notesFactory) { - super(listeners, unrestrictedListeners, permissionBackend, projectCache, notesFactory); + Factory notesFactory, + @Nullable @GerritInstanceId String gerritInstanceId) { + super( + listeners, + unrestrictedListeners, + permissionBackend, + projectCache, + notesFactory, + gerritInstanceId); + } + + private boolean isProducedByLocalInstance(Event event) { + return Objects.equals(event.instanceId, gerritInstanceId); } @Override protected void fireEventForUnrestrictedListeners(Event event) { + // An event should not be dispatched when it is "forwarded". + // Meaning, it was either produced somewhere else + if (!isProducedByLocalInstance(event)) { + Context.setForwardedEvent(true); + } + // or it was consumed by the high-availability rest endpoint and + // thus the context of its consumption has already been set to "forwarded". if (!Context.isForwardedEvent()) { super.fireEventForUnrestrictedListeners(event); }
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java index 23712be..945cefa 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java +++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/index/ChangeCheckerImpl.java
@@ -87,7 +87,7 @@ @Override public Optional<ChangeNotes> getChangeNotes() { try (ManualRequestContext ctx = oneOffReqCtx.open()) { - changeNotes = Optional.ofNullable(changeFinder.findOne(changeId)); + changeNotes = changeFinder.findOne(changeId); return changeNotes; } }
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 5186bd1..3b1fee2 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -3,8 +3,8 @@ ========================= The @PLUGIN@ plugin must be installed on all the instances. Each instance should -be configured with the same [gerrit.serverId](https://gerrit-documentation.storage.googleapis.com/Documentation/3.1.0/config-gerrit.html#gerrit.serverId). -If there are existing changes in [NoteDb](https://gerrit-documentation.storage.googleapis.com/Documentation/3.1.0/note-db.html) +be configured with the same [gerrit.serverId](https://gerrit-documentation.storage.googleapis.com/Documentation/3.2.0/config-gerrit.html#gerrit.serverId). +If there are existing changes in [NoteDb](https://gerrit-documentation.storage.googleapis.com/Documentation/3.2.0/note-db.html) made with another `serverId`, then this plugin might not be able to access them. Likewise, if the HA gerrit.serverIds differ, then changes conveyed by one instance will not be accessible by the other. @@ -238,3 +238,23 @@ ```healthcheck.enable``` : Whether to enable the health check endpoint. Defaults to 'true'. + +File 'gerrit.config' +-------------------- + +```gerrit.instanceId``` +: Optional identifier for this Gerrit instance. +[[docs](https://gerrit-documentation.storage.googleapis.com/Documentation/3.2.0/config-gerrit.html#gerrit.instanceId)]. + +Whilst this is not, specifically, a high-availability plugin configuration, it plays +an important role on which events are forwarded to peers. + +If `instanceId` is set, events produced by that Gerrit instance will contain its +origin in the `instanceId` field, allowing to track where they are coming from. + +The high-availability plugin will check the `instanceId` value to decide whether +an event should be forwarded: events that originated from different Gerrit instances +will not be forwarded. + +When neither `gerrit.instanceId` nor `event.instanceId` are set, it is not possible +to identify the origin of the event and thus the event is always forwarded.
diff --git a/src/test/docker/gerrit/Dockerfile b/src/test/docker/gerrit/Dockerfile index c40d91b..3c74b10 100644 --- a/src/test/docker/gerrit/Dockerfile +++ b/src/test/docker/gerrit/Dockerfile
@@ -1,12 +1,12 @@ -FROM gerritcodereview/gerrit:3.1.15 +FROM gerritcodereview/gerrit:3.2.10 -ENV GERRIT_BRANCH=stable-3.1 +ENV GERRIT_BRANCH=stable-3.2 -ENV GERRIT_CI_URL=https://archive-ci.gerritforge.com/job +ENV GERRIT_CI_URL=https://gerrit-ci.gerritforge.com/job USER root -RUN yum install -y iputils-ping netcat curl lsof gettext moreutils net-tools netcat inetutils-ping sudo +RUN yum install -y iputils nmap curl lsof gettext net-tools sudo USER gerrit
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java index 5139a66..ce11242 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/autoreindex/GroupReindexRunnableTest.java
@@ -194,13 +194,16 @@ } private Optional<InternalGroup> getInternalGroup(Timestamp timestamp) { - AccountGroup accountGroup = - new AccountGroup(AccountGroup.nameKey("Test"), AccountGroup.id(1), uuid, timestamp); return Optional.ofNullable( - InternalGroup.create( - accountGroup, - ImmutableSet.<Id>builder().build(), - ImmutableSet.<UUID>builder().build(), - null)); + InternalGroup.builder() + .setId(AccountGroup.Id.parse("1")) + .setNameKey(AccountGroup.nameKey("Test")) + .setOwnerGroupUUID(uuid) + .setVisibleToAll(true) + .setGroupUUID(UUID.parse("12")) + .setCreatedOn(timestamp) + .setMembers(ImmutableSet.<Id>builder().build()) + .setSubgroups(ImmutableSet.<UUID>builder().build()) + .build()); } }
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionHandlerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionHandlerTest.java new file mode 100644 index 0000000..7077319 --- /dev/null +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionHandlerTest.java
@@ -0,0 +1,63 @@ +// 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.ericsson.gerrit.plugins.highavailability.cache; + +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; + +import com.ericsson.gerrit.plugins.highavailability.Configuration; +import com.ericsson.gerrit.plugins.highavailability.forwarder.Forwarder; +import com.google.common.cache.RemovalCause; +import com.google.common.cache.RemovalNotification; +import com.google.gerrit.server.config.PluginConfigFactory; +import com.google.gerrit.server.config.SitePaths; +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.concurrent.Executor; +import org.eclipse.jgit.lib.Config; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class CacheEvictionHandlerTest { + @Mock private Executor executorMock; + @Mock private Forwarder forwarder; + @Mock private PluginConfigFactory pluginConfigFactoryMock; + + private static final String PLUGIN_NAME = "high-availability"; + private static final Path SITE_PATH = Paths.get("/site_path"); + private CachePatternMatcher defaultCacheMatcher; + + @Before + public void setUp() throws IOException { + when(pluginConfigFactoryMock.getGlobalPluginConfig(PLUGIN_NAME)).thenReturn(new Config()); + defaultCacheMatcher = + new CachePatternMatcher( + new Configuration(pluginConfigFactoryMock, PLUGIN_NAME, new SitePaths(SITE_PATH))); + } + + @Test + public void shouldNotPublishAccountsCacheEvictions() { + CacheEvictionHandler<String, String> handler = + new CacheEvictionHandler<>(forwarder, executorMock, PLUGIN_NAME, defaultCacheMatcher); + handler.onRemoval( + "test", "accounts", RemovalNotification.create("test", "accounts", RemovalCause.EXPLICIT)); + verifyZeroInteractions(executorMock); + } +}
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java index e91e2ae..180c68f 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CacheEvictionIT.java
@@ -26,13 +26,13 @@ import com.github.tomakehurst.wiremock.junit.WireMockRule; import com.google.common.cache.LoadingCache; -import com.google.gerrit.acceptance.GerritConfig; -import com.google.gerrit.acceptance.GlobalPluginConfig; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.acceptance.UseSsh; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.acceptance.config.GlobalPluginConfig; import com.google.gerrit.entities.AccountGroup; import com.google.inject.Inject; import com.google.inject.name.Named;
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java index 9b6e66b..caf50b9 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/CachePatternMatcherTest.java
@@ -38,7 +38,6 @@ CachePatternMatcher matcher = new CachePatternMatcher(configurationMock); for (String cache : ImmutableList.of( - "accounts", "groups", "groups_byinclude", "groups_byname", @@ -55,6 +54,7 @@ } for (String cache : ImmutableList.of( + "accounts", "adv_bases", "change_kind", "change_notes",
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java index 22ef89e..98cdce5 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/cache/ProjectListIT.java
@@ -25,11 +25,11 @@ import static com.google.common.truth.Truth.assertThat; import com.github.tomakehurst.wiremock.junit.WireMockRule; -import com.google.gerrit.acceptance.GlobalPluginConfig; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GlobalPluginConfig; import com.google.gerrit.extensions.restapi.Url; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit;
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java index 36cc953..cfe0475 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedAwareEventBrokerTest.java
@@ -30,7 +30,9 @@ private EventListener listenerMock; private ForwardedAwareEventBroker broker; - private Event event = new TestEvent(); + private ForwardedAwareEventBroker brokerWithGerritInstanceId; + private Event event; + private String gerritInstanceId = "gerrit-instance-id"; @Before public void setUp() { @@ -38,8 +40,11 @@ listenerMock = mock(EventListener.class); DynamicSet<EventListener> set = DynamicSet.emptySet(); set.add("high-availability", listenerMock); + event = new TestEvent(); PluginSetContext<EventListener> listeners = new PluginSetContext<>(set, mockMetrics); - broker = new ForwardedAwareEventBroker(null, listeners, null, null, null); + broker = new ForwardedAwareEventBroker(null, listeners, null, null, null, null); + brokerWithGerritInstanceId = + new ForwardedAwareEventBroker(null, listeners, null, null, null, gerritInstanceId); } @Test @@ -58,4 +63,43 @@ } verifyZeroInteractions(listenerMock); } + + @Test + public void shouldNotDispatchEventWhenEventInstanceIdIsDefinedButGerritInstanceIdIsNot() { + event.instanceId = gerritInstanceId; + try { + broker.fireEventForUnrestrictedListeners(event); + } finally { + Context.unsetForwardedEvent(); + } + verifyZeroInteractions(listenerMock); + } + + @Test + public void shouldNotDispatchEventWhenGerritInstanceIdIsDefinedButEventInstanceIdIsNot() { + try { + brokerWithGerritInstanceId.fireEventForUnrestrictedListeners(event); + } finally { + Context.unsetForwardedEvent(); + } + verifyZeroInteractions(listenerMock); + } + + @Test + public void shouldNotDispatchEventWhenInstanceIdsAreDifferent() { + event.instanceId = "some-other-gerrit-instance-id"; + try { + brokerWithGerritInstanceId.fireEventForUnrestrictedListeners(event); + } finally { + Context.unsetForwardedEvent(); + } + verifyZeroInteractions(listenerMock); + } + + @Test + public void shouldDispatchEventWhenInstanceIdsAreEqual() { + event.instanceId = gerritInstanceId; + brokerWithGerritInstanceId.fireEventForUnrestrictedListeners(event); + verify(listenerMock).onEvent(event); + } }
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java index af4637e..5843908 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java +++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/index/AbstractIndexForwardingIT.java
@@ -26,11 +26,11 @@ import static com.google.common.truth.Truth.assertThat; import com.github.tomakehurst.wiremock.junit.WireMockRule; -import com.google.gerrit.acceptance.GlobalPluginConfig; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GlobalPluginConfig; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.apache.http.HttpStatus;