diff options
66 files changed, 1235 insertions, 1271 deletions
diff --git a/config-model/src/main/java/com/yahoo/schema/OnnxModel.java b/config-model/src/main/java/com/yahoo/schema/OnnxModel.java index 6baaea6ea05..272b668b5fb 100644 --- a/config-model/src/main/java/com/yahoo/schema/OnnxModel.java +++ b/config-model/src/main/java/com/yahoo/schema/OnnxModel.java @@ -55,7 +55,7 @@ public class OnnxModel extends DistributableResource { return ref.toString(); } // or a function (evaluated by backend) - if (ref.isSimple() && "rankingExpression".equals(ref.name())) { + if (ref.isSimpleRankingExpressionWrapper()) { var arg = ref.simpleArgument(); if (arg.isPresent()) { return ref.toString(); diff --git a/config-model/src/main/java/com/yahoo/schema/RankProfile.java b/config-model/src/main/java/com/yahoo/schema/RankProfile.java index 7cb0a088f5f..a00bbb682a8 100644 --- a/config-model/src/main/java/com/yahoo/schema/RankProfile.java +++ b/config-model/src/main/java/com/yahoo/schema/RankProfile.java @@ -1169,7 +1169,7 @@ public class RankProfile implements Cloneable { // Source is either a simple reference (query/attribute/constant/rankingExpression)... Optional<Reference> reference = Reference.simple(source); if (reference.isPresent()) { - if (reference.get().name().equals("rankingExpression") && reference.get().simpleArgument().isPresent()) { + if (reference.get().isSimpleRankingExpressionWrapper()) { source = reference.get().simpleArgument().get(); // look up function below } else { return Optional.of(context.getType(reference.get())); diff --git a/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java b/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java index 31a38752bec..acb125197d2 100644 --- a/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java +++ b/config-model/src/main/java/com/yahoo/schema/derived/RawRankProfile.java @@ -20,6 +20,7 @@ import com.yahoo.searchlib.rankingexpression.parser.ParseException; import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; import com.yahoo.searchlib.rankingexpression.rule.SerializationContext; import com.yahoo.vespa.config.search.RankProfilesConfig; +import static com.yahoo.searchlib.rankingexpression.Reference.wrapInRankingExpression; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; @@ -273,9 +274,9 @@ public class RawRankProfile implements RankProfilesConfig.Producer { String propertyName = RankingExpression.propertyName(referenceNode.getName()); String expressionString = function.getBody().getRoot().toString(context).toString(); context.addFunctionSerialization(propertyName, expressionString); - ReferenceNode backendReferenceNode = new ReferenceNode("rankingExpression(" + referenceNode.getName() + ")", - referenceNode.getArguments().expressions(), - referenceNode.getOutput()); + var backendReferenceNode = new ReferenceNode(wrapInRankingExpression(referenceNode.getName()), + referenceNode.getArguments().expressions(), + referenceNode.getOutput()); // tell backend to map back to the name the user expects: featureRenames.put(backendReferenceNode.toString(), referenceNode.toString()); functionFeatures.put(referenceNode.getName(), backendReferenceNode); @@ -499,7 +500,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer { if (expression.getRoot() instanceof ReferenceNode) { properties.add(new Pair<>("vespa.rank." + phase, expression.getRoot().toString())); } else { - properties.add(new Pair<>("vespa.rank." + phase, "rankingExpression(" + name + ")")); + properties.add(new Pair<>("vespa.rank." + phase, wrapInRankingExpression(name))); properties.add(new Pair<>(RankingExpression.propertyName(name), expression.getRoot().toString())); } return properties; @@ -520,7 +521,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer { for (Map.Entry<String, String> mapping : onnxModel.getInputMap().entrySet()) { String source = mapping.getValue(); if (functionNames.contains(source)) { - onnxModel.addInputNameMapping(mapping.getKey(), "rankingExpression(" + source + ")"); + onnxModel.addInputNameMapping(mapping.getKey(), wrapInRankingExpression(source)); } } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java index 5d4ec598250..d97840d4cbf 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/ContainerSearch.java @@ -148,17 +148,17 @@ public class ContainerSearch extends ContainerSubsystem<SearchChains> public void getConfig(QrSearchersConfig.Builder builder) { for (int i = 0; i < searchClusters.size(); i++) { SearchCluster sys = findClusterWithId(searchClusters, i); - QrSearchersConfig.Searchcluster.Builder scB = new QrSearchersConfig.Searchcluster.Builder(). - name(sys.getClusterName()); - for (SchemaInfo spec : sys.schemas().values()) { - scB.searchdef(spec.fullSchema().getName()); - } - scB.rankprofiles(new QrSearchersConfig.Searchcluster.Rankprofiles.Builder().configid(sys.getConfigId())); - scB.indexingmode(QrSearchersConfig.Searchcluster.Indexingmode.Enum.valueOf(sys.getIndexingModeName())); - if ( ! (sys instanceof IndexedSearchCluster)) { + QrSearchersConfig.Searchcluster.Builder scB = new QrSearchersConfig.Searchcluster.Builder(). + name(sys.getClusterName()); + for (SchemaInfo spec : sys.schemas().values()) { + scB.searchdef(spec.fullSchema().getName()); + } + scB.rankprofiles(new QrSearchersConfig.Searchcluster.Rankprofiles.Builder().configid(sys.getConfigId())); + scB.indexingmode(QrSearchersConfig.Searchcluster.Indexingmode.Enum.valueOf(sys.getIndexingModeName())); + scB.globalphase(globalPhase); + if ( ! (sys instanceof IndexedSearchCluster)) { scB.storagecluster(new QrSearchersConfig.Searchcluster.Storagecluster.Builder(). - routespec(((StreamingSearchCluster)sys).getStorageRouteSpec())); - scB.globalphase(globalPhase); + routespec(((StreamingSearchCluster)sys).getStorageRouteSpec())); } builder.searchcluster(scB); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java index 71726b50391..a0240d28a3c 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/ContentSearchCluster.java @@ -426,6 +426,16 @@ public class ContentSearchCluster extends TreeConfigProducer<AnyConfigProducer> } builder.indexing.optimize(feedSequencerType); + setMaxFlushed(builder); + } + + private void setMaxFlushed(ProtonConfig.Builder builder) { + // maxflushed should be moved down to proton + double concurrency = builder.feeding.build().concurrency(); + if (concurrency > defaultFeedConcurrency) { + int maxFlushes = (int)Math.ceil(4 * concurrency); + builder.index.maxflushed(maxFlushes); + } } private boolean isGloballyDistributed(NewDocumentType docType) { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/search/NodeResourcesTuning.java b/config-model/src/main/java/com/yahoo/vespa/model/search/NodeResourcesTuning.java index b6a7fb6182e..ee18eceb719 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/search/NodeResourcesTuning.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/search/NodeResourcesTuning.java @@ -97,8 +97,10 @@ public class NodeResourcesTuning implements ProtonConfig.Producer { if (usableMemoryGb() < MIN_MEMORY_PER_FLUSH_THREAD_GB) { builder.maxconcurrent(1); } + double min_concurrent_mem = usableMemoryGb() / (2*MIN_MEMORY_PER_FLUSH_THREAD_GB); + double min_concurrent_cpu = resources.vcpu() * MAX_FLUSH_THREAD_RATIO; builder.maxconcurrent(Math.min(builder.build().maxconcurrent(), - Math.max(1, (int)Math.ceil(resources.vcpu()*MAX_FLUSH_THREAD_RATIO)))); + (int)Math.ceil(Math.max(min_concurrent_mem, min_concurrent_cpu)))); } private void tuneFlushStrategyTlsSize(ProtonConfig.Flush.Memory.Builder builder) { diff --git a/config-model/src/test/derived/rankingmacros/rank-profiles.cfg b/config-model/src/test/derived/rankingmacros/rank-profiles.cfg new file mode 100644 index 00000000000..71b7bc7166c --- /dev/null +++ b/config-model/src/test/derived/rankingmacros/rank-profiles.cfg @@ -0,0 +1,83 @@ +rankprofile[].name "default" +rankprofile[].name "unranked" +rankprofile[].fef.property[].name "vespa.rank.firstphase" +rankprofile[].fef.property[].value "value(0)" +rankprofile[].fef.property[].name "vespa.hitcollector.heapsize" +rankprofile[].fef.property[].value "0" +rankprofile[].fef.property[].name "vespa.hitcollector.arraysize" +rankprofile[].fef.property[].value "0" +rankprofile[].fef.property[].name "vespa.dump.ignoredefaultfeatures" +rankprofile[].fef.property[].value "true" +rankprofile[].name "standalone" +rankprofile[].fef.property[].name "rankingExpression(myfeature).rankingScript" +rankprofile[].fef.property[].value "7 * attribute(num)" +rankprofile[].fef.property[].name "rankingExpression(fourtimessum@2b1138e8965e7ff5.67f1e87166cfef86).rankingScript" +rankprofile[].fef.property[].value "4 * (match + match)" +rankprofile[].fef.property[].name "rankingExpression(macro_with_dollar$).rankingScript" +rankprofile[].fef.property[].value "69" +rankprofile[].fef.property[].name "rankingExpression(anotherfeature).rankingScript" +rankprofile[].fef.property[].value "10 * rankingExpression(myfeature)" +rankprofile[].fef.property[].name "rankingExpression(yetanotherfeature).rankingScript" +rankprofile[].fef.property[].value "100 * rankingExpression(myfeature)" +rankprofile[].fef.property[].name "rankingExpression(fourtimessum).rankingScript" +rankprofile[].fef.property[].value "4 * (var1 + var2)" +rankprofile[].fef.property[].name "vespa.rank.firstphase" +rankprofile[].fef.property[].value "rankingExpression(firstphase)" +rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" +rankprofile[].fef.property[].value "match + fieldMatch(title) + rankingExpression(myfeature)" +rankprofile[].fef.property[].name "vespa.rank.secondphase" +rankprofile[].fef.property[].value "rankingExpression(secondphase)" +rankprofile[].fef.property[].name "rankingExpression(secondphase).rankingScript" +rankprofile[].fef.property[].value "rankingExpression(fourtimessum@2b1138e8965e7ff5.67f1e87166cfef86) + 0 * rankingExpression(macro_with_dollar$)" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "firstPhase" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "rankingExpression(myfeature)" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "rankingExpression(anotherfeature)" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "rankingExpression(yetanotherfeature)" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "rankingExpression(macro_with_dollar$)" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "rankingExpression(anotherfeature)" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "anotherfeature" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "rankingExpression(yetanotherfeature)" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "yetanotherfeature" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "rankingExpression(macro_with_dollar$)" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "macro_with_dollar$" +rankprofile[].name "constantsAndMacro" +rankprofile[].fef.property[].name "rankingExpression(c).rankingScript" +rankprofile[].fef.property[].value "attribute(num)" +rankprofile[].fef.property[].name "vespa.rank.firstphase" +rankprofile[].fef.property[].value "rankingExpression(firstphase)" +rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" +rankprofile[].fef.property[].value "attribute(num) * 2.0 + 3.0" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "firstPhase" +rankprofile[].name "doc" +rankprofile[].fef.property[].name "rankingExpression(myfeature).rankingScript" +rankprofile[].fef.property[].value "fieldMatch(title) + freshness(timestamp)" +rankprofile[].fef.property[].name "rankingExpression(otherfeature@6b0a229a66fcaa04).rankingScript" +rankprofile[].fef.property[].value "nativeRank(title,body)" +rankprofile[].fef.property[].name "rankingExpression(otherfeature).rankingScript" +rankprofile[].fef.property[].value "nativeRank(foo,body)" +rankprofile[].fef.property[].name "vespa.rank.firstphase" +rankprofile[].fef.property[].value "rankingExpression(firstphase)" +rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" +rankprofile[].fef.property[].value "rankingExpression(myfeature) * 10" +rankprofile[].fef.property[].name "vespa.rank.secondphase" +rankprofile[].fef.property[].value "rankingExpression(secondphase)" +rankprofile[].fef.property[].name "rankingExpression(secondphase).rankingScript" +rankprofile[].fef.property[].value "rankingExpression(otherfeature@6b0a229a66fcaa04) * rankingExpression(myfeature)" +rankprofile[].fef.property[].name "vespa.summary.feature" +rankprofile[].fef.property[].value "rankingExpression(myfeature)" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "rankingExpression(myfeature)" +rankprofile[].fef.property[].name "vespa.feature.rename" +rankprofile[].fef.property[].value "myfeature" diff --git a/config-model/src/test/derived/rankingmacros/rankingmacros.sd b/config-model/src/test/derived/rankingmacros/rankingmacros.sd new file mode 100644 index 00000000000..84598cb483a --- /dev/null +++ b/config-model/src/test/derived/rankingmacros/rankingmacros.sd @@ -0,0 +1,105 @@ +# Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +schema rankingmacros { + + document rankingmacros { + field title type string { + indexing: index + } + field timestamp type long { + indexing: attribute + } + field description type string { + indexing: index + } + field num type int { + indexing: attribute + } + field abstract type string { + indexing: index + } + field body type string { + indexing: index + } + field usstaticrank type string { + indexing: attribute + } + field boostmax type string { + indexing: index + } + field entitytitle type string { + indexing: index + } + } + + rank-profile standalone { + macro fourtimessum(var1, var2) { + expression: 4*(var1+var2) + } + macro myfeature() { + expression { + 7 * attribute(num) + } + } + macro anotherfeature() { + expression: 10*myfeature + } + macro yetanotherfeature() { + expression: 100*rankingExpression(myfeature) # legacy form + } + macro macro_with_dollar$() { # Not allowed + expression: 69 + } + first-phase { + expression: match + fieldMatch(title) + myfeature + } + second-phase { + expression: fourtimessum(match,match) + 0 * macro_with_dollar$ + } + summary-features { + firstPhase + rankingExpression(myfeature) + anotherfeature + yetanotherfeature + macro_with_dollar$ + } + } + + # Profile with macro and constants + rank-profile constantsAndMacro { + macro c() { + expression: attribute(num) + } + + constants { + a: 2 + b: 3 + } + + first-phase { + expression: attribute(num) * a + b + } + + summary-features { + firstPhase + } + } + + # The example in the docs + rank-profile doc inherits default { + macro myfeature() { + expression: fieldMatch(title) + freshness(timestamp) + } + macro otherfeature(foo) { + expression{ nativeRank(foo, body) } + } + + first-phase { + expression: myfeature * 10 + } + second-phase { + expression: otherfeature(title) * myfeature + } + summary-features: myfeature + } + +} diff --git a/config-model/src/test/java/com/yahoo/schema/derived/RankProfilesTestCase.java b/config-model/src/test/java/com/yahoo/schema/derived/RankProfilesTestCase.java index 5c24b32e275..a4c03291ce2 100644 --- a/config-model/src/test/java/com/yahoo/schema/derived/RankProfilesTestCase.java +++ b/config-model/src/test/java/com/yahoo/schema/derived/RankProfilesTestCase.java @@ -17,4 +17,9 @@ public class RankProfilesTestCase extends AbstractExportingTestCase { void testRankProfiles() throws IOException, ParseException { assertCorrectDeriving("rankprofiles", null, new TestProperties(), new TestableDeployLogger()); } + + @Test + void testMacrosInRankProfiles() throws IOException, ParseException { + assertCorrectDeriving("rankingmacros", null, new TestProperties(), new TestableDeployLogger()); + } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/search/NodeResourcesTuningTest.java b/config-model/src/test/java/com/yahoo/vespa/model/search/NodeResourcesTuningTest.java index a6d44d46dcb..9fe38512fc0 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/search/NodeResourcesTuningTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/search/NodeResourcesTuningTest.java @@ -183,6 +183,10 @@ public class NodeResourcesTuningTest { @Test public void require_that_concurrent_flush_threads_is_1_with_low_memory() { assertEquals(2, fromMemAndCpu(17, 9).flush().maxconcurrent()); + assertEquals(2, fromMemAndCpu(17, 64).flush().maxconcurrent()); // still capped by max + assertEquals(2, fromMemAndCpu(65, 8).flush().maxconcurrent()); // still capped by max + assertEquals(2, fromMemAndCpu(33, 8).flush().maxconcurrent()); + assertEquals(1, fromMemAndCpu(31, 8).flush().maxconcurrent()); assertEquals(1, fromMemAndCpu(15, 8).flush().maxconcurrent()); assertEquals(1, fromMemAndCpu(17, 8).flush().maxconcurrent()); assertEquals(1, fromMemAndCpu(15, 8).flush().maxconcurrent()); diff --git a/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java index ee885cdf43e..1df297c2bfc 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/search/test/DocumentDatabaseTestCase.java @@ -87,7 +87,7 @@ public class DocumentDatabaseTestCase { verifyConcurrency(nameAndModes, xmlTuning, expectedConcurrency, null); } - private void verifyConcurrency(List<DocType> nameAndModes, String xmlTuning, double expectedConcurrency, Double featureFlagConcurrency) { + private ProtonConfig getConfig(List<DocType> nameAndModes, String xmlTuning, Double featureFlagConcurrency) { TestProperties properties = new TestProperties(); if (featureFlagConcurrency != null) { properties.setFeedConcurrency(featureFlagConcurrency); @@ -95,10 +95,30 @@ public class DocumentDatabaseTestCase { var tester = new SchemaTester(); VespaModel model = tester.createModel(nameAndModes, xmlTuning, new DeployState.Builder().properties(properties)); ContentSearchCluster contentSearchCluster = model.getContentClusters().get("test").getSearch(); - ProtonConfig proton = tester.getProtonConfig(contentSearchCluster); + return tester.getProtonConfig(contentSearchCluster); + } + + private void verifyConcurrency(List<DocType> nameAndModes, String xmlTuning, double expectedConcurrency, Double featureFlagConcurrency) { + ProtonConfig proton = getConfig(nameAndModes, xmlTuning, featureFlagConcurrency); assertEquals(expectedConcurrency, proton.feeding().concurrency(), SMALL); } + private void verifyMaxflushedFollowsConcurrency(double concurrency, int maxFlushed) { + String feedTuning = "<feeding> <concurrency>" + concurrency +"</concurrency>" + "</feeding>\n"; + ProtonConfig proton = getConfig(List.of(DocType.create("a", "index")), feedTuning, null); + assertEquals(maxFlushed, proton.index().maxflushed()); + } + + @Test + public void verifyThatMaxFlushedFollowsConcurrency() { + verifyMaxflushedFollowsConcurrency(0.1, 2); + verifyMaxflushedFollowsConcurrency(0.50, 2); + verifyMaxflushedFollowsConcurrency(0.51, 3); + verifyMaxflushedFollowsConcurrency(0.75, 3); + verifyMaxflushedFollowsConcurrency(0.76, 4); + verifyMaxflushedFollowsConcurrency(1.0, 4); + } + private void verifyFeedNiceness(List<DocType> nameAndModes, Double expectedNiceness, Double featureFlagNiceness) { TestProperties properties = new TestProperties(); if (featureFlagNiceness != null) { diff --git a/container-search/src/main/java/com/yahoo/search/ranking/HitRescorer.java b/container-search/src/main/java/com/yahoo/search/ranking/HitRescorer.java index ebdbbb693f1..cce6b42d323 100644 --- a/container-search/src/main/java/com/yahoo/search/ranking/HitRescorer.java +++ b/container-search/src/main/java/com/yahoo/search/ranking/HitRescorer.java @@ -3,6 +3,7 @@ package com.yahoo.search.ranking; import com.yahoo.search.result.FeatureData; import com.yahoo.search.result.Hit; +import static com.yahoo.searchlib.rankingexpression.Reference.RANKING_EXPRESSION_WRAPPER; import java.util.function.Supplier; import java.util.logging.Logger; @@ -42,7 +43,7 @@ class HitRescorer { } } - private static final String RE_PREFIX = "rankingExpression("; + private static final String RE_PREFIX = RANKING_EXPRESSION_WRAPPER + "("; private static final String RE_SUFFIX = ")"; private static final int RE_PRE_LEN = RE_PREFIX.length(); private static final int RE_SUF_LEN = RE_SUFFIX.length(); diff --git a/container-search/src/main/java/com/yahoo/search/result/FeatureData.java b/container-search/src/main/java/com/yahoo/search/result/FeatureData.java index 421f19475a6..7e9fa3f748a 100644 --- a/container-search/src/main/java/com/yahoo/search/result/FeatureData.java +++ b/container-search/src/main/java/com/yahoo/search/result/FeatureData.java @@ -11,6 +11,7 @@ import com.yahoo.io.GrowableByteBuffer; import com.yahoo.tensor.Tensor; import com.yahoo.tensor.serialization.JsonFormat; import com.yahoo.tensor.serialization.TypedBinaryFormat; +import static com.yahoo.searchlib.rankingexpression.Reference.wrapInRankingExpression; import java.nio.charset.StandardCharsets; import java.util.Collections; @@ -144,7 +145,7 @@ public class FeatureData implements Inspectable, JsonProducer { if (featureValue.valid()) return featureValue; // Try to wrap by rankingExpression(name) - return value.field("rankingExpression(" + featureName + ")"); + return value.field(wrapInRankingExpression(featureName)); } /** Returns the names of the features available in this */ diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveBuckets.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveBuckets.java new file mode 100644 index 00000000000..62e341c674c --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveBuckets.java @@ -0,0 +1,34 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.archive; + +import java.util.HashSet; +import java.util.Set; + +/** + * @author freva + */ +public record ArchiveBuckets(Set<VespaManagedArchiveBucket> vespaManaged, + Set<TenantManagedArchiveBucket> tenantManaged) { + public static final ArchiveBuckets EMPTY = new ArchiveBuckets(Set.of(), Set.of()); + + public ArchiveBuckets(Set<VespaManagedArchiveBucket> vespaManaged, Set<TenantManagedArchiveBucket> tenantManaged) { + this.vespaManaged = Set.copyOf(vespaManaged); + this.tenantManaged = Set.copyOf(tenantManaged); + } + + /** Adds or replaces a VespaManagedArchive bucket with the given archive bucket */ + public ArchiveBuckets with(VespaManagedArchiveBucket vespaManagedArchiveBucket) { + Set<VespaManagedArchiveBucket> updated = new HashSet<>(vespaManaged); + updated.removeIf(bucket -> bucket.bucketName().equals(vespaManagedArchiveBucket.bucketName())); + updated.add(vespaManagedArchiveBucket); + return new ArchiveBuckets(updated, tenantManaged); + } + + /** Adds or replaces a TenantManagedArchive bucket with the given archive bucket */ + public ArchiveBuckets with(TenantManagedArchiveBucket tenantManagedArchiveBucket) { + Set<TenantManagedArchiveBucket> updated = new HashSet<>(tenantManaged); + updated.removeIf(bucket -> bucket.cloudAccount().equals(tenantManagedArchiveBucket.cloudAccount())); + updated.add(tenantManagedArchiveBucket); + return new ArchiveBuckets(vespaManaged, updated); + } +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java index 46e7fb48553..ed965f4331e 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java @@ -1,12 +1,14 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.archive; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.hosted.controller.tenant.ArchiveAccess; import java.net.URI; import java.util.Map; +import java.util.Optional; import java.util.Set; /** @@ -17,11 +19,13 @@ import java.util.Set; */ public interface ArchiveService { - ArchiveBucket createArchiveBucketFor(ZoneId zoneId); + VespaManagedArchiveBucket createArchiveBucketFor(ZoneId zoneId); - void updatePolicies(ZoneId zoneId, Set<ArchiveBucket> buckets, Map<TenantName,ArchiveAccess> authorizeAccessByTenantName); + void updatePolicies(ZoneId zoneId, Set<VespaManagedArchiveBucket> buckets, Map<TenantName,ArchiveAccess> authorizeAccessByTenantName); - boolean canAddTenantToBucket(ZoneId zoneId, ArchiveBucket bucket); + boolean canAddTenantToBucket(ZoneId zoneId, VespaManagedArchiveBucket bucket); - URI bucketURI(ZoneId zoneId, String bucketName, TenantName tenantName); + Optional<String> findEnclaveArchiveBucket(ZoneId zoneId, CloudAccount cloudAccount); + + URI bucketURI(ZoneId zoneId, String bucketName); } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java index a2847439ce7..7461d3aa47e 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java @@ -1,16 +1,18 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.archive; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.hosted.controller.tenant.ArchiveAccess; import java.net.URI; +import java.time.Clock; import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; -import java.util.TreeMap; /** * @author freva @@ -18,29 +20,54 @@ import java.util.TreeMap; */ public class MockArchiveService implements ArchiveService { - - public Set<ArchiveBucket> archiveBuckets = new HashSet<>(); + private final Map<ZoneId, Set<TenantManagedArchiveBucket>> tenantArchiveBucketsByZone = new HashMap<>(); + public Set<VespaManagedArchiveBucket> archiveBuckets = new HashSet<>(); public Map<TenantName, ArchiveAccess> authorizeAccessByTenantName = new HashMap<>(); + private final Clock clock; + + public MockArchiveService(Clock clock) { + this.clock = clock; + } @Override - public ArchiveBucket createArchiveBucketFor(ZoneId zoneId) { - return new ArchiveBucket("bucketName", "keyArn"); + public VespaManagedArchiveBucket createArchiveBucketFor(ZoneId zoneId) { + return new VespaManagedArchiveBucket("bucketName", "keyArn"); } @Override - public void updatePolicies(ZoneId zoneId, Set<ArchiveBucket> buckets, Map<TenantName, ArchiveAccess> authorizeAccessByTenantName) { + public void updatePolicies(ZoneId zoneId, Set<VespaManagedArchiveBucket> buckets, Map<TenantName, ArchiveAccess> authorizeAccessByTenantName) { this.archiveBuckets = new HashSet<>(buckets); this.authorizeAccessByTenantName = new HashMap<>(authorizeAccessByTenantName); } @Override - public boolean canAddTenantToBucket(ZoneId zoneId, ArchiveBucket bucket) { + public boolean canAddTenantToBucket(ZoneId zoneId, VespaManagedArchiveBucket bucket) { return bucket.tenants().size() < 5; } @Override - public URI bucketURI(ZoneId zoneId, String bucketName, TenantName tenantName) { - return URI.create(String.format("s3://%s/%s/", bucketName, tenantName.value())); + public Optional<String> findEnclaveArchiveBucket(ZoneId zoneId, CloudAccount cloudAccount) { + return tenantArchiveBucketsByZone.getOrDefault(zoneId, Set.of()).stream() + .filter(bucket -> bucket.cloudAccount().equals(cloudAccount)) + .findFirst() + .map(TenantManagedArchiveBucket::bucketName); + } + + @Override + public URI bucketURI(ZoneId zoneId, String bucketName) { + return URI.create(String.format("s3://%s/", bucketName)); + } + + + public void setEnclaveArchiveBucket(ZoneId zoneId, CloudAccount cloudAccount, String bucketName) { + removeEnclaveArchiveBucket(zoneId, cloudAccount); + tenantArchiveBucketsByZone.computeIfAbsent(zoneId, z -> new HashSet<>()) + .add(new TenantManagedArchiveBucket(bucketName, cloudAccount, clock.instant())); + } + + public void removeEnclaveArchiveBucket(ZoneId zoneId, CloudAccount cloudAccount) { + Optional.ofNullable(tenantArchiveBucketsByZone.get(zoneId)) + .ifPresent(set -> set.removeIf(bucket -> bucket.cloudAccount().equals(cloudAccount))); } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/TenantManagedArchiveBucket.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/TenantManagedArchiveBucket.java new file mode 100644 index 00000000000..80e9762f84b --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/TenantManagedArchiveBucket.java @@ -0,0 +1,15 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.archive; + +import com.yahoo.config.provision.CloudAccount; + +import java.time.Instant; + +/** + * Represents a cloud storage bucket (e.g. AWS S3 or Google Storage) used to store archive data - logs, heap/core dumps, etc. + * that is managed by the tenant directly. + * + * @author freva + */ +public record TenantManagedArchiveBucket(String bucketName, CloudAccount cloudAccount, Instant updatedAt) { +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveBucket.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/VespaManagedArchiveBucket.java index be3b87ddc5c..c80e9b3780d 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveBucket.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/VespaManagedArchiveBucket.java @@ -8,20 +8,21 @@ import java.util.Objects; import java.util.Set; /** - * Represents an S3 bucket used to store archive data - logs, heap/core dumps, etc. + * Represents a cloud storage bucket (e.g. AWS S3 or Google Storage) used to store archive data - logs, heap/core dumps, etc. + * that is managed by the Vespa controller. * * @author andreer */ -public class ArchiveBucket { +public class VespaManagedArchiveBucket { private final String bucketName; private final String keyArn; private final Set<TenantName> tenants; - public ArchiveBucket(String bucketName, String keyArn) { + public VespaManagedArchiveBucket(String bucketName, String keyArn) { this(bucketName, keyArn, Set.of()); } - private ArchiveBucket(String bucketName, String keyArn, Set<TenantName> tenants) { + private VespaManagedArchiveBucket(String bucketName, String keyArn, Set<TenantName> tenants) { this.bucketName = bucketName; this.keyArn = keyArn; this.tenants = Set.copyOf(tenants); @@ -39,19 +40,19 @@ public class ArchiveBucket { return tenants; } - public ArchiveBucket withTenant(TenantName tenant) { + public VespaManagedArchiveBucket withTenant(TenantName tenant) { return withTenants(Set.of(tenant)); } - public ArchiveBucket withTenants(Set<TenantName> tenants) { - return new ArchiveBucket(bucketName, keyArn, Sets.union(this.tenants, tenants)); + public VespaManagedArchiveBucket withTenants(Set<TenantName> tenants) { + return new VespaManagedArchiveBucket(bucketName, keyArn, Sets.union(this.tenants, tenants)); } @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; - ArchiveBucket that = (ArchiveBucket) o; + VespaManagedArchiveBucket that = (VespaManagedArchiveBucket) o; return bucketName.equals(that.bucketName) && keyArn.equals(that.keyArn) && tenants.equals(that.tenants); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java index ac32fe5799d..962bd144a21 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java @@ -1,18 +1,22 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.archive; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.hosted.controller.Controller; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBuckets; import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveService; +import com.yahoo.vespa.hosted.controller.api.integration.archive.TenantManagedArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.VespaManagedArchiveBucket; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import java.net.URI; -import java.util.HashSet; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; import java.util.Map; import java.util.Optional; -import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; @@ -23,81 +27,100 @@ import java.util.stream.Collectors; */ public class CuratorArchiveBucketDb { + private static final Duration ENCLAVE_BUCKET_CACHE_LIFETIME = Duration.ofMinutes(60); + /** * Archive URIs are often requested because they are returned in /application/v4 API. Since they * never change, it's safe to cache them and only update on misses */ private final Map<ZoneId, Map<TenantName, String>> archiveUriCache = new ConcurrentHashMap<>(); + private final Map<ZoneId, Map<CloudAccount, TenantManagedArchiveBucket>> tenantArchiveCache = new ConcurrentHashMap<>(); private final ArchiveService archiveService; private final CuratorDb curatorDb; + private final Clock clock; public CuratorArchiveBucketDb(Controller controller) { this.archiveService = controller.serviceRegistry().archiveService(); this.curatorDb = controller.curator(); + this.clock = controller.clock(); } public Optional<URI> archiveUriFor(ZoneId zoneId, TenantName tenant, boolean createIfMissing) { return getBucketNameFromCache(zoneId, tenant) - .or(() -> findAndUpdateArchiveUriCache(zoneId, tenant, buckets(zoneId))) .or(() -> createIfMissing ? Optional.of(assignToBucket(zoneId, tenant)) : Optional.empty()) - .map(bucketName -> archiveService.bucketURI(zoneId, bucketName, tenant)); + .map(bucketName -> archiveService.bucketURI(zoneId, bucketName)); + } + + public Optional<URI> archiveUriFor(ZoneId zoneId, CloudAccount account, boolean searchIfMissing) { + Instant updatedAfter = searchIfMissing ? clock.instant().minus(ENCLAVE_BUCKET_CACHE_LIFETIME) : Instant.MIN; + return getBucketNameFromCache(zoneId, account, updatedAfter) + .or(() -> { + if (!searchIfMissing) return Optional.empty(); + try (var lock = curatorDb.lockArchiveBuckets(zoneId)) { + ArchiveBuckets archiveBuckets = buckets(zoneId); + updateArchiveUriCache(zoneId, archiveBuckets); + + return getBucketNameFromCache(zoneId, account, updatedAfter) + .or(() -> archiveService.findEnclaveArchiveBucket(zoneId, account) + .map(bucketName -> { + var bucket = new TenantManagedArchiveBucket(bucketName, account, clock.instant()); + ArchiveBuckets updated = archiveBuckets.with(bucket); + curatorDb.writeArchiveBuckets(zoneId, updated); + updateArchiveUriCache(zoneId, updated); + return bucket; + })); + } + }) + .map(TenantManagedArchiveBucket::bucketName) + .map(bucketName -> archiveService.bucketURI(zoneId, bucketName)); } private String assignToBucket(ZoneId zoneId, TenantName tenant) { try (var lock = curatorDb.lockArchiveBuckets(zoneId)) { - Set<ArchiveBucket> zoneBuckets = new HashSet<>(buckets(zoneId)); + ArchiveBuckets archiveBuckets = buckets(zoneId); + updateArchiveUriCache(zoneId, archiveBuckets); - return findAndUpdateArchiveUriCache(zoneId, tenant, zoneBuckets) // Some other thread might have assigned it before we grabbed the lock + return getBucketNameFromCache(zoneId, tenant) // Some other thread might have assigned it before we grabbed the lock .orElseGet(() -> { // If not, find an existing bucket with space - Optional<ArchiveBucket> unfilledBucket = zoneBuckets.stream() + VespaManagedArchiveBucket bucketToAssignTo = archiveBuckets.vespaManaged().stream() .filter(bucket -> archiveService.canAddTenantToBucket(zoneId, bucket)) - .findAny(); - - // And place the tenant in that bucket. - if (unfilledBucket.isPresent()) { - var unfilled = unfilledBucket.get(); - - zoneBuckets.remove(unfilled); - zoneBuckets.add(unfilled.withTenant(tenant)); - curatorDb.writeArchiveBuckets(zoneId, zoneBuckets); + .findAny() + // Or create a new one + .orElseGet(() -> archiveService.createArchiveBucketFor(zoneId)); - return unfilled.bucketName(); - } + ArchiveBuckets updated = archiveBuckets.with(bucketToAssignTo.withTenant(tenant)); + curatorDb.writeArchiveBuckets(zoneId, updated); + updateArchiveUriCache(zoneId, updated); - // We'll have to create a new bucket - var newBucket = archiveService.createArchiveBucketFor(zoneId).withTenant(tenant); - zoneBuckets.add(newBucket); - curatorDb.writeArchiveBuckets(zoneId, zoneBuckets); - updateArchiveUriCache(zoneId, zoneBuckets); - return newBucket.bucketName(); + return bucketToAssignTo.bucketName(); }); } } - public Set<ArchiveBucket> buckets(ZoneId zoneId) { + public ArchiveBuckets buckets(ZoneId zoneId) { return curatorDb.readArchiveBuckets(zoneId); } - private Optional<String> findAndUpdateArchiveUriCache(ZoneId zoneId, TenantName tenant, Set<ArchiveBucket> zoneBuckets) { - Optional<String> bucketName = zoneBuckets.stream() - .filter(bucket -> bucket.tenants().contains(tenant)) - .findAny() - .map(ArchiveBucket::bucketName); - if (bucketName.isPresent()) updateArchiveUriCache(zoneId, zoneBuckets); - return bucketName; - } - private Optional<String> getBucketNameFromCache(ZoneId zoneId, TenantName tenantName) { return Optional.ofNullable(archiveUriCache.get(zoneId)).map(map -> map.get(tenantName)); } - private void updateArchiveUriCache(ZoneId zoneId, Set<ArchiveBucket> zoneBuckets) { - Map<TenantName, String> bucketNameByTenant = zoneBuckets.stream() - .flatMap(bucket -> bucket.tenants().stream() - .map(tenant -> Map.entry(tenant, bucket.bucketName()))) + private Optional<TenantManagedArchiveBucket> getBucketNameFromCache(ZoneId zoneId, CloudAccount cloudAccount, Instant updatedAfter) { + return Optional.ofNullable(tenantArchiveCache.get(zoneId)) + .map(map -> map.get(cloudAccount)) + .filter(bucket -> bucket.updatedAt().isAfter(updatedAfter)); + } + + private void updateArchiveUriCache(ZoneId zoneId, ArchiveBuckets archiveBuckets) { + Map<TenantName, String> bucketNameByTenant = archiveBuckets.vespaManaged().stream() + .flatMap(bucket -> bucket.tenants().stream().map(tenant -> Map.entry(tenant, bucket.bucketName()))) .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue)); archiveUriCache.put(zoneId, bucketNameByTenant); + + Map<CloudAccount, TenantManagedArchiveBucket> bucketByAccount = archiveBuckets.tenantManaged().stream() + .collect(Collectors.toUnmodifiableMap(TenantManagedArchiveBucket::cloudAccount, bucket -> bucket)); + tenantArchiveCache.put(zoneId, bucketByAccount); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java index eed4fd0245d..b2ed0941c8e 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java @@ -1,12 +1,10 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; -import com.google.common.collect.Maps; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.controller.Controller; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveService; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; import com.yahoo.vespa.hosted.controller.archive.CuratorArchiveBucketDb; @@ -17,11 +15,8 @@ import com.yahoo.vespa.hosted.controller.tenant.Tenant; import java.time.Duration; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.stream.Collectors; -import static java.util.stream.Collectors.groupingBy; - /** * Update archive access permissions with roles from tenants * @@ -48,7 +43,7 @@ public class ArchiveAccessMaintainer extends ControllerMaintainer { protected double maintain() { // Count buckets - so we can alert if we get close to the AWS account limit of 1000 zoneRegistry.zonesIncludingSystem().all().zones().forEach(z -> - metric.set(bucketCountMetricName, archiveBucketDb.buckets(z.getVirtualId()).size(), + metric.set(bucketCountMetricName, archiveBucketDb.buckets(z.getVirtualId()).vespaManaged().size(), metric.createContext(Map.of( "zone", z.getVirtualId().value(), "cloud", z.getCloudName().value())))); @@ -57,7 +52,7 @@ public class ArchiveAccessMaintainer extends ControllerMaintainer { ZoneId zoneId = z.getVirtualId(); try { var tenantArchiveAccessRoles = cloudTenantArchiveExternalAccessRoles(); - var buckets = archiveBucketDb.buckets(zoneId); + var buckets = archiveBucketDb.buckets(zoneId).vespaManaged(); archiveService.updatePolicies(zoneId, buckets, tenantArchiveAccessRoles); } catch (Exception e) { throw new RuntimeException("Failed to maintain archive access in " + zoneId.value(), e); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdater.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdater.java index ddb1365f2de..1083e545b33 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdater.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdater.java @@ -1,10 +1,12 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.hosted.controller.ApplicationController; import com.yahoo.vespa.hosted.controller.Controller; +import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveUriUpdate; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ArchiveUris; import com.yahoo.vespa.hosted.controller.api.integration.configserver.NodeRepository; @@ -18,6 +20,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.logging.Level; +import java.util.stream.Stream; /** * Updates archive URIs for tenants in all zones. @@ -42,14 +45,19 @@ public class ArchiveUriUpdater extends ControllerMaintainer { @Override protected double maintain() { Map<ZoneId, Set<TenantName>> tenantsByZone = new HashMap<>(); + Map<ZoneId, Set<CloudAccount>> accountsByZone = new HashMap<>(); - controller().zoneRegistry().zonesIncludingSystem().reachable().zones().forEach( - z -> tenantsByZone.put(z.getVirtualId(), new HashSet<>(INFRASTRUCTURE_TENANTS))); + controller().zoneRegistry().zonesIncludingSystem().reachable().zones().forEach(zone -> { + tenantsByZone.put(zone.getVirtualId(), new HashSet<>(INFRASTRUCTURE_TENANTS)); + accountsByZone.put(zone.getVirtualId(), new HashSet<>()); + }); for (var application : applications.asList()) { for (var instance : application.instances().values()) { for (var deployment : instance.deployments().values()) { tenantsByZone.get(deployment.zone()).add(instance.id().tenant()); + applications.decideCloudAccountOf(new DeploymentId(instance.id(), deployment.zone()), application.deploymentSpec()) + .ifPresent(account -> accountsByZone.get(deployment.zone()).add(account)); } } } @@ -59,17 +67,29 @@ public class ArchiveUriUpdater extends ControllerMaintainer { try { ArchiveUris zoneArchiveUris = nodeRepository.getArchiveUris(zone); - for (TenantName tenant : tenantsByZone.get(zone)) { - archiveBucketDb.archiveUriFor(zone, tenant, true) - .filter(uri -> !uri.equals(zoneArchiveUris.tenantArchiveUris().get(tenant))) - .ifPresent(uri -> nodeRepository.updateArchiveUri(zone, ArchiveUriUpdate.setArchiveUriFor(tenant, uri))); - } - - zoneArchiveUris.tenantArchiveUris().keySet().stream() - .filter(tenant -> !tenantsByZone.get(zone).contains(tenant)) - .forEach(tenant -> nodeRepository.updateArchiveUri(zone, ArchiveUriUpdate.deleteArchiveUriFor(tenant))); - - // TODO (freva): Update account archive URIs + Stream.of( + // Tenant URIs that need to be added or updated + tenantsByZone.get(zone).stream() + .flatMap(tenant -> archiveBucketDb.archiveUriFor(zone, tenant, true) + .filter(uri -> !uri.equals(zoneArchiveUris.tenantArchiveUris().get(tenant))) + .map(uri -> ArchiveUriUpdate.setArchiveUriFor(tenant, uri)) + .stream()), + // Account URIs that need to be added or updated + accountsByZone.get(zone).stream() + .flatMap(account -> archiveBucketDb.archiveUriFor(zone, account, true) + .filter(uri -> !uri.equals(zoneArchiveUris.accountArchiveUris().get(account))) + .map(uri -> ArchiveUriUpdate.setArchiveUriFor(account, uri)) + .stream()), + // Tenant URIs that need to be deleted + zoneArchiveUris.tenantArchiveUris().keySet().stream() + .filter(tenant -> !tenantsByZone.get(zone).contains(tenant)) + .map(ArchiveUriUpdate::deleteArchiveUriFor), + // Account URIs that need to be deleted + zoneArchiveUris.accountArchiveUris().keySet().stream() + .filter(account -> !accountsByZone.get(zone).contains(account)) + .map(ArchiveUriUpdate::deleteArchiveUriFor)) + .flatMap(s -> s) + .forEach(update -> nodeRepository.updateArchiveUri(zone, update)); } catch (Exception e) { log.log(Level.WARNING, "Failed to update archive URI in " + zone + ". Retrying in " + interval() + ". Error: " + Exceptions.toMessageString(e)); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializer.java index a4c7c50085c..f40193510ce 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializer.java @@ -1,12 +1,15 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.persistence; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.TenantName; import com.yahoo.slime.Cursor; import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; import com.yahoo.slime.SlimeUtils; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBuckets; +import com.yahoo.vespa.hosted.controller.api.integration.archive.TenantManagedArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.VespaManagedArchiveBucket; import java.util.Set; import java.util.stream.Collectors; @@ -25,46 +28,64 @@ public class ArchiveBucketsSerializer { // - REMOVING FIELDS: Stop reading the field first. Stop writing it on a later version. // - CHANGING THE FORMAT OF A FIELD: Don't do it bro. - private final static String bucketsFieldName = "buckets"; + private final static String vespaManagedBucketsFieldName = "buckets"; + private final static String tenantManagedBucketsFieldName = "tenantManagedBuckets"; private final static String bucketNameFieldName = "bucketName"; private final static String keyArnFieldName = "keyArn"; private final static String tenantsFieldName = "tenantIds"; + private final static String accountFieldName = "account"; + private final static String updatedAtFieldName = "updatedAt"; - public static Slime toSlime(Set<ArchiveBucket> archiveBuckets) { + public static Slime toSlime(ArchiveBuckets archiveBuckets) { Slime slime = new Slime(); Cursor rootObject = slime.setObject(); - Cursor bucketsArray = rootObject.setArray(bucketsFieldName); - archiveBuckets.forEach(bucket -> { - Cursor cursor = bucketsArray.addObject(); - cursor.setString(bucketNameFieldName, bucket.bucketName()); - cursor.setString(keyArnFieldName, bucket.keyArn()); - Cursor tenants = cursor.setArray(tenantsFieldName); - bucket.tenants().forEach(tenantName -> tenants.addString(tenantName.value())); - } - ); + Cursor vespaBucketsArray = rootObject.setArray(vespaManagedBucketsFieldName); + archiveBuckets.vespaManaged().forEach(bucket -> { + Cursor cursor = vespaBucketsArray.addObject(); + cursor.setString(bucketNameFieldName, bucket.bucketName()); + cursor.setString(keyArnFieldName, bucket.keyArn()); + Cursor tenants = cursor.setArray(tenantsFieldName); + bucket.tenants().forEach(tenantName -> tenants.addString(tenantName.value())); + }); + + Cursor tenantBucketsArray = rootObject.setArray(tenantManagedBucketsFieldName); + archiveBuckets.tenantManaged().forEach(bucket -> { + Cursor cursor = tenantBucketsArray.addObject(); + cursor.setString(bucketNameFieldName, bucket.bucketName()); + cursor.setString(accountFieldName, bucket.cloudAccount().value()); + cursor.setLong(updatedAtFieldName, bucket.updatedAt().toEpochMilli()); + }); return slime; } - public static Set<ArchiveBucket> fromSlime(Inspector inspector) { - return SlimeUtils.entriesStream(inspector.field(bucketsFieldName)) - .map(ArchiveBucketsSerializer::fromInspector) - .collect(Collectors.toUnmodifiableSet()); + public static ArchiveBuckets fromSlime(Slime slime) { + Inspector inspector = slime.get(); + return new ArchiveBuckets( + SlimeUtils.entriesStream(inspector.field(vespaManagedBucketsFieldName)) + .map(ArchiveBucketsSerializer::vespaManagedArchiveBucketFromInspector) + .collect(Collectors.toUnmodifiableSet()), + SlimeUtils.entriesStream(inspector.field(tenantManagedBucketsFieldName)) + .map(ArchiveBucketsSerializer::tenantManagedArchiveBucketFromInspector) + .collect(Collectors.toUnmodifiableSet())); } - private static ArchiveBucket fromInspector(Inspector inspector) { + private static VespaManagedArchiveBucket vespaManagedArchiveBucketFromInspector(Inspector inspector) { Set<TenantName> tenants = SlimeUtils.entriesStream(inspector.field(tenantsFieldName)) .map(i -> TenantName.from(i.asString())) .collect(Collectors.toUnmodifiableSet()); - return new ArchiveBucket( + return new VespaManagedArchiveBucket( inspector.field(bucketNameFieldName).asString(), inspector.field(keyArnFieldName).asString()) .withTenants(tenants); } - public static Set<ArchiveBucket> fromJsonString(String zkData) { - return fromSlime(SlimeUtils.jsonToSlime(zkData).get()); + private static TenantManagedArchiveBucket tenantManagedArchiveBucketFromInspector(Inspector inspector) { + return new TenantManagedArchiveBucket( + inspector.field(bucketNameFieldName).asString(), + CloudAccount.from(inspector.field(accountFieldName).asString()), + SlimeUtils.instant(inspector.field(updatedAtFieldName))); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java index f4980073d6c..d4e6d7af4b4 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java @@ -19,7 +19,7 @@ import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.api.identifiers.ClusterId; import com.yahoo.vespa.hosted.controller.api.identifiers.ControllerVersion; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBuckets; import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; @@ -632,12 +632,12 @@ public class CuratorDb { // -------------- Archive buckets ----------------------------------------- - public Set<ArchiveBucket> readArchiveBuckets(ZoneId zoneId) { - return curator.getData(archiveBucketsPath(zoneId)).map(String::new).map(ArchiveBucketsSerializer::fromJsonString) - .orElseGet(Set::of); + public ArchiveBuckets readArchiveBuckets(ZoneId zoneId) { + return readSlime(archiveBucketsPath(zoneId)).map(ArchiveBucketsSerializer::fromSlime) + .orElse(ArchiveBuckets.EMPTY); } - public void writeArchiveBuckets(ZoneId zoneid, Set<ArchiveBucket> archiveBuckets) { + public void writeArchiveBuckets(ZoneId zoneid, ArchiveBuckets archiveBuckets) { curator.set(archiveBucketsPath(zoneid), asJson(ArchiveBucketsSerializer.toSlime(archiveBuckets))); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDbTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDbTest.java index 081056e5184..e5571c0e0ca 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDbTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDbTest.java @@ -1,15 +1,21 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.archive; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.test.ManualClock; +import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.ControllerTester; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBuckets; +import com.yahoo.vespa.hosted.controller.api.integration.archive.MockArchiveService; +import com.yahoo.vespa.hosted.controller.api.integration.archive.VespaManagedArchiveBucket; import org.apache.curator.shaded.com.google.common.collect.Streams; import org.junit.jupiter.api.Test; import java.net.URI; +import java.time.Duration; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -21,27 +27,27 @@ import static org.junit.jupiter.api.Assertions.assertEquals; public class CuratorArchiveBucketDbTest { @Test - void archiveUriFor() { + void archiveUriForTenant() { ControllerTester tester = new ControllerTester(SystemName.Public); CuratorArchiveBucketDb bucketDb = new CuratorArchiveBucketDb(tester.controller()); tester.curator().writeArchiveBuckets(ZoneId.defaultId(), - Set.of(new ArchiveBucket("existingBucket", "keyArn").withTenant(TenantName.defaultName()))); + ArchiveBuckets.EMPTY.with(new VespaManagedArchiveBucket("existingBucket", "keyArn").withTenant(TenantName.defaultName()))); // Finds existing bucket in db - assertEquals(Optional.of(URI.create("s3://existingBucket/default/")), bucketDb.archiveUriFor(ZoneId.defaultId(), TenantName.defaultName(), true)); + assertEquals(Optional.of(URI.create("s3://existingBucket/")), bucketDb.archiveUriFor(ZoneId.defaultId(), TenantName.defaultName(), true)); // Assigns to existing bucket while there is space IntStream.range(0, 4).forEach(i -> assertEquals( - Optional.of(URI.create("s3://existingBucket/tenant" + i + "/")), bucketDb + Optional.of(URI.create("s3://existingBucket/")), bucketDb .archiveUriFor(ZoneId.defaultId(), TenantName.from("tenant" + i), true))); // Creates new bucket when existing buckets are full - assertEquals(Optional.of(URI.create("s3://bucketName/lastDrop/")), bucketDb.archiveUriFor(ZoneId.defaultId(), TenantName.from("lastDrop"), true)); + assertEquals(Optional.of(URI.create("s3://bucketName/")), bucketDb.archiveUriFor(ZoneId.defaultId(), TenantName.from("lastDrop"), true)); // Creates new bucket when there are no existing buckets in zone - assertEquals(Optional.of(URI.create("s3://bucketName/firstInZone/")), bucketDb.archiveUriFor(ZoneId.from("prod.us-east-3"), TenantName.from("firstInZone"), true)); + assertEquals(Optional.of(URI.create("s3://bucketName/")), bucketDb.archiveUriFor(ZoneId.from("prod.us-east-3"), TenantName.from("firstInZone"), true)); // Does not create bucket if not required assertEquals(Optional.empty(), bucketDb.archiveUriFor(ZoneId.from("prod.us-east-3"), TenantName.from("newTenant"), false)); @@ -50,11 +56,36 @@ public class CuratorArchiveBucketDbTest { Set<TenantName> existingBucketTenants = Streams.concat(Stream.of(TenantName.defaultName()), IntStream.range(0, 4).mapToObj(i -> TenantName.from("tenant" + i))).collect(Collectors.toUnmodifiableSet()); assertEquals( Set.of( - new ArchiveBucket("existingBucket", "keyArn").withTenants(existingBucketTenants), - new ArchiveBucket("bucketName", "keyArn").withTenant(TenantName.from("lastDrop"))), - bucketDb.buckets(ZoneId.defaultId())); + new VespaManagedArchiveBucket("existingBucket", "keyArn").withTenants(existingBucketTenants), + new VespaManagedArchiveBucket("bucketName", "keyArn").withTenant(TenantName.from("lastDrop"))), + bucketDb.buckets(ZoneId.defaultId()).vespaManaged()); assertEquals( - Set.of(new ArchiveBucket("bucketName", "keyArn").withTenant(TenantName.from("firstInZone"))), - bucketDb.buckets(ZoneId.from("prod.us-east-3"))); + Set.of(new VespaManagedArchiveBucket("bucketName", "keyArn").withTenant(TenantName.from("firstInZone"))), + bucketDb.buckets(ZoneId.from("prod.us-east-3")).vespaManaged()); + } + + @Test + void archiveUriForAccount() { + Controller controller = new ControllerTester(SystemName.Public).controller(); + CuratorArchiveBucketDb bucketDb = new CuratorArchiveBucketDb(controller); + MockArchiveService service = (MockArchiveService) controller.serviceRegistry().archiveService(); + ManualClock clock = (ManualClock) controller.clock(); + + CloudAccount acc1 = CloudAccount.from("001122334455"); + ZoneId z1 = ZoneId.from("prod.us-east-3"); + + assertEquals(Optional.empty(), bucketDb.archiveUriFor(z1, acc1, true)); // Initially not set + service.setEnclaveArchiveBucket(z1, acc1, "bucket-1"); + assertEquals(Optional.empty(), bucketDb.archiveUriFor(z1, acc1, false)); + assertEquals(Optional.of(URI.create("s3://bucket-1/")), bucketDb.archiveUriFor(z1, acc1, true)); + assertEquals(Optional.of(URI.create("s3://bucket-1/")), bucketDb.archiveUriFor(z1, acc1, false)); + + service.setEnclaveArchiveBucket(z1, acc1, "bucket-2"); + assertEquals(Optional.of(URI.create("s3://bucket-1/")), bucketDb.archiveUriFor(z1, acc1, true)); // Returns old value even with search + + clock.advance(Duration.ofMinutes(61)); // After expiry the cache is expired, new search is performed + assertEquals(Optional.of(URI.create("s3://bucket-1/")), bucketDb.archiveUriFor(z1, acc1, false)); // When requesting without search, return previous value even if expired + assertEquals(Optional.of(URI.create("s3://bucket-2/")), bucketDb.archiveUriFor(z1, acc1, true)); + assertEquals(Optional.of(URI.create("s3://bucket-2/")), bucketDb.archiveUriFor(z1, acc1, false)); } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java index be257daa211..998b371bbf1 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java @@ -37,11 +37,11 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.MockIssueH import com.yahoo.vespa.hosted.controller.api.integration.resource.CostReportConsumerMock; import com.yahoo.vespa.hosted.controller.api.integration.resource.ResourceDatabaseClient; import com.yahoo.vespa.hosted.controller.api.integration.resource.ResourceDatabaseClientMock; +import com.yahoo.vespa.hosted.controller.api.integration.secrets.EndpointSecretManager; import com.yahoo.vespa.hosted.controller.api.integration.secrets.GcpSecretStore; -import com.yahoo.vespa.hosted.controller.api.integration.secrets.NoopGcpSecretStore; import com.yahoo.vespa.hosted.controller.api.integration.secrets.NoopEndpointSecretManager; +import com.yahoo.vespa.hosted.controller.api.integration.secrets.NoopGcpSecretStore; import com.yahoo.vespa.hosted.controller.api.integration.secrets.NoopTenantSecretService; -import com.yahoo.vespa.hosted.controller.api.integration.secrets.EndpointSecretManager; import com.yahoo.vespa.hosted.controller.api.integration.stubs.DummyOwnershipIssues; import com.yahoo.vespa.hosted.controller.api.integration.stubs.DummySystemMonitor; import com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingDeploymentIssues; @@ -89,7 +89,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg private final ArtifactRegistryMock containerRegistry = new ArtifactRegistryMock(); private final NoopTenantSecretService tenantSecretService = new NoopTenantSecretService(); private final NoopEndpointSecretManager secretManager = new NoopEndpointSecretManager(); - private final ArchiveService archiveService = new MockArchiveService(); + private final ArchiveService archiveService = new MockArchiveService(clock); private final MockChangeRequestClient changeRequestClient = new MockChangeRequestClient(); private final AccessControlService accessControlService = new MockAccessControlService(); private final HorizonClient horizonClient = new MockHorizonClient(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java index 38ff9967ef6..de186109784 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java @@ -266,7 +266,7 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry @Override public boolean hasZone(ZoneId zoneId, CloudAccount cloudAccount) { - return hasZone(zoneId) && cloudAccountZones.getOrDefault(cloudAccount, Set.of()).contains(zoneId); + return hasZone(zoneId) && (system.isPublic() || cloudAccountZones.getOrDefault(cloudAccount, Set.of()).contains(zoneId)); } @Override diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainerTest.java index c10b77d853a..0490a9bdcc5 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainerTest.java @@ -7,7 +7,6 @@ import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.jdisc.test.MockMetric; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.LockedTenant; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; import com.yahoo.vespa.hosted.controller.api.integration.archive.MockArchiveService; import com.yahoo.vespa.hosted.controller.tenant.ArchiveAccess; import com.yahoo.vespa.hosted.controller.tenant.Tenant; @@ -15,8 +14,6 @@ import org.junit.jupiter.api.Test; import java.time.Duration; import java.util.Map; -import java.util.Optional; -import java.util.Set; import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -37,7 +34,6 @@ public class ArchiveAccessMaintainerTest { ZoneId testZone = ZoneId.from("prod.aws-us-east-1c"); tester.controller().archiveBucketDb().archiveUriFor(testZone, tenant1, true); - var testBucket = new ArchiveBucket("bucketName", "keyArn").withTenant(tenant1); MockArchiveService archiveService = (MockArchiveService) tester.controller().serviceRegistry().archiveService(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdaterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdaterTest.java index de62a2fc48c..d6b59ef860f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdaterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveUriUpdaterTest.java @@ -1,23 +1,28 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.vespa.flags.InMemoryFlagSource; +import com.yahoo.vespa.flags.PermanentFlags; import com.yahoo.vespa.hosted.controller.ControllerTester; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBuckets; import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveUriUpdate; +import com.yahoo.vespa.hosted.controller.api.integration.archive.MockArchiveService; +import com.yahoo.vespa.hosted.controller.api.integration.archive.VespaManagedArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.configserver.ArchiveUris; import com.yahoo.vespa.hosted.controller.api.integration.configserver.NodeRepository; -import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.application.SystemApplication; -import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage; +import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; import com.yahoo.vespa.hosted.controller.deployment.DeploymentContext; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import org.junit.jupiter.api.Test; import java.net.URI; import java.time.Duration; -import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.stream.Collectors; @@ -36,42 +41,49 @@ public class ArchiveUriUpdaterTest { var tenant1 = TenantName.from("tenant1"); var tenant2 = TenantName.from("tenant2"); + var account1 = CloudAccount.from("001122334455"); var tenantInfra = SystemApplication.TENANT; var application = tester.newDeploymentContext(tenant1.value(), "app1", "instance1"); ZoneId zone = ZoneId.from("prod", "aws-us-east-1c"); // Initially we should only is the bucket for hosted-vespa tenant updater.maintain(); - assertArchiveUris(Map.of(TenantName.from("hosted-vespa"), "s3://bucketName/hosted-vespa/"), zone); - assertArchiveUris(Map.of(TenantName.from("hosted-vespa"), "s3://bucketName/hosted-vespa/"), ZoneId.from("prod", "controller")); + assertArchiveUris(zone, Map.of(TenantName.from("hosted-vespa"), "s3://bucketName/"), Map.of()); + assertArchiveUris(ZoneId.from("prod", "controller"), Map.of(TenantName.from("hosted-vespa"), "s3://bucketName/"), Map.of()); // Archive service now has URI for tenant1, but tenant1 is not deployed in zone setBucketNameInService(Map.of(tenant1, "uri-1"), zone); + setAccountBucketNameInService(zone, account1, "bkt-1"); updater.maintain(); - assertArchiveUris(Map.of(TenantName.from("hosted-vespa"), "s3://bucketName/hosted-vespa/"), zone); + assertArchiveUris(zone, Map.of(TenantName.from("hosted-vespa"), "s3://bucketName/"), Map.of()); - deploy(application, zone); + ((InMemoryFlagSource) tester.controller().flagSource()) + .withListFlag(PermanentFlags.CLOUD_ACCOUNTS.id(), List.of(account1.value()), String.class); + deploy(application, zone, account1); updater.maintain(); - assertArchiveUris(Map.of(tenant1, "s3://uri-1/tenant1/", tenantInfra, "s3://bucketName/hosted-vespa/"), zone); + assertArchiveUris(zone, Map.of(tenant1, "s3://uri-1/", tenantInfra, "s3://bucketName/"), Map.of(account1, "s3://bkt-1/")); // URI for tenant1 should be updated and removed for tenant2 setArchiveUriInNodeRepo(Map.of(tenant1, "wrong-uri", tenant2, "uri-2"), zone); updater.maintain(); - assertArchiveUris(Map.of(tenant1, "s3://uri-1/tenant1/", tenantInfra, "s3://bucketName/hosted-vespa/"), zone); + assertArchiveUris(zone, Map.of(tenant1, "s3://uri-1/", tenantInfra, "s3://bucketName/"), Map.of(account1, "s3://bkt-1/")); } - private void assertArchiveUris(Map<TenantName, String> expectedUris, ZoneId zone) { - Map<TenantName, String> actualUris = tester.controller().serviceRegistry().configServer().nodeRepository() - .getArchiveUris(zone).tenantArchiveUris().entrySet().stream() - .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString())); - assertEquals(expectedUris, actualUris); + private void assertArchiveUris(ZoneId zone, Map<TenantName, String> expectedTenantUris, Map<CloudAccount, String> expectedAccountUris) { + ArchiveUris archiveUris = tester.controller().serviceRegistry().configServer().nodeRepository().getArchiveUris(zone); + assertEquals(expectedTenantUris, archiveUris.tenantArchiveUris().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString()))); + assertEquals(expectedAccountUris, archiveUris.accountArchiveUris().entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString()))); } private void setBucketNameInService(Map<TenantName, String> bucketNames, ZoneId zone) { - var archiveBuckets = new LinkedHashSet<>(tester.controller().curator().readArchiveBuckets(zone)); - bucketNames.forEach((tenantName, bucketName) -> - archiveBuckets.add(new ArchiveBucket(bucketName, "keyArn").withTenant(tenantName))); - tester.controller().curator().writeArchiveBuckets(zone, archiveBuckets); + ArchiveBuckets buckets = tester.controller().curator().readArchiveBuckets(zone); + for (var entry : bucketNames.entrySet()) + buckets = buckets.with(new VespaManagedArchiveBucket(entry.getValue(), "keyArn").withTenant(entry.getKey())); + tester.controller().curator().writeArchiveBuckets(zone, buckets); + } + + private void setAccountBucketNameInService(ZoneId zone, CloudAccount cloudAccount, String bucketName) { + ((MockArchiveService) tester.controller().serviceRegistry().archiveService()).setEnclaveArchiveBucket(zone, cloudAccount, bucketName); } private void setArchiveUriInNodeRepo(Map<TenantName, String> archiveUris, ZoneId zone) { @@ -79,8 +91,11 @@ public class ArchiveUriUpdaterTest { archiveUris.forEach((tenant, uri) -> nodeRepository.updateArchiveUri(zone, ArchiveUriUpdate.setArchiveUriFor(tenant, URI.create(uri)))); } - private void deploy(DeploymentContext application, ZoneId zone) { - application.runJob(JobType.deploymentTo(zone), new ApplicationPackage(new byte[0])); + private void deploy(DeploymentContext application, ZoneId zone, CloudAccount cloudAccount) { + application.submit(new ApplicationPackageBuilder() + .cloudAccount(cloudAccount.value()) + .region(zone.region().value()) + .build()).deploy(); } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializerTest.java index 82c5a6fc0c1..1d1b1124d22 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ArchiveBucketsSerializerTest.java @@ -1,11 +1,15 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.persistence; +import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.TenantName; -import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBuckets; +import com.yahoo.vespa.hosted.controller.api.integration.archive.TenantManagedArchiveBucket; +import com.yahoo.vespa.hosted.controller.api.integration.archive.VespaManagedArchiveBucket; import org.junit.jupiter.api.Test; -import java.util.LinkedHashSet; +import java.time.Instant; +import java.util.Set; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -13,17 +17,12 @@ public class ArchiveBucketsSerializerTest { @Test void serdes() { - var testTenants = new LinkedHashSet<TenantName>(); - testTenants.add(TenantName.from("tenant1")); - testTenants.add(TenantName.from("tenant2")); + ArchiveBuckets archiveBuckets = new ArchiveBuckets( + Set.of(new VespaManagedArchiveBucket("bucket1Name", "key1Arn").withTenants(Set.of(TenantName.from("t1"), TenantName.from("t2"))), + new VespaManagedArchiveBucket("bucket2Name", "key2Arn").withTenant(TenantName.from("t3"))), + Set.of(new TenantManagedArchiveBucket("bucket3Name", CloudAccount.from("acct-1"), Instant.ofEpochMilli(1234)), + new TenantManagedArchiveBucket("bucket4Name", CloudAccount.from("acct-2"), Instant.ofEpochMilli(5678)))); - var testBuckets = new LinkedHashSet<ArchiveBucket>(); - testBuckets.add(new ArchiveBucket("bucket1Name", "key1Arn").withTenants(testTenants)); - testBuckets.add(new ArchiveBucket("bucket2Name", "key2Arn")); - - String zkData = "{\"buckets\":[{\"bucketName\":\"bucket1Name\",\"keyArn\":\"key1Arn\",\"tenantIds\":[\"tenant1\",\"tenant2\"]},{\"bucketName\":\"bucket2Name\",\"keyArn\":\"key2Arn\",\"tenantIds\":[]}]}"; - - assertEquals(testBuckets, ArchiveBucketsSerializer.fromJsonString(zkData)); - assertEquals(testBuckets, ArchiveBucketsSerializer.fromJsonString(ArchiveBucketsSerializer.toSlime(testBuckets).toString())); + assertEquals(archiveBuckets, ArchiveBucketsSerializer.fromSlime(ArchiveBucketsSerializer.toSlime(archiveBuckets))); } } diff --git a/metrics/src/tests/metricmanagertest.cpp b/metrics/src/tests/metricmanagertest.cpp index 6d6f21ea7b0..52b17576c5c 100644 --- a/metrics/src/tests/metricmanagertest.cpp +++ b/metrics/src/tests/metricmanagertest.cpp @@ -29,7 +29,7 @@ struct MetricManagerTest : public ::testing::Test { // MetricManager that aren't accessible to "freestanding" fixtures. So we // get the test to do the necessary poking and prodding for us instead. void takeSnapshots(MetricManager& mm, time_t timeToProcess) { - mm.takeSnapshots(mm.getMetricLock(), timeToProcess); + mm.takeSnapshots(mm.getMetricLock(), system_time(vespalib::from_s(timeToProcess))); } }; @@ -165,7 +165,7 @@ getMatchedMetrics(const vespalib::string& config) MetricLockGuard g(mm.getMetricLock()); mm.visit(g, mm.getActiveMetrics(g), visitor, "consumer"); - MetricManager::ConsumerSpec::SP consumerSpec(mm.getConsumerSpec(g, "consumer")); + const MetricManager::ConsumerSpec * consumerSpec = mm.getConsumerSpec(g, "consumer"); return { visitor.toString(), consumerSpec ? consumerSpec->toString() : "Non-existing consumer" }; } @@ -384,11 +384,11 @@ struct BriefValuePrinter : public MetricVisitor { } }; -bool waitForTimeProcessed(const MetricManager& mm, time_t processtime, uint32_t timeout = 120) +bool waitForTimeProcessed(const MetricManager& mm, vespalib::duration processtime, uint32_t timeout = 120) { uint32_t lastchance = time(0) + timeout; while (time(0) < lastchance) { - if (mm.getLastProcessedTime() >= processtime) return true; + if (mm.getLastProcessedTime() >= time_point(processtime)) return true; mm.timeChangedNotification(); std::this_thread::sleep_for(10ms); } @@ -411,14 +411,11 @@ std::string dumpAllSnapshots(const MetricManager& mm, const std::string& consume mm.visit(metricLock, mm.getTotalMetricSnapshot(metricLock), briefValuePrinter, consumer); ost << "Total: " << briefValuePrinter.ost.str() << "\n"; } - std::vector<uint32_t> periods; - { - MetricLockGuard metricLock(mm.getMetricLock()); - periods = mm.getSnapshotPeriods(metricLock); - } - for (uint32_t i=0; i<periods.size(); ++i) { - MetricLockGuard metricLock(mm.getMetricLock()); - const MetricSnapshotSet& set(mm.getMetricSnapshotSet(metricLock, periods[i])); + + MetricLockGuard metricLock(mm.getMetricLock()); + auto periods = mm.getSnapshotPeriods(metricLock); + for (vespalib::duration period : periods) { + const MetricSnapshotSet& set(mm.getMetricSnapshotSet(metricLock, period)); ost << set.getName() << "\n"; for (uint32_t count=0,j=0; j<2; ++j) { if (set.getCount() == 1 && j == 1) continue; @@ -438,9 +435,9 @@ std::string dumpAllSnapshots(const MetricManager& mm, const std::string& consume { \ MetricLockGuard lockGuard(mm.getMetricLock()); \ BriefValuePrinter briefValuePrinter; \ - if (period == -1) { \ + if (period < vespalib::duration::zero()) { \ mm.visit(lockGuard, mm.getActiveMetrics(lockGuard), briefValuePrinter, "snapper"); \ - } else if (period == 0) { \ + } else if (period == vespalib::duration::zero()) { \ mm.visit(lockGuard, mm.getTotalMetricSnapshot(lockGuard), briefValuePrinter, "snapper"); \ } else { \ mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, period), briefValuePrinter, "snapper"); \ @@ -450,8 +447,8 @@ std::string dumpAllSnapshots(const MetricManager& mm, const std::string& consume #define ASSERT_PROCESS_TIME(mm, time) \ { \ - LOG(info, "Waiting for processed time %u.", time); \ - bool gotToCorrectProgress = waitForTimeProcessed(mm, time); \ + LOG(info, "Waiting for processed time %s.", vespalib::to_string(time_point(time)).c_str()); \ + bool gotToCorrectProgress = waitForTimeProcessed(mm, (time)); \ if (!gotToCorrectProgress) \ FAIL() << "Failed to get to processed time within timeout"; \ } @@ -478,8 +475,7 @@ TEST_F(MetricManagerTest, test_snapshots) { MetricLockGuard lockGuard(mm.getMetricLock()); mm.visit(lockGuard, mm.getActiveMetrics(lockGuard), visitor, "snapper"); - MetricManager::ConsumerSpec::SP consumerSpec( - mm.getConsumerSpec(lockGuard, "snapper")); + const MetricManager::ConsumerSpec * consumerSpec = mm.getConsumerSpec(lockGuard, "snapper"); EXPECT_EQ(std::string("\n" "temp.val6\n" "temp.sub.val1\n" @@ -492,12 +488,11 @@ TEST_F(MetricManagerTest, test_snapshots) "*temp.multisub.sum.val1\n" "*temp.multisub.sum.val2\n" "*temp.multisub.sum.valsum\n"), - "\n" + visitor.toString()) << (consumerSpec.get() ? consumerSpec->toString() - : "Non-existing consumer"); + "\n" + visitor.toString()) << (consumerSpec ? consumerSpec->toString() : "Non-existing consumer"); } // Initially, there should be no metrics logged - ASSERT_PROCESS_TIME(mm, 1000); - ASSERT_VALUES(mm, 5 * 60, ""); + ASSERT_PROCESS_TIME(mm, 1000s); + ASSERT_VALUES(mm, 5 * 60s, ""); // Adding metrics done in first five minutes. mySet.val6.addValue(2); @@ -507,10 +502,10 @@ TEST_F(MetricManagerTest, test_snapshots) mySet.val10.a.val2.addValue(2); mySet.val10.b.val1.addValue(1); timer.add_time(5 * 60); - ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60); - ASSERT_VALUES(mm, 5 * 60, "2,4,4,1,7,9,1,1,8,2,10"); - ASSERT_VALUES(mm, 60 * 60, ""); - ASSERT_VALUES(mm, 0 * 60, "2,4,4,1,7,9,1,1,8,2,10"); + ASSERT_PROCESS_TIME(mm, 1000s + 5 * 60s); + ASSERT_VALUES(mm, 5 * 60s, "2,4,4,1,7,9,1,1,8,2,10"); + ASSERT_VALUES(mm, 60 * 60s, ""); + ASSERT_VALUES(mm, 0 * 60s, "2,4,4,1,7,9,1,1,8,2,10"); // Adding metrics done in second five minute period. Total should // be updated to account for both @@ -521,18 +516,18 @@ TEST_F(MetricManagerTest, test_snapshots) mySet.val10.a.val2.addValue(3); mySet.val10.b.val1.addValue(2); timer.add_time(5 * 60); - ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60 * 2); - ASSERT_VALUES(mm, 5 * 60, "4,5,5,1,8,11,2,2,10,3,13"); - ASSERT_VALUES(mm, 60 * 60, ""); - ASSERT_VALUES(mm, 0 * 60, "4,5,5,2,8,11,2,2,10,3,13"); + ASSERT_PROCESS_TIME(mm, 1000s + 5 * 60 * 2s); + ASSERT_VALUES(mm, 5 * 60s, "4,5,5,1,8,11,2,2,10,3,13"); + ASSERT_VALUES(mm, 60 * 60s, ""); + ASSERT_VALUES(mm, 0 * 60s, "4,5,5,2,8,11,2,2,10,3,13"); // Adding another five minute period where nothing have happened. // Metric for last 5 minutes should be 0. timer.add_time(5 * 60); - ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60 * 3); - ASSERT_VALUES(mm, 5 * 60, "0,0,0,0,0,0,0,0,0,0,0"); - ASSERT_VALUES(mm, 60 * 60, ""); - ASSERT_VALUES(mm, 0 * 60, "4,5,5,2,8,11,2,2,10,3,13"); + ASSERT_PROCESS_TIME(mm, 1000s + 5 * 60s * 3); + ASSERT_VALUES(mm, 5 * 60s, "0,0,0,0,0,0,0,0,0,0,0"); + ASSERT_VALUES(mm, 60 * 60s, ""); + ASSERT_VALUES(mm, 0 * 60s, "4,5,5,2,8,11,2,2,10,3,13"); // Advancing time to 60 minute period, we should create a proper // 60 minute period timer. @@ -540,18 +535,18 @@ TEST_F(MetricManagerTest, test_snapshots) for (uint32_t i=0; i<9; ++i) { // 9 x 5 minutes. Avoid snapshot bumping // due to taking snapshots in the past timer.add_time(5 * 60); - ASSERT_PROCESS_TIME(mm, 1000 + 5 * 60 * (4 + i)); + ASSERT_PROCESS_TIME(mm, 1000s + 5 * 60s * (4 + i)); } - ASSERT_VALUES(mm, 5 * 60, "0,0,0,0,0,0,0,0,0,0,0"); - ASSERT_VALUES(mm, 60 * 60, "6,5,5,2,8,11,2,2,10,3,13"); - ASSERT_VALUES(mm, 0 * 60, "6,5,5,2,8,11,2,2,10,3,13"); + ASSERT_VALUES(mm, 5 * 60s, "0,0,0,0,0,0,0,0,0,0,0"); + ASSERT_VALUES(mm, 60 * 60s, "6,5,5,2,8,11,2,2,10,3,13"); + ASSERT_VALUES(mm, 0 * 60s, "6,5,5,2,8,11,2,2,10,3,13"); // Test that reset works - mm.reset(1000); - ASSERT_VALUES(mm, -1, "0,0,0,0,0,0,0,0,0,0,0"); - ASSERT_VALUES(mm, 5 * 60, "0,0,0,0,0,0,0,0,0,0,0"); - ASSERT_VALUES(mm, 60 * 60, "0,0,0,0,0,0,0,0,0,0,0"); - ASSERT_VALUES(mm, 0 * 60, "0,0,0,0,0,0,0,0,0,0,0"); + mm.reset(system_time(1000s)); + ASSERT_VALUES(mm, -1s, "0,0,0,0,0,0,0,0,0,0,0"); + ASSERT_VALUES(mm, 5 * 60s, "0,0,0,0,0,0,0,0,0,0,0"); + ASSERT_VALUES(mm, 60 * 60s, "0,0,0,0,0,0,0,0,0,0,0"); + ASSERT_VALUES(mm, 0 * 60s, "0,0,0,0,0,0,0,0,0,0,0"); } TEST_F(MetricManagerTest, test_json_output) @@ -591,7 +586,7 @@ TEST_F(MetricManagerTest, test_json_output) JsonWriter writer(jsonStream); { MetricLockGuard lockGuard(mm.getMetricLock()); - mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, 300, false), writer, "snapper"); + mm.visit(lockGuard, mm.getMetricSnapshot(lockGuard, 300s, false), writer, "snapper"); } jsonStream.finalize(); std::string jsonData = as.str(); @@ -680,7 +675,7 @@ struct MetricSnapshotTestFixture JsonWriter writer(jsonStream); { MetricLockGuard lockGuard(manager.getMetricLock()); - manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300, false), writer, "snapper"); + manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300s, false), writer, "snapper"); } jsonStream.finalize(); return as.str(); @@ -689,10 +684,10 @@ struct MetricSnapshotTestFixture std::string renderLastSnapshotAsText(const std::string& matchPattern = ".*") const { std::ostringstream ss; - TextWriter writer(ss, 300, matchPattern, true); + TextWriter writer(ss, 300s, matchPattern, true); { MetricLockGuard lockGuard(manager.getMetricLock()); - manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300, false), writer, "snapper"); + manager.visit(lockGuard, manager.getMetricSnapshot(lockGuard, 300s, false), writer, "snapper"); } return ss.str(); } @@ -902,7 +897,7 @@ TEST_F(MetricManagerTest, test_text_output) "consumer[1].tags[1]\n" "consumer[1].tags[0] snaptest\n")); std::string expected( - "snapshot \"Active metrics showing updates since last snapshot\" from 1000 to 0 period 0\n" + "snapshot \"Active metrics showing updates since last snapshot\" from 1970-01-01 00:16:40.000 UTC to 1970-01-01 00:00:00.000 UTC period 0\n" "temp.val6 average=2 last=2 min=2 max=2 count=1 total=2\n" "temp.sub.val1 average=4 last=4 min=4 max=4 count=1 total=4\n" "temp.sub.valsum average=4 last=4 min=4 max=4 count=1 total=4\n" @@ -915,7 +910,7 @@ TEST_F(MetricManagerTest, test_text_output) "temp.multisub.sum.val2 average=2 last=2 min=2 max=2 count=1 total=2\n" "temp.multisub.sum.valsum average=10 last=10"); std::ostringstream ost; - TextWriter writer(ost, 300, ".*", true); + TextWriter writer(ost, 300s, ".*", true); { MetricLockGuard lockGuard(mm.getMetricLock()); mm.visit(lockGuard, mm.getActiveMetrics(lockGuard), writer, "snapper"); @@ -938,11 +933,9 @@ TEST_F(MetricManagerTest, text_output_supports_dimensions) fixture.takeSnapshotsOnce(); std::string actual = fixture.renderLastSnapshotAsText("outer.*temp.*val"); std::string expected( - "snapshot \"5 minute\" from 1000 to 1300 period 300\n" - "outer{fancy:stuff}.temp{bar:hyperbar,foo:megafoo}.val1 " - "average=2 last=2 min=2 max=2 count=1 total=2\n" - "outer{fancy:stuff}.temp{bar:hyperbar,foo:megafoo}.val2" - "{baz:superbaz} count=1"); + "snapshot \"5 minute\" from 1970-01-01 00:16:40.000 UTC to 1970-01-01 00:21:40.000 UTC period 300\n" + "outer{fancy:stuff}.temp{bar:hyperbar,foo:megafoo}.val1 average=2 last=2 min=2 max=2 count=1 total=2\n" + "outer{fancy:stuff}.temp{bar:hyperbar,foo:megafoo}.val2{baz:superbaz} count=1"); EXPECT_EQ(expected, actual); } @@ -952,8 +945,8 @@ namespace { std::mutex& _output_mutex; FakeTimer& _timer; - MyUpdateHook(std::ostringstream& output, std::mutex& output_mutex, const char* name, FakeTimer& timer) - : UpdateHook(name), + MyUpdateHook(std::ostringstream& output, std::mutex& output_mutex, const char* name, vespalib::duration period, FakeTimer& timer) + : UpdateHook(name, period), _output(output), _output_mutex(output_mutex), _timer(timer) @@ -981,12 +974,12 @@ TEST_F(MetricManagerTest, test_update_hooks) mm.registerMetric(lockGuard, mySet.set); } - MyUpdateHook preInitShort(output, output_mutex, "BIS", timer); - MyUpdateHook preInitLong(output, output_mutex, "BIL", timer); - MyUpdateHook preInitInfinite(output, output_mutex, "BII", timer); - mm.addMetricUpdateHook(preInitShort, 5); - mm.addMetricUpdateHook(preInitLong, 300); - mm.addMetricUpdateHook(preInitInfinite, 0); + MyUpdateHook preInitShort(output, output_mutex, "BIS", 5s, timer); + MyUpdateHook preInitLong(output, output_mutex, "BIL", 300s, timer); + MyUpdateHook preInitInfinite(output, output_mutex, "BII", 0s, timer); + mm.addMetricUpdateHook(preInitShort); + mm.addMetricUpdateHook(preInitLong); + mm.addMetricUpdateHook(preInitInfinite); // All hooks should get called during initialization @@ -1002,56 +995,56 @@ TEST_F(MetricManagerTest, test_update_hooks) "consumer[1].tags[0] snaptest\n")); output << "Init done\n"; - MyUpdateHook postInitShort(output, output_mutex, "AIS", timer); - MyUpdateHook postInitLong(output, output_mutex, "AIL", timer); - MyUpdateHook postInitInfinite(output, output_mutex, "AII", timer); - mm.addMetricUpdateHook(postInitShort, 5); - mm.addMetricUpdateHook(postInitLong, 400); - mm.addMetricUpdateHook(postInitInfinite, 0); + MyUpdateHook postInitShort(output, output_mutex, "AIS", 5s, timer); + MyUpdateHook postInitLong(output, output_mutex, "AIL", 400s, timer); + MyUpdateHook postInitInfinite(output, output_mutex, "AII", 0s, timer); + mm.addMetricUpdateHook(postInitShort); + mm.addMetricUpdateHook(postInitLong); + mm.addMetricUpdateHook(postInitInfinite); // After 5 seconds the short ones should get another. timer.set_time(1006); - waitForTimeProcessed(mm, 1006); + waitForTimeProcessed(mm, 1006s); // After 4 more seconds the short ones should get another // since last update was a second late. (Stable periods, process time // should not affect how often they are updated) timer.set_time(1010); - waitForTimeProcessed(mm, 1010); + waitForTimeProcessed(mm, 1010s); // Bumping considerably ahead, such that next update is in the past, // we should only get one update called in this period. timer.set_time(1200); - waitForTimeProcessed(mm, 1200); + waitForTimeProcessed(mm, 1200s); // No updates at this time. timer.set_time(1204); - waitForTimeProcessed(mm, 1204); + waitForTimeProcessed(mm, 1204s); // Give all hooks an update - mm.updateMetrics(true); + mm.updateMetrics(); // Last update should not have interfered with periods timer.set_time(1205); - waitForTimeProcessed(mm, 1205); + waitForTimeProcessed(mm, 1205s); // Time is just ahead of a snapshot. timer.set_time(1299); - waitForTimeProcessed(mm, 1299); + waitForTimeProcessed(mm, 1299s); // At time 1300 we are at a 5 minute snapshot bump // All hooks should thus get an update. The one with matching period // should only get one timer.set_time(1300); - waitForTimeProcessed(mm, 1300); + waitForTimeProcessed(mm, 1300s); // The snapshot time currently doesn't count for the metric at period // 400. It will get an event at this time. timer.set_time(1450); - waitForTimeProcessed(mm, 1450); + waitForTimeProcessed(mm, 1450s); std::string expected( "Running init\n" diff --git a/metrics/src/tests/snapshottest.cpp b/metrics/src/tests/snapshottest.cpp index 4d2ea96c36d..5561825e2ba 100644 --- a/metrics/src/tests/snapshottest.cpp +++ b/metrics/src/tests/snapshottest.cpp @@ -4,7 +4,6 @@ #include <vespa/metrics/metrics.h> #include <vespa/metrics/summetric.hpp> #include <vespa/vespalib/gtest/gtest.h> -#include <vespa/vespalib/util/size_literals.h> namespace metrics { @@ -166,8 +165,8 @@ void ASSERT_VALUE(int32_t value, const MetricSnapshot & snapshot, const char *na } struct SnapshotTest : public ::testing::Test { - time_t tick(MetricManager& mgr, time_t currentTime) { - return mgr.tick(mgr.getMetricLock(), currentTime); + void tick(MetricManager& mgr, time_t currentTime) { + mgr.tick(mgr.getMetricLock(), time_point(vespalib::from_s(currentTime))); } }; @@ -221,7 +220,7 @@ TEST_F(SnapshotTest, test_snapshot_two_days) ASSERT_VALUE(0, *snap, "test.set1.set1.countSum"); // 5 minute snapshot - snap = &mm.getMetricSnapshot(lockGuard, 5 * 60); + snap = &mm.getMetricSnapshot(lockGuard, 5 * 60s); ASSERT_VALUE(1, *snap, "test.set1.set1.count1"); ASSERT_VALUE(2, *snap, "test.set1.set1.countSum"); @@ -229,7 +228,7 @@ TEST_F(SnapshotTest, test_snapshot_two_days) ASSERT_VALUE(1, *snap, "test.set1.set1.averageSum"); // 1 hour snapshot - snap = &mm.getMetricSnapshot(lockGuard, 60 * 60); + snap = &mm.getMetricSnapshot(lockGuard, 60 * 60s); ASSERT_VALUE(12, *snap, "test.set1.set1.count1"); ASSERT_VALUE(24, *snap, "test.set1.set1.countSum"); @@ -237,7 +236,7 @@ TEST_F(SnapshotTest, test_snapshot_two_days) ASSERT_VALUE(1, *snap, "test.set1.set1.averageSum"); // 1 day snapshot - snap = &mm.getMetricSnapshot(lockGuard, 24 * 60 * 60); + snap = &mm.getMetricSnapshot(lockGuard, 24 * 60 * 60s); ASSERT_VALUE(288, *snap, "test.set1.set1.count1"); ASSERT_VALUE(576, *snap, "test.set1.set1.countSum"); diff --git a/metrics/src/tests/summetrictest.cpp b/metrics/src/tests/summetrictest.cpp index 09495ff038d..8e988b65b96 100644 --- a/metrics/src/tests/summetrictest.cpp +++ b/metrics/src/tests/summetrictest.cpp @@ -104,8 +104,7 @@ TEST(SumMetricTest, test_remove) TEST(SumMetricTest, test_start_value) { MetricSnapshot snapshot("active"); - SumMetric<LongValueMetric> sum("foo", {}, "foodesc", - &snapshot.getMetrics()); + SumMetric<LongValueMetric> sum("foo", {}, "foodesc", &snapshot.getMetrics()); LongValueMetric start("start", {}, "", 0); start.set(50); sum.setStartValue(start); @@ -115,7 +114,7 @@ TEST(SumMetricTest, test_start_value) MetricSnapshot copy("copy"); copy.recreateSnapshot(snapshot.getMetrics(), true); - snapshot.addToSnapshot(copy, 100); + snapshot.addToSnapshot(copy, system_time(100s)); LongValueMetric value("value", {}, "", &snapshot.getMetrics()); sum.addMetricToSum(value); diff --git a/metrics/src/vespa/metrics/jsonwriter.cpp b/metrics/src/vespa/metrics/jsonwriter.cpp index c0d227b8f5a..1b9df3988e1 100644 --- a/metrics/src/vespa/metrics/jsonwriter.cpp +++ b/metrics/src/vespa/metrics/jsonwriter.cpp @@ -23,12 +23,12 @@ JsonWriter::visitSnapshot(const MetricSnapshot& snapshot) { _stream << Object() << "snapshot" << Object() - << "from" << snapshot.getFromTime() - << "to" << snapshot.getToTime() + << "from" << vespalib::count_s(snapshot.getFromTime().time_since_epoch()) + << "to" << vespalib::count_s(snapshot.getToTime().time_since_epoch()) << End() << "values" << Array(); _flag = SNAPSHOT_STARTED; - _period = snapshot.getPeriod(); + _period = vespalib::count_s(snapshot.getPeriod()); // Only prints second resolution return true; } diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp index df83001a4e2..7fc8222c703 100644 --- a/metrics/src/vespa/metrics/metricmanager.cpp +++ b/metrics/src/vespa/metrics/metricmanager.cpp @@ -10,7 +10,6 @@ #include <vespa/vespalib/util/exceptions.h> #include <vespa/vespalib/util/time.h> #include <vespa/vespalib/stllike/asciistream.h> -#include <vespa/vespalib/stllike/hashtable.hpp> #include <vespa/config/subscription/configsubscriber.hpp> #include <set> #include <sstream> @@ -28,6 +27,8 @@ using vespalib::make_string_short::fmt; using vespalib::count_ms; using vespalib::count_s; using vespalib::from_s; +using vespalib::to_s; +using vespalib::to_string; MetricManager::ConsumerSpec::ConsumerSpec() = default; MetricManager::ConsumerSpec::~ConsumerSpec() = default; @@ -44,19 +45,20 @@ MetricManager::assertMetricLockLocked(const MetricLockGuard& g) const { } } -void -MetricManager::ConsumerSpec::print(std::ostream& out, bool verbose, const std::string& indent) const +vespalib::string +MetricManager::ConsumerSpec::toString() const { - (void) verbose; + vespalib::asciistream out; out << "ConsumerSpec("; std::set<Metric::String> sortedMetrics; for (const Metric::String & name : includedMetrics) { sortedMetrics.insert(name); } for (const auto & s : sortedMetrics) { - out << "\n" << indent << " " << s; + out << "\n" << " " << s; } out << ")"; + return out.str(); } void @@ -79,9 +81,9 @@ MetricManager::MetricManager(std::unique_ptr<Timer> timer) _config(), _consumerConfig(), _snapshots(), - _totalMetrics(std::make_shared<MetricSnapshot>("Empty metrics before init", 0, _activeMetrics.getMetrics(), false)), + _totalMetrics(std::make_shared<MetricSnapshot>("Empty metrics before init", 0s, _activeMetrics.getMetrics(), false)), _timer(std::move(timer)), - _lastProcessedTime(0), + _lastProcessedTime(), _snapshotUnsetMetrics(false), _consumerConfigChanged(false), _metricManagerMetrics("metricmanager", {}, "Metrics for the metric manager upkeep tasks", nullptr), @@ -115,29 +117,26 @@ MetricManager::stop() } void -MetricManager::addMetricUpdateHook(UpdateHook& hook, uint32_t period) +MetricManager::addMetricUpdateHook(UpdateHook& hook) { - hook._period = period; + hook.updateNextCall(_timer->getTime()); std::lock_guard sync(_waiter); - // If we've already initialized manager, log period has been set. - // In this case. Call first time after period - hook._nextCall = count_s(_timer->getTime().time_since_epoch()) + period; - if (period == 0) { - for (UpdateHook * sHook : _snapshotUpdateHooks) { - if (sHook == &hook) { + if ( hook.is_periodic() ) { + for (UpdateHook * pHook : _periodicUpdateHooks) { + if (pHook == &hook) { LOG(warning, "Update hook already registered"); return; } } - _snapshotUpdateHooks.push_back(&hook); + _periodicUpdateHooks.push_back(&hook); } else { - for (UpdateHook * pHook : _periodicUpdateHooks) { - if (pHook == &hook) { + for (UpdateHook * sHook : _snapshotUpdateHooks) { + if (sHook == &hook) { LOG(warning, "Update hook already registered"); return; } } - _periodicUpdateHooks.push_back(&hook); + _snapshotUpdateHooks.push_back(&hook); } } @@ -145,17 +144,17 @@ void MetricManager::removeMetricUpdateHook(UpdateHook& hook) { std::lock_guard sync(_waiter); - if (hook._period == 0) { - for (auto it = _snapshotUpdateHooks.begin(); it != _snapshotUpdateHooks.end(); it++) { + if (hook.is_periodic()) { + for (auto it = _periodicUpdateHooks.begin(); it != _periodicUpdateHooks.end(); it++) { if (*it == &hook) { - _snapshotUpdateHooks.erase(it); + _periodicUpdateHooks.erase(it); return; } } } else { - for (auto it = _periodicUpdateHooks.begin(); it != _periodicUpdateHooks.end(); it++) { + for (auto it = _snapshotUpdateHooks.begin(); it != _snapshotUpdateHooks.end(); it++) { if (*it == &hook) { - _periodicUpdateHooks.erase(it); + _snapshotUpdateHooks.erase(it); return; } } @@ -186,7 +185,7 @@ MetricManager::init(const config::ConfigUri & uri, bool startThread) // Wait for first iteration to have completed, such that it is safe // to access snapshots afterwards. MetricLockGuard sync(_waiter); - while (_lastProcessedTime.load(std::memory_order_relaxed) == 0) { + while (_lastProcessedTime.load(std::memory_order_relaxed) == time_point()) { _cond.wait_for(sync, 1ms); } } else { @@ -382,12 +381,12 @@ MetricManager::handleMetricsAltered(const MetricLockGuard & guard) LOG(info, "Metrics registration changes detected. Handling changes."); } _activeMetrics.getMetrics().clearRegistrationAltered(); - std::map<Metric::String, ConsumerSpec::SP> configMap; + std::map<Metric::String, ConsumerSpec> configMap; LOG(debug, "Calculating new consumer config"); for (const auto & consumer : _config->consumer) { ConsumerMetricBuilder consumerMetricBuilder(consumer); _activeMetrics.getMetrics().visit(consumerMetricBuilder); - configMap[consumer.name] = std::make_shared<ConsumerSpec>(std::move(consumerMetricBuilder._matchedMetrics)); + configMap[consumer.name] = ConsumerSpec(std::move(consumerMetricBuilder._matchedMetrics)); } LOG(debug, "Recreating snapshots to include altered metrics"); _totalMetrics->recreateSnapshot(_activeMetrics.getMetrics(), _snapshotUnsetMetrics); @@ -427,10 +426,10 @@ MetricManager::createSnapshotPeriods(const Config& config) } else { name << length << " seconds"; } - result.push_back(SnapSpec(length, name.str())); + result.emplace_back(vespalib::from_s(length), name.str()); } for (uint32_t i=1; i<result.size(); ++i) { - if (result[i].first % result[i-1].first != 0) { + if (result[i].first % result[i-1].first != vespalib::duration::zero()) { std::ostringstream ost; ost << "Period " << result[i].first << " is not a multiplum of period " << result[i-1].first << " which is needs to be."; @@ -442,10 +441,10 @@ MetricManager::createSnapshotPeriods(const Config& config) result.clear(); } if (result.empty()) { - result.push_back(SnapSpec(60 * 5, "5 minute")); - result.push_back(SnapSpec(60 * 60, "1 hour")); - result.push_back(SnapSpec(60 * 60 * 24, "1 day")); - result.push_back(SnapSpec(60 * 60 * 24 * 7, "1 week")); + result.emplace_back(60s * 5, "5 minute"); + result.emplace_back(60s * 60, "1 hour"); + result.emplace_back(60s * 60 * 24, "1 day"); + result.emplace_back(60s * 60 * 24 * 7, "1 week"); } return result; } @@ -464,16 +463,16 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr<Config> confi LOG(debug, "Initializing snapshots as this is first configure call"); std::vector<SnapSpec> snapshotPeriods(createSnapshotPeriods(*config)); - // Set up snapshots only first time. We don't allow live reconfig - // of snapshot periods. - time_t currentTime = count_s(_timer->getTime().time_since_epoch()); + // Set up snapshots only first time. We don't allow live reconfig + // of snapshot periods. + time_point currentTime = _timer->getTime(); _activeMetrics.setFromTime(currentTime); uint32_t count = 1; for (uint32_t i = 0; i< snapshotPeriods.size(); ++i) { uint32_t nextCount = 1; if (i + 1 < snapshotPeriods.size()) { nextCount = snapshotPeriods[i + 1].first / snapshotPeriods[i].first; - if ((snapshotPeriods[i + 1].first % snapshotPeriods[i].first) != 0) { + if ((snapshotPeriods[i + 1].first % snapshotPeriods[i].first) != vespalib::duration::zero()) { throw IllegalStateException("Snapshot periods must be multiplum of each other",VESPA_STRLOC); } } @@ -482,8 +481,8 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr<Config> confi _activeMetrics.getMetrics(), _snapshotUnsetMetrics)); count = nextCount; } - // Add all time snapshot. - _totalMetrics = std::make_shared<MetricSnapshot>("All time snapshot", 0, _activeMetrics.getMetrics(), _snapshotUnsetMetrics); + // Add all time snapshot. + _totalMetrics = std::make_shared<MetricSnapshot>("All time snapshot", 0s, _activeMetrics.getMetrics(), _snapshotUnsetMetrics); _totalMetrics->reset(currentTime); } if (_config.get() == 0 || (_config->consumer.size() != config->consumer.size())) { @@ -503,27 +502,24 @@ MetricManager::configure(const MetricLockGuard & , std::unique_ptr<Config> confi } -MetricManager::ConsumerSpec::SP +const MetricManager::ConsumerSpec * MetricManager::getConsumerSpec(const MetricLockGuard &, const Metric::String& consumer) const { auto it(_consumerConfig.find(consumer)); - return (it != _consumerConfig.end() ? it->second : ConsumerSpec::SP()); + return (it != _consumerConfig.end() ? &it->second : nullptr); } -//#define VERIFY_ALL_METRICS_VISITED 1 namespace { struct ConsumerMetricVisitor : public MetricVisitor { const MetricManager::ConsumerSpec& _metricsToMatch; MetricVisitor& _client; -#ifdef VERIFY_ALL_METRICS_VISITED - std::set<Metric::String> _visitedMetrics; -#endif ConsumerMetricVisitor(const MetricManager::ConsumerSpec& spec, MetricVisitor& clientVisitor) - : _metricsToMatch(spec), _client(clientVisitor) - {} + : _metricsToMatch(spec), + _client(clientVisitor) + { } bool visitMetricSet(const MetricSet& metricSet, bool autoGenerated) override { if (metricSet.isTopSet()) return true; @@ -532,26 +528,17 @@ struct ConsumerMetricVisitor : public MetricVisitor { } void doneVisitingMetricSet(const MetricSet& metricSet) override { if (!metricSet.isTopSet()) { -#ifdef VERIFY_ALL_METRICS_VISITED - _visitedMetrics.insert(metricSet.getPath()); -#endif _client.doneVisitingMetricSet(metricSet); } } bool visitCountMetric(const AbstractCountMetric& metric, bool autoGenerated) override { if (_metricsToMatch.contains(metric)) { -#ifdef VERIFY_ALL_METRICS_VISITED - _visitedMetrics.insert(metric.getPath()); -#endif return _client.visitCountMetric(metric, autoGenerated); } return true; } bool visitValueMetric(const AbstractValueMetric& metric, bool autoGenerated) override { if (_metricsToMatch.contains(metric)) { -#ifdef VERIFY_ALL_METRICS_VISITED - _visitedMetrics.insert(metric.getPath()); -#endif return _client.visitValueMetric(metric, autoGenerated); } return true; @@ -568,19 +555,11 @@ MetricManager::visit(const MetricLockGuard & guard, const MetricSnapshot& snapsh if (consumer == "") { snapshot.getMetrics().visit(visitor); } else { - ConsumerSpec::SP consumerSpec(getConsumerSpec(guard, consumer)); - if (consumerSpec.get()) { + const ConsumerSpec * consumerSpec = getConsumerSpec(guard, consumer); + if (consumerSpec) { + ConsumerMetricVisitor consumerVis(*consumerSpec, visitor); snapshot.getMetrics().visit(consumerVis); -#ifdef VERIFY_ALL_METRICS_VISITED - for (auto metric = consumerSpec->includedMetrics) { - if (consumerVis._visitedMetrics.find(metric) - == consumerVis._visitedMetrics.end()) - { - LOG(debug, "Failed to find metric %s to be visited.", metric.c_str()); - } - } -#endif } else { LOGBP(debug, "Requested metrics for non-defined consumer '%s'.", consumer.c_str()); } @@ -590,20 +569,21 @@ MetricManager::visit(const MetricLockGuard & guard, const MetricSnapshot& snapsh visitor.doneVisiting(); } -std::vector<uint32_t> +std::vector<vespalib::duration> MetricManager::getSnapshotPeriods(const MetricLockGuard& l) const { assertMetricLockLocked(l); - std::vector<uint32_t> result(_snapshots.size()); - for (uint32_t i=0; i<_snapshots.size(); ++i) { - result[i] = _snapshots[i]->getPeriod(); + std::vector<vespalib::duration> result; + result.reserve(_snapshots.size()); + for (const auto & snapshot : _snapshots) { + result.emplace_back(snapshot->getPeriod()); } return result; } // Client should have grabbed metrics lock before doing this const MetricSnapshot& -MetricManager::getMetricSnapshot(const MetricLockGuard& l, uint32_t period, bool getInProgressSet) const +MetricManager::getMetricSnapshot(const MetricLockGuard& l, vespalib::duration period, bool getInProgressSet) const { assertMetricLockLocked(l); for (const auto & snapshot : _snapshots) { @@ -614,12 +594,12 @@ MetricManager::getMetricSnapshot(const MetricLockGuard& l, uint32_t period, bool return snapshot->getSnapshot(getInProgressSet); } } - throw IllegalArgumentException(fmt("No snapshot for period of length %u exist.", period), VESPA_STRLOC); + throw IllegalArgumentException(fmt("No snapshot for period of length %f exist.", vespalib::to_s(period)), VESPA_STRLOC); } // Client should have grabbed metrics lock before doing this const MetricSnapshotSet& -MetricManager::getMetricSnapshotSet(const MetricLockGuard& l, uint32_t period) const +MetricManager::getMetricSnapshotSet(const MetricLockGuard& l, vespalib::duration period) const { assertMetricLockLocked(l); for (const auto & snapshot : _snapshots) { @@ -627,7 +607,7 @@ MetricManager::getMetricSnapshotSet(const MetricLockGuard& l, uint32_t period) c return *snapshot; } } - throw IllegalArgumentException(fmt("No snapshot set for period of length %u exist.", period), VESPA_STRLOC); + throw IllegalArgumentException(fmt("No snapshot set for period of length %f exist.", to_s(period)), VESPA_STRLOC); } void @@ -638,38 +618,35 @@ MetricManager::timeChangedNotification() const } void -MetricManager::updateMetrics(bool includeSnapshotOnlyHooks) +MetricManager::updateMetrics() { - LOG(debug, "Calling metric update hooks%s.", includeSnapshotOnlyHooks ? ", including snapshot hooks" : ""); // Ensure we're not in the way of the background thread MetricLockGuard sync(_waiter); - LOG(debug, "Giving %zu periodic update hooks.", _periodicUpdateHooks.size()); - updatePeriodicMetrics(sync, 0, true); - if (includeSnapshotOnlyHooks) { - LOG(debug, "Giving %zu snapshot update hooks.", _snapshotUpdateHooks.size()); - updateSnapshotMetrics(sync); - } + LOG(debug, "Calling %zu periodic update hooks.", _periodicUpdateHooks.size()); + updatePeriodicMetrics(sync, time_point(), true); + updateSnapshotMetrics(sync); } // When this is called, the thread monitor lock has already been grabbed -time_t -MetricManager::updatePeriodicMetrics(const MetricLockGuard & guard, time_t updateTime, bool outOfSchedule) +time_point +MetricManager::updatePeriodicMetrics(const MetricLockGuard & guard, time_point updateTime, bool outOfSchedule) { assertMetricLockLocked(guard); - time_t nextUpdateTime = std::numeric_limits<time_t>::max(); + time_point nextUpdateTime = time_point::max(); time_point preTime = _timer->getTimeInMilliSecs(); for (auto hook : _periodicUpdateHooks) { - if (hook->_nextCall <= updateTime) { + if (hook->expired(updateTime)) { hook->updateMetrics(guard); - if (hook->_nextCall + hook->_period < updateTime) { - if (hook->_nextCall != 0) { - LOG(debug, "Updated hook %s at time %" PRIu64 ", but next run in %u seconds have already passed as " - "time is %" PRIu64 ". Bumping next call to current time + period.", - hook->_name, static_cast<uint64_t>(hook->_nextCall), hook->_period, static_cast<uint64_t>(updateTime)); + if (hook->expired(updateTime - hook->getPeriod())) { + if (hook->has_valid_expiry()) { + LOG(debug, "Updated hook %s at time %s, but next run in %ld seconds have already passed as " + "time is %s. Bumping next call to current time + period.", + hook->getName(), to_string(hook->getNextCall()).c_str(), + count_s(hook->getPeriod()), to_string(updateTime).c_str()); } - hook->_nextCall = updateTime + hook->_period; + hook->updateNextCall(updateTime); } else { - hook->_nextCall += hook->_period; + hook->updateNextCall(); } time_point postTime = _timer->getTimeInMilliSecs(); _periodicHookLatency.addValue(count_ms(postTime - preTime)); @@ -680,7 +657,7 @@ MetricManager::updatePeriodicMetrics(const MetricLockGuard & guard, time_t updat _periodicHookLatency.addValue(count_ms(postTime - preTime)); preTime = postTime; } - nextUpdateTime = std::min(nextUpdateTime, hook->_nextCall); + nextUpdateTime = std::min(nextUpdateTime, hook->getNextCall()); } return nextUpdateTime; } @@ -709,7 +686,7 @@ MetricManager::forceEventLogging() } void -MetricManager::reset(time_t currentTime) +MetricManager::reset(system_time currentTime) { time_point preTime = _timer->getTimeInMilliSecs(); // Resetting implies visiting metrics, which needs to grab metric lock @@ -733,21 +710,20 @@ MetricManager::run() // we constantly add next time to do something from the last timer. // For that to work, we need to initialize timers on first iteration // to set them to current time. - time_t currentTime = count_s(_timer->getTime().time_since_epoch()); + system_time currentTime = _timer->getTime(); for (auto & snapshot : _snapshots) { snapshot->setFromTime(currentTime); } for (auto hook : _periodicUpdateHooks) { - hook->_nextCall = currentTime; + hook->setNextCall(currentTime); } // Ensure correct time for first snapshot _snapshots[0]->getSnapshot().setToTime(currentTime); while (!stop_requested()) { - time_point now = _timer->getTime(); - currentTime = count_s(now.time_since_epoch()); - time_t next = tick(sync, currentTime); + currentTime = _timer->getTime(); + time_point next = tick(sync, currentTime); if (currentTime < next) { - vespalib::duration wait_time = from_s(next - currentTime); + vespalib::duration wait_time = next - currentTime; _cond.wait_for(sync, wait_time); _sleepTimes.addValue(count_ms(wait_time)); } else { @@ -756,10 +732,10 @@ MetricManager::run() } } -time_t -MetricManager::tick(const MetricLockGuard & guard, time_t currentTime) +time_point +MetricManager::tick(const MetricLockGuard & guard, time_point currentTime) { - LOG(spam, "Worker thread starting to process for time %" PRIu64 ".", static_cast<uint64_t>(currentTime)); + LOG(spam, "Worker thread starting to process for time %s.", to_string(currentTime).c_str()); // Check for new config and reconfigure if (_configSubscriber && _configSubscriber->nextConfigNow()) { @@ -772,8 +748,8 @@ MetricManager::tick(const MetricLockGuard & guard, time_t currentTime) checkMetricsAltered(guard); // Set next work time to the time we want to take next snapshot. - time_t nextWorkTime = _snapshots[0]->getToTime() + _snapshots[0]->getPeriod(); - time_t nextUpdateHookTime; + time_point nextWorkTime = _snapshots[0]->getNextWorkTime(); + time_point nextUpdateHookTime; if (nextWorkTime <= currentTime) { // If taking a new snapshot or logging, force calls to all // update hooks. @@ -789,39 +765,38 @@ MetricManager::tick(const MetricLockGuard & guard, time_t currentTime) // Do snapshotting if it is time if (nextWorkTime <= currentTime) takeSnapshots(guard, nextWorkTime); - _lastProcessedTime.store(nextWorkTime <= currentTime ? nextWorkTime : currentTime, std::memory_order_relaxed); - LOG(spam, "Worker thread done with processing for time %" PRIu64 ".", - static_cast<uint64_t>(_lastProcessedTime.load(std::memory_order_relaxed))); - time_t next = _snapshots[0]->getPeriod() + _snapshots[0]->getToTime(); + _lastProcessedTime.store((nextWorkTime <= currentTime ? nextWorkTime : currentTime), std::memory_order_relaxed); + LOG(spam, "Worker thread done with processing for time %s.", + to_string(_lastProcessedTime.load(std::memory_order_relaxed)).c_str()); + time_point next = _snapshots[0]->getNextWorkTime(); next = std::min(next, nextUpdateHookTime); return next; } void -MetricManager::takeSnapshots(const MetricLockGuard & guard, time_t timeToProcess) +MetricManager::takeSnapshots(const MetricLockGuard & guard, system_time timeToProcess) { assertMetricLockLocked(guard); // If not time to do dump data from active snapshot yet, nothing to do if (!_snapshots[0]->timeForAnotherSnapshot(timeToProcess)) { - LOG(spam, "Not time to process snapshot %s at time %" PRIu64 ". Current " - "first period (%u) snapshot goes from %" PRIu64 " to %" PRIu64, - _snapshots[0]->getName().c_str(), static_cast<uint64_t>(timeToProcess), - _snapshots[0]->getPeriod(), static_cast<uint64_t>(_snapshots[0]->getFromTime()), - static_cast<uint64_t>(_snapshots[0]->getToTime())); + LOG(spam, "Not time to process snapshot %s at time %s. Current " + "first period (%f) snapshot goes from %s to %s", + _snapshots[0]->getName().c_str(), to_string(timeToProcess).c_str(), + to_s(_snapshots[0]->getPeriod()), to_string(_snapshots[0]->getFromTime()).c_str(), + to_string(_snapshots[0]->getToTime()).c_str()); return; } time_point preTime = _timer->getTimeInMilliSecs(); - LOG(debug, "Updating %s snapshot and total metrics at time %" PRIu64 ".", - _snapshots[0]->getName().c_str(), static_cast<uint64_t>(timeToProcess)); + LOG(debug, "Updating %s snapshot and total metrics at time %s.", + _snapshots[0]->getName().c_str(), to_string(timeToProcess).c_str()); MetricSnapshot& firstTarget(_snapshots[0]->getNextTarget()); firstTarget.reset(_activeMetrics.getFromTime()); _activeMetrics.addToSnapshot(firstTarget, false, timeToProcess); _activeMetrics.addToSnapshot(*_totalMetrics, false, timeToProcess); _activeMetrics.reset(timeToProcess); - LOG(debug, "After snapshotting, active metrics goes from %" PRIu64 " to %" PRIu64", " - "and 5 minute metrics goes from %" PRIu64 " to %" PRIu64".", - static_cast<uint64_t>(_activeMetrics.getFromTime()), static_cast<uint64_t>(_activeMetrics.getToTime()), - static_cast<uint64_t>(firstTarget.getFromTime()), static_cast<uint64_t>(firstTarget.getToTime())); + LOG(debug, "After snapshotting, active metrics goes from %s to %s, and 5 minute metrics goes from %s to %s.", + to_string(_activeMetrics.getFromTime()).c_str(), to_string(_activeMetrics.getToTime()).c_str(), + to_string(firstTarget.getFromTime()).c_str(), to_string(firstTarget.getToTime()).c_str()); // Update later snapshots if it is time for it for (uint32_t i=1; i<_snapshots.size(); ++i) { @@ -831,15 +806,15 @@ MetricManager::takeSnapshots(const MetricLockGuard & guard, time_t timeToProcess _snapshots[i-1]->getSnapshot().addToSnapshot(target, false, timeToProcess); target.setToTime(timeToProcess); if (!_snapshots[i]->haveCompletedNewPeriod(timeToProcess)) { - LOG(debug, "Not time to roll snapshot %s yet. %u of %u snapshot taken at time %" PRIu64 ", and period of %u " - "is not up yet as we're currently processing for time %" PRIu64 ".", + LOG(debug, "Not time to roll snapshot %s yet. %u of %u snapshot taken at time %s, and period of %f " + "is not up yet as we're currently processing for time %s.", _snapshots[i]->getName().c_str(), _snapshots[i]->getBuilderCount(), _snapshots[i]->getCount(), - static_cast<uint64_t>(_snapshots[i]->getBuilderCount() * _snapshots[i]->getPeriod() + _snapshots[i]->getFromTime()), - _snapshots[i]->getPeriod(), static_cast<uint64_t>(timeToProcess)); + to_string(_snapshots[i]->getBuilderCount() * _snapshots[i]->getPeriod() + _snapshots[i]->getFromTime()).c_str(), + to_s(_snapshots[i]->getPeriod()), to_string(timeToProcess).c_str()); break; } else { - LOG(debug, "Rolled snapshot %s at time %" PRIu64 ".", - _snapshots[i]->getName().c_str(), static_cast<uint64_t>(timeToProcess)); + LOG(debug, "Rolled snapshot %s at time %s.", + _snapshots[i]->getName().c_str(), to_string(timeToProcess).c_str()); } } time_point postTime = _timer->getTimeInMilliSecs(); @@ -852,10 +827,10 @@ MetricManager::getMemoryConsumption(const MetricLockGuard & guard) const assertMetricLockLocked(guard); auto mc = std::make_unique<MemoryConsumption>(); mc->_consumerCount += _consumerConfig.size(); - mc->_consumerMeta += (sizeof(ConsumerSpec::SP) + sizeof(ConsumerSpec)) * _consumerConfig.size(); + mc->_consumerMeta += sizeof(ConsumerSpec) * _consumerConfig.size(); for (const auto & consumer : _consumerConfig) { mc->_consumerId += mc->getStringMemoryUsage(consumer.first, mc->_consumerIdUnique) + sizeof(Metric::String); - consumer.second->addMemoryUsage(*mc); + consumer.second.addMemoryUsage(*mc); } uint32_t preTotal = mc->getTotalMemoryUsage(); _activeMetrics.addMemoryUsage(*mc); diff --git a/metrics/src/vespa/metrics/metricmanager.h b/metrics/src/vespa/metrics/metricmanager.h index c3ce37b451f..99fad4f2ad3 100644 --- a/metrics/src/vespa/metrics/metricmanager.h +++ b/metrics/src/vespa/metrics/metricmanager.h @@ -57,8 +57,6 @@ #include <list> #include <thread> -template class vespalib::hash_set<metrics::Metric::String>; - namespace metrics { class MetricManager @@ -75,20 +73,19 @@ public: * Spec saved from config. If metricSetChildren has content, metric pointed * to is a metric set. */ - struct ConsumerSpec : public vespalib::Printable { - using SP = std::shared_ptr<ConsumerSpec>; - + struct ConsumerSpec { vespalib::hash_set<Metric::String> includedMetrics; + ConsumerSpec(ConsumerSpec &&) noexcept = default; ConsumerSpec & operator= (ConsumerSpec &&) noexcept = default; ConsumerSpec(); - ~ConsumerSpec() override; + ~ConsumerSpec(); bool contains(const Metric& m) const { return (includedMetrics.find(m.getPath()) != includedMetrics.end()); } - void print(std::ostream& out, bool verbose, const std::string& indent) const override; + vespalib::string toString() const; void addMemoryUsage(MemoryConsumption&) const; }; @@ -98,15 +95,15 @@ private: std::unique_ptr<config::ConfigSubscriber> _configSubscriber; std::unique_ptr<config::ConfigHandle<MetricsmanagerConfig>> _configHandle; std::unique_ptr<MetricsmanagerConfig> _config; - std::map<Metric::String, ConsumerSpec::SP> _consumerConfig; + std::map<Metric::String, ConsumerSpec> _consumerConfig; std::list<UpdateHook*> _periodicUpdateHooks; std::list<UpdateHook*> _snapshotUpdateHooks; mutable std::mutex _waiter; mutable std::condition_variable _cond; - std::vector<MetricSnapshotSet::SP> _snapshots; - MetricSnapshot::SP _totalMetrics; + std::vector<std::shared_ptr<MetricSnapshotSet>> _snapshots; + std::shared_ptr<MetricSnapshot> _totalMetrics; std::unique_ptr<Timer> _timer; - std::atomic<time_t> _lastProcessedTime; + std::atomic<time_point> _lastProcessedTime; // Should be added to config, but wont now due to problems with // upgrading bool _snapshotUnsetMetrics; @@ -138,7 +135,7 @@ public: * snapshotting and metric logging, to make the metrics the best as they can * be at those occasions. * - * @param period Period in seconds for how often callback should be called. + * @param period Period for how often callback should be called. * The default value of 0, means only before snapshotting or * logging, while another value will give callbacks each * period seconds. Expensive metrics to calculate will @@ -147,7 +144,7 @@ public: * seconds or so. Any value of period >= the smallest snapshot * time will behave identically as if period is set to 0. */ - void addMetricUpdateHook(UpdateHook&, uint32_t period = 0); + void addMetricUpdateHook(UpdateHook&); /** Remove a metric update hook so it won't get any more updates. */ void removeMetricUpdateHook(UpdateHook&); @@ -157,7 +154,7 @@ public: * nice values before reporting something. * This function can not be called from an update hook callback. */ - void updateMetrics(bool includeSnapshotOnlyHooks = false); + void updateMetrics(); /** * Force event logging to happen now. @@ -191,7 +188,7 @@ public: * Reset all metrics including all snapshots. * This function can not be called from an update hook callback. */ - void reset(time_t currentTime); + void reset(system_time currentTime); /** * Read configuration. Before reading config, all metrics should be set @@ -199,7 +196,10 @@ public: * of consumers. readConfig() will start a config subscription. It should * not be called multiple times. */ - void init(const config::ConfigUri & uri, bool startThread = true); + void init(const config::ConfigUri & uri, bool startThread); + void init(const config::ConfigUri & uri) { + init(uri, true); + } /** * Visit a given snapshot for a given consumer. (Empty consumer name means @@ -237,15 +237,16 @@ public: return *_totalMetrics; } /** While accessing snapshots you should have the metric lock. */ - const MetricSnapshot& getMetricSnapshot( const MetricLockGuard& guard, uint32_t period) const { + const MetricSnapshot& getMetricSnapshot( const MetricLockGuard& guard, vespalib::duration period) const { return getMetricSnapshot(guard, period, false); } - const MetricSnapshot& getMetricSnapshot( const MetricLockGuard&, uint32_t period, bool getInProgressSet) const; - const MetricSnapshotSet& getMetricSnapshotSet(const MetricLockGuard&, uint32_t period) const; + const MetricSnapshot& getMetricSnapshot( const MetricLockGuard&, vespalib::duration period, bool getInProgressSet) const; + const MetricSnapshotSet& getMetricSnapshotSet(const MetricLockGuard&, vespalib::duration period) const; - std::vector<uint32_t> getSnapshotPeriods(const MetricLockGuard& l) const; + std::vector<vespalib::duration> getSnapshotPeriods(const MetricLockGuard& l) const; - ConsumerSpec::SP getConsumerSpec(const MetricLockGuard & guard, const Metric::String& consumer) const; + // Public only for testing. The returned pointer is only valid while holding the lock. + const ConsumerSpec * getConsumerSpec(const MetricLockGuard & guard, const Metric::String& consumer) const; /** * If you add or remove metrics from the active metric sets, normally, @@ -257,7 +258,7 @@ public: void checkMetricsAltered(const MetricLockGuard &); /** Used by unit tests to verify that we have processed for a given time. */ - time_t getLastProcessedTime() const { return _lastProcessedTime.load(std::memory_order_relaxed); } + time_point getLastProcessedTime() const { return _lastProcessedTime.load(std::memory_order_relaxed); } /** Used by unit tests to wake waiters after altering time. */ void timeChangedNotification() const; @@ -267,14 +268,14 @@ public: bool isInitialized() const; private: - void takeSnapshots(const MetricLockGuard &, time_t timeToProcess); + void takeSnapshots(const MetricLockGuard &, system_time timeToProcess); friend struct MetricManagerTest; friend struct SnapshotTest; void configure(const MetricLockGuard & guard, std::unique_ptr<MetricsmanagerConfig> conf); void run(); - time_t tick(const MetricLockGuard & guard, time_t currentTime); + time_point tick(const MetricLockGuard & guard, time_point currentTime); /** * Utility function for updating periodic metrics. * @@ -284,12 +285,12 @@ private: * without adjusting schedule for next update. * @return Time of next hook to be called in the future. */ - time_t updatePeriodicMetrics(const MetricLockGuard & guard, time_t updateTime, bool outOfSchedule); + time_point updatePeriodicMetrics(const MetricLockGuard & guard, time_point updateTime, bool outOfSchedule); void updateSnapshotMetrics(const MetricLockGuard & guard); void handleMetricsAltered(const MetricLockGuard & guard); - using SnapSpec = std::pair<uint32_t, std::string>; + using SnapSpec = std::pair<vespalib::duration, std::string>; static std::vector<SnapSpec> createSnapshotPeriods( const MetricsmanagerConfig& config); void assertMetricLockLocked(const MetricLockGuard& g) const; }; diff --git a/metrics/src/vespa/metrics/metricsnapshot.cpp b/metrics/src/vespa/metrics/metricsnapshot.cpp index 1580f340f0e..cd06fb731c2 100644 --- a/metrics/src/vespa/metrics/metricsnapshot.cpp +++ b/metrics/src/vespa/metrics/metricsnapshot.cpp @@ -6,39 +6,47 @@ #include <vespa/log/log.h> LOG_SETUP(".metrics.snapshot"); +using vespalib::to_string; +using vespalib::to_s; + + namespace metrics { +static constexpr system_time system_time_epoch = system_time(); + MetricSnapshot::MetricSnapshot(const Metric::String& name) : _name(name), _period(0), - _fromTime(0), - _toTime(0), + _fromTime(system_time_epoch), + _toTime(system_time_epoch), _snapshot(new MetricSet("top", {}, "", nullptr)), _metrics() { } -MetricSnapshot::MetricSnapshot(const Metric::String& name, uint32_t period, const MetricSet& source, bool copyUnset) +MetricSnapshot::MetricSnapshot(const Metric::String& name, vespalib::duration period, const MetricSet& source, bool copyUnset) : _name(name), _period(period), - _fromTime(0), - _toTime(0), + _fromTime(system_time_epoch), + _toTime(system_time_epoch), _snapshot(), _metrics() { - Metric* m = source.clone(_metrics, Metric::INACTIVE, 0, copyUnset); - assert(m->isMetricSet()); - _snapshot.reset(static_cast<MetricSet*>(m)); + _snapshot.reset(source.clone(_metrics, Metric::INACTIVE, 0, copyUnset)); _metrics.shrink_to_fit(); } MetricSnapshot::~MetricSnapshot() = default; void -MetricSnapshot::reset(time_t currentTime) +MetricSnapshot::reset() { + reset(system_time_epoch); +} +void +MetricSnapshot::reset(system_time currentTime) { _fromTime = currentTime; - _toTime = 0; + _toTime = system_time_epoch; _snapshot->reset(); } @@ -61,22 +69,19 @@ MetricSnapshot::addMemoryUsage(MemoryConsumption& mc) const { ++mc._snapshotCount; mc._snapshotName += mc.getStringMemoryUsage(_name, mc._snapshotNameUnique); - mc._snapshotMeta += sizeof(MetricSnapshot) - + _metrics.capacity() * sizeof(Metric::SP); + mc._snapshotMeta += sizeof(MetricSnapshot) + _metrics.capacity() * sizeof(Metric::SP); _snapshot->addMemoryUsage(mc); } -MetricSnapshotSet::MetricSnapshotSet( - const Metric::String& name, uint32_t period, - uint32_t count, const MetricSet& source, bool snapshotUnsetMetrics) +MetricSnapshotSet::MetricSnapshotSet(const Metric::String& name, vespalib::duration period, uint32_t count, + const MetricSet& source, bool snapshotUnsetMetrics) : _count(count), _builderCount(0), - _current(new MetricSnapshot(name, period, source, snapshotUnsetMetrics)), - _building(count == 1 ? 0 : new MetricSnapshot( - name, period, source, snapshotUnsetMetrics)) + _current(std::make_unique<MetricSnapshot>(name, period, source, snapshotUnsetMetrics)), + _building(count == 1 ? nullptr : new MetricSnapshot(name, period, source, snapshotUnsetMetrics)) { - _current->reset(0); - if (_building.get()) _building->reset(0); + _current->reset(); + if (_building.get()) _building->reset(); } MetricSnapshot& @@ -87,35 +92,32 @@ MetricSnapshotSet::getNextTarget() } bool -MetricSnapshotSet::haveCompletedNewPeriod(time_t newFromTime) +MetricSnapshotSet::haveCompletedNewPeriod(system_time newFromTime) { if (_count == 1) { _current->setToTime(newFromTime); return true; } _building->setToTime(newFromTime); - // If not time to roll yet, just return + // If not time to roll yet, just return if (++_builderCount < _count) return false; - // Building buffer done. Use that as current and reset current. - MetricSnapshot::UP tmp(std::move(_current)); - _current = std::move(_building); - _building = std::move(tmp); + // Building buffer done. Use that as current and reset current. + std::swap(_current, _building); _building->reset(newFromTime); _builderCount = 0; return true; } bool -MetricSnapshotSet::timeForAnotherSnapshot(time_t currentTime) { - time_t lastTime = getToTime(); - if (currentTime >= lastTime + getPeriod()) { - if (currentTime >= lastTime + 2 * getPeriod()) { - LOG(warning, "Metric snapshot set %s was asked if it was time for " - "another snapshot, a whole period beyond when it " - "should have been done (Last update was at time %lu" - ", current time is %lu and period is %u). " - "Clearing data and updating time to current time.", - getName().c_str(), lastTime, currentTime, getPeriod()); +MetricSnapshotSet::timeForAnotherSnapshot(system_time currentTime) { + system_time lastTime = getToTime(); + vespalib::duration period = getPeriod(); + if (currentTime >= lastTime + period) { + if (currentTime >= lastTime + 2 * period) { + LOG(warning, "Metric snapshot set %s was asked if it was time for another snapshot, a whole period beyond " + "when it should have been done (Last update was at time %s, current time is %s and period " + "is %f seconds). Clearing data and updating time to current time.", + getName().c_str(), to_string(lastTime).c_str(), to_string(currentTime).c_str(), to_s(getPeriod())); reset(currentTime); } return true; @@ -124,7 +126,7 @@ MetricSnapshotSet::timeForAnotherSnapshot(time_t currentTime) { } void -MetricSnapshotSet::reset(time_t currentTime) { +MetricSnapshotSet::reset(system_time currentTime) { if (_count != 1) _building->reset(currentTime); _current->reset(currentTime); _builderCount = 0; @@ -147,7 +149,7 @@ MetricSnapshotSet::addMemoryUsage(MemoryConsumption& mc) const } void -MetricSnapshotSet::setFromTime(time_t fromTime) +MetricSnapshotSet::setFromTime(system_time fromTime) { if (_count != 1) _building->setFromTime(fromTime); _current->setFromTime(fromTime); diff --git a/metrics/src/vespa/metrics/metricsnapshot.h b/metrics/src/vespa/metrics/metricsnapshot.h index cc6ec4cbb2e..945f9dc7326 100644 --- a/metrics/src/vespa/metrics/metricsnapshot.h +++ b/metrics/src/vespa/metrics/metricsnapshot.h @@ -15,54 +15,52 @@ namespace metrics { +using system_time = vespalib::system_time; + class MetricManager; class MetricSnapshot { Metric::String _name; // Period length of this snapshot - uint32_t _period; + vespalib::duration _period; // Time this snapshot was last updated. - time_t _fromTime; + system_time _fromTime; // If set to 0, use _fromTime + _period. - time_t _toTime; + system_time _toTime; // Keeps the metrics set view of the snapshot std::unique_ptr<MetricSet> _snapshot; // Snapshots must own their own metrics mutable std::vector<Metric::UP> _metrics; public: - using UP = std::unique_ptr<MetricSnapshot>; - using SP = std::shared_ptr<MetricSnapshot>; - /** Create a fresh empty top level snapshot. */ MetricSnapshot(const Metric::String& name); /** Create a snapshot of another metric source. */ - MetricSnapshot(const Metric::String& name, uint32_t period, + MetricSnapshot(const Metric::String& name, vespalib::duration period, const MetricSet& source, bool copyUnset); - virtual ~MetricSnapshot(); + ~MetricSnapshot(); - void addToSnapshot(MetricSnapshot& other, bool reset_, time_t currentTime) { + void addToSnapshot(MetricSnapshot& other, bool reset_, system_time currentTime) { _snapshot->addToSnapshot(other.getMetrics(), other._metrics); if (reset_) reset(currentTime); other._toTime = currentTime; } - void addToSnapshot(MetricSnapshot& other, time_t currentTime) const { + void addToSnapshot(MetricSnapshot& other, system_time currentTime) const { _snapshot->addToSnapshot(other.getMetrics(), other._metrics); other._toTime = currentTime; } - void setFromTime(time_t fromTime) { _fromTime = fromTime; } - void setToTime(time_t toTime) { _toTime = toTime; } + void setFromTime(system_time fromTime) { _fromTime = fromTime; } + void setToTime(system_time toTime) { _toTime = toTime; } const Metric::String& getName() const { return _name; } - uint32_t getPeriod() const { return _period; } - time_t getFromTime() const { return _fromTime; } - time_t getToTime() const { return _toTime; } - time_t getLength() const - { return (_toTime != 0 ? _toTime - _fromTime : _fromTime + _period); } + vespalib::duration getPeriod() const { return _period; } + system_time getFromTime() const { return _fromTime; } + system_time getToTime() const { return _toTime; } const MetricSet& getMetrics() const { return *_snapshot; } MetricSet& getMetrics() { return *_snapshot; } - void reset(time_t currentTime); + void reset(system_time currentTime); + void reset(); /** * Recreate snapshot by cloning given metric set and then add the data * from the old one. New metrics have been added. @@ -77,37 +75,42 @@ class MetricSnapshotSet { // before we have a full time window. uint32_t _builderCount; // Number of times we've currently added to the // building instance. - MetricSnapshot::UP _current; // The last full period - MetricSnapshot::UP _building; // The building period + std::unique_ptr<MetricSnapshot> _current; // The last full period + std::unique_ptr<MetricSnapshot> _building; // The building period public: - using SP = std::shared_ptr<MetricSnapshotSet>; - - MetricSnapshotSet(const Metric::String& name, uint32_t period, - uint32_t count, const MetricSet& source, - bool snapshotUnsetMetrics); + MetricSnapshotSet(const Metric::String& name, vespalib::duration period, uint32_t count, + const MetricSet& source, bool snapshotUnsetMetrics); const Metric::String& getName() const { return _current->getName(); } - uint32_t getPeriod() const { return _current->getPeriod(); } - time_t getFromTime() const { return _current->getFromTime(); } - time_t getToTime() const { return _current->getToTime(); } + vespalib::duration getPeriod() const { return _current->getPeriod(); } + system_time getFromTime() const { return _current->getFromTime(); } + system_time getToTime() const { return _current->getToTime(); } + system_time getNextWorkTime() const { return getToTime() + getPeriod(); } uint32_t getCount() const { return _count; } uint32_t getBuilderCount() const { return _builderCount; } - bool hasTemporarySnapshot() const { return (_building.get() != 0); } - MetricSnapshot& getSnapshot(bool temporary = false) - { return *((temporary && _count > 1) ? _building : _current); } - const MetricSnapshot& getSnapshot(bool temporary = false) const - { return *((temporary && _count > 1) ? _building : _current); } + MetricSnapshot& getSnapshot() { + return getSnapshot(false); + } + MetricSnapshot& getSnapshot(bool temporary) { + return *((temporary && _count > 1) ? _building : _current); + } + const MetricSnapshot& getSnapshot() const { + return getSnapshot(false); + } + const MetricSnapshot& getSnapshot(bool temporary) const { + return *((temporary && _count > 1) ? _building : _current); + } MetricSnapshot& getNextTarget(); - bool timeForAnotherSnapshot(time_t currentTime); - bool haveCompletedNewPeriod(time_t newFromTime); - void reset(time_t currentTime); + bool timeForAnotherSnapshot(system_time currentTime); + bool haveCompletedNewPeriod(system_time newFromTime); + void reset(system_time currentTime); /** * Recreate snapshot by cloning given metric set and then add the data * from the old one. New metrics have been added. */ void recreateSnapshot(const MetricSet& metrics, bool copyUnset); void addMemoryUsage(MemoryConsumption&) const; - void setFromTime(time_t fromTime); + void setFromTime(system_time fromTime); }; } // metrics diff --git a/metrics/src/vespa/metrics/state_api_adapter.cpp b/metrics/src/vespa/metrics/state_api_adapter.cpp index 61a1ce7c2a9..136ccf6e06a 100644 --- a/metrics/src/vespa/metrics/state_api_adapter.cpp +++ b/metrics/src/vespa/metrics/state_api_adapter.cpp @@ -9,12 +9,12 @@ namespace metrics { vespalib::string StateApiAdapter::getMetrics(const vespalib::string &consumer) { - metrics::MetricLockGuard guard(_manager.getMetricLock()); - std::vector<uint32_t> periods = _manager.getSnapshotPeriods(guard); + MetricLockGuard guard(_manager.getMetricLock()); + auto periods = _manager.getSnapshotPeriods(guard); if (periods.empty()) { return ""; // no configuration yet } - const metrics::MetricSnapshot &snapshot(_manager.getMetricSnapshot(guard, periods[0])); + const MetricSnapshot &snapshot(_manager.getMetricSnapshot(guard, periods[0])); vespalib::asciistream json; vespalib::JsonStream stream(json); metrics::JsonWriter metricJsonWriter(stream); @@ -26,17 +26,15 @@ StateApiAdapter::getMetrics(const vespalib::string &consumer) vespalib::string StateApiAdapter::getTotalMetrics(const vespalib::string &consumer) { - _manager.updateMetrics(true); - metrics::MetricLockGuard guard(_manager.getMetricLock()); + _manager.updateMetrics(); + MetricLockGuard guard(_manager.getMetricLock()); _manager.checkMetricsAltered(guard); - time_t currentTime = vespalib::count_s(vespalib::steady_clock::now().time_since_epoch()); - auto generated = std::make_unique<metrics::MetricSnapshot>( - "Total metrics from start until current time", 0, - _manager.getTotalMetricSnapshot(guard).getMetrics(), - true); + system_time currentTime = vespalib::system_clock::now(); + auto generated = std::make_unique<MetricSnapshot>("Total metrics from start until current time", 0s, + _manager.getTotalMetricSnapshot(guard).getMetrics(), true); _manager.getActiveMetrics(guard).addToSnapshot(*generated, false, currentTime); generated->setFromTime(_manager.getTotalMetricSnapshot(guard).getFromTime()); - const metrics::MetricSnapshot &snapshot = *generated; + const MetricSnapshot &snapshot = *generated; vespalib::asciistream json; vespalib::JsonStream stream(json); metrics::JsonWriter metricJsonWriter(stream); diff --git a/metrics/src/vespa/metrics/textwriter.cpp b/metrics/src/vespa/metrics/textwriter.cpp index 94a7b7df73b..fbb31e7013a 100644 --- a/metrics/src/vespa/metrics/textwriter.cpp +++ b/metrics/src/vespa/metrics/textwriter.cpp @@ -7,9 +7,11 @@ #include "valuemetric.h" #include <sstream> +using vespalib::to_string; + namespace metrics { -TextWriter::TextWriter(std::ostream& out, uint32_t period, +TextWriter::TextWriter(std::ostream& out, vespalib::duration period, const std::string& regex, bool verbose) : _period(period), _out(out), _regex(), _verbose(verbose) { @@ -19,14 +21,14 @@ TextWriter::TextWriter(std::ostream& out, uint32_t period, } } -TextWriter::~TextWriter() { } +TextWriter::~TextWriter() = default; bool TextWriter::visitSnapshot(const MetricSnapshot& snapshot) { _out << "snapshot \"" << snapshot.getName() << "\" from " - << snapshot.getFromTime() << " to " << snapshot.getToTime() - << " period " << snapshot.getPeriod(); + << to_string(snapshot.getFromTime()) << " to " << to_string(snapshot.getToTime()) + << " period " << vespalib::count_s(snapshot.getPeriod()); return true; } @@ -82,7 +84,7 @@ bool TextWriter::visitValueMetric(const AbstractValueMetric& m, bool) { if (writeCommon(m)) { - m.print(_out, _verbose, " ", _period); + m.print(_out, _verbose, " ", vespalib::count_s(_period)); } return true; } diff --git a/metrics/src/vespa/metrics/textwriter.h b/metrics/src/vespa/metrics/textwriter.h index f060429e931..f23d1cf585c 100644 --- a/metrics/src/vespa/metrics/textwriter.h +++ b/metrics/src/vespa/metrics/textwriter.h @@ -3,20 +3,21 @@ #pragma once #include "metric.h" +#include <vespa/vespalib/util/time.h> #include <regex> #include <optional> namespace metrics { class TextWriter : public MetricVisitor { - uint32_t _period; + vespalib::duration _period; std::ostream& _out; std::vector<std::string> _path; std::optional<std::regex> _regex; bool _verbose; public: - TextWriter(std::ostream& out, uint32_t period, + TextWriter(std::ostream& out, vespalib::duration period, const std::string& regex, bool verbose); ~TextWriter(); diff --git a/metrics/src/vespa/metrics/updatehook.h b/metrics/src/vespa/metrics/updatehook.h index 58d9ef0d743..997bdf0b5a4 100644 --- a/metrics/src/vespa/metrics/updatehook.h +++ b/metrics/src/vespa/metrics/updatehook.h @@ -26,17 +26,28 @@ private: class MetricManager; class UpdateHook { - const char* _name; - time_t _nextCall; - uint32_t _period; - friend class MetricManager; - public: using MetricLockGuard = metrics::MetricLockGuard; - UpdateHook(const char* name) : _name(name), _nextCall(0), _period(0) {} + UpdateHook(const char* name, vespalib::duration period) + : _name(name), + _period(period), + _nextCall() + {} virtual ~UpdateHook() = default; virtual void updateMetrics(const MetricLockGuard & guard) = 0; const char* getName() const { return _name; } + void updateNextCall() { updateNextCall(_nextCall); } + void updateNextCall(time_point now) { setNextCall(now + _period); } + bool is_periodic() const noexcept { return _period != vespalib::duration::zero(); } + bool expired(time_point now) { return _nextCall <= now; } + bool has_valid_expiry() const noexcept { return _nextCall != time_point(); } + vespalib::duration getPeriod() const noexcept { return _period; } + time_point getNextCall() const noexcept { return _nextCall; } + void setNextCall(time_point now) { _nextCall = now; } +private: + const char* _name; + const vespalib::duration _period; + time_point _nextCall; }; } diff --git a/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionReference.java b/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionReference.java index 46134074137..34e34a3341d 100644 --- a/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionReference.java +++ b/model-evaluation/src/main/java/ai/vespa/models/evaluation/FunctionReference.java @@ -2,6 +2,7 @@ package ai.vespa.models.evaluation; import com.yahoo.collections.Pair; +import static com.yahoo.searchlib.rankingexpression.Reference.wrapInRankingExpression; import java.util.Objects; import java.util.Optional; @@ -51,7 +52,8 @@ class FunctionReference { } String serialForm() { - return "rankingExpression(" + name + (instance != null ? instance : "") + ")"; + String extra = (instance != null ? instance : ""); + return wrapInRankingExpression(name + extra); } @Override diff --git a/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java b/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java index 81325740218..47c246c008e 100644 --- a/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java +++ b/model-evaluation/src/main/java/ai/vespa/models/evaluation/LazyArrayContext.java @@ -16,6 +16,7 @@ import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; import com.yahoo.stream.CustomCollectors; import com.yahoo.tensor.Tensor; import com.yahoo.tensor.TensorType; +import static com.yahoo.searchlib.rankingexpression.Reference.RANKING_EXPRESSION_WRAPPER; import java.util.Arrays; import java.util.HashMap; @@ -233,7 +234,11 @@ public final class LazyArrayContext extends Context implements ContextIndex { List<OnnxModel> onnxModels, Map<String, OnnxModel> onnxModelsInUse) { if (isFunctionReference(node)) { - FunctionReference reference = FunctionReference.fromSerial(node.toString()).get(); + var opt = FunctionReference.fromSerial(node.toString()); + if (opt.isEmpty()) { + throw new IllegalArgumentException("Could not extract function " + node + " from serialized form '" + node.toString() +"'"); + } + FunctionReference reference = opt.get(); bindTargets.add(reference.serialForm()); ExpressionFunction function = functions.get(reference); @@ -313,7 +318,7 @@ public final class LazyArrayContext extends Context implements ContextIndex { private boolean isFunctionReference(ExpressionNode node) { if ( ! (node instanceof ReferenceNode reference)) return false; - return reference.getName().equals("rankingExpression") && reference.getArguments().size() == 1; + return reference.getName().equals(RANKING_EXPRESSION_WRAPPER) && reference.getArguments().size() == 1; } private boolean isOnnx(ExpressionNode node) { diff --git a/model-evaluation/src/test/java/ai/vespa/models/evaluation/RankProfileImportingTest.java b/model-evaluation/src/test/java/ai/vespa/models/evaluation/RankProfileImportingTest.java index 1a6f6925caf..c5084166c1f 100644 --- a/model-evaluation/src/test/java/ai/vespa/models/evaluation/RankProfileImportingTest.java +++ b/model-evaluation/src/test/java/ai/vespa/models/evaluation/RankProfileImportingTest.java @@ -49,4 +49,9 @@ public class RankProfileImportingTest { assertEquals("tensor()", rt.get().toString()); } + @Test + public void testImportingExpressionsAsArguments() { + ModelTester tester = new ModelTester("src/test/resources/config/expressions-as-arguments/"); + assertEquals(3, tester.models().size()); + } } diff --git a/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp b/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp index 1ccb3956fc2..4f7e0e66d9f 100644 --- a/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp +++ b/searchcore/src/vespa/searchcore/proton/metrics/metrics_engine.cpp @@ -42,7 +42,7 @@ MetricsEngine::start(const config::ConfigUri &) void MetricsEngine::addMetricsHook(metrics::UpdateHook &hook) { - _manager->addMetricUpdateHook(hook, 5); + _manager->addMetricUpdateHook(hook); } void diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp index 9a84383c2f4..3f28f75c521 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp @@ -94,7 +94,7 @@ class MetricsUpdateHook : public metrics::UpdateHook { DocumentDB &_db; public: explicit MetricsUpdateHook(DocumentDB &s) - : metrics::UpdateHook("documentdb-hook"), + : metrics::UpdateHook("documentdb-hook", 5s), _db(s) {} void updateMetrics(const MetricLockGuard & guard) override { diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp index c508706ad28..8ec904760cf 100644 --- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp @@ -136,7 +136,7 @@ struct MetricsUpdateHook : metrics::UpdateHook { Proton &self; explicit MetricsUpdateHook(Proton &s) - : metrics::UpdateHook("proton-hook"), + : metrics::UpdateHook("proton-hook", 5s), self(s) {} void updateMetrics(const MetricLockGuard &guard) override { diff --git a/searchlib/abi-spec.json b/searchlib/abi-spec.json index 5413907e967..be0414421fe 100644 --- a/searchlib/abi-spec.json +++ b/searchlib/abi-spec.json @@ -364,6 +364,8 @@ "public boolean equals(java.lang.Object)", "public int hashCode()", "public java.lang.String toString()", + "public static java.lang.String wrapInRankingExpression(java.lang.String)", + "public boolean isSimpleRankingExpressionWrapper()", "public java.lang.StringBuilder toString(java.lang.StringBuilder, com.yahoo.searchlib.rankingexpression.rule.SerializationContext, java.util.Deque, com.yahoo.searchlib.rankingexpression.rule.CompositeNode)", "public int compareTo(com.yahoo.searchlib.rankingexpression.Reference)", "public static com.yahoo.searchlib.rankingexpression.Reference fromIdentifier(java.lang.String)", @@ -371,7 +373,9 @@ "public static java.util.Optional simple(java.lang.String)", "public bridge synthetic int compareTo(java.lang.Object)" ], - "fields" : [ ] + "fields" : [ + "public static final java.lang.String RANKING_EXPRESSION_WRAPPER" + ] }, "com.yahoo.searchlib.rankingexpression.evaluation.AbstractArrayContext" : { "superClass" : "com.yahoo.searchlib.rankingexpression.evaluation.Context", diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/ExpressionFunction.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/ExpressionFunction.java index 171151bfdf4..c7d69d7a36a 100755 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/ExpressionFunction.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/ExpressionFunction.java @@ -10,6 +10,7 @@ import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; import com.yahoo.searchlib.rankingexpression.rule.SerializationContext; import com.yahoo.tensor.TensorType; import com.yahoo.text.Utf8; +import static com.yahoo.searchlib.rankingexpression.Reference.wrapInRankingExpression; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -142,7 +143,7 @@ public class ExpressionFunction { if (shouldGenerateFeature(expr)) { String funcName = "autogenerated_ranking_feature@" + Long.toHexString(symbolCode(key + "=" + binding)); context.addFunctionSerialization(RankingExpression.propertyName(funcName), binding); - binding = "rankingExpression(" + funcName + ")"; + binding = wrapInRankingExpression(funcName); } argumentBindings.put(key, binding); } diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java index c9f818544e3..c6de04ed755 100755 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/RankingExpression.java @@ -11,6 +11,7 @@ import com.yahoo.searchlib.rankingexpression.rule.SerializationContext; import com.yahoo.tensor.TensorType; import com.yahoo.tensor.evaluation.TypeContext; import com.yahoo.text.Text; +import static com.yahoo.searchlib.rankingexpression.Reference.RANKING_EXPRESSION_WRAPPER; import java.io.File; import java.io.FileNotFoundException; @@ -80,7 +81,7 @@ public class RankingExpression implements Serializable { private String name = ""; private ExpressionNode root; - private final static String RANKEXPRESSION = "rankingExpression("; + private final static String RANKEXPRESSION = RANKING_EXPRESSION_WRAPPER + "("; private final static String RANKINGSCRIPT = ").rankingScript"; private final static String EXPRESSION_NAME = ").expressionName"; diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java index eaecdf78162..64b251c0cd4 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java @@ -120,17 +120,30 @@ public class Reference extends Name implements Comparable<Reference> { return toString(new StringBuilder(), new SerializationContext(), null, null).toString(); } + public static final String RANKING_EXPRESSION_WRAPPER = "rankingExpression"; + + public static String wrapInRankingExpression(String name) { + return RANKING_EXPRESSION_WRAPPER + "(" + name + ")"; + } + + public boolean isSimpleRankingExpressionWrapper() { + return name().equals(RANKING_EXPRESSION_WRAPPER) && isSimple(); + } + public StringBuilder toString(StringBuilder b, SerializationContext context, Deque<String> path, CompositeNode parent) { b.append(name()); if (arguments.expressions().size() > 0) { b.append("("); - for (int i = 0; i < arguments.expressions().size(); i++) { - ExpressionNode e = arguments.expressions().get(i); - e.toString(b, context, path, parent); - if (i+1 < arguments.expressions().size()) { - b.append(','); + if (isSimpleRankingExpressionWrapper()) { + b.append(simpleArgument().get()); + } else { + for (int i = 0; i < arguments.expressions().size(); i++) { + ExpressionNode e = arguments.expressions().get(i); + e.toString(b, context, path, parent); + if (i+1 < arguments.expressions().size()) { + b.append(','); + } } - } b.append(")"); } diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java index 85a12a49958..ec377c6f5d9 100755 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java @@ -8,6 +8,7 @@ import com.yahoo.searchlib.rankingexpression.evaluation.Context; import com.yahoo.searchlib.rankingexpression.evaluation.Value; import com.yahoo.tensor.TensorType; import com.yahoo.tensor.evaluation.TypeContext; +import static com.yahoo.searchlib.rankingexpression.Reference.wrapInRankingExpression; import java.util.ArrayDeque; import java.util.Deque; @@ -95,7 +96,7 @@ public final class ReferenceNode extends CompositeNode { context.addFunctionTypeSerialization(functionName, function.returnType().get()); } path.removeLast(); - return string.append("rankingExpression(").append(functionName).append(')'); + return string.append(wrapInRankingExpression(functionName)); } // Not resolved in this context: output as-is diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SerializationContext.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SerializationContext.java index 7d0c0b98910..e2fffd824b9 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SerializationContext.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SerializationContext.java @@ -6,6 +6,7 @@ import com.yahoo.searchlib.rankingexpression.RankingExpression; import com.yahoo.searchlib.rankingexpression.Reference; import com.yahoo.tensor.TensorType; import com.yahoo.tensor.evaluation.TypeContext; +import static com.yahoo.searchlib.rankingexpression.Reference.wrapInRankingExpression; import java.util.Collection; import java.util.Collections; @@ -97,13 +98,13 @@ public class SerializationContext extends FunctionReferenceContext { /** Adds the serialization of the argument type to a function */ public void addArgumentTypeSerialization(String functionName, String argumentName, TensorType type) { - serializedFunctions.put("rankingExpression(" + functionName + ")." + argumentName + ".type", type.toString()); + serializedFunctions.put(wrapInRankingExpression(functionName) + "." + argumentName + ".type", type.toString()); } /** Adds the serialization of the return type of a function */ public void addFunctionTypeSerialization(String functionName, TensorType type) { if (type.rank() == 0) return; // no explicit type implies scalar (aka rank 0 tensor) - serializedFunctions.put("rankingExpression(" + functionName + ").type", type.toString()); + serializedFunctions.put(wrapInRankingExpression(functionName) + ".type", type.toString()); } @Override diff --git a/searchlib/src/tests/features/tensor/tensor_test.cpp b/searchlib/src/tests/features/tensor/tensor_test.cpp index 54807273aea..5ad30c61c37 100644 --- a/searchlib/src/tests/features/tensor/tensor_test.cpp +++ b/searchlib/src/tests/features/tensor/tensor_test.cpp @@ -121,7 +121,7 @@ struct ExecFixture .add({{"x", "b"}}, 5) .add({{"x", "c"}}, 7)); tensorAttr->setTensor(1, *doc_tensor); - directAttr->set_tensor(1, std::move(doc_tensor)); + directAttr->setTensor(1, *doc_tensor); for (const auto &attr : attrs) { attr->commit(); diff --git a/searchlib/src/vespa/searchlib/tensor/direct_tensor_attribute.h b/searchlib/src/vespa/searchlib/tensor/direct_tensor_attribute.h index d331cdca440..73bd929aee6 100644 --- a/searchlib/src/vespa/searchlib/tensor/direct_tensor_attribute.h +++ b/searchlib/src/vespa/searchlib/tensor/direct_tensor_attribute.h @@ -14,6 +14,7 @@ class DirectTensorAttribute final : public TensorAttribute { DirectTensorStore _direct_store; + void set_tensor(DocId docId, std::unique_ptr<vespalib::eval::Value> tensor); public: DirectTensorAttribute(vespalib::stringref baseFileName, const Config &cfg, const NearestNeighborIndexFactory& index_factory = DefaultNearestNeighborIndexFactory()); ~DirectTensorAttribute() override; @@ -21,7 +22,6 @@ public: void update_tensor(DocId docId, const document::TensorUpdate &update, bool create_empty_if_non_existing) override; - void set_tensor(DocId docId, std::unique_ptr<vespalib::eval::Value> tensor); const vespalib::eval::Value &get_tensor_ref(DocId docId) const override; bool supports_get_tensor_ref() const override { return true; } diff --git a/storage/src/tests/common/metricstest.cpp b/storage/src/tests/common/metricstest.cpp index 7231a071319..63d7be7c499 100644 --- a/storage/src/tests/common/metricstest.cpp +++ b/storage/src/tests/common/metricstest.cpp @@ -185,7 +185,7 @@ void MetricsTest::createFakeLoad() } _clock->addSecondsToTime(60); _metricManager->timeChangedNotification(); - while (int64_t(_metricManager->getLastProcessedTime()) < vespalib::count_s(_clock->getMonotonicTime().time_since_epoch())) { + while (_metricManager->getLastProcessedTime() < _clock->getSystemTime()) { std::this_thread::sleep_for(5ms); _metricManager->timeChangedNotification(); } @@ -235,7 +235,7 @@ TEST_F(MetricsTest, snapshot_presenting) { for (uint32_t i=0; i<6; ++i) { _clock->addSecondsToTime(60); _metricManager->timeChangedNotification(); - while (int64_t(_metricManager->getLastProcessedTime()) < vespalib::count_s(_clock->getMonotonicTime().time_since_epoch())) { + while (_metricManager->getLastProcessedTime() < _clock->getSystemTime()) { std::this_thread::sleep_for(1ms); } } @@ -295,8 +295,8 @@ MetricsTest::createSnapshotForPeriod(std::chrono::seconds secs) const { _clock->addSecondsToTime(secs.count()); _metricManager->timeChangedNotification(); - while (int64_t(_metricManager->getLastProcessedTime()) < vespalib::count_s(_clock->getMonotonicTime().time_since_epoch())) { - std::this_thread::sleep_for(100ms); + while (_metricManager->getLastProcessedTime() < _clock->getSystemTime()) { + std::this_thread::sleep_for(5ms); } } diff --git a/storage/src/tests/storageserver/statereportertest.cpp b/storage/src/tests/storageserver/statereportertest.cpp index 3a772c1ddde..380604ae77b 100644 --- a/storage/src/tests/storageserver/statereportertest.cpp +++ b/storage/src/tests/storageserver/statereportertest.cpp @@ -211,7 +211,7 @@ TEST_F(StateReporterTest, report_metrics) { for (uint32_t i = 0; i < 6; ++i) { _clock->addSecondsToTime(60); _metricManager->timeChangedNotification(); - while (int64_t(_metricManager->getLastProcessedTime()) < vespalib::count_s(_clock->getMonotonicTime().time_since_epoch())) { + while (_metricManager->getLastProcessedTime() < _clock->getSystemTime()) { std::this_thread::sleep_for(1ms); } } diff --git a/storage/src/vespa/storage/common/statusmetricconsumer.cpp b/storage/src/vespa/storage/common/statusmetricconsumer.cpp index c6f73540605..68866027cd1 100644 --- a/storage/src/vespa/storage/common/statusmetricconsumer.cpp +++ b/storage/src/vespa/storage/common/statusmetricconsumer.cpp @@ -51,17 +51,9 @@ bool StatusMetricConsumer::reportStatus(std::ostream& out, const framework::HttpUrlPath& path) const { - // Update metrics unless 'dontcallupdatehooks' is 1. Update - // snapshot metrics too, if callsnapshothooks is set to 1. - if (path.get("dontcallupdatehooks", 0) == 0) { - bool updateSnapshotHooks = path.get("callsnapshothooks", 0) == 1; - LOG(debug, "Updating metrics ahead of status page view%s", - updateSnapshotHooks ? ", calling snapshot hooks too" : "."); - _manager.updateMetrics(updateSnapshotHooks); - } else { - LOG(debug, "Not calling update hooks as dontcallupdatehooks option has been given"); - } - int64_t currentTimeS(vespalib::count_s(_component.getClock().getMonotonicTime().time_since_epoch())); + _manager.updateMetrics(); + + vespalib::system_time currentTime = _component.getClock().getSystemTime(); bool json = (path.getAttribute("format") == "json"); int verbosity(path.get("verbosity", 0)); @@ -72,52 +64,53 @@ StatusMetricConsumer::reportStatus(std::ostream& out, if (path.hasAttribute("task") && path.getAttribute("task") == "reset") { std::lock_guard guard(_lock); - _manager.reset(currentTimeS); + _manager.reset(currentTime); } if (path.hasAttribute("interval")) { // Grab the snapshot we want to view more of - int32_t interval(boost::lexical_cast<int32_t>(path.getAttribute("interval"))); + int32_t intervalS(boost::lexical_cast<int32_t>(path.getAttribute("interval"))); metrics::MetricLockGuard metricLock(_manager.getMetricLock()); std::unique_ptr<metrics::MetricSnapshot> generated; const metrics::MetricSnapshot* snapshot; - if (interval == -2) { + if (intervalS == -2) { snapshot = &_manager.getActiveMetrics(metricLock); - _manager.getActiveMetrics(metricLock).setToTime(currentTimeS); - } else if (interval == -1) { + _manager.getActiveMetrics(metricLock).setToTime(currentTime); + } else if (intervalS == -1) { // "Prime" the metric structure by first fetching the set of active // metrics (complete with structure) and resetting these. This // leaves us with an empty metrics set to which we can (in order) // add the total and the active metrics. If this is not done, non- // written metrics won't be included even if copyUnset is true. generated = std::make_unique<metrics::MetricSnapshot>( - "Total metrics from start until current time", 0, + "Total metrics from start until current time", 0s, _manager.getActiveMetrics(metricLock).getMetrics(), copyUnset); - generated->reset(0); - _manager.getTotalMetricSnapshot(metricLock).addToSnapshot(*generated, currentTimeS); - _manager.getActiveMetrics(metricLock).addToSnapshot(*generated, currentTimeS); + generated->reset(); + _manager.getTotalMetricSnapshot(metricLock).addToSnapshot(*generated, currentTime); + _manager.getActiveMetrics(metricLock).addToSnapshot(*generated, currentTime); generated->setFromTime(_manager.getTotalMetricSnapshot(metricLock).getFromTime()); snapshot = generated.get(); - } else if (interval == 0) { + } else if (intervalS == 0) { if (copyUnset) { generated = std::make_unique<metrics::MetricSnapshot>( - _manager.getTotalMetricSnapshot(metricLock).getName(), 0, + _manager.getTotalMetricSnapshot(metricLock).getName(), 0s, _manager.getActiveMetrics(metricLock).getMetrics(), true); - generated->reset(0); - _manager.getTotalMetricSnapshot(metricLock).addToSnapshot(*generated, currentTimeS); + generated->reset(); + _manager.getTotalMetricSnapshot(metricLock).addToSnapshot(*generated, currentTime); snapshot = generated.get(); } else { snapshot = &_manager.getTotalMetricSnapshot(metricLock); } } else { + vespalib::duration interval = vespalib::from_s(intervalS); if (copyUnset) { generated = std::make_unique<metrics::MetricSnapshot>( - _manager.getMetricSnapshot(metricLock, interval).getName(), 0, + _manager.getMetricSnapshot(metricLock, interval).getName(), 0s, _manager.getActiveMetrics(metricLock).getMetrics(), true); - generated->reset(0); + generated->reset(); _manager.getMetricSnapshot(metricLock, interval, temporarySnap) - .addToSnapshot(*generated, currentTimeS); + .addToSnapshot(*generated, currentTime); snapshot = generated.get(); } else { snapshot = &_manager.getMetricSnapshot(metricLock, interval, temporarySnap); diff --git a/storage/src/vespa/storage/storageserver/statemanager.cpp b/storage/src/vespa/storage/storageserver/statemanager.cpp index c59f7797bb8..654fe0e1f5d 100644 --- a/storage/src/vespa/storage/storageserver/statemanager.cpp +++ b/storage/src/vespa/storage/storageserver/statemanager.cpp @@ -18,7 +18,6 @@ #include <vespa/vespalib/util/string_escape.h> #include <vespa/vespalib/util/stringfmt.h> #include <fstream> -#include <ranges> #include <vespa/log/log.h> LOG_SETUP(".state.manager"); @@ -545,10 +544,9 @@ StateManager::getNodeInfo() const stream << "metrics"; try { metrics::MetricLockGuard lock(_metricManager.getMetricLock()); - std::vector<uint32_t> periods(_metricManager.getSnapshotPeriods(lock)); + auto periods(_metricManager.getSnapshotPeriods(lock)); if (!periods.empty()) { - uint32_t period = periods[0]; - const metrics::MetricSnapshot& snapshot(_metricManager.getMetricSnapshot(lock, period)); + const metrics::MetricSnapshot& snapshot(_metricManager.getMetricSnapshot(lock, periods[0])); metrics::JsonWriter metricJsonWriter(stream); _metricManager.visit(lock, snapshot, metricJsonWriter, "fleetcontroller"); } else { diff --git a/storage/src/vespa/storage/storageserver/statereporter.cpp b/storage/src/vespa/storage/storageserver/statereporter.cpp index 205a2d710a6..d9e79d3b7b4 100644 --- a/storage/src/vespa/storage/storageserver/statereporter.cpp +++ b/storage/src/vespa/storage/storageserver/statereporter.cpp @@ -69,11 +69,11 @@ vespalib::string StateReporter::getMetrics(const vespalib::string &consumer) { metrics::MetricLockGuard guard(_manager.getMetricLock()); - std::vector<uint32_t> periods = _manager.getSnapshotPeriods(guard); + auto periods = _manager.getSnapshotPeriods(guard); if (periods.empty()) { return ""; // no configuration yet } - uint32_t interval = periods[0]; + vespalib::duration interval = periods[0]; // To get unset metrics, we have to copy active metrics, clear them // and then assign the snapshot @@ -81,9 +81,8 @@ StateReporter::getMetrics(const vespalib::string &consumer) _manager.getMetricSnapshot(guard, interval).getName(), interval, _manager.getActiveMetrics(guard).getMetrics(), true); - snapshot.reset(0); - _manager.getMetricSnapshot(guard, interval).addToSnapshot( - snapshot, vespalib::count_s(_component.getClock().getSystemTime().time_since_epoch())); + snapshot.reset(); + _manager.getMetricSnapshot(guard, interval).addToSnapshot(snapshot, _component.getClock().getSystemTime()); vespalib::asciistream json; vespalib::JsonStream stream(json); diff --git a/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp b/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp index 40c2cc3b111..74d58244636 100644 --- a/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp +++ b/storage/src/vespa/storageframework/defaultimplementation/component/componentregisterimpl.cpp @@ -126,8 +126,8 @@ namespace { struct MetricHookWrapper : public metrics::UpdateHook { MetricUpdateHook& _hook; - MetricHookWrapper(vespalib::stringref name, MetricUpdateHook& hook) - : metrics::UpdateHook(name.data()), // Expected to point to static name + MetricHookWrapper(vespalib::stringref name, MetricUpdateHook& hook, vespalib::duration period) + : metrics::UpdateHook(name.data(), period), // Expected to point to static name _hook(hook) { } @@ -142,8 +142,8 @@ ComponentRegisterImpl::registerUpdateHook(vespalib::stringref name, vespalib::duration period) { std::lock_guard lock(_componentLock); - auto hookPtr = std::make_unique<MetricHookWrapper>(name, hook); - _metricManager->addMetricUpdateHook(*hookPtr, vespalib::to_s(period)); + auto hookPtr = std::make_unique<MetricHookWrapper>(name, hook, period); + _metricManager->addMetricUpdateHook(*hookPtr); _hooks.emplace_back(std::move(hookPtr)); } diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java index 438248f31a7..b44fe82a303 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java @@ -188,16 +188,12 @@ class ClientFeederV3 { outstandingOperations.incrementAndGet(); updateOpsPerSec(); log(Level.FINE, "Sent message successfully, document id: ", message.get().getOperationId()); - } else if (!result.getError().isFatal()) { - repliesFromOldMessages.add(createOperationStatus(message.get().getOperationId(), - result.getError().getMessage(), - ErrorCode.TRANSIENT_ERROR, - message.get().getMessage())); } else { - repliesFromOldMessages.add(createOperationStatus(message.get().getOperationId(), - result.getError().getMessage(), - ErrorCode.ERROR, - message.get().getMessage())); + var err = result.getError(); + var msg = message.get(); + repliesFromOldMessages.add( + createOperationStatus( + msg.getOperationId(), err.getMessage(), ErrorCode.fromBusError(err), msg.getMessage())); } } } diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ErrorCode.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ErrorCode.java index f819ecccbb1..90c6ffd042d 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ErrorCode.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ErrorCode.java @@ -1,6 +1,11 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.http.server; +import com.yahoo.messagebus.Error; + +import java.util.Collection; +import java.util.Set; + /** * Return types for the server. * @@ -14,6 +19,15 @@ enum ErrorCode { TRANSIENT_ERROR(false, true), END_OF_FEED(true, true); + private static final Collection<Integer> MBUS_FATALS_HANDLED_AS_TRANSIENT = Set.of( + com.yahoo.messagebus.ErrorCode.SEND_QUEUE_CLOSED, + com.yahoo.messagebus.ErrorCode.ILLEGAL_ROUTE, + com.yahoo.messagebus.ErrorCode.NO_SERVICES_FOR_ROUTE, + com.yahoo.messagebus.ErrorCode.NETWORK_ERROR, + com.yahoo.messagebus.ErrorCode.SEQUENCE_ERROR, + com.yahoo.messagebus.ErrorCode.NETWORK_SHUTDOWN, + com.yahoo.messagebus.ErrorCode.TIMEOUT); + private final boolean success; private final boolean _transient; @@ -30,4 +44,9 @@ enum ErrorCode { return _transient; } + static ErrorCode fromBusError(Error mbusError) { + return mbusError.isFatal() && !MBUS_FATALS_HANDLED_AS_TRANSIENT.contains(mbusError.getCode()) + ? ERROR : TRANSIENT_ERROR; + } + } diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedReplyReader.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedReplyReader.java index 377e91f6490..b504de64c63 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedReplyReader.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedReplyReader.java @@ -50,7 +50,7 @@ public class FeedReplyReader implements ReplyHandler { DocumentOperationStatus status = DocumentOperationStatus.fromMessageBusErrorCodes(reply.getErrorCodes()); metricsHelper.reportFailure(type, status); metric.add(MetricNames.FAILED, 1, null); - enqueue(context, reply.getError(0).getMessage(), ErrorCode.ERROR, false, reply.getTrace()); + enqueue(context, reply.getError(0).getMessage(), ErrorCode.fromBusError(reply.getError(0)), false, reply.getTrace()); } else { metricsHelper.reportSuccessful(type, latencyInSeconds); if ( ! conditionMet) diff --git a/vespalib/src/tests/fastos/file_test.cpp b/vespalib/src/tests/fastos/file_test.cpp index d0f8bbfd98b..6b58a4a1fd8 100644 --- a/vespalib/src/tests/fastos/file_test.cpp +++ b/vespalib/src/tests/fastos/file_test.cpp @@ -1,555 +1,233 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include "tests.h" #include <vespa/fastos/file.h> +#include <vespa/vespalib/gtest/gtest.h> #include <memory> #include <cassert> #include <sys/mman.h> #include <filesystem> -class FileTest : public BaseTest -{ -public: - const std::string srcDir = getenv("SOURCE_DIRECTORY") ? getenv("SOURCE_DIRECTORY") : "."; - const std::string roFilename = srcDir + "/hello.txt"; - const std::string woFilename = "generated/writeonlytest.txt"; - const std::string rwFilename = "generated/readwritetest.txt"; +const std::string srcDir = getenv("SOURCE_DIRECTORY") ? getenv("SOURCE_DIRECTORY") : "."; +const std::string roFilename = srcDir + "/hello.txt"; +const std::string woFilename = "generated/writeonlytest.txt"; +const std::string rwFilename = "generated/readwritetest.txt"; - void GetCurrentDirTest () - { - TestHeader ("Get Current Directory Test"); - - std::string currentDir = FastOS_File::getCurrentDirectory(); - - Progress(!currentDir.empty(), - "Current dir: %s", !currentDir.empty() ? - currentDir.c_str() : "<failed>"); - - bool dirrc = FastOS_File::SetCurrentDirectory(".."); - - std::string parentDir; - - if (dirrc) { - parentDir = FastOS_File::getCurrentDirectory(); - } - - Progress(dirrc && strcmp(currentDir.c_str(), parentDir.c_str()) != 0, - "Parent dir: %s", !parentDir.empty() ? - parentDir.c_str() : "<failed>"); - - dirrc = FastOS_File::SetCurrentDirectory(currentDir.c_str()); +// create and remove 'generated' sub-directory +struct Generated { + Generated() { std::filesystem::create_directory(std::filesystem::path("generated")); } + ~Generated() { std::filesystem::remove_all(std::filesystem::path("generated")); } +}; - Progress(dirrc, "Changed back to working directory."); +TEST(FileTest, GetCurrentDirTest) { + std::string currentDir = FastOS_File::getCurrentDirectory(); + EXPECT_FALSE(currentDir.empty()); + EXPECT_TRUE(FastOS_File::SetCurrentDirectory("..")); + std::string parentDir = FastOS_File::getCurrentDirectory(); + EXPECT_FALSE(parentDir.empty()); + EXPECT_NE(currentDir, parentDir); + EXPECT_TRUE(FastOS_File::SetCurrentDirectory(currentDir.c_str())); +} - PrintSeparator(); +void MemoryMapTestImpl(int mmap_flags) { + Generated guard; + const int bufSize = 1000; + FastOS_File file("generated/memorymaptest"); + ASSERT_TRUE(file.OpenReadWrite()); + std::vector<char> space(bufSize); + char *buffer = space.data(); + for (int i = 0; i < bufSize; i++) { + buffer[i] = i % 256; } - - void MemoryMapTest (int mmap_flags) - { - TestHeader ("Memory Map Test"); - - int i; - const int bufSize = 1000; - - std::filesystem::create_directory(std::filesystem::path("generated")); - FastOS_File file("generated/memorymaptest"); - - bool rc = file.OpenReadWrite(); - Progress(rc, "Opening file 'generated/memorymaptest'"); - - if (rc) { - char *buffer = new char [bufSize]; - for (i = 0; i < bufSize; i++) { - buffer[i] = i % 256; - } - ssize_t wroteB = file.Write2(buffer, bufSize); - Progress(wroteB == bufSize, "Writing %d bytes to file", bufSize); - - bool close_ok = file.Close(); - assert(close_ok); - file.enableMemoryMap(mmap_flags); - - rc = file.OpenReadOnly(); - - Progress(rc, "Opening file 'generated/memorymaptest' read-only"); - if (rc) { - bool mmapEnabled; - char *mmapBuffer = nullptr; - - mmapEnabled = file.IsMemoryMapped(); - mmapBuffer = static_cast<char *>(file.MemoryMapPtr(0)); - - Progress(rc, "Memory mapping %s", - mmapEnabled ? "enabled" : "disabled"); - Progress(rc, "Map address: 0x%p", mmapBuffer); - - if (mmapEnabled) { - rc = 0; - for (i = 0; i < bufSize; i++) { - rc |= (mmapBuffer[i] == i % 256); - } - Progress(rc, "Reading %d bytes from memory map", bufSize); - } - } - delete [] buffer; + EXPECT_EQ(file.Write2(buffer, bufSize), bufSize); + bool close_ok = file.Close(); + assert(close_ok); + file.enableMemoryMap(mmap_flags); + ASSERT_TRUE(file.OpenReadOnly()); + bool mmapEnabled = file.IsMemoryMapped(); + char *mmapBuffer = static_cast<char *>(file.MemoryMapPtr(0)); + fprintf(stderr, "Memory mapping %s\n", mmapEnabled ? "enabled" : "disabled"); + fprintf(stderr, "Map address: 0x%p\n", mmapBuffer); + if (mmapEnabled) { + for (int i = 0; i < bufSize; i++) { + EXPECT_EQ(mmapBuffer[i], char(i % 256)); } - std::filesystem::remove_all(std::filesystem::path("generated")); - PrintSeparator(); } +} - void DirectIOTest () - { - TestHeader ("Direct Disk IO Test"); - - int i; - const int bufSize = 40000; - - std::filesystem::create_directory(std::filesystem::path("generated")); - FastOS_File file("generated/diotest"); - - bool rc = file.OpenWriteOnly(); - Progress(rc, "Opening file 'generated/diotest' write-only"); - - if (rc) { - char *buffer = new char [bufSize]; - - for (i=0; i<bufSize; i++) { - buffer[i] = 'A' + (i % 17); - } - ssize_t wroteB = file.Write2(buffer, bufSize); - Progress(wroteB == bufSize, "Writing %d bytes to file", bufSize); - - bool close_ok = file.Close(); - assert(close_ok); - - if (rc) { - file.EnableDirectIO(); - - rc = file.OpenReadOnly(); - Progress(rc, "Opening file 'generated/diotest' read-only"); - if (rc) { - bool dioEnabled; - size_t memoryAlignment=0; - size_t transferGranularity=0; - size_t transferMaximum=0; - - dioEnabled = file.GetDirectIORestrictions(memoryAlignment, - transferGranularity, - transferMaximum); - - Progress(rc, "DirectIO %s", dioEnabled ? "enabled" : "disabled"); - Progress(rc, "Memory alignment: %u bytes", memoryAlignment); - Progress(rc, "Transfer granularity: %u bytes", transferGranularity); - Progress(rc, "Transfer maximum: %u bytes", transferMaximum); - - if (dioEnabled) { - int eachRead = (8192 + transferGranularity - 1) / transferGranularity; - - char *buffer2 = new char [(eachRead * transferGranularity + - memoryAlignment - 1)]; - char *alignPtr = buffer2; - unsigned int align = - static_cast<unsigned int> - (reinterpret_cast<unsigned long>(alignPtr) & - (memoryAlignment - 1)); - if (align != 0) { - alignPtr = &alignPtr[memoryAlignment - align]; - } - int residue = bufSize; - int pos=0; - while (residue > 0) { - int readThisTime = eachRead * transferGranularity; - if (readThisTime > residue) { - readThisTime = residue; - } - file.ReadBuf(alignPtr, readThisTime, pos); - - for (i=0; i<readThisTime; i++) { - rc = (alignPtr[i] == 'A' + ((i+pos) % 17)); - if (!rc) { - Progress(false, "Read error at offset %d", i); - break; - } - } - residue -= readThisTime; - pos += readThisTime; - - if (!rc) break; - } - if (rc) { - Progress(true, "Read success"); - - rc = file.SetPosition(1); - Progress(rc, "SetPosition(1)"); - if (rc) { - try { - const int attemptReadBytes = 173; - ssize_t readB = file.Read(buffer, attemptReadBytes); - Progress(false, "Expected to get an exception for unaligned read"); - ProgressI64(readB == attemptReadBytes, "Got %ld bytes from attempted 173", readB); - } catch(const DirectIOException &e) { - Progress(true, "Got exception as expected"); - } - } - if (rc) { - rc = file.SetPosition(1); - Progress(rc, "SetPosition(1)"); - if (rc) { - try { - const int attemptReadBytes = 4096; - ssize_t readB = file.Read(buffer, attemptReadBytes); - Progress(false, "Expected to get an exception for unaligned read"); - ProgressI64(readB == attemptReadBytes, "Got %ld bytes from attempted 4096", readB); - } catch(const DirectIOException &e) { - Progress(true, "Got exception as expected"); - } - } - } - } - delete [] buffer2; - } else { - memset(buffer, 0, bufSize); - - ssize_t readBytes = file.Read(buffer, bufSize); - Progress(readBytes == bufSize, - "Reading %d bytes from file", bufSize); +TEST(FileTest, MemoryMapTest) { MemoryMapTestImpl(0); } - for (i=0; i<bufSize; i++) { - rc = (buffer[i] == 'A' + (i % 17)); - if (!rc) { - Progress(false, "Read error at offset %d", i); - break; - } - } - if (rc) Progress(true, "Read success"); - } - } - } - delete [] buffer; - } +#ifdef __linux__ +TEST(FileTest, MemoryMapTestHuge) { MemoryMapTestImpl(MAP_HUGETLB); } +#endif - std::filesystem::remove_all(std::filesystem::path("generated")); - PrintSeparator(); +TEST(FileTest, DirectIOTest) { + Generated guard; + const int bufSize = 40000; + FastOS_File file("generated/diotest"); + ASSERT_TRUE(file.OpenWriteOnly()); + std::vector<char> space(bufSize); + char *buffer = space.data(); + for (int i = 0; i < bufSize; i++) { + buffer[i] = 'A' + (i % 17); } - - void ReadOnlyTest () - { - TestHeader("Read-Only Test"); - - FastOS_File *myFile = new FastOS_File(roFilename.c_str()); - - if (myFile->OpenReadOnly()) { - int64_t filesize; - filesize = myFile->GetSize(); - ProgressI64((filesize == 27), "File size: %ld", filesize); - - char dummyData[6] = "Dummy"; - bool writeResult = myFile->CheckedWrite(dummyData, 6); - - if (writeResult) { - Progress(false, "Should not be able to write a file opened for read-only access."); - } else { - char dummyData2[28]; - Progress(true, "Write failed with read-only access."); - - bool rc = myFile->SetPosition(1); - Progress(rc, "Setting position to 1"); - - if (rc) { - ssize_t readBytes; - int64_t filePosition; - readBytes = myFile->Read(dummyData2, 28); - - Progress(readBytes == 26, "Attempting to read 28 bytes, should get 26. Got: %d", readBytes); - - filePosition = myFile->GetPosition(); - Progress(filePosition == 27, "File position should now be 27. Was: %d", int(filePosition)); - - readBytes = myFile->Read(dummyData2, 6); - Progress(readBytes == 0, "We should now get 0 bytes. Read: %d bytes", readBytes); - - filePosition = myFile->GetPosition(); - Progress(filePosition == 27, "File position should now be 27. Was: %d", int(filePosition)); - } - } - } else { - Progress(false, "Unable to open file '%s'.", roFilename.c_str()); + EXPECT_EQ(file.Write2(buffer, bufSize), bufSize); + bool close_ok = file.Close(); + assert(close_ok); + file.EnableDirectIO(); + ASSERT_TRUE(file.OpenReadOnly()); + size_t memoryAlignment = 0; + size_t transferGranularity = 0; + size_t transferMaximum = 0; + bool dioEnabled = file.GetDirectIORestrictions(memoryAlignment, + transferGranularity, + transferMaximum); + fprintf(stderr, "DirectIO %s\n", dioEnabled ? "enabled" : "disabled"); + fprintf(stderr, "Memory alignment: %zu bytes\n", memoryAlignment); + fprintf(stderr, "Transfer granularity: %zu bytes\n", transferGranularity); + fprintf(stderr, "Transfer maximum: %zu bytes\n", transferMaximum); + if (dioEnabled) { + int eachRead = (8192 + transferGranularity - 1) / transferGranularity; + std::vector<char> space2(eachRead * transferGranularity + memoryAlignment - 1); + char *buffer2 = space2.data(); + char *alignPtr = buffer2; + unsigned int align = + static_cast<unsigned int> + (reinterpret_cast<unsigned long>(alignPtr) & + (memoryAlignment - 1)); + if (align != 0) { + alignPtr = &alignPtr[memoryAlignment - align]; } - delete(myFile); - PrintSeparator(); - } - - void WriteOnlyTest () - { - TestHeader("Write-Only Test"); - std::filesystem::create_directory(std::filesystem::path("generated")); - - FastOS_File *myFile = new FastOS_File(woFilename.c_str()); - - if (myFile->OpenWriteOnly()) { - int64_t filesize; - filesize = myFile->GetSize(); - - ProgressI64((filesize == 0), "File size: %ld", filesize); - - char dummyData[6] = "Dummy"; - bool writeResult = myFile->CheckedWrite(dummyData, 6); - - if (!writeResult) { - Progress(false, "Should be able to write to file opened for write-only access."); - } else { - Progress(true, "Write 6 bytes ok."); - - int64_t filePosition = myFile->GetPosition(); - if (filePosition == 6) { - Progress(true, "Fileposition is now 6."); - - if (myFile->SetPosition(0)) { - Progress(true, "SetPosition(0) success."); - filePosition = myFile->GetPosition(); - - if (filePosition == 0) { - Progress(true, "Fileposition is now 0."); - - int readBytes = myFile->Read(dummyData, 6); - - if (readBytes != 6) { - Progress(true, "Trying to read a write-only file should fail and it did."); - Progress(true, "Return code was: %d.", readBytes); - } else { - Progress(false, "Read on a file with write-only access should fail, but it didn't."); - } - } else { - ProgressI64(false, "Fileposition should be 6, but was %ld.", filePosition); - } - } else { - Progress(false, "SetPosition(0) failed"); - } - } else { - ProgressI64(false, "Fileposition should be 6, but was %ld.", filePosition); - } + int residue = bufSize; + int pos = 0; + while (residue > 0) { + int readThisTime = eachRead * transferGranularity; + if (readThisTime > residue) { + readThisTime = residue; } - bool closeResult = myFile->Close(); - Progress(closeResult, "Close file."); - } else { - Progress(false, "Unable to open file '%s'.", woFilename.c_str()); - } - - - bool deleteResult = myFile->Delete(); - Progress(deleteResult, "Delete file '%s'.", woFilename.c_str()); - - delete(myFile); - std::filesystem::remove_all(std::filesystem::path("generated")); - PrintSeparator(); - } - - void ReadWriteTest () - { - TestHeader("Read/Write Test"); - std::filesystem::create_directory(std::filesystem::path("generated")); - - FastOS_File *myFile = new FastOS_File(rwFilename.c_str()); - - if (myFile->OpenExisting()) { - Progress(false, "OpenExisting() should not work when '%s' does not exist.", rwFilename.c_str()); - bool close_ok = myFile->Close(); - assert(close_ok); - } else { - Progress(true, "OpenExisting() should fail when '%s' does not exist, and it did.", rwFilename.c_str()); - } - - if (myFile->OpenReadWrite()) { - int64_t filesize; - - filesize = myFile->GetSize(); - - ProgressI64((filesize == 0), "File size: %ld", filesize); - - char dummyData[6] = "Dummy"; - - bool writeResult = myFile->CheckedWrite(dummyData, 6); - - if (!writeResult) { - Progress(false, "Should be able to write to file opened for read/write access."); - } else { - Progress(true, "Write 6 bytes ok."); - - int64_t filePosition = myFile->GetPosition(); - - if (filePosition == 6) { - Progress(true, "Fileposition is now 6."); - - if (myFile->SetPosition(0)) { - Progress(true, "SetPosition(0) success."); - filePosition = myFile->GetPosition(); - - if (filePosition == 0) { - Progress(true, "Fileposition is now 0."); - - char dummyData2[7]; - int readBytes = myFile->Read(dummyData2, 6); - - if (readBytes == 6) { - Progress(true, "Reading 6 bytes worked."); - - int cmpResult = memcmp(dummyData, dummyData2, 6); - Progress((cmpResult == 0), "Comparing the written and read result.\n"); - - bool rc = myFile->SetPosition(1); - Progress(rc, "Setting position to 1"); - - if (rc) { - readBytes = myFile->Read(dummyData2, 7); - - Progress(readBytes == 5, "Attempting to read 7 bytes, should get 5. Got: %d", readBytes); - - filePosition = myFile->GetPosition(); - Progress(filePosition == 6, "File position should now be 6. Was: %d", int(filePosition)); - - readBytes = myFile->Read(dummyData2, 6); - Progress(readBytes == 0, "We should not be able to read any more. Read: %d bytes", readBytes); - - filePosition = myFile->GetPosition(); - Progress(filePosition == 6, "File position should now be 6. Was: %d", int(filePosition)); - } - } else { - Progress(false, "Reading 6 bytes failed."); - } - } else { - ProgressI64(false, "Fileposition should be 6, but was %ld.", filePosition); - } - } else { - Progress(false, "SetPosition(0) failed"); - } - } else { - ProgressI64(false, "Fileposition should be 6, but was %ld.", filePosition); - } + file.ReadBuf(alignPtr, readThisTime, pos); + for (int i = 0; i < readThisTime; i++) { + ASSERT_EQ(alignPtr[i], char('A' + ((i+pos) % 17))); } - - bool closeResult = myFile->Close(); - Progress(closeResult, "Close file."); - } else { - Progress(false, "Unable to open file '%s'.", rwFilename.c_str()); + residue -= readThisTime; + pos += readThisTime; } - bool deleteResult = myFile->Delete(); - Progress(deleteResult, "Delete file '%s'.", rwFilename.c_str()); - - delete(myFile); - std::filesystem::remove_all(std::filesystem::path("generated")); - PrintSeparator(); - } - - void ScanDirectoryTest() - { - TestHeader("DirectoryScan Test"); - - FastOS_DirectoryScan *scanDir = new FastOS_DirectoryScan("."); - - while (scanDir->ReadNext()) { - const char *name = scanDir->GetName(); - bool isDirectory = scanDir->IsDirectory(); - bool isRegular = scanDir->IsRegular(); - - printf("%-30s %s\n", name, - isDirectory ? "DIR" : (isRegular ? "FILE" : "UNKN")); + ASSERT_TRUE(file.SetPosition(1)); + try { + const int attemptReadBytes = 173; + [[maybe_unused]] auto res = file.Read(buffer, attemptReadBytes); + EXPECT_TRUE(false); + } catch (const DirectIOException &) { + fprintf(stderr, "got DirectIOException as expected\n"); + } catch (...) { + EXPECT_TRUE(false); } - - delete(scanDir); - PrintSeparator(); - } - - void ReadBufTest () - { - TestHeader("ReadBuf Test"); - - FastOS_File file(roFilename.c_str()); - - char buffer[20]; - - if (file.OpenReadOnly()) { - int64_t position = file.GetPosition(); - Progress(position == 0, "File pointer should be 0 after opening file"); - - ssize_t has_read = file.Read(buffer, 4); - Progress(has_read == 4, "Must read 4 bytes"); - buffer[4] = '\0'; - position = file.GetPosition(); - Progress(position == 4, "File pointer should be 4 after reading 4 bytes"); - Progress(strcmp(buffer, "This") == 0, "[This]=[%s]", buffer); - - file.ReadBuf(buffer, 6, 8); - buffer[6] = '\0'; - position = file.GetPosition(); - Progress(position == 4, "File pointer should still be 4 after ReadBuf"); - Progress(strcmp(buffer, "a test") == 0, "[a test]=[%s]", buffer); + ASSERT_TRUE(file.SetPosition(1)); + try { + const int attemptReadBytes = 4096; + [[maybe_unused]] auto res = file.Read(buffer, attemptReadBytes); + EXPECT_TRUE(false); + } catch (const DirectIOException &) { + fprintf(stderr, "got DirectIOException as expected\n"); + } catch (...) { + EXPECT_TRUE(false); + } + } else { + memset(buffer, 0, bufSize); + ssize_t readBytes = file.Read(buffer, bufSize); + ASSERT_EQ(readBytes, bufSize); + for (int i = 0; i < bufSize; i++) { + ASSERT_EQ(buffer[i], char('A' + (i % 17))); } - - PrintSeparator(); - } - - void DiskFreeSpaceTest () - { - TestHeader("DiskFreeSpace Test"); - - int64_t freeSpace = FastOS_File::GetFreeDiskSpace(roFilename.c_str()); - ProgressI64(freeSpace != -1, "DiskFreeSpace using file ('hello.txt'): %ld MB.", freeSpace == -1 ? -1 : freeSpace/(1024*1024)); - freeSpace = FastOS_File::GetFreeDiskSpace("."); - ProgressI64(freeSpace != -1, "DiskFreeSpace using dir (.): %ld MB.", freeSpace == -1 ? -1 : freeSpace/(1024*1024)); - PrintSeparator(); - } - - void MaxLengthTest () - { - TestHeader ("Max Lengths Test"); - - int maxval = FastOS_File::GetMaximumFilenameLength("."); - Progress(maxval > 5 && maxval < (512*1024), - "Maximum filename length = %d", maxval); - - maxval = FastOS_File::GetMaximumPathLength("."); - Progress(maxval > 5 && maxval < (512*1024), - "Maximum path length = %d", maxval); - - PrintSeparator(); } +} - int Main () override - { - printf("This test should be run in the 'tests' directory.\n\n"); - printf("grep for the string '%s' to detect failures.\n\n", failString); +TEST(FileTest, ReadOnlyTest) { + auto myFile = std::make_unique<FastOS_File>(roFilename.c_str()); + ASSERT_TRUE(myFile->OpenReadOnly()); + EXPECT_EQ(myFile->GetSize(), 27); + char dummyData[6] = "Dummy"; + ASSERT_FALSE(myFile->CheckedWrite(dummyData, 6)); + char dummyData2[28]; + ASSERT_TRUE(myFile->SetPosition(1)); + EXPECT_EQ(myFile->Read(dummyData2, 28), 26); + EXPECT_EQ(myFile->GetPosition(), 27); +} - GetCurrentDirTest(); - DirectIOTest(); - MaxLengthTest(); - DiskFreeSpaceTest(); - ReadOnlyTest(); - WriteOnlyTest(); - ReadWriteTest(); - ScanDirectoryTest(); - ReadBufTest(); - MemoryMapTest(0); -#ifdef __linux__ - MemoryMapTest(MAP_HUGETLB); -#endif +TEST(FileTest, WriteOnlyTest) { + Generated guard; + auto myFile = std::make_unique<FastOS_File>(woFilename.c_str()); + ASSERT_TRUE(myFile->OpenWriteOnly()); + EXPECT_EQ(myFile->GetSize(), 0); + char dummyData[6] = "Dummy"; + ASSERT_TRUE(myFile->CheckedWrite(dummyData, 6)); + ASSERT_EQ(myFile->GetPosition(), 6); + ASSERT_TRUE(myFile->SetPosition(0)); + ASSERT_EQ(myFile->GetPosition(), 0); + EXPECT_LT(myFile->Read(dummyData, 6), 0); + EXPECT_TRUE(myFile->Close()); + EXPECT_TRUE(myFile->Delete()); +} - PrintSeparator(); - printf("END OF TEST (%s)\n", _argv[0]); +TEST(FileTest, ReadWriteTest) { + Generated guard; + auto myFile = std::make_unique<FastOS_File>(rwFilename.c_str()); + ASSERT_FALSE(myFile->OpenExisting()); + ASSERT_TRUE(myFile->OpenReadWrite()); + ASSERT_EQ(myFile->GetSize(), 0); + char dummyData[6] = "Dummy"; + ASSERT_TRUE(myFile->CheckedWrite(dummyData, 6)); + ASSERT_EQ(myFile->GetPosition(), 6); + ASSERT_TRUE(myFile->SetPosition(0)); + ASSERT_EQ(myFile->GetPosition(), 0); + char dummyData2[7]; + ASSERT_EQ(myFile->Read(dummyData2, 6), 6); + EXPECT_EQ(memcmp(dummyData, dummyData2, 6), 0); + ASSERT_TRUE(myFile->SetPosition(1)); + EXPECT_EQ(myFile->Read(dummyData2, 7), 5); + EXPECT_EQ(myFile->GetPosition(), 6); + EXPECT_EQ(myFile->Read(dummyData2, 6), 0); + EXPECT_EQ(myFile->GetPosition(), 6); + EXPECT_TRUE(myFile->Close()); + EXPECT_TRUE(myFile->Delete()); +} - return allWasOk() ? 0 : 1; +TEST(FileTest, ScanDirectoryTest) { + auto scanDir = std::make_unique<FastOS_DirectoryScan>("."); + while (scanDir->ReadNext()) { + const char *name = scanDir->GetName(); + bool isDirectory = scanDir->IsDirectory(); + bool isRegular = scanDir->IsRegular(); + fprintf(stderr, "%-30s %s\n", name, isDirectory ? "DIR" : (isRegular ? "FILE" : "UNKN")); } - FileTest(); - ~FileTest(); -}; - -FileTest::FileTest() { } -FileTest::~FileTest() { } +} +TEST(FileTest, ReadBufTest) { + FastOS_File file(roFilename.c_str()); + char buffer[20]; + ASSERT_TRUE(file.OpenReadOnly()); + EXPECT_EQ(file.GetPosition(), 0); + EXPECT_EQ(file.Read(buffer, 4), 4); + buffer[4] = '\0'; + EXPECT_EQ(file.GetPosition(), 4); + EXPECT_EQ(strcmp(buffer, "This"), 0); + file.ReadBuf(buffer, 6, 8); + buffer[6] = '\0'; + EXPECT_EQ(file.GetPosition(), 4); + EXPECT_EQ(strcmp(buffer, "a test"), 0); +} -int main (int argc, char **argv) -{ - FileTest app; +TEST(FileTest, DiskFreeSpaceTest) { + EXPECT_NE(FastOS_File::GetFreeDiskSpace(roFilename.c_str()), int64_t(-1)); + EXPECT_NE(FastOS_File::GetFreeDiskSpace("."), int64_t(-1)); +} - setvbuf(stdout, nullptr, _IOLBF, 8192); - return app.Entry(argc, argv); +TEST(FileTest, MaxLengthTest) { + int maxval = FastOS_File::GetMaximumFilenameLength("."); + EXPECT_GT(maxval, 5); + EXPECT_LT(maxval, (512*1024)); + maxval = FastOS_File::GetMaximumPathLength("."); + EXPECT_GT(maxval, 5); + EXPECT_LT(maxval, (512*1024)); } + +GTEST_MAIN_RUN_ALL_TESTS() diff --git a/vespalib/src/tests/fastos/tests.h b/vespalib/src/tests/fastos/tests.h deleted file mode 100644 index 9cd7a10ab48..00000000000 --- a/vespalib/src/tests/fastos/tests.h +++ /dev/null @@ -1,149 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include <cstring> -#include <csignal> -#include <cstdio> -#include <cstdint> - -class BaseTest -{ -private: - BaseTest(const BaseTest&); - BaseTest &operator=(const BaseTest&); - - int totallen; - bool _allOkFlag; -public: - int _argc; - char **_argv; - - const char *okString; - const char *failString; - - BaseTest () - : totallen(70), - _allOkFlag(true), - _argc(0), - _argv(nullptr), - okString("SUCCESS"), - failString("FAILURE") - { - } - - virtual int Main() = 0; - - int Entry(int argc, char **argv) { - _argc = argc; - _argv = argv; - return Main(); - } - - virtual ~BaseTest() {}; - - bool allWasOk() const { return _allOkFlag; } - - void PrintSeparator () - { - for(int i=0; i<totallen; i++) printf("-"); - printf("\n"); - } - - virtual void PrintProgress (char *string) - { - printf("%s", string); - } -#define MAX_STR_LEN 3000 - bool Progress (bool result, const char *str) - { - char string[MAX_STR_LEN]; - snprintf(string, sizeof(string), "%s: %s\n", - result ? okString : failString, str); - PrintProgress(string); - if (! result) { _allOkFlag = false; } - return result; - } - - bool Progress (bool result, const char *str, int d1) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, d1); - return Progress(result, string); - } - - bool Progress (bool result, const char *str, int d1, int d2) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, d1, d2); - return Progress(result, string); - } - - bool Progress (bool result, const char *str, const char *s1) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, s1); - return Progress(result, string); - } - - bool Progress (bool result, const char *str, const char *s1, const char *s2) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, s1, s2); - return Progress(result, string); - } - - bool Progress (bool result, const char *str, const char *s1, int d1) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, s1, d1); - return Progress(result, string); - } - - bool Progress (bool result, const char *str, int d1, const char *s1) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, d1, s1); - return Progress(result, string); - } - - bool ProgressI64 (bool result, const char *str, int64_t val) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, val); - return Progress(result, string); - } - - bool ProgressFloat (bool result, const char *str, float val) - { - char string[MAX_STR_LEN-100]; - snprintf(string, sizeof(string), str, val); - return Progress(result, string); - } - - void Ok (const char *string) - { - Progress(true, string); - } - - void Fail (const char *string) - { - Progress(false, string); - } - - void TestHeader (const char *string) - { - int len = strlen(string); - int leftspace = (totallen - len)/2 - 2; - int rightspace = totallen - 4 - len - leftspace; - int i; - - printf("\n\n"); - for(i=0; i<totallen; i++) printf("*"); - printf("\n**"); - for(i=0; i<leftspace; i++) printf(" "); //forgot printf-specifier.. - printf("%s", string); - for(i=0; i<rightspace; i++) printf(" "); - printf("**\n"); - for(i=0; i<totallen; i++) printf("*"); - printf("\n"); - } -}; |