diff options
49 files changed, 793 insertions, 467 deletions
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/search/NodeFlavorTuning.java b/config-model/src/main/java/com/yahoo/vespa/model/search/NodeFlavorTuning.java index e4806cb2353..f6f64eba482 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/search/NodeFlavorTuning.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/search/NodeFlavorTuning.java @@ -30,6 +30,12 @@ public class NodeFlavorTuning implements ProtonConfig.Producer { tuneFlushStrategyMemoryLimits(builder.flush.memory); tuneFlushStrategyTlsSize(builder.flush.memory); tuneSummaryReadIo(builder.summary.read); + tuneSummaryCache(builder.summary.cache); + } + + private void tuneSummaryCache(ProtonConfig.Summary.Cache.Builder builder) { + long memoryLimitBytes = (long) ((nodeFlavor.getMinMainMemoryAvailableGb() * 0.05) * GB); + builder.maxbytes(memoryLimitBytes); } private void setHwInfo(ProtonConfig.Builder builder) { diff --git a/config-model/src/test/java/com/yahoo/vespa/model/search/NodeFlavorTuningTest.java b/config-model/src/test/java/com/yahoo/vespa/model/search/NodeFlavorTuningTest.java index 3d62670bb52..3019b35cd2e 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/search/NodeFlavorTuningTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/search/NodeFlavorTuningTest.java @@ -87,6 +87,12 @@ public class NodeFlavorTuningTest { } @Test + public void require_that_summary_cache_max_bytes_is_set_based_on_memory() { + assertEquals(1*GB/20, configFromMemorySetting(1).summary().cache().maxbytes()); + assertEquals(256*GB/20, configFromMemorySetting(256).summary().cache().maxbytes()); + } + + @Test public void require_that_docker_node_is_tagged_with_shared_disk() { assertSharedDisk(true, true); assertSharedDisk(false, false); diff --git a/configdefinitions/src/vespa/configserver.def b/configdefinitions/src/vespa/configserver.def index 958db866f3e..fb929bfde8a 100644 --- a/configdefinitions/src/vespa/configserver.def +++ b/configdefinitions/src/vespa/configserver.def @@ -1,33 +1,31 @@ # Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. namespace=cloud.config +# Ports rpcport int default=19070 httpport int default=19071 numthreads int default=16 -# TODO: This seems to be only used by the the status API? If so this is unnecessary duplication and potentially lying: Remove -zookeepercfg string default="conf/zookeeper/zookeeper.cfg" - +# ZooKeeper zookeeperserver[].hostname string zookeeperserver[].port int default=2181 -# in seconds -zookeeper.barrierTimeout long default=120 +zookeeper.barrierTimeout long default=120 # in seconds +zookeeperLocalhostAffinity bool default=true + +# Misc configModelPluginDir[] string configServerDBDir string default="var/db/vespa/config_server/serverdb/" configDefinitionsDir string default="share/vespa/configdefinitions/" -maxgetconfigclients int default=1000000 -maxoutputbuffersize int default=65536 -# in seconds -sessionLifetime long default=3600 -applicationDirectory string default="conf/configserver-app" +sessionLifetime long default=3600 # in seconds masterGeneration long default=0 multitenant bool default=false numDelayedResponseThreads int default=1 -payloadCompressionType enum { UNCOMPRESSED, LZ4 } default=LZ4 serverId string default="localhost" hostedVespa bool default=false numParallelTenantLoaders int default=4 -zookeeperLocalhostAffinity bool default=true + +# Configserver app +applicationDirectory string default="conf/configserver-app" # Zone information environment string default="prod" @@ -38,7 +36,11 @@ defaultAdminFlavor string default="default" defaultContainerFlavor string default="default" defaultContentFlavor string default="default" +# RPC protocol +maxgetconfigclients int default=1000000 +maxoutputbuffersize int default=65536 useVespaVersionInRequest bool default=false +payloadCompressionType enum { UNCOMPRESSED, LZ4 } default=LZ4 # Docker config dockerRegistry string default="" diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/CombinedLegacyDistribution.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/CombinedLegacyDistribution.java index a45e6dc3b6b..2b179a6a2e4 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/CombinedLegacyDistribution.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/CombinedLegacyDistribution.java @@ -69,5 +69,6 @@ public class CombinedLegacyDistribution implements FileDistribution { if (request.isError() && request.errorCode() != ErrorCode.CONNECTION) { log.log(LogLevel.INFO, request.methodName() + " failed: " + request.errorCode() + " (" + request.errorMessage() + ")"); } + target.close(); } } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java index 59a0bb2e3de..6a3e9c77809 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSessionRepo.java @@ -65,7 +65,7 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> { // public for testing public void purgeOldSessions() { - log.log(LogLevel.INFO, "Purging old sessions"); // TODO: Use debug level after 2018-01-29 + log.log(LogLevel.DEBUG, "Purging old sessions"); try { List<LocalSession> sessions = new ArrayList<>(listSessions()); for (LocalSession candidate : sessions) { @@ -77,7 +77,7 @@ public class LocalSessionRepo extends SessionRepo<LocalSession> { } catch (Throwable e) { log.log(LogLevel.WARNING, "Error when purging old sessions ", e); } - log.log(LogLevel.INFO, "Done purging old sessions"); // TODO: Use debug level after 2018-01-29 + log.log(LogLevel.DEBUG, "Done purging old sessions"); } private boolean hasExpired(LocalSession candidate) { diff --git a/container-core/OWNERS b/container-core/OWNERS index 3b2ba1ede81..d2719284202 100644 --- a/container-core/OWNERS +++ b/container-core/OWNERS @@ -1 +1,2 @@ +bjorncs gjoranv diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java b/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java index 6ccd25ad6c7..a5be5cb0b0a 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java @@ -11,6 +11,7 @@ import com.yahoo.log.LogLevel; import java.util.Map; import java.util.TreeSet; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.logging.Logger; @@ -38,12 +39,19 @@ public class StateMonitor extends AbstractComponent { @Inject public StateMonitor(HealthMonitorConfig config, Timer timer) { + this(config, timer, runnable -> { + Thread thread = new Thread(runnable, "StateMonitor"); + thread.setDaemon(true); + return thread; + }); + } + + StateMonitor(HealthMonitorConfig config, Timer timer, ThreadFactory threadFactory) { this.timer = timer; this.snapshotIntervalMs = (long)(config.snapshot_interval() * TimeUnit.SECONDS.toMillis(1)); this.lastSnapshotTimeMs = timer.currentTimeMillis(); this.status = Status.valueOf(config.initialStatus()); - thread = new Thread(StateMonitor.this::run, "StateMonitor"); - thread.setDaemon(true); + thread = threadFactory.newThread(this::run); thread.start(); } @@ -69,6 +77,7 @@ public class StateMonitor extends AbstractComponent { /** Returns the interval between each metrics snapshot used by this */ public long getSnapshotIntervalMillis() { return snapshotIntervalMs; } + /** NOTE: For unit testing only. May lead to undefined behaviour if StateMonitor thread is running simultaneously **/ boolean checkTime() { long now = timer.currentTimeMillis(); if (now < lastSnapshotTimeMs + snapshotIntervalMs) { diff --git a/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTest.java b/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTest.java index 5d8f885f8d0..41b195baeb3 100644 --- a/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTest.java +++ b/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTest.java @@ -29,6 +29,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.TreeMap; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -36,6 +37,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; /** * @author Simon Thoresen Hult @@ -51,30 +53,20 @@ public class StateHandlerTest { @Before public void startTestDriver() { - driver = TestDriver.newSimpleApplicationInstanceWithoutOsgi(new AbstractModule() { - + Timer timer = this.currentTimeMillis::get; + this.driver = TestDriver.newSimpleApplicationInstanceWithoutOsgi(new AbstractModule() { @Override protected void configure() { - bind(Timer.class).toInstance(new Timer() { - - @Override - public long currentTimeMillis() { - return currentTimeMillis.get(); - } - }); + bind(Timer.class).toInstance(timer); } }); ContainerBuilder builder = driver.newContainerBuilder(); - builder.guiceModules().install(new AbstractModule() { - - @Override - protected void configure() { - bind(HealthMonitorConfig.class) - .toInstance(new HealthMonitorConfig(new HealthMonitorConfig.Builder().snapshot_interval( - TimeUnit.MILLISECONDS.toSeconds(SNAPSHOT_INTERVAL)))); - } - }); - monitor = builder.guiceModules().getInstance(StateMonitor.class); + HealthMonitorConfig healthMonitorConfig = + new HealthMonitorConfig( + new HealthMonitorConfig.Builder() + .snapshot_interval(TimeUnit.MILLISECONDS.toSeconds(SNAPSHOT_INTERVAL))); + ThreadFactory threadFactory = ignored -> mock(Thread.class); + this.monitor = new StateMonitor(healthMonitorConfig, timer, threadFactory); builder.guiceModules().install(new AbstractModule() { @Override @@ -110,13 +102,13 @@ public class StateHandlerTest { public void testReportIncludesMetricsAfterSnapshot() throws Exception { metric.add("foo", 1, null); metric.set("bar", 4, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json1 = requestAsJson("http://localhost/state/v1/metrics"); assertEquals(json1.toString(), "up", json1.get("status").get("code").asText()); assertEquals(json1.toString(), 2, json1.get("metrics").get("values").size()); metric.add("fuz", 1, metric.createContext(new HashMap<>(0))); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json2 = requestAsJson("http://localhost/state/v1/metrics"); assertEquals(json2.toString(), "up", json2.get("status").get("code").asText()); assertEquals(json2.toString(), 3, json2.get("metrics").get("values").size()); @@ -137,7 +129,7 @@ public class StateHandlerTest { metric.add(metricName, 2, metricContext); // Change it to a gauge metric metric.set(metricName, 9, metricContext); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); MetricValue resultingMetric = monitor.snapshot().iterator().next().getValue().get(metricName); assertEquals(GaugeMetric.class, resultingMetric.getClass()); assertEquals("Value was reset and produces the last gauge value", @@ -150,7 +142,7 @@ public class StateHandlerTest { // Change it to a count metric metric.add(metricName, 1, metricContext); metric.add(metricName, 2, metricContext); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); MetricValue resultingMetric = monitor.snapshot().iterator().next().getValue().get(metricName); assertEquals(CountMetric.class, resultingMetric.getClass()); assertEquals("Value was reset, and changed to add semantics giving 1+2", @@ -164,7 +156,7 @@ public class StateHandlerTest { metric.set("bar", 5, null); metric.set("bar", 7, null); metric.set("bar", 2, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertEquals(json.toString(), "up", json.get("status").get("code").asText()); assertEquals(json.toString(), 1, json.get("metrics").get("values").size()); @@ -178,7 +170,7 @@ public class StateHandlerTest { metric.add("foo", 1, null); metric.add("foo", 2, null); metric.add("foo", 1, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertEquals(json.toString(), "up", json.get("status").get("code").asText()); assertEquals(json.toString(), 1, json.get("metrics").get("values").size()); @@ -189,7 +181,7 @@ public class StateHandlerTest { @Test public void testReadabilityOfJsonReport() throws Exception { metric.add("foo", 1, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); assertEquals("{\n" + " \"metrics\": {\n" + " \"snapshot\": {\n" + @@ -214,7 +206,7 @@ public class StateHandlerTest { metric.set("bar", 3, ctx); metric.set("bar", 4, ctx); metric.set("bar", 5, ctx); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); assertEquals("{\n" + " \"metrics\": {\n" + " \"snapshot\": {\n" + @@ -253,10 +245,10 @@ public class StateHandlerTest { public void testNotAggregatingCountsBeyondSnapshots() throws Exception { metric.add("foo", 1, null); metric.add("foo", 1, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); metric.add("foo", 2, null); metric.add("foo", 1, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertEquals(json.toString(), "up", json.get("status").get("code").asText()); assertEquals(json.toString(), 1, json.get("metrics").get("values").size()); @@ -269,13 +261,13 @@ public class StateHandlerTest { metric.add("foo", 1, null); metric.set("bar", 3, null); // At this time we should not have done any snapshotting - incrementCurrentTime(SNAPSHOT_INTERVAL - 1); + incrementCurrentTimeAndAssertNoSnapshot(SNAPSHOT_INTERVAL - 1); { JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertFalse(json.toString(), json.get("metrics").has("snapshot")); } // At this time first snapshot should have been generated - incrementCurrentTime(1); + incrementCurrentTimeAndAssertSnapshot(1); { JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertTrue(json.toString(), json.get("metrics").has("snapshot")); @@ -283,7 +275,7 @@ public class StateHandlerTest { assertEquals(300.0, json.get("metrics").get("snapshot").get("to").asDouble(), 0.00001); } // No new snapshot at this time - incrementCurrentTime(SNAPSHOT_INTERVAL - 1); + incrementCurrentTimeAndAssertNoSnapshot(SNAPSHOT_INTERVAL - 1); { JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertTrue(json.toString(), json.get("metrics").has("snapshot")); @@ -291,7 +283,7 @@ public class StateHandlerTest { assertEquals(300.0, json.get("metrics").get("snapshot").get("to").asDouble(), 0.00001); } // A new snapshot - incrementCurrentTime(1); + incrementCurrentTimeAndAssertSnapshot(1); { JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertTrue(json.toString(), json.get("metrics").has("snapshot")); @@ -304,10 +296,10 @@ public class StateHandlerTest { public void testFreshStartOfValuesBeyondSnapshot() throws Exception { metric.set("bar", 4, null); metric.set("bar", 5, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); metric.set("bar", 4, null); metric.set("bar", 2, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/all"); assertEquals(json.toString(), "up", json.get("status").get("code").asText()); assertEquals(json.toString(), 1, json.get("metrics").get("values").size()); @@ -318,8 +310,8 @@ public class StateHandlerTest { @Test public void snapshotsPreserveLastGaugeValue() throws Exception { metric.set("bar", 4, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/all"); JsonNode metricValues = getFirstMetricValueNode(json); assertEquals(json.toString(), 4, metricValues.get("last").asDouble(), 0.001); @@ -341,10 +333,10 @@ public class StateHandlerTest { @Test public void gaugeSnapshotsTracksCountMinMaxAvgPerPeriod() throws Exception { metric.set("bar", 10000, null); // Ensure any cross-snapshot noise is visible - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); metric.set("bar", 20, null); metric.set("bar", 40, null); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/all"); JsonNode metricValues = getFirstMetricValueNode(json); assertEquals(json.toString(), 40, metricValues.get("last").asDouble(), 0.001); @@ -369,7 +361,7 @@ public class StateHandlerTest { metric.set("serverTotalSuccessfulResponseLatency", 20, context1); metric.set("serverTotalSuccessfulResponseLatency", 40, context2); metric.add("random", 3, context1); - incrementCurrentTime(SNAPSHOT_INTERVAL); + incrementCurrentTimeAndAssertSnapshot(SNAPSHOT_INTERVAL); JsonNode json = requestAsJson("http://localhost/state/v1/health"); assertEquals(json.toString(), "up", json.get("status").get("code").asText()); assertEquals(json.toString(), 2, json.get("metrics").get("values").size()); @@ -400,9 +392,14 @@ public class StateHandlerTest { assertEquals(Vtag.currentVersion.toString(), version.asText()); } - private void incrementCurrentTime(long val) { + private void incrementCurrentTimeAndAssertSnapshot(long val) { + currentTimeMillis.addAndGet(val); + assertTrue("Expected a new snapshot to be generated", monitor.checkTime()); + } + + private void incrementCurrentTimeAndAssertNoSnapshot(long val) { currentTimeMillis.addAndGet(val); - monitor.checkTime(); + assertFalse("Expected no snapshot", monitor.checkTime());; } private String requestAsString(String requestUri) throws Exception { diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java index d9d1b4984eb..43a5eae90bf 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java @@ -75,17 +75,11 @@ public class FileReceiver { currentHash = 0; fileReferenceDir = new File(downloadDirectory, reference.value()); this.tmpDir = tmpDirectory; - try { - Files.createDirectories(fileReferenceDir.toPath()); - } catch (IOException e) { - log.log(LogLevel.ERROR, "Failed creating directory(" + fileReferenceDir.toPath() + "): " + e.getMessage(), e); - throw new RuntimeException("Failed creating directory(" + fileReferenceDir.toPath() + "): ", e); - } try { inprogressFile = Files.createTempFile(tmpDirectory.toPath(), fileName, ".inprogress").toFile(); } catch (IOException e) { - String msg = "Failed creating temp file for inprogress file for(" + fileName + ") in '" + fileReferenceDir.toPath() + "': "; + String msg = "Failed creating temp file for inprogress file for " + fileName + " in '" + tmpDirectory.toPath() + "': "; log.log(LogLevel.ERROR, msg + e.getMessage(), e); throw new RuntimeException(msg, e); } @@ -124,6 +118,12 @@ public class FileReceiver { CompressedFileReference.decompress(inprogressFile, decompressedDir); moveFileToDestination(decompressedDir, fileReferenceDir); } else { + try { + Files.createDirectories(fileReferenceDir.toPath()); + } catch (IOException e) { + log.log(LogLevel.ERROR, "Failed creating directory (" + fileReferenceDir.toPath() + "): " + e.getMessage(), e); + throw new RuntimeException("Failed creating directory (" + fileReferenceDir.toPath() + "): ", e); + } log.log(LogLevel.DEBUG, "Uncompressed file, moving to " + file.getAbsolutePath()); moveFileToDestination(inprogressFile, file); } diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDataBlob.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDataBlob.java index f0db12a45fc..c22770395d3 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDataBlob.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDataBlob.java @@ -9,6 +9,7 @@ import java.nio.ByteBuffer; public class FileReferenceDataBlob extends FileReferenceData { private final byte[] content; private final long xxhash; + private boolean contentRead = false; public FileReferenceDataBlob(FileReference fileReference, String filename, Type type, byte[] content) { this(fileReference, filename, type, content, XXHashFactory.fastestInstance().hash64().hash(ByteBuffer.wrap(content), 0)); @@ -27,10 +28,16 @@ public class FileReferenceDataBlob extends FileReferenceData { public ByteBuffer content() { return ByteBuffer.wrap(content); } + @Override public int nextContent(ByteBuffer bb) { - bb.put(content); - return content.length; + if (contentRead) { + return -1; + } else { + contentRead = true; + bb.put(content); + return content.length; + } } @Override diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/status/FileDistributionStatusClient.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/status/FileDistributionStatusClient.java index b50416ac159..1675366fc5e 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/status/FileDistributionStatusClient.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/status/FileDistributionStatusClient.java @@ -34,7 +34,6 @@ public class FileDistributionStatusClient { private static final String statusInProgress = "IN_PROGRESS"; private static final String statusFinished = "FINISHED"; - private final String tenantName; private final String applicationName; private final String instanceName; @@ -134,23 +133,28 @@ public class FileDistributionStatusClient { private String inProgressOutput(JsonNode hosts) { ArrayList<String> statusPerHost = new ArrayList<>(); for (JsonNode host : hosts) { - StringBuilder sb = new StringBuilder(); String status = host.get("status").asText(); - sb.append(host.get("hostname").asText()).append(": ").append(status); - if (status.equals(statusUnknown)) - sb.append(" (").append(host.get("message").asText()).append(")"); - else if (status.equals(statusInProgress)) { - JsonNode fileReferencesArray = host.get("fileReferences"); - int size = fileReferencesArray.size(); - int finished = 0; - for (JsonNode element : fileReferencesArray) { - for (Iterator<Map.Entry<String, JsonNode>> it = element.fields(); it.hasNext(); ) { - Map.Entry<String, JsonNode> fileReferenceStatus = it.next(); - if (fileReferenceStatus.getValue().asDouble() == 1.0) - finished++; + StringBuilder sb = new StringBuilder(host.get("hostname").asText()).append(": ").append(status); + switch (status) { + case statusUnknown: + sb.append(" (").append(host.get("message").asText()).append(")"); + break; + case statusInProgress: + JsonNode fileReferencesArray = host.get("fileReferences"); + int finished = 0; + for (JsonNode element : fileReferencesArray) { + for (Iterator<Map.Entry<String, JsonNode>> it = element.fields(); it.hasNext(); ) { + Map.Entry<String, JsonNode> fileReferenceStatus = it.next(); + if (fileReferenceStatus.getValue().asDouble() == 1.0) + finished++; + } } - } - sb.append(" (" + finished + " of " + size + " finished)"); + sb.append(" (" + finished + " of " + fileReferencesArray.size() + " finished)"); + break; + case statusFinished: + break; // Nothing to add + default: + throw new RuntimeException("Unknown status " + status); } statusPerHost.add(sb.toString()); } diff --git a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java index afa66f89efc..78fc094a9ef 100644 --- a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java +++ b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.filedistribution; import com.yahoo.config.FileReference; +import com.yahoo.io.IOUtils; import com.yahoo.text.Utf8; import net.jpountz.xxhash.XXHash64; import net.jpountz.xxhash.XXHashFactory; @@ -12,11 +13,15 @@ import org.junit.rules.TemporaryFolder; import static org.junit.Assert.assertEquals; - import java.io.File; +import java.io.FileOutputStream; +import java.io.FileReader; +import java.io.FileWriter; import java.io.IOException; import java.nio.ByteBuffer; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; public class FileReceiverTest { private File root; @@ -48,6 +53,23 @@ public class FileReceiverTest { transferPartsAndAssert(new FileReference("ref-a"), "myfile-3", all, 3); } + @Test + public void receiveCompressedParts() throws IOException{ + File dirWithFiles = temporaryFolder.newFolder("files"); + FileWriter writerA = new FileWriter(new File(dirWithFiles, "a")); + writerA.write("1"); + writerA.close(); + FileWriter writerB = new FileWriter(new File(dirWithFiles, "b")); + writerB.write("2"); + writerB.close(); + + byte[] data = CompressedFileReference.compress(dirWithFiles); + transferCompressedData(new FileReference("ref"), "a", data); + File downloadDir = new File(root, "ref"); + assertEquals("1", IOUtils.readFile(new File(downloadDir, "a"))); + assertEquals("2", IOUtils.readFile(new File(downloadDir, "b"))); + } + private void transferPartsAndAssert(FileReference ref, String fileName, String all, int numParts) throws IOException { byte [] allContent = Utf8.toBytes(all); @@ -69,4 +91,11 @@ public class FileReceiverTest { file.delete(); assertEquals(all, Utf8.toString(allReadBytes)); } + + private void transferCompressedData(FileReference ref, String fileName, byte[] data) throws IOException { + FileReceiver.Session session = + new FileReceiver.Session(root, tempDir, 1, ref, FileReferenceData.Type.compressed, fileName, data.length); + session.addPart(0, data); + session.close(hasher.hash(ByteBuffer.wrap(data), 0)); + } } diff --git a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReferenceDataTest.java b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReferenceDataTest.java new file mode 100644 index 00000000000..0b85def5809 --- /dev/null +++ b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReferenceDataTest.java @@ -0,0 +1,28 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.filedistribution; + +import com.yahoo.config.FileReference; +import com.yahoo.text.Utf8; +import org.junit.Test; + +import java.nio.ByteBuffer; +import java.util.Arrays; + +import static org.junit.Assert.assertEquals; + +public class FileReferenceDataTest { + + @Test + public void testDataBlob() { + String content = "blob"; + FileReferenceData fileReferenceData = + new FileReferenceDataBlob(new FileReference("ref"), "foo", FileReferenceData.Type.compressed, Utf8.toBytes(content)); + ByteBuffer byteBuffer = ByteBuffer.allocate(100); + assertEquals(4, fileReferenceData.nextContent(byteBuffer)); + assertEquals(content, Utf8.toString(Arrays.copyOfRange(byteBuffer.array(), 0, 4))); + + // nextContent() will always return everything for FileReferenceDataBlob, so nothing more should be read + assertEquals(-1, fileReferenceData.nextContent(byteBuffer)); + } + +} diff --git a/node-admin/pom.xml b/node-admin/pom.xml index 161769a4edf..1818694ced5 100644 --- a/node-admin/pom.xml +++ b/node-admin/pom.xml @@ -86,6 +86,12 @@ <version>${project.version}</version> <scope>compile</scope> </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>vespa-athenz</artifactId> + <version>${project.version}</version> + <scope>compile</scope> + </dependency> <dependency> <groupId>org.hamcrest</groupId> diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java index 87491367514..cee2dc9b66b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java @@ -18,6 +18,9 @@ public interface TaskContext { FileSystem fileSystem(); void logSystemModification(Logger logger, String actionDescription); + default void logSystemModification(Logger logger, String format, String... args) { + logSystemModification(logger, String.format(format, (Object[]) args)); + } default boolean executeSubtask(IdempotentTask task) { return false; } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminComponent.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminComponent.java index 7c3e04c18c1..15ade142b5d 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminComponent.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminComponent.java @@ -64,7 +64,11 @@ public class DockerAdminComponent implements AdminComponent { Environment environment = new Environment(configServerConfig); requestExecutor = ConfigServerHttpRequestExecutor.create( - environment.getConfigServerUris(), environment.getKeyStoreOptions(), environment.getTrustStoreOptions()); + environment.getConfigServerUris(), + environment.getKeyStoreOptions(), + environment.getTrustStoreOptions(), + environment.getAthenzIdentity()); + NodeRepository nodeRepository = new NodeRepositoryImpl(requestExecutor); Orchestrator orchestrator = new OrchestratorImpl(requestExecutor); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java new file mode 100644 index 00000000000..a20d30b2bf9 --- /dev/null +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java @@ -0,0 +1,126 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +package com.yahoo.vespa.hosted.node.admin.task.util.file; + +import com.yahoo.vespa.hosted.node.admin.component.TaskContext; + +import java.nio.file.Path; +import java.util.Objects; +import java.util.Optional; +import java.util.function.Consumer; +import java.util.function.Supplier; +import java.util.logging.Logger; + +/** + * Class to converge file/directory attributes like owner and permissions to wanted values. + * Typically used by higher abstraction layers working on files (FileSync/FileWriter) or + * directories (MakeDirectory). + * + * @author hakonhall + */ +public class AttributeSync { + private static final Logger logger = Logger.getLogger(AttributeSync.class.getName()); + + private final UnixPath path; + + private Optional<String> owner = Optional.empty(); + private Optional<String> group = Optional.empty(); + private Optional<String> permissions = Optional.empty(); + + public AttributeSync(Path path) { + this.path = new UnixPath(path); + } + + public Optional<String> getPermissions() { + return permissions; + } + + public AttributeSync withPermissions(String permissions) { + this.permissions = Optional.of(permissions); + return this; + } + + public Optional<String> getOwner() { + return owner; + } + + public AttributeSync withOwner(String owner) { + this.owner = Optional.of(owner); + return this; + } + + public Optional<String> getGroup() { + return group; + } + + public AttributeSync withGroup(String group) { + this.group = Optional.of(group); + return this; + } + + public AttributeSync with(PartialFileData fileData) { + owner = fileData.getOwner(); + group = fileData.getGroup(); + permissions = fileData.getPermissions(); + return this; + } + + public boolean converge(TaskContext context) { + return converge(context, new FileAttributesCache(path)); + } + + /** + * Path must exist before calling converge. + */ + public boolean converge(TaskContext context, FileAttributesCache currentAttributes) { + boolean systemModified = updateAttribute( + context, + "owner", + owner, + () -> currentAttributes.get().owner(), + path::setOwner); + + systemModified |= updateAttribute( + context, + "group", + group, + () -> currentAttributes.get().group(), + path::setGroup); + + systemModified |= updateAttribute( + context, + "permissions", + permissions, + () -> currentAttributes.get().permissions(), + path::setPermissions); + + return systemModified; + } + + private boolean updateAttribute(TaskContext context, + String attributeName, + Optional<String> wantedValue, + Supplier<String> currentValueSupplier, + Consumer<String> valueSetter) { + if (!wantedValue.isPresent()) { + return false; + } + + String currentValue = currentValueSupplier.get(); + if (Objects.equals(currentValue, wantedValue.get())) { + return false; + } + + context.logSystemModification( + logger, + "Changing %s of %s from %s to %s", + attributeName, + path.toString(), + currentValue, + wantedValue.get()); + + valueSetter.accept(wantedValue.get()); + + return true; + } +} diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java index d8b8aadfff7..18d2a2e3aa9 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java @@ -7,8 +7,6 @@ import com.yahoo.vespa.hosted.node.admin.component.TaskContext; import java.nio.file.Path; import java.util.Objects; import java.util.Optional; -import java.util.function.Consumer; -import java.util.function.Supplier; import java.util.logging.Logger; /** @@ -39,58 +37,14 @@ public class FileSync { public boolean convergeTo(TaskContext taskContext, PartialFileData partialFileData) { FileAttributesCache currentAttributes = new FileAttributesCache(path); - boolean modifiedSystem = false; + boolean modifiedSystem = maybeUpdateContent(taskContext, partialFileData.getContent(), currentAttributes); - modifiedSystem |= maybeUpdateContent(taskContext, partialFileData.getContent(), currentAttributes); - - modifiedSystem |= convergeAttribute( - taskContext, - "owner", - partialFileData.getOwner(), - () -> currentAttributes.get().owner(), - path::setOwner); - - modifiedSystem |= convergeAttribute( - taskContext, - "group", - partialFileData.getGroup(), - () -> currentAttributes.get().group(), - path::setGroup); - - modifiedSystem |= convergeAttribute( - taskContext, - "permissions", - partialFileData.getPermissions(), - () -> currentAttributes.get().permissions(), - path::setPermissions); + AttributeSync attributeSync = new AttributeSync(path.toPath()).with(partialFileData); + modifiedSystem |= attributeSync.converge(taskContext, currentAttributes); return modifiedSystem; } - private boolean convergeAttribute(TaskContext taskContext, - String attributeName, - Optional<String> wantedValue, - Supplier<String> currentValueSupplier, - Consumer<String> valueSetter) { - if (!wantedValue.isPresent()) { - return false; - } - - String currentValue = currentValueSupplier.get(); - if (Objects.equals(wantedValue.get(), currentValue)) { - return false; - } else { - String actionDescription = String.format("Changing %s of %s from %s to %s", - attributeName, - path, - currentValue, - wantedValue.get()); - taskContext.logSystemModification(logger, actionDescription); - valueSetter.accept(wantedValue.get()); - return true; - } - } - private boolean maybeUpdateContent(TaskContext taskContext, Optional<String> content, FileAttributesCache currentAttributes) { diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java new file mode 100644 index 00000000000..e815ab8bd86 --- /dev/null +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java @@ -0,0 +1,72 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.node.admin.task.util.file; + +import com.yahoo.vespa.hosted.node.admin.component.TaskContext; + +import java.io.UncheckedIOException; +import java.nio.file.NotDirectoryException; +import java.nio.file.Path; +import java.util.Optional; +import java.util.logging.Logger; + +/** + * Class to ensure a directory exists with the correct owner, group, and permissions. + * + * @author hakonhall + */ +public class MakeDirectory { + private static final Logger logger = Logger.getLogger(MakeDirectory.class.getName()); + + private final UnixPath path; + private final AttributeSync attributeSync; + + private boolean createParents = false; + + public MakeDirectory(Path path) { + this.path = new UnixPath(path); + this.attributeSync = new AttributeSync(path); + } + + /** + * Warning: The owner, group, and permissions of any created parent directories are NOT modified + */ + public MakeDirectory createParents() { this.createParents = true; return this; } + + public MakeDirectory withOwner(String owner) { attributeSync.withOwner(owner); return this; } + public MakeDirectory withGroup(String group) { attributeSync.withGroup(group); return this; } + public MakeDirectory withPermissions(String permissions) { + attributeSync.withPermissions(permissions); + return this; + } + + public boolean converge(TaskContext context) { + boolean systemModified = false; + + FileAttributesCache attributes = new FileAttributesCache(path); + if (attributes.exists()) { + if (!attributes.get().isDirectory()) { + throw new UncheckedIOException(new NotDirectoryException(path.toString())); + } + } else { + if (createParents) { + // We'll skip logginer system modification here, as we'll log about the creation + // of the directory next. + path.createParents(); + } + + context.logSystemModification(logger, "Creating directory " + path); + systemModified = true; + + Optional<String> permissions = attributeSync.getPermissions(); + if (permissions.isPresent()) { + path.createDirectory(permissions.get()); + } else { + path.createDirectory(); + } + } + + systemModified |= attributeSync.converge(context, attributes); + + return systemModified; + } +} diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java index aaffea05d1e..ac4230ca7c6 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java @@ -6,6 +6,7 @@ import java.nio.file.Files; import java.nio.file.OpenOption; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.GroupPrincipal; import java.nio.file.attribute.PosixFileAttributeView; import java.nio.file.attribute.PosixFileAttributes; @@ -69,14 +70,7 @@ public class UnixPath { * and no permissions for others. */ public void setPermissions(String permissions) { - Set<PosixFilePermission> permissionSet; - try { - permissionSet = PosixFilePermissions.fromString(permissions); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException("Failed to set permissions '" + - permissions + "' on path " + path, e); - } - + Set<PosixFilePermission> permissionSet = getPosixFilePermissionsFromString(permissions); uncheck(() -> Files.setPosixFilePermissions(path, permissionSet)); } @@ -114,8 +108,27 @@ public class UnixPath { return IOExceptionUtil.ifExists(() -> getAttributes()); } + public void createDirectory(String permissions) { + Set<PosixFilePermission> set = getPosixFilePermissionsFromString(permissions); + FileAttribute<Set<PosixFilePermission>> attribute = PosixFilePermissions.asFileAttribute(set); + uncheck(() -> Files.createDirectory(path, attribute)); + } + + public void createDirectory() { + uncheck(() -> Files.createDirectory(path)); + } + @Override public String toString() { return path.toString(); } + + private Set<PosixFilePermission> getPosixFilePermissionsFromString(String permissions) { + try { + return PosixFilePermissions.fromString(permissions); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Failed to set permissions '" + + permissions + "' on path " + path, e); + } + } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java index 00bcca71970..71a4c7c109b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java @@ -2,16 +2,27 @@ package com.yahoo.vespa.hosted.node.admin.task.util.process; import java.nio.file.Path; +import java.util.logging.Logger; /** * @author hakonhall */ public interface ChildProcess extends AutoCloseable { + String commandLine(); ChildProcess waitForTermination(); int exitValue(); ChildProcess throwIfFailed(); String getUtf8Output(); + /** + * Only call this if process was spawned under the assumption the program had no side + * effects (see Command::spawnProgramWithoutSideEffects). If it is determined later + * that the program did in fact have side effects (modified system), this method can + * be used to log that fact. Alternatively, call TaskContext::logSystemModification + * directly. + */ + void logAsModifyingSystemAfterAll(Logger logger); + @Override void close(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java index 367688f0bb4..31ccbe90fda 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java @@ -1,9 +1,11 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.task.util.process; +import com.yahoo.vespa.hosted.node.admin.component.TaskContext; import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; import java.nio.file.Path; +import java.util.logging.Logger; /** * Represents a forked child process that still exists or has terminated. @@ -11,17 +13,28 @@ import java.nio.file.Path; * @author hakonhall */ public class ChildProcessImpl implements ChildProcess { + private final TaskContext taskContext; private final Process process; private final Path processOutputPath; private final String commandLine; - ChildProcessImpl(Process process, Path processOutputPath, String commandLine) { + ChildProcessImpl(TaskContext taskContext, + Process process, + Path processOutputPath, + String commandLine) { + this.taskContext = taskContext; this.process = process; this.processOutputPath = processOutputPath; this.commandLine = commandLine; } + @Override + public String commandLine() { + return commandLine; + } + public String getUtf8Output() { + waitForTermination(); return new UnixPath(processOutputPath).readUtf8File(); } @@ -39,10 +52,12 @@ public class ChildProcessImpl implements ChildProcess { } public int exitValue() { + waitForTermination(); return process.exitValue(); } public ChildProcess throwIfFailed() { + waitForTermination(); if (process.exitValue() != 0) { throw new CommandException("Execution of program [" + commandLine + "] failed, stdout/stderr was: <" + suffixOfOutputForLog() + ">"); @@ -65,6 +80,11 @@ public class ChildProcessImpl implements ChildProcess { } @Override + public void logAsModifyingSystemAfterAll(Logger logger) { + taskContext.logSystemModification(logger, "Executed command: " + commandLine); + } + + @Override public void close() { if (process.isAlive()) { process.destroyForcibly(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java index 049490f2705..51acb1d2bca 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java @@ -56,7 +56,7 @@ public class Command { .redirectOutput(temporaryFile.toFile()); Process process = IOExceptionUtil.uncheck(builder::start); - return new ChildProcessImpl(process, temporaryFile, commandLine); + return new ChildProcessImpl(context, process, temporaryFile, commandLine); } String commandLine() { @@ -89,7 +89,15 @@ public class Command { return "\"" + doubleQuoteEscaped + "\""; } - public ChildProcess spawnWithoutLoggingCommand() { + /** + * Spawns a process that do not modify the system. + * + * This method can also be used to spawn a process that MAY have side effects + * to be determined at some later time. The caller is then responsible for calling + * TaskContext::logSystemModification afterwards. The caller is encouraged to + * call ChildProcess::logAsModifyingSystemAfterAll to do this. + */ + public ChildProcess spawnProgramWithoutSideEffects() { return spawn(null); } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java index c1514f1056b..35729b5a428 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/Yum.java @@ -5,22 +5,26 @@ import com.yahoo.vespa.hosted.node.admin.component.TaskContext; import com.yahoo.vespa.hosted.node.admin.task.util.process.ChildProcess; import com.yahoo.vespa.hosted.node.admin.task.util.process.Command; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.function.Supplier; import java.util.logging.Logger; +import java.util.regex.Pattern; /** * @author hakonhall */ public class Yum { - private static Logger logger = Logger.getLogger(Yum.class.getName()); + private static final Pattern INSTALL_NOOP_PATTERN = + Pattern.compile("\nNothing to do\n$"); + private static final Pattern UPGRADE_NOOP_PATTERN = + Pattern.compile("\nNo packages marked for update\n$"); + private static final Pattern REMOVE_NOOP_PATTERN = + Pattern.compile("\nNo Packages marked for removal\n$"); private final TaskContext taskContext; private final Supplier<Command> commandSupplier; - private List<String> packages = new ArrayList<>(); public Yum(TaskContext taskContext) { this.taskContext = taskContext; @@ -30,57 +34,86 @@ public class Yum { /** * @param packages A list of packages, each package being of the form name-1.2.3-1.el7.noarch */ - public Install install(String... packages) { - return new Install(taskContext, Arrays.asList(packages)); + public GenericYumCommand install(String... packages) { + return newYumCommand("install", packages, INSTALL_NOOP_PATTERN); } - public class Install { + public GenericYumCommand upgrade(String... packages) { + return newYumCommand("upgrade", packages, UPGRADE_NOOP_PATTERN); + } + + public GenericYumCommand remove(String... packages) { + return newYumCommand("remove", packages, REMOVE_NOOP_PATTERN); + } + + private GenericYumCommand newYumCommand(String yumCommand, + String[] packages, + Pattern noopPattern) { + return new GenericYumCommand( + taskContext, + commandSupplier, + yumCommand, + Arrays.asList(packages), + noopPattern); + } + + public static class GenericYumCommand { + private static Logger logger = Logger.getLogger(Yum.class.getName()); + private final TaskContext taskContext; + private final Supplier<Command> commandSupplier; + private final String yumCommand; private final List<String> packages; + private final Pattern commandOutputNoopPattern; private Optional<String> enabledRepo = Optional.empty(); - public Install(TaskContext taskContext, List<String> packages) { + private GenericYumCommand(TaskContext taskContext, + Supplier<Command> commandSupplier, + String yumCommand, + List<String> packages, + Pattern commandOutputNoopPattern) { this.taskContext = taskContext; + this.commandSupplier = commandSupplier; + this.yumCommand = yumCommand; this.packages = packages; + this.commandOutputNoopPattern = commandOutputNoopPattern; if (packages.isEmpty()) { throw new IllegalArgumentException("No packages specified"); } } - public Install enableRepo(String repo) { + @SuppressWarnings("unchecked") + public GenericYumCommand enableRepo(String repo) { enabledRepo = Optional.of(repo); return this; } public boolean converge() { - if (packages.stream().allMatch(Yum.this::isInstalled)) { - return false; - } - - execute(); - return true; - } - - private void execute() { Command command = commandSupplier.get(); - command.add("yum", "install", "--assumeyes"); + command.add("yum", yumCommand, "--assumeyes"); enabledRepo.ifPresent(repo -> command.add("--enablerepo=" + repo)); command.add(packages); - command.spawn(logger).waitForTermination().throwIfFailed(); + ChildProcess childProcess = command + .spawnProgramWithoutSideEffects() + .waitForTermination() + .throwIfFailed(); + + // There's no way to figure out whether a yum command would have been a no-op. + // Therefore, run the command and parse the output to decide. + String output = childProcess.getUtf8Output(); + if (commandOutputNoopPattern.matcher(output).matches()) { + return false; + } else { + childProcess.logAsModifyingSystemAfterAll(logger); + return true; + } } } + // For testing Yum(TaskContext taskContext, Supplier<Command> commandSupplier) { this.taskContext = taskContext; this.commandSupplier = commandSupplier; } - - private boolean isInstalled(String package_) { - ChildProcess childProcess = commandSupplier.get() - .add("yum", "list", "installed", package_) - .spawnWithoutLoggingCommand(); - childProcess.waitForTermination(); - return childProcess.exitValue() == 0; - } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java index 954de4b271a..850161d9801 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/ConfigServerHttpRequestExecutor.java @@ -4,6 +4,9 @@ package com.yahoo.vespa.hosted.node.admin.util; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.yahoo.concurrent.ThreadFactoryFactory; +import com.yahoo.vespa.athenz.api.AthenzIdentity; +import com.yahoo.vespa.athenz.tls.AthenzIdentityVerifier; +import com.yahoo.vespa.athenz.tls.AthenzSslContextBuilder; import org.apache.http.HttpHeaders; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpDelete; @@ -13,17 +16,14 @@ import org.apache.http.client.methods.HttpPost; import org.apache.http.client.methods.HttpPut; import org.apache.http.client.methods.HttpRequestBase; import org.apache.http.client.methods.HttpUriRequest; -import org.apache.http.conn.ssl.NoopHostnameVerifier; import org.apache.http.conn.ssl.SSLConnectionSocketFactory; import org.apache.http.entity.StringEntity; -import org.apache.http.ssl.SSLContextBuilder; +import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLContext; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URI; -import java.security.KeyManagementException; -import java.security.NoSuchAlgorithmException; import java.time.Duration; import java.util.ArrayList; import java.util.Collection; @@ -61,8 +61,11 @@ public class ConfigServerHttpRequestExecutor implements AutoCloseable { private volatile SelfCloseableHttpClient client; public static ConfigServerHttpRequestExecutor create( - Collection<URI> configServerUris, Optional<KeyStoreOptions> keyStoreOptions, Optional<KeyStoreOptions> trustStoreOptions) { - Supplier<SelfCloseableHttpClient> clientSupplier = () -> createHttpClient(keyStoreOptions, trustStoreOptions); + Collection<URI> configServerUris, + Optional<KeyStoreOptions> keyStoreOptions, + Optional<KeyStoreOptions> trustStoreOptions, + Optional<AthenzIdentity> athenzIdentity) { + Supplier<SelfCloseableHttpClient> clientSupplier = () -> createHttpClient(keyStoreOptions, trustStoreOptions, athenzIdentity); ConfigServerHttpRequestExecutor requestExecutor = new ConfigServerHttpRequestExecutor( randomizeConfigServerUris(configServerUris), clientSupplier.get()); @@ -178,11 +181,13 @@ public class ConfigServerHttpRequestExecutor implements AutoCloseable { } private static SelfCloseableHttpClient createHttpClient(Optional<KeyStoreOptions> keyStoreOptions, - Optional<KeyStoreOptions> trustStoreOptions) { + Optional<KeyStoreOptions> trustStoreOptions, + Optional<AthenzIdentity> athenzIdentity) { NODE_ADMIN_LOGGER.info("Creating new HTTP client"); try { - SSLConnectionSocketFactory sslSocketFactory = new SSLConnectionSocketFactory( - makeSslContext(keyStoreOptions, trustStoreOptions), NoopHostnameVerifier.INSTANCE); + SSLContext sslContext = makeSslContext(keyStoreOptions, trustStoreOptions); + HostnameVerifier hostnameVerifier = makeHostnameVerifier(athenzIdentity); + SSLConnectionSocketFactory sslSocketFactory = new SSLConnectionSocketFactory(sslContext, hostnameVerifier); return new SelfCloseableHttpClient(sslSocketFactory); } catch (Exception e) { NODE_ADMIN_LOGGER.error("Failed to create HTTP client with custom SSL Context, proceeding with default", e); @@ -190,28 +195,21 @@ public class ConfigServerHttpRequestExecutor implements AutoCloseable { } } - private static SSLContext makeSslContext(Optional<KeyStoreOptions> keyStoreOptions, Optional<KeyStoreOptions> trustStoreOptions) - throws KeyManagementException, NoSuchAlgorithmException { - SSLContextBuilder sslContextBuilder = new SSLContextBuilder(); - keyStoreOptions.ifPresent(options -> { - try { - sslContextBuilder.loadKeyMaterial(options.getKeyStore(), options.password); - } catch (Exception e) { - throw new RuntimeException(e); - } - }); - - trustStoreOptions.ifPresent(options -> { - try { - sslContextBuilder.loadTrustMaterial(options.getKeyStore(), null); - } catch (Exception e) { - throw new RuntimeException(e); - } - }); + private static SSLContext makeSslContext(Optional<KeyStoreOptions> keyStoreOptions, Optional<KeyStoreOptions> trustStoreOptions) { + AthenzSslContextBuilder sslContextBuilder = new AthenzSslContextBuilder(); + trustStoreOptions.ifPresent(options -> sslContextBuilder.withTrustStore(options.path.toFile(), options.type)); + keyStoreOptions.ifPresent(options -> + sslContextBuilder.withKeyStore(options.path.toFile(), options.password, options.type)); return sslContextBuilder.build(); } + private static HostnameVerifier makeHostnameVerifier(Optional<AthenzIdentity> athenzIdentity) { + return athenzIdentity + .map(identity -> (HostnameVerifier) new AthenzIdentityVerifier(Collections.singleton(identity))) + .orElse(SSLConnectionSocketFactory.getDefaultHostnameVerifier()); + } + @Override public void close() { clientRefresherScheduler.shutdown(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/Environment.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/Environment.java index 0f8baf9dabe..0415bbc34c2 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/Environment.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/Environment.java @@ -2,6 +2,8 @@ package com.yahoo.vespa.hosted.node.admin.util; import com.google.common.base.Strings; +import com.yahoo.vespa.athenz.api.AthenzIdentity; +import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.defaults.Defaults; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.node.admin.config.ConfigServerConfig; @@ -47,6 +49,7 @@ public class Environment { private final String feedEndpoint; private final Optional<KeyStoreOptions> keyStoreOptions; private final Optional<KeyStoreOptions> trustStoreOptions; + private final Optional<AthenzIdentity> athenzIdentity; static { filenameFormatter.setTimeZone(TimeZone.getTimeZone("UTC")); @@ -73,7 +76,10 @@ public class Environment { createKeyStoreOptions( configServerConfig.trustStoreConfig().path(), configServerConfig.trustStoreConfig().password().toCharArray(), - configServerConfig.trustStoreConfig().type().name()) + configServerConfig.trustStoreConfig().type().name()), + createAthenzIdentity( + configServerConfig.athenzDomain(), + configServerConfig.serviceName()) ); } @@ -86,7 +92,8 @@ public class Environment { List<String> logstashNodes, String feedEndpoint, Optional<KeyStoreOptions> keyStoreOptions, - Optional<KeyStoreOptions> trustStoreOptions) { + Optional<KeyStoreOptions> trustStoreOptions, + Optional<AthenzIdentity> athenzIdentity) { this.configServerHosts = configServerHosts; this.environment = environment; this.region = region; @@ -97,6 +104,7 @@ public class Environment { this.feedEndpoint = feedEndpoint; this.keyStoreOptions = keyStoreOptions; this.trustStoreOptions = trustStoreOptions; + this.athenzIdentity = athenzIdentity; } public List<URI> getConfigServerUris() { return configServerHosts; } @@ -145,6 +153,11 @@ public class Environment { .map(path -> new KeyStoreOptions(Paths.get(path), password, type)); } + private static Optional<AthenzIdentity> createAthenzIdentity(String athenzDomain, String serviceName) { + if (Strings.isNullOrEmpty(athenzDomain) || Strings.isNullOrEmpty(serviceName)) return Optional.empty(); + return Optional.of(new AthenzService(athenzDomain, serviceName)); + } + public InetAddress getInetAddressForHost(String hostname) throws UnknownHostException { return inetAddressResolver.getInetAddressForHost(hostname); } @@ -219,6 +232,10 @@ public class Environment { return trustStoreOptions; } + public Optional<AthenzIdentity> getAthenzIdentity() { + return athenzIdentity; + } + public static class Builder { private List<URI> configServerHosts = Collections.emptyList(); @@ -231,6 +248,7 @@ public class Environment { private String feedEndpoint; private KeyStoreOptions keyStoreOptions; private KeyStoreOptions trustStoreOptions; + private AthenzIdentity athenzIdentity; public Builder configServerUris(String... hosts) { configServerHosts = Arrays.stream(hosts) @@ -284,11 +302,16 @@ public class Environment { return this; } + public Builder athenzIdentity(AthenzIdentity athenzIdentity) { + this.athenzIdentity = athenzIdentity; + return this; + } public Environment build() { return new Environment(configServerHosts, environment, region, parentHostHostname, inetAddressResolver, pathResolver, logstashNodes, feedEndpoint, - Optional.ofNullable(keyStoreOptions), Optional.ofNullable(trustStoreOptions)); + Optional.ofNullable(keyStoreOptions), Optional.ofNullable(trustStoreOptions), + Optional.ofNullable(athenzIdentity)); } } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/KeyStoreOptions.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/KeyStoreOptions.java index 84db5840909..643abde101b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/KeyStoreOptions.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/util/KeyStoreOptions.java @@ -1,15 +1,9 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.util; -import java.io.FileInputStream; -import java.io.IOException; import java.nio.file.Path; -import java.security.KeyStore; -import java.security.KeyStoreException; -import java.security.NoSuchAlgorithmException; -import java.security.cert.CertificateException; -class KeyStoreOptions { +public class KeyStoreOptions { public final Path path; public final char[] password; public final String type; @@ -19,13 +13,4 @@ class KeyStoreOptions { this.password = password; this.type = type; } - - public KeyStore getKeyStore() throws IOException, KeyStoreException, CertificateException, NoSuchAlgorithmException { - try (FileInputStream fis = new FileInputStream(path.toFile())) { - KeyStore keyStore = KeyStore.getInstance(type); - keyStore.load(fis, password); - - return keyStore; - } - } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java index c6e4447a606..85d92dbee25 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImplTest.java @@ -57,7 +57,7 @@ public class NodeRepositoryImplTest { public void startContainer() throws Exception { final int port = findRandomOpenPort(); requestExecutor = ConfigServerHttpRequestExecutor.create( - Collections.singleton(URI.create("http://127.0.0.1:" + port)), Optional.empty(), Optional.empty()); + Collections.singleton(URI.create("http://127.0.0.1:" + port)), Optional.empty(), Optional.empty(), Optional.empty()); container = JDisc.fromServicesXml(ContainerConfig.servicesXmlV2(port), Networking.enable); } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java new file mode 100644 index 00000000000..05662de3b95 --- /dev/null +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectoryTest.java @@ -0,0 +1,91 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +package com.yahoo.vespa.hosted.node.admin.task.util.file; + +import com.yahoo.vespa.test.file.TestFileSystem; +import org.junit.Test; + +import java.io.UncheckedIOException; +import java.nio.file.FileSystem; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.util.Arrays; +import java.util.Collections; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * @author hakonhall + */ +public class MakeDirectoryTest { + private final FileSystem fileSystem = TestFileSystem.create(); + private final TestTaskContext context = new TestTaskContext(); + + private String path = "/parent/dir"; + private String permissions = "rwxr----x"; + private String owner = "test-owner"; + private String group = "test-group"; + + @Test + public void newDirectory() { + verifySystemModifications( + "Creating directory " + path, + "Changing owner of /parent/dir from user to test-owner", + "Changing group of /parent/dir from group to test-group"); + + owner = "new-owner"; + verifySystemModifications("Changing owner of /parent/dir from test-owner to new-owner"); + + group = "new-group"; + verifySystemModifications("Changing group of /parent/dir from test-group to new-group"); + + permissions = "--x---r--"; + verifySystemModifications("Changing permissions of /parent/dir from rwxr----x to --x---r--"); + } + + private void verifySystemModifications(String... modifications) { + context.clearSystemModificationLog(); + MakeDirectory makeDirectory = new MakeDirectory(fileSystem.getPath(path)) + .createParents() + .withPermissions(permissions) + .withOwner(owner) + .withGroup(group); + assertTrue(makeDirectory.converge(context)); + + assertEquals(Arrays.asList(modifications), context.getSystemModificationLog()); + + context.clearSystemModificationLog(); + assertFalse(makeDirectory.converge(context)); + assertEquals(Collections.emptyList(), context.getSystemModificationLog()); + } + + @Test + public void exceptionIfMissingParent() { + String path = "/parent/dir"; + MakeDirectory makeDirectory = new MakeDirectory(fileSystem.getPath(path)); + + try { + makeDirectory.converge(context); + } catch (UncheckedIOException e) { + if (e.getCause() instanceof NoSuchFileException) { + return; + } + throw e; + } + fail(); + } + + @Test + public void okIfParentExists() { + String path = "/dir"; + MakeDirectory makeDirectory = new MakeDirectory(fileSystem.getPath(path)); + assertTrue(makeDirectory.converge(context)); + assertTrue(Files.isDirectory(fileSystem.getPath(path))); + + MakeDirectory makeDirectory2 = new MakeDirectory(fileSystem.getPath(path)); + assertFalse(makeDirectory2.converge(context)); + } +}
\ No newline at end of file diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPathTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPathTest.java index bd29f239e1d..6f1991ec3d4 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPathTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPathTest.java @@ -13,6 +13,9 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +/** + * @author hakonhall + */ public class UnixPathTest { final FileSystem fileSystem = TestFileSystem.create(); @@ -63,4 +66,15 @@ public class UnixPathTest { unixPath.setGroup("group"); assertEquals("group", unixPath.getGroup()); } + + @Test + public void createDirectoryWithPermissions() { + FileSystem fs = TestFileSystem.create(); + Path path = fs.getPath("dir"); + UnixPath unixPath = new UnixPath(path); + String permissions = "rwxr-xr--"; + unixPath.createDirectory(permissions); + assertTrue(Files.isDirectory(path)); + assertEquals(permissions, unixPath.getPermissions()); + } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java index 59c853f949d..edd7aa13627 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/TestCommand.java @@ -50,6 +50,14 @@ public class TestCommand extends Command { @Override public Path getProcessOutputPath() { return null; } + + @Override + public void logAsModifyingSystemAfterAll(Logger logger) { } + + @Override + public String commandLine() { + return "program"; + } }; } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java index d852be26229..4a3c1061ea6 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/YumTest.java @@ -4,55 +4,90 @@ package com.yahoo.vespa.hosted.node.admin.task.util.yum; import com.yahoo.vespa.hosted.node.admin.component.TaskContext; import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandException; import com.yahoo.vespa.hosted.node.admin.task.util.process.TestCommandSupplier; +import org.junit.Before; import org.junit.Test; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; public class YumTest { + TaskContext taskContext = mock(TaskContext.class); + TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext); + + @Before + public void tearDown() { + commandSupplier.verifyInvocations(); + } + @Test public void testAlreadyInstalled() { - TaskContext taskContext = mock(TaskContext.class); - TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext); - - commandSupplier.expectCommand("yum list installed package-1", 0, ""); - commandSupplier.expectCommand("yum list installed package-2", 0, ""); + commandSupplier.expectCommand( + "yum install --assumeyes --enablerepo=repo-name package-1 package-2", + 0, + "\nNothing to do\n"); Yum yum = new Yum(taskContext, commandSupplier); - yum.install("package-1", "package-2") + assertFalse(yum + .install("package-1", "package-2") .enableRepo("repo-name") - .converge(); + .converge()); + } - commandSupplier.verifyInvocations(); + @Test + public void testAlreadyUpgraded() { + commandSupplier.expectCommand( + "yum upgrade --assumeyes package-1 package-2", + 0, + "\nNo packages marked for update\n"); + + assertFalse(new Yum(taskContext, commandSupplier) + .upgrade("package-1", "package-2") + .converge()); + } + + @Test + public void testAlreadyRemoved() { + commandSupplier.expectCommand( + "yum remove --assumeyes package-1 package-2", + 0, + "\nNo Packages marked for removal\n"); + + assertFalse(new Yum(taskContext, commandSupplier) + .remove("package-1", "package-2") + .converge()); } @Test public void testInstall() { - TaskContext taskContext = mock(TaskContext.class); - TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext); + commandSupplier.expectCommand( + "yum install --assumeyes package-1 package-2", + 0, + "installing, installing"); + + Yum yum = new Yum(taskContext, commandSupplier); + assertTrue(yum + .install("package-1", "package-2") + .converge()); + } - commandSupplier.expectCommand("yum list installed package-1", 0, ""); - commandSupplier.expectCommand("yum list installed package-2", 1, ""); + @Test + public void testInstallWithEnablerepo() { commandSupplier.expectCommand( "yum install --assumeyes --enablerepo=repo-name package-1 package-2", 0, - ""); + "installing, installing"); Yum yum = new Yum(taskContext, commandSupplier); - yum.install("package-1", "package-2") + assertTrue(yum + .install("package-1", "package-2") .enableRepo("repo-name") - .converge(); - - commandSupplier.verifyInvocations(); + .converge()); } @Test(expected = CommandException.class) public void testFailedInstall() { - TaskContext taskContext = mock(TaskContext.class); - TestCommandSupplier commandSupplier = new TestCommandSupplier(taskContext); - - commandSupplier.expectCommand("yum list installed package-1", 0, ""); - commandSupplier.expectCommand("yum list installed package-2", 1, ""); commandSupplier.expectCommand( "yum install --assumeyes --enablerepo=repo-name package-1 package-2", 1, diff --git a/searchcore/src/tests/proton/documentdb/fileconfigmanager/fileconfigmanager_test.cpp b/searchcore/src/tests/proton/documentdb/fileconfigmanager/fileconfigmanager_test.cpp index a781085769c..6b1031fe558 100644 --- a/searchcore/src/tests/proton/documentdb/fileconfigmanager/fileconfigmanager_test.cpp +++ b/searchcore/src/tests/proton/documentdb/fileconfigmanager/fileconfigmanager_test.cpp @@ -37,9 +37,8 @@ DocumentDBConfig::SP makeBaseConfigSnapshot() { config::DirSpec spec(TEST_PATH("cfg")); - ConfigKeySet extraKeySet; - extraKeySet.add<MycfgConfig>(""); - DBCM dbcm(spec, "test", extraKeySet); + + DBCM dbcm(spec, "test"); DocumenttypesConfigSP dtcfg(config::ConfigGetter<DocumenttypesConfig>::getConfig("", spec).release()); BootstrapConfig::SP b(new BootstrapConfig(1, dtcfg, @@ -73,19 +72,6 @@ makeEmptyConfigSnapshot() void incInt(int *i, const DocumentType&) { ++*i; } void -assertEqualExtraConfigs(const DocumentDBConfig &expSnap, const DocumentDBConfig &actSnap) -{ - const ConfigSnapshot &exp = expSnap.getExtraConfigs(); - const ConfigSnapshot &act = actSnap.getExtraConfigs(); - EXPECT_EQUAL(1u, exp.size()); - EXPECT_EQUAL(1u, act.size()); - std::unique_ptr<MycfgConfig> expCfg = exp.getConfig<MycfgConfig>(""); - std::unique_ptr<MycfgConfig> actCfg = act.getConfig<MycfgConfig>(""); - EXPECT_EQUAL("foo", expCfg->myField); - EXPECT_EQUAL("foo", actCfg->myField); -} - -void assertEqualSnapshot(const DocumentDBConfig &exp, const DocumentDBConfig &act) { EXPECT_TRUE(exp.getRankProfilesConfig() == act.getRankProfilesConfig()); @@ -109,7 +95,6 @@ assertEqualSnapshot(const DocumentDBConfig &exp, const DocumentDBConfig &act) EXPECT_TRUE(*exp.getSchemaSP() == *act.getSchemaSP()); EXPECT_EQUAL(expTypeCount, actTypeCount); EXPECT_EQUAL(exp.getConfigId(), act.getConfigId()); - assertEqualExtraConfigs(exp, act); } DocumentDBConfig::SP @@ -163,13 +148,12 @@ TEST_F("requireThatConfigCanBeSerializedAndDeserialized", DocumentDBConfig::SP(m TEST_F("requireThatConfigCanBeLoadedWithoutExtraConfigsDataFile", DocumentDBConfig::SP(makeBaseConfigSnapshot())) { saveBaseConfigSnapshot(*f, 70); - EXPECT_TRUE(vespalib::unlink("out/config-70/extraconfigs.dat")); + EXPECT_FALSE(vespalib::unlink("out/config-70/extraconfigs.dat")); DocumentDBConfig::SP esnap(makeEmptyConfigSnapshot()); { FileConfigManager cm("out", myId, "dummy"); cm.loadConfig(*esnap, 70, esnap); } - EXPECT_EQUAL(0u, esnap->getExtraConfigs().size()); } @@ -187,11 +171,9 @@ TEST_F("requireThatVisibilityDelayIsPropagated", protonConfigBuilder.maxvisibilitydelay = 100.0; FileConfigManager cm("out", myId, "dummy"); using ProtonConfigSP = BootstrapConfig::ProtonConfigSP; - cm.setProtonConfig( - ProtonConfigSP(new ProtonConfig(protonConfigBuilder))); + cm.setProtonConfig(ProtonConfigSP(new ProtonConfig(protonConfigBuilder))); cm.loadConfig(*esnap, 70, esnap); } - EXPECT_EQUAL(0u, esnap->getExtraConfigs().size()); EXPECT_EQUAL(61.0, esnap->getMaintenanceConfigSP()->getVisibilityDelay().sec()); } diff --git a/searchcore/src/tests/proton/proton_configurer/proton_configurer_test.cpp b/searchcore/src/tests/proton/proton_configurer/proton_configurer_test.cpp index f2e1b9d7805..a8336e29f9f 100644 --- a/searchcore/src/tests/proton/proton_configurer/proton_configurer_test.cpp +++ b/searchcore/src/tests/proton/proton_configurer/proton_configurer_test.cpp @@ -91,8 +91,7 @@ struct DBConfigFixture { std::make_shared<DocumentDBMaintenanceConfig>(), search::LogDocumentStore::Config(), configId, - docTypeName, - config::ConfigSnapshot()); + docTypeName); } }; diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp index b83040b04ff..db73e9f38b8 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.cpp @@ -63,8 +63,7 @@ DocumentDBConfig::DocumentDBConfig( const DocumentDBMaintenanceConfig::SP &maintenance, const search::LogDocumentStore::Config & storeConfig, const vespalib::string &configId, - const vespalib::string &docTypeName, - const config::ConfigSnapshot & extraConfigs) + const vespalib::string &docTypeName) : _configId(configId), _docTypeName(docTypeName), _generation(generation), @@ -82,7 +81,6 @@ DocumentDBConfig::DocumentDBConfig( _schema(schema), _maintenance(maintenance), _storeConfig(storeConfig), - _extraConfigs(extraConfigs), _orig(), _delayedAttributeAspects(false) { } @@ -107,7 +105,6 @@ DocumentDBConfig(const DocumentDBConfig &cfg) _schema(cfg._schema), _maintenance(cfg._maintenance), _storeConfig(cfg._storeConfig), - _extraConfigs(cfg._extraConfigs), _orig(cfg._orig), _delayedAttributeAspects(false) { } @@ -216,8 +213,7 @@ DocumentDBConfig::makeReplayConfig(const SP & orig) o._maintenance, o._storeConfig, o._configId, - o._docTypeName, - o._extraConfigs); + o._docTypeName); ret->_orig = orig; return ret; } @@ -257,8 +253,7 @@ DocumentDBConfig::newFromAttributesConfig(const AttributesConfigSP &attributes) _maintenance, _storeConfig, _configId, - _docTypeName, - _extraConfigs); + _docTypeName); } DocumentDBConfig::SP @@ -293,8 +288,7 @@ DocumentDBConfig::makeDelayedAttributeAspectConfig(const SP &newCfg, const Docum n._maintenance, n._storeConfig, n._configId, - n._docTypeName, - n._extraConfigs); + n._docTypeName); result->_delayedAttributeAspects = true; return result; } diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h index cfca754a3d6..df023028123 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h +++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfig.h @@ -118,7 +118,6 @@ private: search::index::Schema::SP _schema; MaintenanceConfigSP _maintenance; search::LogDocumentStore::Config _storeConfig; - config::ConfigSnapshot _extraConfigs; SP _orig; bool _delayedAttributeAspects; @@ -156,8 +155,7 @@ public: const DocumentDBMaintenanceConfig::SP &maintenance, const search::LogDocumentStore::Config & storeConfig, const vespalib::string &configId, - const vespalib::string &docTypeName, - const config::ConfigSnapshot &extraConfig = config::ConfigSnapshot()); + const vespalib::string &docTypeName); DocumentDBConfig(const DocumentDBConfig &cfg); ~DocumentDBConfig(); @@ -203,9 +201,6 @@ public: bool valid() const; - const config::ConfigSnapshot &getExtraConfigs() const { return _extraConfigs; } - void setExtraConfigs(const config::ConfigSnapshot &extraConfigs) { _extraConfigs = extraConfigs; } - /** * Only keep configs needed for replay of transaction log. */ diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.cpp index ef43839f313..def18b76360 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.cpp @@ -32,6 +32,8 @@ using search::LogDocumentStore; using search::LogDataStore; using search::DocumentStore; using search::WriteableFileChunk; +using std::make_shared; +using std::make_unique; namespace proton { @@ -47,7 +49,6 @@ DocumentDBConfigManager::createConfigKeySet() const SummarymapConfig, JuniperrcConfig, ImportedFieldsConfig>(_configId); - set.add(_extraConfigKeys); return set; } @@ -166,8 +167,6 @@ deriveConfig(const ProtonConfig::Summary & summary, const ProtonConfig::Flush::M DocumentStore::Config config(getStoreConfig(summary.cache)); const ProtonConfig::Summary::Log & log(summary.log); const ProtonConfig::Summary::Log::Chunk & chunk(log.chunk); - const - WriteableFileChunk::Config fileConfig(deriveCompression(chunk.compression), chunk.maxbytes); LogDataStore::Config logConfig; logConfig.setMaxFileSize(log.maxfilesize) @@ -318,7 +317,6 @@ DocumentDBConfigManager::update(const ConfigSnapshot &snapshot) if (newMaintenanceConfig && oldMaintenanceConfig && *newMaintenanceConfig == *oldMaintenanceConfig) { newMaintenanceConfig = oldMaintenanceConfig; } - ConfigSnapshot extraConfigs(snapshot.subset(_extraConfigKeys)); DocumentDBConfig::SP newSnapshot( new DocumentDBConfig(generation, newRankProfilesConfig, @@ -336,8 +334,7 @@ DocumentDBConfigManager::update(const ConfigSnapshot &snapshot) newMaintenanceConfig, storeConfig, _configId, - _docTypeName, - extraConfigs)); + _docTypeName)); assert(newSnapshot->valid()); { std::lock_guard<std::mutex> lock(_pendingConfigMutex); @@ -353,8 +350,7 @@ DocumentDBConfigManager(const vespalib::string &configId, const vespalib::string _bootstrapConfig(), _pendingConfigSnapshot(), _ignoreForwardedConfig(true), - _pendingConfigMutex(), - _extraConfigKeys() + _pendingConfigMutex() { } DocumentDBConfigManager::~DocumentDBConfigManager() { } @@ -378,26 +374,17 @@ forwardConfig(const BootstrapConfig::SP & config) } } -DocumentDBConfigHelper::DocumentDBConfigHelper(const config::DirSpec &spec, const vespalib::string &docTypeName) - : DocumentDBConfigHelper(spec, docTypeName, config::ConfigKeySet()) +DocumentDBConfigHelper::DocumentDBConfigHelper(const DirSpec &spec, const vespalib::string &docTypeName) + : _mgr("", docTypeName), + _retriever(make_unique<ConfigRetriever>(_mgr.createConfigKeySet(), make_shared<ConfigContext>(spec))) { } -DocumentDBConfigHelper::DocumentDBConfigHelper(const config::DirSpec &spec, const vespalib::string &docTypeName, - const config::ConfigKeySet &extraConfigKeys) - : _mgr("", docTypeName), - _retriever() -{ - _mgr.setExtraConfigKeys(extraConfigKeys); - _retriever.reset(new config::ConfigRetriever(_mgr.createConfigKeySet(), - config::IConfigContext::SP(new config::ConfigContext(spec)))); -} - -DocumentDBConfigHelper::~DocumentDBConfigHelper() { } +DocumentDBConfigHelper::~DocumentDBConfigHelper() = default; bool DocumentDBConfigHelper::nextGeneration(int timeoutInMillis) { - config::ConfigSnapshot snapshot(_retriever->getBootstrapConfigs(timeoutInMillis)); + ConfigSnapshot snapshot(_retriever->getBootstrapConfigs(timeoutInMillis)); if (snapshot.empty()) return false; _mgr.update(snapshot); diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.h b/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.h index ebab75da1b0..80654c7588b 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.h +++ b/searchcore/src/vespa/searchcore/proton/server/documentdbconfigmanager.h @@ -26,7 +26,6 @@ private: DocumentDBConfig::SP _pendingConfigSnapshot; bool _ignoreForwardedConfig; mutable std::mutex _pendingConfigMutex; - config::ConfigKeySet _extraConfigKeys; search::index::Schema::SP buildSchema(const DocumentDBConfig::AttributesConfig & newAttributesConfig, @@ -42,8 +41,6 @@ public: void forwardConfig(const BootstrapConfigSP & config); const config::ConfigKeySet createConfigKeySet() const; - void setExtraConfigKeys(const config::ConfigKeySet & extraConfigKeys) { _extraConfigKeys = extraConfigKeys; } - const config::ConfigKeySet & getExtraConfigKeys() const { return _extraConfigKeys; } const vespalib::string & getConfigId() const { return _configId; } }; @@ -54,10 +51,8 @@ class DocumentDBConfigHelper { public: DocumentDBConfigHelper(const config::DirSpec &spec, const vespalib::string &docTypeName); - DocumentDBConfigHelper(const config::DirSpec &spec, const vespalib::string &docTypeName, - const config::ConfigKeySet &extraConfigKeys); - ~DocumentDBConfigHelper(); + bool nextGeneration(int timeoutInMillis); DocumentDBConfig::SP getConfig() const; void forwardConfig(const std::shared_ptr<BootstrapConfig> & config); diff --git a/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp b/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp index f7bec951bbd..cfb96172800 100644 --- a/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp @@ -50,12 +50,6 @@ makeSnapDirBaseName(SerialNum serialNum) return os.str(); } -vespalib::string -makeExtraConfigsFileName(const vespalib::string &snapDir) -{ - return snapDir + "/extraconfigs.dat"; -} - void fsyncFile(const vespalib::string &fileName) @@ -88,37 +82,11 @@ saveHelper(const vespalib::string &snapDir, template <class Config> void -save(const vespalib::string &snapDir, - const Config &config) +save(const vespalib::string &snapDir, const Config &config) { saveHelper(snapDir, config.defName(), config); } -void writeExtraConfigs(const vespalib::string &snapDir, - const DocumentDBConfig &snapshot) -{ - vespalib::string extraName(makeExtraConfigsFileName(snapDir)); - config::FileConfigSnapshotWriter writer(extraName); - bool extraConfigsWriterResult = writer.write(snapshot.getExtraConfigs()); - assert(extraConfigsWriterResult); - (void) extraConfigsWriterResult; - fsyncFile(extraName); -} - -config::ConfigSnapshot -readExtraConfigs(const vespalib::string &snapDir) -{ - vespalib::string fileName = makeExtraConfigsFileName(snapDir); - if (vespalib::fileExists(fileName)) { - config::FileConfigSnapshotReader reader(fileName); - return reader.read(); - } else { - LOG(warning, "Did not find data file for extra configs '%s' during loading of config snapshot. " - "Using empty extra configs set.", fileName.c_str()); - } - return config::ConfigSnapshot(); -} - class ConfigFile { @@ -329,7 +297,6 @@ FileConfigManager::saveConfig(const DocumentDBConfig &snapshot, assert(saveHistorySchemaRes); (void) saveHistorySchemaRes; - writeExtraConfigs(snapDir, snapshot); _info.validateSnapshot(serialNum); bool saveValidSnap = _info.save(); @@ -393,20 +360,13 @@ FileConfigManager::loadConfig(const DocumentDBConfig ¤tSnapshot, * of default values here instead of the current values from the config * server. */ - BootstrapConfig::SP bootstrap( - new BootstrapConfig(1, - docTypesCfg, - repo, - _protonConfig, - filedistRpcConf, - bucketspaces, - currentSnapshot.getTuneFileDocumentDBSP())); + auto bootstrap = std::make_shared<BootstrapConfig>(1, docTypesCfg, repo, _protonConfig, filedistRpcConf, + bucketspaces,currentSnapshot.getTuneFileDocumentDBSP()); dbc.forwardConfig(bootstrap); dbc.nextGeneration(0); loadedSnapshot = dbc.getConfig(); loadedSnapshot->setConfigId(_configId); - loadedSnapshot->setExtraConfigs(readExtraConfigs(snapDir)); } diff --git a/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.cpp b/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.cpp index adb52583a58..deeec695f26 100644 --- a/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.cpp @@ -6,7 +6,6 @@ #include "i_proton_configurer.h" #include <vespa/config/common/exceptions.h> #include <vespa/vespalib/util/exceptions.h> -#include <thread> #include <vespa/log/log.h> LOG_SETUP(".proton.server.proton_config_fetcher"); @@ -80,11 +79,9 @@ void ProtonConfigFetcher::updateDocumentDBConfigs(const BootstrapConfig::SP & bootstrapConfig, const ConfigSnapshot & snapshot) { lock_guard guard(_mutex); - for (DBManagerMap::iterator it(_dbManagerMap.begin()), mt(_dbManagerMap.end()); - it != mt; - it++) { - it->second->forwardConfig(bootstrapConfig); - it->second->update(snapshot); + for (auto & entry : _dbManagerMap) { + entry.second->forwardConfig(bootstrapConfig); + entry.second->update(snapshot); } } @@ -181,17 +178,6 @@ ProtonConfigFetcher::close() } } -DocumentDBConfig::SP -ProtonConfigFetcher::getDocumentDBConfig(const DocTypeName & docTypeName) const -{ - lock_guard guard(_mutex); - DBManagerMap::const_iterator it(_dbManagerMap.find(docTypeName)); - if (it == _dbManagerMap.end()) - return DocumentDBConfig::SP(); - - return it->second->getConfig(); -} - void ProtonConfigFetcher::rememberDocumentTypeRepo(std::shared_ptr<document::DocumentTypeRepo> repo) { diff --git a/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.h b/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.h index fa1f75ebe91..c8d1e55e4e4 100644 --- a/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.h +++ b/searchcore/src/vespa/searchcore/proton/server/proton_config_fetcher.h @@ -44,8 +44,6 @@ public: */ void close(); - DocumentDBConfig::SP getDocumentDBConfig(const DocTypeName & docTypeName) const; - void Run(FastOS_ThreadInterface * thread, void *arg) override; private: diff --git a/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.cpp b/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.cpp index e085d1c52f8..2bfbbf97001 100644 --- a/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.cpp +++ b/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.cpp @@ -43,10 +43,8 @@ DocumentDBConfigBuilder::DocumentDBConfigBuilder(int64_t generation, _maintenance(std::make_shared<DocumentDBMaintenanceConfig>()), _store(), _configId(configId), - _docTypeName(docTypeName), - _extraConfig() -{ -} + _docTypeName(docTypeName) +{ } DocumentDBConfigBuilder::DocumentDBConfigBuilder(const DocumentDBConfig &cfg) @@ -66,10 +64,8 @@ DocumentDBConfigBuilder::DocumentDBConfigBuilder(const DocumentDBConfig &cfg) _maintenance(cfg.getMaintenanceConfigSP()), _store(cfg.getStoreConfig()), _configId(cfg.getConfigId()), - _docTypeName(cfg.getDocTypeName()), - _extraConfig(cfg.getExtraConfigs()) -{ -} + _docTypeName(cfg.getDocTypeName()) +{} DocumentDBConfigBuilder::~DocumentDBConfigBuilder() {} @@ -93,8 +89,7 @@ DocumentDBConfigBuilder::build() _maintenance, _store, _configId, - _docTypeName, - _extraConfig); + _docTypeName); } } diff --git a/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.h b/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.h index 87369dab123..0dd812c354c 100644 --- a/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.h +++ b/searchcore/src/vespa/searchcore/proton/test/documentdb_config_builder.h @@ -28,7 +28,6 @@ private: search::LogDocumentStore::Config _store; vespalib::string _configId; vespalib::string _docTypeName; - config::ConfigSnapshot _extraConfig; public: DocumentDBConfigBuilder(int64_t generation, diff --git a/storage/src/tests/storageserver/bouncertest.cpp b/storage/src/tests/storageserver/bouncertest.cpp index a4f7ed915d8..43683132bc9 100644 --- a/storage/src/tests/storageserver/bouncertest.cpp +++ b/storage/src/tests/storageserver/bouncertest.cpp @@ -131,7 +131,7 @@ BouncerTest::testFutureTimestamp() CPPUNIT_ASSERT_EQUAL(1, (int)_upper->getNumReplies()); CPPUNIT_ASSERT_EQUAL(0, (int)_upper->getNumCommands()); - CPPUNIT_ASSERT_EQUAL(api::ReturnCode::ABORTED, + CPPUNIT_ASSERT_EQUAL(api::ReturnCode::REJECTED, static_cast<api::RemoveReply&>(*_upper->getReply(0)). getResult().getResult()); _upper->reset(); diff --git a/storage/src/tests/storageserver/communicationmanagertest.cpp b/storage/src/tests/storageserver/communicationmanagertest.cpp index e7e08f28ce3..aa20f32450d 100644 --- a/storage/src/tests/storageserver/communicationmanagertest.cpp +++ b/storage/src/tests/storageserver/communicationmanagertest.cpp @@ -23,7 +23,7 @@ struct CommunicationManagerTest : public CppUnit::TestFixture { void testSimple(); void testDistPendingLimitConfigsArePropagatedToMessageBus(); void testStorPendingLimitConfigsArePropagatedToMessageBus(); - void testCommandsAreDequeuedInPriorityOrder(); + void testCommandsAreDequeuedInFifoOrder(); void testRepliesAreDequeuedInFifoOrder(); void bucket_space_config_can_be_updated_live(); void unmapped_bucket_space_documentapi_request_returns_error_reply(); @@ -47,7 +47,7 @@ struct CommunicationManagerTest : public CppUnit::TestFixture { CPPUNIT_TEST(testSimple); CPPUNIT_TEST(testDistPendingLimitConfigsArePropagatedToMessageBus); CPPUNIT_TEST(testStorPendingLimitConfigsArePropagatedToMessageBus); - CPPUNIT_TEST(testCommandsAreDequeuedInPriorityOrder); + CPPUNIT_TEST(testCommandsAreDequeuedInFifoOrder); CPPUNIT_TEST(testRepliesAreDequeuedInFifoOrder); CPPUNIT_TEST(bucket_space_config_can_be_updated_live); CPPUNIT_TEST(unmapped_bucket_space_documentapi_request_returns_error_reply); @@ -175,7 +175,7 @@ CommunicationManagerTest::testStorPendingLimitConfigsArePropagatedToMessageBus() } void -CommunicationManagerTest::testCommandsAreDequeuedInPriorityOrder() +CommunicationManagerTest::testCommandsAreDequeuedInFifoOrder() { mbus::Slobrok slobrok; vdstestlib::DirConfig storConfig(getStandardConfig(true)); @@ -190,8 +190,8 @@ CommunicationManagerTest::testCommandsAreDequeuedInPriorityOrder() // Message dequeing does not start before we invoke `open` on the storage // link chain, so we enqueue messages in randomized priority order before - // doing so. After starting the thread, we should then get messages down - // the chain in a deterministic, prioritized order. + // doing so. After starting the thread, we should get messages down + // the chain in a deterministic FIFO order and _not_ priority-order. // Lower number == higher priority. std::vector<api::StorageMessage::Priority> pris{200, 0, 255, 128}; for (auto pri : pris) { @@ -200,7 +200,6 @@ CommunicationManagerTest::testCommandsAreDequeuedInPriorityOrder() storage.open(); storageLink->waitForMessages(pris.size(), MESSAGE_WAIT_TIME_SEC); - std::sort(pris.begin(), pris.end()); for (size_t i = 0; i < pris.size(); ++i) { // Casting is just to avoid getting mismatched values printed to the // output verbatim as chars. diff --git a/storage/src/vespa/storage/storageserver/bouncer.cpp b/storage/src/vespa/storage/storageserver/bouncer.cpp index 3d068ec5f81..7648c07d7da 100644 --- a/storage/src/vespa/storage/storageserver/bouncer.cpp +++ b/storage/src/vespa/storage/storageserver/bouncer.cpp @@ -113,19 +113,19 @@ Bouncer::abortCommandForUnavailableNode(api::StorageMessage& msg, } void -Bouncer::abortCommandWithTooHighClockSkew(api::StorageMessage& msg, +Bouncer::rejectCommandWithTooHighClockSkew(api::StorageMessage& msg, int maxClockSkewInSeconds) { auto& as_cmd = dynamic_cast<api::StorageCommand&>(msg); std::ostringstream ost; ost << "Message " << msg.getType() << " is more than " << maxClockSkewInSeconds << " seconds in the future."; - LOGBP(warning, "Aborting operation from distributor %u: %s", + LOGBP(warning, "Rejecting operation from distributor %u: %s", as_cmd.getSourceIndex(), ost.str().c_str()); _metrics->clock_skew_aborts.inc(); std::shared_ptr<api::StorageReply> reply(as_cmd.makeReply().release()); - reply->setResult(api::ReturnCode(api::ReturnCode::ABORTED, ost.str())); + reply->setResult(api::ReturnCode(api::ReturnCode::REJECTED, ost.str())); sendUp(reply); } @@ -271,7 +271,7 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg) timestamp /= 1000000; uint64_t currentTime = _component.getClock().getTimeInSeconds().getTime(); if (timestamp > currentTime + maxClockSkewInSeconds) { - abortCommandWithTooHighClockSkew(*msg, maxClockSkewInSeconds); + rejectCommandWithTooHighClockSkew(*msg, maxClockSkewInSeconds); return true; } } diff --git a/storage/src/vespa/storage/storageserver/bouncer.h b/storage/src/vespa/storage/storageserver/bouncer.h index 6c81cb1b47a..b46bf3fedc6 100644 --- a/storage/src/vespa/storage/storageserver/bouncer.h +++ b/storage/src/vespa/storage/storageserver/bouncer.h @@ -57,8 +57,8 @@ private: void abortCommandForUnavailableNode(api::StorageMessage&, const lib::State&); - void abortCommandWithTooHighClockSkew(api::StorageMessage& msg, - int maxClockSkewInSeconds); + void rejectCommandWithTooHighClockSkew(api::StorageMessage& msg, + int maxClockSkewInSeconds); void abortCommandDueToClusterDown(api::StorageMessage&); diff --git a/storage/src/vespa/storage/storageserver/communicationmanager.cpp b/storage/src/vespa/storage/storageserver/communicationmanager.cpp index 4cf4839319f..b61717e5b67 100644 --- a/storage/src/vespa/storage/storageserver/communicationmanager.cpp +++ b/storage/src/vespa/storage/storageserver/communicationmanager.cpp @@ -26,23 +26,16 @@ using vespalib::make_string; namespace storage { -PriorityQueue::PriorityQueue() : - _queue(), - _queueMonitor(), - _msgCounter(0) -{ } - -PriorityQueue::~PriorityQueue() -{ } +Queue::Queue() = default; +Queue::~Queue() = default; -bool PriorityQueue::getNext(std::shared_ptr<api::StorageMessage>& msg, int timeout) -{ +bool Queue::getNext(std::shared_ptr<api::StorageMessage>& msg, int timeout) { vespalib::MonitorGuard sync(_queueMonitor); bool first = true; while (true) { // Max twice if (!_queue.empty()) { LOG(spam, "Picking message from queue"); - msg = _queue.top().second; + msg = _queue.front(); _queue.pop(); return true; } @@ -56,31 +49,18 @@ bool PriorityQueue::getNext(std::shared_ptr<api::StorageMessage>& msg, int timeo return false; } -void -PriorityQueue::enqueue(const std::shared_ptr<api::StorageMessage>& msg) -{ +void Queue::enqueue(const std::shared_ptr<api::StorageMessage>& msg) { vespalib::MonitorGuard sync(_queueMonitor); - const uint8_t priority(msg->getType().isReply() - ? FIXED_REPLY_PRIORITY - : msg->getPriority()); - Key key(priority, _msgCounter); - // We make a simplifying--though reasonable--assumption that we'll never - // process more than UINT64_MAX replies before process restart. - ++_msgCounter; - _queue.push(std::make_pair(key, msg)); + _queue.emplace(msg); sync.unsafeSignalUnlock(); } -void -PriorityQueue::signal() -{ +void Queue::signal() { vespalib::MonitorGuard sync(_queueMonitor); sync.unsafeSignalUnlock(); } -int -PriorityQueue::size() -{ +size_t Queue::size() const { vespalib::MonitorGuard sync(_queueMonitor); return _queue.size(); } diff --git a/storage/src/vespa/storage/storageserver/communicationmanager.h b/storage/src/vespa/storage/storageserver/communicationmanager.h index e2c890bfc9b..ff41e59846a 100644 --- a/storage/src/vespa/storage/storageserver/communicationmanager.h +++ b/storage/src/vespa/storage/storageserver/communicationmanager.h @@ -43,49 +43,15 @@ class VisitorThread; class FNetListener; class RPCRequestWrapper; -class PriorityQueue { +class Queue { private: - struct Key { - uint8_t priority {255}; - uint64_t seqNum {0}; - - Key(uint8_t pri, uint64_t seq) - : priority(pri), seqNum(seq) - { - } - }; - using ValueType = std::pair<Key, api::StorageMessage::SP>; - - struct PriorityThenFifoCmp { - bool operator()(const ValueType& lhs, - const ValueType& rhs) const noexcept - { - // priority_queue has largest element on top, so reverse order - // since our semantics have 0 as the highest priority. - if (lhs.first.priority != rhs.first.priority) { - return (lhs.first.priority > rhs.first.priority); - } - return (lhs.first.seqNum > rhs.first.seqNum); - } - }; - - using QueueType = std::priority_queue< - ValueType, - std::vector<ValueType>, - PriorityThenFifoCmp>; - - // Sneakily chosen priority such that effectively only RPC commands are - // allowed in front of replies. Replies must have the same effective - // priority or they will get reordered and all hell breaks loose. - static constexpr uint8_t FIXED_REPLY_PRIORITY = 1; - + using QueueType = std::queue<api::StorageMessage::SP>; QueueType _queue; vespalib::Monitor _queueMonitor; - uint64_t _msgCounter; public: - PriorityQueue(); - virtual ~PriorityQueue(); + Queue(); + ~Queue(); /** * Returns the next event from the event queue @@ -97,17 +63,14 @@ public: bool getNext(std::shared_ptr<api::StorageMessage>& msg, int timeout); /** - * If `msg` is a StorageCommand, enqueues it using the priority stored in - * the command. If it's a reply, enqueues it using a fixed but very high - * priority that ensure replies are processed before commands but also - * ensures that replies are FIFO-ordered relative to each other. + * Enqueue msg in FIFO order. */ void enqueue(const std::shared_ptr<api::StorageMessage>& msg); /** Signal queue monitor. */ void signal(); - int size(); + size_t size() const; }; class StorageTransportContext : public api::TransportContext { @@ -137,7 +100,7 @@ private: CommunicationManagerMetrics _metrics; std::unique_ptr<FNetListener> _listener; - PriorityQueue _eventQueue; + Queue _eventQueue; // XXX: Should perhaps use a configsubscriber and poll from StorageComponent ? std::unique_ptr<config::ConfigFetcher> _configFetcher; using EarlierProtocol = std::pair<framework::SecondTime, mbus::IProtocol::SP>; |