diff options
55 files changed, 712 insertions, 320 deletions
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetControllerOptions.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetControllerOptions.java index a088b50f078..0ad0a59a72a 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetControllerOptions.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetControllerOptions.java @@ -16,6 +16,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import java.util.stream.Collectors; /** * This class represents all the options that can be set in the fleetcontroller. @@ -249,6 +250,12 @@ public class FleetControllerOptions implements Cloneable { sb.append("<tr><td><nobr>Max deferred task version wait time</nobr></td><td align=\"right\">").append(maxDeferredTaskVersionWaitTime.toMillis()).append("ms</td></tr>"); sb.append("<tr><td><nobr>Cluster has global document types configured</nobr></td><td align=\"right\">").append(clusterHasGlobalDocumentTypes).append("</td></tr>"); sb.append("<tr><td><nobr>Enable 2-phase cluster state activation protocol</nobr></td><td align=\"right\">").append(enableTwoPhaseClusterStateActivation).append("</td></tr>"); + sb.append("<tr><td><nobr>Cluster auto feed block on resource exhaustion enabled</nobr></td><td align=\"right\">") + .append(clusterFeedBlockEnabled).append("</td></tr>"); + sb.append("<tr><td><nobr>Feed block limits</nobr></td><td align=\"right\">") + .append(clusterFeedBlockLimit.entrySet().stream() + .map(kv -> String.format("%s: %.2f%%", kv.getKey(), kv.getValue() * 100.0)) + .collect(Collectors.joining("<br/>"))).append("</td></tr>"); sb.append("</table>"); } diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterSpec.java b/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterSpec.java index 14658e57c1b..f4b79129ada 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterSpec.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterSpec.java @@ -77,10 +77,8 @@ public final class ClusterSpec { */ public boolean isExclusive() { return exclusive; } - /** Whether this cluster has state */ - public boolean isStateful() { - return stateful; - } + /** Returns whether this cluster has state */ + public boolean isStateful() { return stateful; } public ClusterSpec with(Optional<Group> newGroup) { return new ClusterSpec(type, id, newGroup, vespaVersion, exclusive, combinedId, dockerImageRepo, stateful); diff --git a/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java b/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java index 21f3e38ff4e..8fc31627800 100644 --- a/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java +++ b/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java @@ -55,27 +55,27 @@ public class StatisticsSearcher extends Searcher { private static final String FAILED_QUERIES_METRIC = "failed_queries"; private static final String MEAN_QUERY_LATENCY_METRIC = "mean_query_latency"; private static final String QUERY_LATENCY_METRIC = "query_latency"; - private static final String QUERY_OFFSET_METRIC = "query_hit_offset"; + private static final String QUERY_HIT_OFFSET_METRIC = "query_hit_offset"; private static final String QUERIES_METRIC = "queries"; private static final String ACTIVE_QUERIES_METRIC = "active_queries"; private static final String PEAK_QPS_METRIC = "peak_qps"; private static final String DOCS_COVERED_METRIC = "documents_covered"; private static final String DOCS_TOTAL_METRIC = "documents_total"; - private static final String DEGRADED_METRIC = "degraded_queries"; + private static final String DEGRADED_QUERIES_METRIC = "degraded_queries"; private static final String RELEVANCE_AT_1_METRIC = "relevance.at_1"; private static final String RELEVANCE_AT_3_METRIC = "relevance.at_3"; private static final String RELEVANCE_AT_10_METRIC = "relevance.at_10"; - private final Counter queries; // basic counter - private final Counter failedQueries; // basic counter - private final Counter nullQueries; // basic counter - private final Counter illegalQueries; // basic counter - private final Value queryLatency; // mean pr 5 min + private final Counter queriesCounter; // basic counter + private final Counter failedQueriesCounter; // basic counter + private final Counter nullQueriesCounter; // basic counter + private final Counter illegalQueriesCounter; // basic counter + private final Value meanQueryLatency; // mean pr 5 min private final Value queryLatencyBuckets; private final Value maxQueryLatency; // separate to avoid name mangling @SuppressWarnings("unused") // all the work is done by the callback private final Value peakQPS; // peak 1s QPS - private final Counter emptyResults; // number of results containing no concrete hits + private final Counter emptyResultsCounter; // number of results containing no concrete hits private final Value hitsPerQuery; // mean number of hits per query private final Value totalHitsPerQuery; @@ -129,18 +129,18 @@ public class StatisticsSearcher extends Searcher { this.peakQpsReporter = new PeakQpsReporter(); this.metric = metric; - queries = new Counter(QUERIES_METRIC, manager, false); - failedQueries = new Counter(FAILED_QUERIES_METRIC, manager, false); - nullQueries = new Counter("null_queries", manager, false); - illegalQueries = new Counter("illegal_queries", manager, false); - queryLatency = new Value(MEAN_QUERY_LATENCY_METRIC, manager, new Value.Parameters().setLogRaw(false).setLogMean(true).setNameExtension(false)); + queriesCounter = new Counter(QUERIES_METRIC, manager, false); + failedQueriesCounter = new Counter(FAILED_QUERIES_METRIC, manager, false); + nullQueriesCounter = new Counter("null_queries", manager, false); + illegalQueriesCounter = new Counter("illegal_queries", manager, false); + meanQueryLatency = new Value(MEAN_QUERY_LATENCY_METRIC, manager, new Value.Parameters().setLogRaw(false).setLogMean(true).setNameExtension(false)); maxQueryLatency = new Value(MAX_QUERY_LATENCY_METRIC, manager, new Value.Parameters().setLogRaw(false).setLogMax(true).setNameExtension(false)); queryLatencyBuckets = Value.buildValue(QUERY_LATENCY_METRIC, manager, null); peakQPS = new Value(PEAK_QPS_METRIC, manager, new Value.Parameters().setLogRaw(false).setLogMax(true).setNameExtension(false)); hitsPerQuery = new Value(HITS_PER_QUERY_METRIC, manager, new Value.Parameters().setLogRaw(false).setLogMean(true).setNameExtension(false)); totalHitsPerQuery = new Value(TOTALHITS_PER_QUERY_METRIC, manager, new Value.Parameters().setLogRaw(false).setLogMean(true).setNameExtension(false)); - emptyResults = new Counter(EMPTY_RESULTS_METRIC, manager, false); + emptyResultsCounter = new Counter(EMPTY_RESULTS_METRIC, manager, false); metricReceiver.declareGauge(QUERY_LATENCY_METRIC, Optional.empty(), new MetricSettings.Builder().histogram(true).build()); metricReceiver.declareGauge(HITS_PER_QUERY_METRIC, Optional.empty(), new MetricSettings.Builder().histogram(true).build()); metricReceiver.declareGauge(TOTALHITS_PER_QUERY_METRIC, Optional.empty(), new MetricSettings.Builder().histogram(true).build()); @@ -265,7 +265,7 @@ public class StatisticsSearcher extends Searcher { if (queryCoverage != null) { if (queryCoverage.isDegraded()) { Metric.Context degradedContext = getDegradedMetricContext(execution.chain().getId().stringValue(), queryCoverage); - metric.add(DEGRADED_METRIC, 1, degradedContext); + metric.add(DEGRADED_QUERIES_METRIC, 1, degradedContext); } metric.add(DOCS_COVERED_METRIC, queryCoverage.getDocs(), metricContext); metric.add(DOCS_TOTAL_METRIC, queryCoverage.getActive(), metricContext); @@ -278,9 +278,9 @@ public class StatisticsSearcher extends Searcher { totalHitsPerQuery.put(totalHitCount); metric.set(TOTALHITS_PER_QUERY_METRIC, (double) totalHitCount, metricContext); - metric.set(QUERY_OFFSET_METRIC, (double) (query.getHits() + query.getOffset()), metricContext); + metric.set(QUERY_HIT_OFFSET_METRIC, (double) (query.getHits() + query.getOffset()), metricContext); if (hitCount == 0) { - emptyResults.increment(); + emptyResultsCounter.increment(); metric.add(EMPTY_RESULTS_METRIC, 1, metricContext); } @@ -300,7 +300,7 @@ public class StatisticsSearcher extends Searcher { private void addLatency(long latency_ns, Metric.Context metricContext) { double latency = 0.000001 * latency_ns; //myStats.addLatency(latency); - queryLatency.put(latency); + meanQueryLatency.put(latency); metric.set(QUERY_LATENCY_METRIC, latency, metricContext); metric.set(MEAN_QUERY_LATENCY_METRIC, latency, metricContext); maxQueryLatency.put(latency); @@ -310,20 +310,20 @@ public class StatisticsSearcher extends Searcher { private void incrQueryCount(Metric.Context metricContext) { //myStats.incrQueryCnt(); - queries.increment(); + queriesCounter.increment(); metric.add(QUERIES_METRIC, 1, metricContext); } private void incrErrorCount(Result result, Metric.Context metricContext) { - failedQueries.increment(); + failedQueriesCounter.increment(); metric.add(FAILED_QUERIES_METRIC, 1, metricContext); if (result == null) // the chain threw an exception metric.add("error.unhandled_exception", 1, metricContext); else if (result.hits().getErrorHit().hasOnlyErrorCode(Error.NULL_QUERY.code)) - nullQueries.increment(); + nullQueriesCounter.increment(); else if (result.hits().getErrorHit().hasOnlyErrorCode(Error.ILLEGAL_QUERY.code)) - illegalQueries.increment(); + illegalQueriesCounter.increment(); } /** diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Container.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Container.java index fb979ff639c..9e304e5ef4d 100644 --- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Container.java +++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Container.java @@ -10,6 +10,7 @@ import java.util.Objects; */ // TODO: Move this to node-admin when docker-api module can be removed public class Container { + private final ContainerId id; public final String hostname; public final DockerImage image; public final ContainerResources resources; @@ -18,12 +19,14 @@ public class Container { public final int pid; public Container( + final ContainerId id, final String hostname, final DockerImage image, final ContainerResources resources, final ContainerName containerName, final State state, final int pid) { + this.id = id; this.hostname = hostname; this.image = image; this.resources = resources; @@ -32,13 +35,18 @@ public class Container { this.pid = pid; } + public ContainerId id() { + return id; + } + @Override public boolean equals(final Object obj) { if (!(obj instanceof Container)) { return false; } final Container other = (Container) obj; - return Objects.equals(hostname, other.hostname) + return Objects.equals(id, other.id) + && Objects.equals(hostname, other.hostname) && Objects.equals(image, other.image) && Objects.equals(resources, other.resources) && Objects.equals(name, other.name) @@ -53,6 +61,7 @@ public class Container { @Override public String toString() { return "Container {" + + " id=" + id + " hostname=" + hostname + " image=" + image + " resources=" + resources diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerId.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerId.java new file mode 100644 index 00000000000..b86238324f0 --- /dev/null +++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerId.java @@ -0,0 +1,36 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// +package com.yahoo.vespa.hosted.dockerapi; + +import java.util.Objects; + +/** + * The ID of a container. + * + * @author hakon + */ +public class ContainerId { + private final String id; + + public ContainerId(String id) { + this.id = Objects.requireNonNull(id, "id cannot be null"); + } + + @Override + public String toString() { + return id; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ContainerId that = (ContainerId) o; + return id.equals(that.id); + } + + @Override + public int hashCode() { + return Objects.hash(id); + } +} diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java index c61bc3ed919..37e265bf411 100644 --- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java +++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/ContainerResources.java @@ -63,8 +63,7 @@ public class ContainerResources { return cpus; } - // Although docker allows to update cpu quota to 0, this is not a legal value, must be set -1 for unlimited - // See: https://github.com/docker/for-linux/issues/558 + /** Returns the CFS CPU quota per {@link #cpuPeriod()}, or -1 if disabled. */ public int cpuQuota() { return cpus > 0 ? (int) (cpus * CPU_PERIOD_US) : -1; } diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java index 630efb7990f..3b7b2b8d54c 100644 --- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java +++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java @@ -286,6 +286,7 @@ public class DockerEngine implements ContainerEngine { private Stream<Container> asContainer(String container) { return inspectContainerCmd(container) .map(response -> new Container( + new ContainerId(response.getId()), response.getConfig().getHostName(), DockerImage.fromString(response.getConfig().getImage()), containerResourcesFromHostConfig(response.getHostConfig()), diff --git a/document/src/main/java/com/yahoo/document/DocumentPut.java b/document/src/main/java/com/yahoo/document/DocumentPut.java index e24388cd65f..25246dc9a9e 100644 --- a/document/src/main/java/com/yahoo/document/DocumentPut.java +++ b/document/src/main/java/com/yahoo/document/DocumentPut.java @@ -54,7 +54,7 @@ public class DocumentPut extends DocumentOperation { if (o == null || getClass() != o.getClass()) return false; DocumentPut that = (DocumentPut) o; return document.equals(that.document) && - getCondition().equals(that.getCondition()); + Objects.equals(getCondition(), that.getCondition()); } @Override diff --git a/document/src/main/java/com/yahoo/document/DocumentRemove.java b/document/src/main/java/com/yahoo/document/DocumentRemove.java index 79f80713c44..a815d9c0a5a 100644 --- a/document/src/main/java/com/yahoo/document/DocumentRemove.java +++ b/document/src/main/java/com/yahoo/document/DocumentRemove.java @@ -1,6 +1,8 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.document; +import java.util.Objects; + /** * @author baldersheim */ @@ -21,9 +23,10 @@ public class DocumentRemove extends DocumentOperation { @Override public boolean equals(Object o) { if (this == o) return true; - if (!(o instanceof DocumentRemove)) return false; + if ( ! (o instanceof DocumentRemove)) return false; DocumentRemove that = (DocumentRemove) o; - if (!docId.equals(that.docId)) return false; + if ( ! docId.equals(that.docId)) return false; + if ( ! Objects.equals(getCondition(), that.getCondition())) return false; return true; } diff --git a/document/src/main/java/com/yahoo/document/DocumentUpdate.java b/document/src/main/java/com/yahoo/document/DocumentUpdate.java index 5c748f48f15..cba51ee999e 100644 --- a/document/src/main/java/com/yahoo/document/DocumentUpdate.java +++ b/document/src/main/java/com/yahoo/document/DocumentUpdate.java @@ -19,6 +19,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; /** @@ -362,9 +363,10 @@ public class DocumentUpdate extends DocumentOperation implements Iterable<FieldP if (docId != null ? !docId.equals(that.docId) : that.docId != null) return false; if (documentType != null ? !documentType.equals(that.documentType) : that.documentType != null) return false; - if (!fieldPathUpdates.equals(that.fieldPathUpdates)) return false; - if (!id2FieldUpdates.equals(that.id2FieldUpdates)) return false; - if (this.getCreateIfNonExistent() != ((DocumentUpdate) o).getCreateIfNonExistent()) return false; + if ( ! fieldPathUpdates.equals(that.fieldPathUpdates)) return false; + if ( ! id2FieldUpdates.equals(that.id2FieldUpdates)) return false; + if (this.getCreateIfNonExistent() != that.getCreateIfNonExistent()) return false; + if ( ! Objects.equals(getCondition(), that.getCondition())) return false; return true; } diff --git a/eval/CMakeLists.txt b/eval/CMakeLists.txt index 23127cc12b5..0cba519bf88 100644 --- a/eval/CMakeLists.txt +++ b/eval/CMakeLists.txt @@ -66,6 +66,7 @@ vespa_define_module( src/tests/instruction/pow_as_map_optimizer src/tests/instruction/remove_trivial_dimension_optimizer src/tests/instruction/sparse_dot_product_function + src/tests/instruction/sparse_merge_function src/tests/instruction/sum_max_dot_product_function src/tests/instruction/vector_from_doubles_function src/tests/streamed/value diff --git a/eval/src/tests/instruction/sparse_merge_function/CMakeLists.txt b/eval/src/tests/instruction/sparse_merge_function/CMakeLists.txt new file mode 100644 index 00000000000..f905bdd8c1b --- /dev/null +++ b/eval/src/tests/instruction/sparse_merge_function/CMakeLists.txt @@ -0,0 +1,9 @@ +# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(eval_sparse_merge_function_test_app TEST + SOURCES + sparse_merge_function_test.cpp + DEPENDS + vespaeval + GTest::GTest +) +vespa_add_test(NAME eval_sparse_merge_function_test_app COMMAND eval_sparse_merge_function_test_app) diff --git a/eval/src/tests/instruction/sparse_merge_function/sparse_merge_function_test.cpp b/eval/src/tests/instruction/sparse_merge_function/sparse_merge_function_test.cpp new file mode 100644 index 00000000000..e175286e18c --- /dev/null +++ b/eval/src/tests/instruction/sparse_merge_function/sparse_merge_function_test.cpp @@ -0,0 +1,82 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/eval/eval/fast_value.h> +#include <vespa/eval/eval/simple_value.h> +#include <vespa/eval/instruction/sparse_merge_function.h> +#include <vespa/eval/eval/test/eval_fixture.h> +#include <vespa/eval/eval/test/gen_spec.h> +#include <vespa/vespalib/gtest/gtest.h> + +using namespace vespalib::eval; +using namespace vespalib::eval::test; + +const ValueBuilderFactory &prod_factory = FastValueBuilderFactory::get(); +const ValueBuilderFactory &test_factory = SimpleValueBuilderFactory::get(); + +//----------------------------------------------------------------------------- + +EvalFixture::ParamRepo make_params() { + return EvalFixture::ParamRepo() + .add("scalar1", GenSpec(1.0).gen()) + .add("scalar2", GenSpec(2.0).gen()) + .add_variants("v1_x", GenSpec(3.0).map("x", 32, 1)) + .add_variants("v2_x", GenSpec(4.0).map("x", 16, 2)) + .add_variants("v3_xz", GenSpec(5.0).map("x", 16, 2).idx("z", 1)) + .add("dense", GenSpec(6.0).idx("x", 10).gen()) + .add("m1_xy", GenSpec(7.0).map("x", 32, 1).map("y", 16, 2).gen()) + .add("m2_xy", GenSpec(8.0).map("x", 16, 2).map("y", 32, 1).gen()) + .add("mixed", GenSpec(9.0).map("x", 8, 1).idx("y", 5).gen()); +} +EvalFixture::ParamRepo param_repo = make_params(); + +void assert_optimized(const vespalib::string &expr) { + EvalFixture fast_fixture(prod_factory, expr, param_repo, true); + EvalFixture test_fixture(test_factory, expr, param_repo, true); + EvalFixture slow_fixture(prod_factory, expr, param_repo, false); + EXPECT_EQ(fast_fixture.result(), EvalFixture::ref(expr, param_repo)); + EXPECT_EQ(test_fixture.result(), EvalFixture::ref(expr, param_repo)); + EXPECT_EQ(slow_fixture.result(), EvalFixture::ref(expr, param_repo)); + EXPECT_EQ(fast_fixture.find_all<SparseMergeFunction>().size(), 1u); + EXPECT_EQ(test_fixture.find_all<SparseMergeFunction>().size(), 1u); + EXPECT_EQ(slow_fixture.find_all<SparseMergeFunction>().size(), 0u); +} + +void assert_not_optimized(const vespalib::string &expr) { + EvalFixture fast_fixture(prod_factory, expr, param_repo, true); + EXPECT_EQ(fast_fixture.result(), EvalFixture::ref(expr, param_repo)); + EXPECT_EQ(fast_fixture.find_all<SparseMergeFunction>().size(), 0u); +} + +//----------------------------------------------------------------------------- + +TEST(SparseMerge, expression_can_be_optimized) +{ + assert_optimized("merge(v1_x,v2_x,f(x,y)(x+y))"); + assert_optimized("merge(v1_x,v2_x,f(x,y)(max(x,y)))"); + assert_optimized("merge(v1_x,v2_x,f(x,y)(x+y+1))"); + assert_optimized("merge(v1_x_f,v2_x_f,f(x,y)(x+y))"); + assert_optimized("merge(v3_xz,v3_xz,f(x,y)(x+y))"); +} + +TEST(SparseMerge, multi_dimensional_expression_can_be_optimized) +{ + assert_optimized("merge(m1_xy,m2_xy,f(x,y)(x+y))"); + assert_optimized("merge(m1_xy,m2_xy,f(x,y)(x*y))"); +} + +TEST(SparseMerge, similar_expressions_are_not_optimized) +{ + assert_not_optimized("merge(scalar1,scalar2,f(x,y)(x+y))"); + assert_not_optimized("merge(dense,dense,f(x,y)(x+y))"); + assert_not_optimized("merge(mixed,mixed,f(x,y)(x+y))"); +} + +TEST(SparseMerge, mixed_cell_types_are_not_optimized) +{ + assert_not_optimized("merge(v1_x,v2_x_f,f(x,y)(x+y))"); + assert_not_optimized("merge(v1_x_f,v2_x,f(x,y)(x+y))"); +} + +//----------------------------------------------------------------------------- + +GTEST_MAIN_RUN_ALL_TESTS() diff --git a/eval/src/vespa/eval/eval/fast_value.hpp b/eval/src/vespa/eval/eval/fast_value.hpp index 88319df7590..6673494ccd2 100644 --- a/eval/src/vespa/eval/eval/fast_value.hpp +++ b/eval/src/vespa/eval/eval/fast_value.hpp @@ -155,12 +155,6 @@ struct FastValueIndex final : Value::Index { const std::vector<JoinAddrSource> &addr_sources, ConstArrayRef<LCT> lhs_cells, ConstArrayRef<RCT> rhs_cells, Stash &stash); - template <typename LCT, typename RCT, typename OCT, typename Fun> - static const Value &sparse_only_merge(const ValueType &res_type, const Fun &fun, - const FastValueIndex &lhs, const FastValueIndex &rhs, - ConstArrayRef<LCT> lhs_cells, ConstArrayRef<RCT> rhs_cells, - Stash &stash) __attribute((noinline)); - size_t size() const override { return map.size(); } std::unique_ptr<View> create_view(const std::vector<size_t> &dims) const override; }; @@ -429,32 +423,4 @@ FastValueIndex::sparse_no_overlap_join(const ValueType &res_type, const Fun &fun //----------------------------------------------------------------------------- -template <typename LCT, typename RCT, typename OCT, typename Fun> -const Value & -FastValueIndex::sparse_only_merge(const ValueType &res_type, const Fun &fun, - const FastValueIndex &lhs, const FastValueIndex &rhs, - ConstArrayRef<LCT> lhs_cells, ConstArrayRef<RCT> rhs_cells, Stash &stash) -{ - size_t guess_size = lhs.map.size() + rhs.map.size(); - auto &result = stash.create<FastValue<OCT,true>>(res_type, lhs.map.addr_size(), 1, guess_size); - lhs.map.each_map_entry([&](auto lhs_subspace, auto hash) - { - result.add_mapping(lhs.map.get_addr(lhs_subspace), hash); - result.my_cells.push_back_fast(lhs_cells[lhs_subspace]); - }); - rhs.map.each_map_entry([&](auto rhs_subspace, auto hash) - { - auto rhs_addr = rhs.map.get_addr(rhs_subspace); - auto result_subspace = result.my_index.map.lookup(rhs_addr, hash); - if (result_subspace == FastAddrMap::npos()) { - result.add_mapping(rhs_addr, hash); - result.my_cells.push_back_fast(rhs_cells[rhs_subspace]); - } else { - OCT &out_cell = *result.my_cells.get(result_subspace); - out_cell = fun(out_cell, rhs_cells[rhs_subspace]); - } - }); - return result; -} - } diff --git a/eval/src/vespa/eval/eval/optimize_tensor_function.cpp b/eval/src/vespa/eval/eval/optimize_tensor_function.cpp index 25612b8d5fd..196e8a98679 100644 --- a/eval/src/vespa/eval/eval/optimize_tensor_function.cpp +++ b/eval/src/vespa/eval/eval/optimize_tensor_function.cpp @@ -6,6 +6,7 @@ #include <vespa/eval/instruction/dense_dot_product_function.h> #include <vespa/eval/instruction/sparse_dot_product_function.h> +#include <vespa/eval/instruction/sparse_merge_function.h> #include <vespa/eval/instruction/mixed_inner_product_function.h> #include <vespa/eval/instruction/sum_max_dot_product_function.h> #include <vespa/eval/instruction/dense_xw_product_function.h> @@ -72,6 +73,7 @@ const TensorFunction &optimize_for_factory(const ValueBuilderFactory &, const Te child.set(MixedSimpleJoinFunction::optimize(child.get(), stash)); child.set(JoinWithNumberFunction::optimize(child.get(), stash)); child.set(DenseSingleReduceFunction::optimize(child.get(), stash)); + child.set(SparseMergeFunction::optimize(child.get(), stash)); nodes.pop_back(); } } diff --git a/eval/src/vespa/eval/instruction/CMakeLists.txt b/eval/src/vespa/eval/instruction/CMakeLists.txt index cac69d23640..3def8907ac8 100644 --- a/eval/src/vespa/eval/instruction/CMakeLists.txt +++ b/eval/src/vespa/eval/instruction/CMakeLists.txt @@ -33,6 +33,7 @@ vespa_add_library(eval_instruction OBJECT remove_trivial_dimension_optimizer.cpp replace_type_function.cpp sparse_dot_product_function.cpp + sparse_merge_function.cpp sum_max_dot_product_function.cpp vector_from_doubles_function.cpp ) diff --git a/eval/src/vespa/eval/instruction/generic_merge.cpp b/eval/src/vespa/eval/instruction/generic_merge.cpp index b40388aa547..9b098db7763 100644 --- a/eval/src/vespa/eval/instruction/generic_merge.cpp +++ b/eval/src/vespa/eval/instruction/generic_merge.cpp @@ -17,37 +17,6 @@ namespace vespalib::eval::instruction { using State = InterpretedFunction::State; using Instruction = InterpretedFunction::Instruction; -namespace { - -//----------------------------------------------------------------------------- - -struct MergeParam { - const ValueType res_type; - const join_fun_t function; - const size_t num_mapped_dimensions; - const size_t dense_subspace_size; - std::vector<size_t> all_view_dims; - const ValueBuilderFactory &factory; - MergeParam(const ValueType &lhs_type, const ValueType &rhs_type, - join_fun_t function_in, const ValueBuilderFactory &factory_in) - : res_type(ValueType::join(lhs_type, rhs_type)), - function(function_in), - num_mapped_dimensions(lhs_type.count_mapped_dimensions()), - dense_subspace_size(lhs_type.dense_subspace_size()), - all_view_dims(num_mapped_dimensions), - factory(factory_in) - { - assert(!res_type.is_error()); - assert(num_mapped_dimensions == rhs_type.count_mapped_dimensions()); - assert(num_mapped_dimensions == res_type.count_mapped_dimensions()); - assert(dense_subspace_size == rhs_type.dense_subspace_size()); - assert(dense_subspace_size == res_type.dense_subspace_size()); - for (size_t i = 0; i < num_mapped_dimensions; ++i) { - all_view_dims[i] = i; - } - } - ~MergeParam(); -}; MergeParam::~MergeParam() = default; //----------------------------------------------------------------------------- @@ -108,39 +77,14 @@ generic_mixed_merge(const Value &a, const Value &b, return builder->build(std::move(builder)); } -template <typename LCT, typename RCT, typename OCT, typename Fun> -void my_mixed_merge_op(State &state, uint64_t param_in) { - const auto ¶m = unwrap_param<MergeParam>(param_in); - const Value &lhs = state.peek(1); - const Value &rhs = state.peek(0); - auto up = generic_mixed_merge<LCT, RCT, OCT, Fun>(lhs, rhs, param); - auto &result = state.stash.create<std::unique_ptr<Value>>(std::move(up)); - const Value &result_ref = *(result.get()); - state.pop_pop_push(result_ref); -}; + +namespace { template <typename LCT, typename RCT, typename OCT, typename Fun> -void my_sparse_merge_op(State &state, uint64_t param_in) { +void my_mixed_merge_op(State &state, uint64_t param_in) { const auto ¶m = unwrap_param<MergeParam>(param_in); const Value &lhs = state.peek(1); const Value &rhs = state.peek(0); - if (auto indexes = detect_type<FastValueIndex>(lhs.index(), rhs.index())) { - auto lhs_cells = lhs.cells().typify<LCT>(); - auto rhs_cells = rhs.cells().typify<RCT>(); - if (lhs_cells.size() < rhs_cells.size()) { - return state.pop_pop_push( - FastValueIndex::sparse_only_merge<RCT,LCT,OCT,Fun>( - param.res_type, Fun(param.function), - indexes.get<1>(), indexes.get<0>(), - rhs_cells, lhs_cells, state.stash)); - } else { - return state.pop_pop_push( - FastValueIndex::sparse_only_merge<LCT,RCT,OCT,Fun>( - param.res_type, Fun(param.function), - indexes.get<0>(), indexes.get<1>(), - lhs_cells, rhs_cells, state.stash)); - } - } auto up = generic_mixed_merge<LCT, RCT, OCT, Fun>(lhs, rhs, param); auto &result = state.stash.create<std::unique_ptr<Value>>(std::move(up)); const Value &result_ref = *(result.get()); @@ -148,10 +92,7 @@ void my_sparse_merge_op(State &state, uint64_t param_in) { }; struct SelectGenericMergeOp { - template <typename LCT, typename RCT, typename OCT, typename Fun> static auto invoke(const MergeParam ¶m) { - if (param.dense_subspace_size == 1) { - return my_sparse_merge_op<LCT,RCT,OCT,Fun>; - } + template <typename LCT, typename RCT, typename OCT, typename Fun> static auto invoke() { return my_mixed_merge_op<LCT,RCT,OCT,Fun>; } }; @@ -167,7 +108,7 @@ GenericMerge::make_instruction(const ValueType &lhs_type, const ValueType &rhs_t const ValueBuilderFactory &factory, Stash &stash) { const auto ¶m = stash.create<MergeParam>(lhs_type, rhs_type, function, factory); - auto fun = typify_invoke<4,MergeTypify,SelectGenericMergeOp>(lhs_type.cell_type(), rhs_type.cell_type(), param.res_type.cell_type(), function, param); + auto fun = typify_invoke<4,MergeTypify,SelectGenericMergeOp>(lhs_type.cell_type(), rhs_type.cell_type(), param.res_type.cell_type(), function); return Instruction(fun, wrap_param<MergeParam>(param)); } diff --git a/eval/src/vespa/eval/instruction/generic_merge.h b/eval/src/vespa/eval/instruction/generic_merge.h index 2b2964366cc..0319f1a929f 100644 --- a/eval/src/vespa/eval/instruction/generic_merge.h +++ b/eval/src/vespa/eval/instruction/generic_merge.h @@ -6,6 +6,39 @@ namespace vespalib::eval::instruction { +struct MergeParam { + const ValueType res_type; + const join_fun_t function; + const size_t num_mapped_dimensions; + const size_t dense_subspace_size; + std::vector<size_t> all_view_dims; + const ValueBuilderFactory &factory; + MergeParam(const ValueType &lhs_type, const ValueType &rhs_type, + join_fun_t function_in, const ValueBuilderFactory &factory_in) + : res_type(ValueType::join(lhs_type, rhs_type)), + function(function_in), + num_mapped_dimensions(lhs_type.count_mapped_dimensions()), + dense_subspace_size(lhs_type.dense_subspace_size()), + all_view_dims(num_mapped_dimensions), + factory(factory_in) + { + assert(!res_type.is_error()); + assert(num_mapped_dimensions == rhs_type.count_mapped_dimensions()); + assert(num_mapped_dimensions == res_type.count_mapped_dimensions()); + assert(dense_subspace_size == rhs_type.dense_subspace_size()); + assert(dense_subspace_size == res_type.dense_subspace_size()); + for (size_t i = 0; i < num_mapped_dimensions; ++i) { + all_view_dims[i] = i; + } + } + ~MergeParam(); +}; + +template <typename LCT, typename RCT, typename OCT, typename Fun> +std::unique_ptr<Value> +generic_mixed_merge(const Value &a, const Value &b, + const MergeParam ¶ms); + struct GenericMerge { static InterpretedFunction::Instruction make_instruction(const ValueType &lhs_type, const ValueType &rhs_type, diff --git a/eval/src/vespa/eval/instruction/sparse_merge_function.cpp b/eval/src/vespa/eval/instruction/sparse_merge_function.cpp new file mode 100644 index 00000000000..924c4d69fe9 --- /dev/null +++ b/eval/src/vespa/eval/instruction/sparse_merge_function.cpp @@ -0,0 +1,146 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include "sparse_merge_function.h" +#include "generic_merge.h" +#include <vespa/eval/eval/fast_value.hpp> +#include <vespa/vespalib/util/typify.h> + +namespace vespalib::eval { + +using namespace tensor_function; +using namespace operation; +using namespace instruction; + +namespace { + +template <typename CT, bool single_dim, typename Fun> +const Value& my_fast_sparse_merge(const FastAddrMap &a_map, const FastAddrMap &b_map, + const CT *a_cells, const CT *b_cells, + const MergeParam ¶ms, + Stash &stash) +{ + Fun fun(params.function); + size_t guess_size = a_map.size() + b_map.size(); + auto &result = stash.create<FastValue<CT,true>>(params.res_type, params.num_mapped_dimensions, 1u, guess_size); + if constexpr (single_dim) { + string_id cur_label; + ConstArrayRef<string_id> addr(&cur_label, 1); + const auto &a_labels = a_map.labels(); + for (size_t i = 0; i < a_labels.size(); ++i) { + cur_label = a_labels[i]; + result.add_mapping(addr, cur_label.hash()); + result.my_cells.push_back_fast(a_cells[i]); + } + const auto &b_labels = b_map.labels(); + for (size_t i = 0; i < b_labels.size(); ++i) { + cur_label = b_labels[i]; + auto result_subspace = result.my_index.map.lookup_singledim(cur_label); + if (result_subspace == FastAddrMap::npos()) { + result.add_mapping(addr, cur_label.hash()); + result.my_cells.push_back_fast(b_cells[i]); + } else { + CT *out_cell = result.my_cells.get(result_subspace); + out_cell[0] = fun(out_cell[0], b_cells[i]); + } + } + } else { + a_map.each_map_entry([&](auto lhs_subspace, auto hash) + { + result.add_mapping(a_map.get_addr(lhs_subspace), hash); + result.my_cells.push_back_fast(a_cells[lhs_subspace]); + }); + b_map.each_map_entry([&](auto rhs_subspace, auto hash) + { + auto rhs_addr = b_map.get_addr(rhs_subspace); + auto result_subspace = result.my_index.map.lookup(rhs_addr, hash); + if (result_subspace == FastAddrMap::npos()) { + result.add_mapping(rhs_addr, hash); + result.my_cells.push_back_fast(b_cells[rhs_subspace]); + } else { + CT *out_cell = result.my_cells.get(result_subspace); + out_cell[0] = fun(out_cell[0], b_cells[rhs_subspace]); + } + }); + } + return result; +} + +template <typename CT, bool single_dim, typename Fun> +void my_sparse_merge_op(InterpretedFunction::State &state, uint64_t param_in) { + const auto ¶m = unwrap_param<MergeParam>(param_in); + assert(param.dense_subspace_size == 1u); + const Value &a = state.peek(1); + const Value &b = state.peek(0); + const auto &a_idx = a.index(); + const auto &b_idx = b.index(); + if (__builtin_expect(are_fast(a_idx, b_idx), true)) { + auto a_cells = a.cells().typify<CT>(); + auto b_cells = b.cells().typify<CT>(); + const Value &v = my_fast_sparse_merge<CT,single_dim,Fun>(as_fast(a_idx).map, as_fast(b_idx).map, + a_cells.cbegin(), b_cells.cbegin(), + param, state.stash); + state.pop_pop_push(v); + } else { + auto up = generic_mixed_merge<CT,CT,CT,Fun>(a, b, param); + state.pop_pop_push(*state.stash.create<std::unique_ptr<Value>>(std::move(up))); + } +} + +struct SelectSparseMergeOp { + template <typename CT, typename SINGLE_DIM, typename Fun> + static auto invoke() { return my_sparse_merge_op<CT,SINGLE_DIM::value,Fun>; } +}; + +using MyTypify = TypifyValue<TypifyCellType,TypifyBool,operation::TypifyOp2>; + +} // namespace <unnamed> + +SparseMergeFunction::SparseMergeFunction(const tensor_function::Merge &original) + : tensor_function::Merge(original.result_type(), + original.lhs(), + original.rhs(), + original.function()) +{ + assert(compatible_types(result_type(), lhs().result_type(), rhs().result_type())); +} + +InterpretedFunction::Instruction +SparseMergeFunction::compile_self(const ValueBuilderFactory &factory, Stash &stash) const +{ + const auto ¶m = stash.create<MergeParam>(lhs().result_type(), rhs().result_type(), + function(), factory); + size_t num_dims = result_type().count_mapped_dimensions(); + auto op = typify_invoke<3,MyTypify,SelectSparseMergeOp>(result_type().cell_type(), + num_dims == 1, + function()); + return InterpretedFunction::Instruction(op, wrap_param<MergeParam>(param)); +} + +bool +SparseMergeFunction::compatible_types(const ValueType &res, const ValueType &lhs, const ValueType &rhs) +{ + if ((lhs.cell_type() == rhs.cell_type()) + && (lhs.count_mapped_dimensions() > 0) + && (lhs.dense_subspace_size() == 1)) + { + assert(res == lhs); + assert(res == rhs); + return true; + } + return false; +} + +const TensorFunction & +SparseMergeFunction::optimize(const TensorFunction &expr, Stash &stash) +{ + if (auto merge = as<Merge>(expr)) { + const TensorFunction &lhs = merge->lhs(); + const TensorFunction &rhs = merge->rhs(); + if (compatible_types(expr.result_type(), lhs.result_type(), rhs.result_type())) { + return stash.create<SparseMergeFunction>(*merge); + } + } + return expr; +} + +} // namespace diff --git a/eval/src/vespa/eval/instruction/sparse_merge_function.h b/eval/src/vespa/eval/instruction/sparse_merge_function.h new file mode 100644 index 00000000000..d2b26196ed6 --- /dev/null +++ b/eval/src/vespa/eval/instruction/sparse_merge_function.h @@ -0,0 +1,22 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include <vespa/eval/eval/tensor_function.h> + +namespace vespalib::eval { + +/** + * Tensor function for merging two sparse tensors. + */ +class SparseMergeFunction : public tensor_function::Merge +{ +public: + SparseMergeFunction(const tensor_function::Merge &original); + InterpretedFunction::Instruction compile_self(const ValueBuilderFactory &factory, Stash &stash) const override; + bool result_is_mutable() const override { return true; } + static bool compatible_types(const ValueType &res, const ValueType &lhs, const ValueType &rhs); + static const TensorFunction &optimize(const TensorFunction &expr, Stash &stash); +}; + +} // namespace diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperations.java index dddd023aa85..ed1c035f543 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperations.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperations.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.node.admin.docker; import com.yahoo.config.provision.DockerImage; import com.yahoo.vespa.hosted.dockerapi.Container; +import com.yahoo.vespa.hosted.dockerapi.ContainerId; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ContainerResources; import com.yahoo.vespa.hosted.dockerapi.ContainerStats; @@ -30,7 +31,7 @@ public interface ContainerOperations { void removeContainer(NodeAgentContext context, Container container); - void updateContainer(NodeAgentContext context, ContainerResources containerResources); + void updateContainer(NodeAgentContext context, ContainerId containerId, ContainerResources containerResources); Optional<Container> getContainer(NodeAgentContext context); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImpl.java index 00fed2ba6b7..ed130105fff 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImpl.java @@ -8,6 +8,7 @@ import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.hosted.dockerapi.Container; import com.yahoo.vespa.hosted.dockerapi.ContainerEngine; +import com.yahoo.vespa.hosted.dockerapi.ContainerId; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ContainerResources; import com.yahoo.vespa.hosted.dockerapi.ContainerStats; @@ -203,7 +204,7 @@ public class ContainerOperationsImpl implements ContainerOperations { } @Override - public void updateContainer(NodeAgentContext context, ContainerResources containerResources) { + public void updateContainer(NodeAgentContext context, ContainerId containerId, ContainerResources containerResources) { containerEngine.updateContainer(context.containerName(), containerResources); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java index 8b095a46dcf..c33b084c213 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java @@ -49,12 +49,14 @@ public interface NodeAgentContext extends TaskContext { }; /** - * The vcpu value in NodeSpec is multiplied by the speedup factor per cpu core compared to a historical baseline - * for a particular cpu generation of the host (see flavors.def cpuSpeedup). + * The vcpu value in NodeSpec is the number of vcpus required by the node on a fixed historical + * baseline machine. However the current host has a faster per-vcpu performance by a scale factor + * (see flavors.def cpuSpeedup), and therefore do not need to set aside the full number of vcpus + * to run the node. This method returns that reduced number of vcpus. * - * @return node vcpu without the cpu speedup factor. + * @return the vcpus required by the node on this host. */ - double unscaledVcpu(); + double vcpuOnThisHost(); /** The file system used by the NodeAgentContext. All paths must have the same provider. */ FileSystem fileSystem(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java index 8ac5a89aaef..6ab2469cd64 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java @@ -118,7 +118,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { } @Override - public double unscaledVcpu() { + public double vcpuOnThisHost() { return node.vcpu() / cpuSpeedup; } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java index dc0b6dc9d85..61de751f60d 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java @@ -372,7 +372,7 @@ public class NodeAgentImpl implements NodeAgent { wantedContainerResources.toStringCpu(), existingContainer.resources.toStringCpu()); // Only update CPU resources - containerOperations.updateContainer(context, wantedContainerResources.withMemoryBytes(existingContainer.resources.memoryBytes())); + containerOperations.updateContainer(context, existingContainer.id(), wantedContainerResources.withMemoryBytes(existingContainer.resources.memoryBytes())); return containerOperations.getContainer(context).orElseThrow(() -> new ConvergenceException("Did not find container that was just updated")); } @@ -384,9 +384,9 @@ public class NodeAgentImpl implements NodeAgent { .map(appId -> containerCpuCap.with(FetchVector.Dimension.APPLICATION_ID, appId.serializedForm())) .orElse(containerCpuCap) .with(FetchVector.Dimension.HOSTNAME, context.node().hostname()) - .value() * context.unscaledVcpu(); + .value() * context.vcpuOnThisHost(); - return ContainerResources.from(cpuCap, context.unscaledVcpu(), context.node().memoryGb()); + return ContainerResources.from(cpuCap, context.vcpuOnThisHost(), context.node().memoryGb()); } private boolean noCpuCap(ZoneApi zone) { diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java index 6c1c1a48a9c..cb7f1637410 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/UnixPath.java @@ -208,6 +208,11 @@ public class UnixPath { return this; } + public UnixPath createDirectories() { + uncheck(() -> Files.createDirectories(path)); + return this; + } + /** * Returns whether this path is a directory. Symlinks are followed, so this returns true for symlinks pointing to a * directory. diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImplTest.java index 240fb492aff..e78d5bb754b 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/ContainerOperationsImplTest.java @@ -5,6 +5,7 @@ import com.google.common.net.InetAddresses; import com.yahoo.config.provision.DockerImage; import com.yahoo.vespa.hosted.dockerapi.Container; import com.yahoo.vespa.hosted.dockerapi.ContainerEngine; +import com.yahoo.vespa.hosted.dockerapi.ContainerId; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ProcessResult; import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData; @@ -85,8 +86,8 @@ public class ContainerOperationsImplTest { } private Container makeContainer(String name, Container.State state, int pid) { - final Container container = new Container(name + ".fqdn", DockerImage.fromString("registry.example.com/mock"), null, - new ContainerName(name), state, pid); + final Container container = new Container(new ContainerId(name + "-id"), name + ".fqdn", + DockerImage.fromString("registry.example.com/mock"), null, new ContainerName(name), state, pid); when(containerEngine.getContainer(eq(container.name))).thenReturn(Optional.of(container)); return container; } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ContainerEngineMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ContainerEngineMock.java index ef6564db2a5..7aeaa37b4af 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ContainerEngineMock.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/ContainerEngineMock.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.node.admin.integrationTests; import com.yahoo.config.provision.DockerImage; import com.yahoo.vespa.hosted.dockerapi.Container; import com.yahoo.vespa.hosted.dockerapi.ContainerEngine; +import com.yahoo.vespa.hosted.dockerapi.ContainerId; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ContainerResources; import com.yahoo.vespa.hosted.dockerapi.ContainerStats; @@ -25,12 +26,14 @@ import java.util.OptionalLong; * @author freva */ public class ContainerEngineMock implements ContainerEngine { + public static final ContainerId CONTAINER_ID = new ContainerId("af345"); + private final Map<ContainerName, Container> containersByContainerName = new HashMap<>(); private static final Object monitor = new Object(); @Override public CreateContainerCommand createContainerCommand(DockerImage dockerImage, ContainerName containerName) { - return new StartContainerCommandMock(dockerImage, containerName); + return new StartContainerCommandMock(CONTAINER_ID, dockerImage, containerName); } @Override @@ -48,7 +51,7 @@ public class ContainerEngineMock implements ContainerEngine { synchronized (monitor) { Container container = containersByContainerName.get(containerName); containersByContainerName.put(containerName, - new Container(container.hostname, container.image, container.resources, container.name, Container.State.EXITED, 0)); + new Container(container.id(), container.hostname, container.image, container.resources, container.name, Container.State.EXITED, 0)); } } @@ -64,7 +67,7 @@ public class ContainerEngineMock implements ContainerEngine { synchronized (monitor) { Container container = containersByContainerName.get(containerName); containersByContainerName.put(containerName, - new Container(container.hostname, container.image, containerResources, container.name, container.state, container.pid)); + new Container(container.id(), container.hostname, container.image, containerResources, container.name, container.state, container.pid)); } } @@ -104,12 +107,14 @@ public class ContainerEngineMock implements ContainerEngine { public class StartContainerCommandMock implements CreateContainerCommand { + private final ContainerId containerId; private final DockerImage dockerImage; private final ContainerName containerName; private String hostName; private ContainerResources containerResources; - public StartContainerCommandMock(DockerImage dockerImage, ContainerName containerName) { + public StartContainerCommandMock(ContainerId containerId, DockerImage dockerImage, ContainerName containerName) { + this.containerId = containerId; this.dockerImage = dockerImage; this.containerName = containerName; } @@ -200,7 +205,7 @@ public class ContainerEngineMock implements ContainerEngine { public void create() { synchronized (monitor) { containersByContainerName.put( - containerName, new Container(hostName, dockerImage, containerResources, containerName, Container.State.RUNNING, 2)); + containerName, new Container(containerId, hostName, dockerImage, containerResources, containerName, Container.State.RUNNING, 2)); } } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java index fcd5e8cc187..97c83956a61 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java @@ -10,6 +10,7 @@ import com.yahoo.test.ManualClock; import com.yahoo.vespa.flags.InMemoryFlagSource; import com.yahoo.vespa.flags.PermanentFlags; import com.yahoo.vespa.hosted.dockerapi.Container; +import com.yahoo.vespa.hosted.dockerapi.ContainerId; import com.yahoo.vespa.hosted.dockerapi.ContainerName; import com.yahoo.vespa.hosted.dockerapi.ContainerResources; import com.yahoo.vespa.hosted.dockerapi.RegistryCredentials; @@ -54,6 +55,7 @@ import static org.mockito.Mockito.when; public class NodeAgentImplTest { private static final NodeResources resources = new NodeResources(2, 16, 250, 1, NodeResources.DiskSpeed.fast, NodeResources.StorageType.local); private static final Version vespaVersion = Version.fromString("1.2.3"); + private static final ContainerId containerId = new ContainerId("af23"); private static final String hostName = "host1.test.yahoo.com"; private final NodeAgentContextSupplier contextSupplier = mock(NodeAgentContextSupplier.class); @@ -226,7 +228,7 @@ public class NodeAgentImplTest { mockGetContainer(dockerImage, resourcesAfterThird, true); inOrder.verify(orchestrator, never()).suspend(any()); - inOrder.verify(containerOperations).updateContainer(eq(thirdContext), eq(resourcesAfterThird)); + inOrder.verify(containerOperations).updateContainer(eq(thirdContext), eq(containerId), eq(resourcesAfterThird)); inOrder.verify(containerOperations, never()).removeContainer(any(), any()); inOrder.verify(containerOperations, never()).startContainer(any()); inOrder.verify(orchestrator, never()).resume(any()); @@ -234,7 +236,7 @@ public class NodeAgentImplTest { // No changes nodeAgent.converge(thirdContext); inOrder.verify(orchestrator, never()).suspend(any()); - inOrder.verify(containerOperations, never()).updateContainer(eq(thirdContext), any()); + inOrder.verify(containerOperations, never()).updateContainer(eq(thirdContext), eq(containerId), any()); inOrder.verify(containerOperations, never()).removeContainer(any(), any()); inOrder.verify(orchestrator, never()).resume(any()); @@ -242,7 +244,7 @@ public class NodeAgentImplTest { flagSource.withDoubleFlag(PermanentFlags.CONTAINER_CPU_CAP.id(), 2.3); nodeAgent.doConverge(thirdContext); - inOrder.verify(containerOperations).updateContainer(eq(thirdContext), eq(ContainerResources.from(9.2, 4, 16))); + inOrder.verify(containerOperations).updateContainer(eq(thirdContext), eq(containerId), eq(ContainerResources.from(9.2, 4, 16))); inOrder.verify(orchestrator, never()).resume(any()); } @@ -267,13 +269,13 @@ public class NodeAgentImplTest { InOrder inOrder = inOrder(orchestrator, containerOperations, nodeRepository); inOrder.verify(orchestrator).resume(any(String.class)); inOrder.verify(containerOperations).removeContainer(eq(secondContext), any()); - inOrder.verify(containerOperations, never()).updateContainer(any(), any()); + inOrder.verify(containerOperations, never()).updateContainer(any(), any(), any()); inOrder.verify(containerOperations, never()).restartVespa(any()); inOrder.verify(nodeRepository).updateNodeAttributes(eq(hostName), eq(new NodeAttributes().withRestartGeneration(2))); nodeAgent.doConverge(secondContext); inOrder.verify(orchestrator).resume(any(String.class)); - inOrder.verify(containerOperations, never()).updateContainer(any(), any()); + inOrder.verify(containerOperations, never()).updateContainer(any(), any(), any()); inOrder.verify(containerOperations, never()).removeContainer(any(), any()); } @@ -641,14 +643,14 @@ public class NodeAgentImplTest { } inOrder.verify(orchestrator, never()).resume(any()); inOrder.verify(orchestrator, never()).suspend(any()); - inOrder.verify(containerOperations, never()).updateContainer(any(), any()); + inOrder.verify(containerOperations, never()).updateContainer(any(), any(), any()); clock.advance(Duration.ofSeconds(31)); nodeAgent.doConverge(context); inOrder.verify(orchestrator, never()).suspend(any()); - inOrder.verify(containerOperations).updateContainer(eq(context), eq(ContainerResources.from(0, 2, 16))); + inOrder.verify(containerOperations).updateContainer(eq(context), eq(containerId), eq(ContainerResources.from(0, 2, 16))); inOrder.verify(containerOperations, never()).removeContainer(any(), any()); inOrder.verify(containerOperations, never()).startContainer(any()); inOrder.verify(orchestrator, never()).resume(any()); @@ -656,7 +658,7 @@ public class NodeAgentImplTest { // No changes nodeAgent.converge(context); inOrder.verify(orchestrator, never()).suspend(any()); - inOrder.verify(containerOperations, never()).updateContainer(eq(context), any()); + inOrder.verify(containerOperations, never()).updateContainer(eq(context), eq(containerId), any()); inOrder.verify(containerOperations, never()).removeContainer(any(), any()); inOrder.verify(orchestrator, never()).resume(any()); } @@ -678,7 +680,7 @@ public class NodeAgentImplTest { nodeAgent.converge(context); inOrder.verify(orchestrator, never()).suspend(any(String.class)); - inOrder.verify(containerOperations, never()).updateContainer(eq(context), any()); + inOrder.verify(containerOperations, never()).updateContainer(eq(context), eq(containerId), any()); inOrder.verify(containerOperations, never()).removeContainer(any(), any()); inOrder.verify(orchestrator, never()).resume(any(String.class)); } @@ -728,10 +730,10 @@ public class NodeAgentImplTest { doAnswer(invoc -> { NodeAgentContext context = invoc.getArgument(0, NodeAgentContext.class); - ContainerResources resources = invoc.getArgument(1, ContainerResources.class); + ContainerResources resources = invoc.getArgument(2, ContainerResources.class); mockGetContainer(context.node().wantedDockerImage().get(), resources, true); return null; - }).when(containerOperations).updateContainer(any(), any()); + }).when(containerOperations).updateContainer(any(), any(), any()); return new NodeAgentImpl(contextSupplier, nodeRepository, orchestrator, containerOperations, () -> RegistryCredentials.none, storageMaintainer, flagSource, @@ -750,6 +752,7 @@ public class NodeAgentImplTest { throw new IllegalArgumentException(); return dockerImage != null ? Optional.of(new Container( + containerId, hostName, dockerImage, containerResources, diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java index b0b61e8a6b2..19c1fa090c9 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java @@ -69,6 +69,16 @@ public class NodeList extends AbstractFilteringList<Node, NodeList> { return matching(node -> node.allocation().isPresent() && node.allocation().get().membership().cluster().type().isContainer()); } + /** Returns the subset of nodes that run a stateless service */ + public NodeList stateless() { + return matching(node -> node.allocation().isPresent() && ! node.allocation().get().membership().cluster().isStateful()); + } + + /** Returns the subset of nodes that run a stateful service */ + public NodeList stateful() { + return matching(node -> node.allocation().isPresent() && node.allocation().get().membership().cluster().isStateful()); + } + /** Returns the subset of nodes that are currently changing their Vespa version */ public NodeList changingVersion() { return matching(node -> node.status().vespaVersion().isPresent() && diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index 4eb38fa650e..8e14b61db9a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -29,6 +29,7 @@ import com.yahoo.vespa.hosted.provision.maintenance.NodeFailer; import com.yahoo.vespa.hosted.provision.maintenance.PeriodicApplicationMaintainer; import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.Allocation; +import com.yahoo.vespa.hosted.provision.node.History; import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.node.NodeAcl; import com.yahoo.vespa.hosted.provision.node.filter.NodeFilter; @@ -410,7 +411,7 @@ public class NodeRepository extends AbstractComponent { for (Node node : nodes) { if ( ! node.flavor().getType().equals(Flavor.Type.DOCKER_CONTAINER)) illegal("Cannot add " + node + ": This is not a docker node"); - if ( ! node.allocation().isPresent()) + if (node.allocation().isEmpty()) illegal("Cannot add " + node + ": Docker containers needs to be allocated"); Optional<Node> existing = getNode(node.hostname()); if (existing.isPresent()) @@ -514,7 +515,13 @@ public class NodeRepository extends AbstractComponent { * transaction commits. */ public List<Node> deactivate(List<Node> nodes, ApplicationTransaction transaction) { - return db.writeTo(State.inactive, nodes, Agent.application, Optional.empty(), transaction.nested()); + var stateless = NodeList.copyOf(nodes).stateless(); + var stateful = NodeList.copyOf(nodes).stateful(); + List<Node> written = new ArrayList<>(); + written.addAll(deallocate(stateless.asList(), Agent.application, "Deactivated by application", transaction.nested())); + written.addAll(db.writeTo(State.inactive, stateful.asList(), Agent.application, Optional.empty(), transaction.nested())); + return written; + } /** Removes this application: Active nodes are deactivated while all non-active nodes are set dirty. */ @@ -531,22 +538,11 @@ public class NodeRepository extends AbstractComponent { } /** Move nodes to the dirty state */ - public List<Node> setDirty(List<Node> nodes, Agent agent, String reason) { - return performOn(NodeListFilter.from(nodes), (node, lock) -> setDirty(node, agent, reason)); - } - - /** - * Set a node dirty, allowed if it is in the provisioned, inactive, failed or parked state. - * Use this to clean newly provisioned nodes or to recycle failed nodes which have been repaired or put on hold. - * - * @throws IllegalArgumentException if the node has hardware failure - */ - public Node setDirty(Node node, Agent agent, String reason) { - return db.writeTo(State.dirty, node, agent, Optional.of(reason)); + public List<Node> deallocate(List<Node> nodes, Agent agent, String reason) { + return performOn(NodeListFilter.from(nodes), (node, lock) -> deallocate(node, agent, reason)); } - - public List<Node> dirtyRecursively(String hostname, Agent agent, String reason) { + public List<Node> deallocateRecursively(String hostname, Agent agent, String reason) { Node nodeToDirty = getNode(hostname).orElseThrow(() -> new IllegalArgumentException("Could not deallocate " + hostname + ": Node not found")); @@ -568,7 +564,37 @@ public class NodeRepository extends AbstractComponent { illegal("Could not deallocate " + nodeToDirty + ": " + hostnamesNotAllowedToDirty + " are not in states [provisioned, failed, parked, breakfixed]"); - return nodesToDirty.stream().map(node -> setDirty(node, agent, reason)).collect(Collectors.toList()); + return nodesToDirty.stream().map(node -> deallocate(node, agent, reason)).collect(Collectors.toList()); + } + + /** + * Set a node dirty or parked, allowed if it is in the provisioned, inactive, failed or parked state. + * Use this to clean newly provisioned nodes or to recycle failed nodes which have been repaired or put on hold. + */ + public Node deallocate(Node node, Agent agent, String reason) { + NestedTransaction transaction = new NestedTransaction(); + Node deallocated = deallocate(node, agent, reason, transaction); + transaction.commit(); + return deallocated; + } + + public List<Node> deallocate(List<Node> nodes, Agent agent, String reason, NestedTransaction transaction) { + return nodes.stream().map(node -> deallocate(node, agent, reason, transaction)).collect(Collectors.toList()); + } + + public Node deallocate(Node node, Agent agent, String reason, NestedTransaction transaction) { + if (node.state() != State.parked && agent != Agent.operator + && (node.status().wantToDeprovision() || retiredByOperator(node))) + return park(node.hostname(), false, agent, reason, transaction); + else + return db.writeTo(State.dirty, List.of(node), agent, Optional.of(reason), transaction).get(0); + } + + private static boolean retiredByOperator(Node node) { + return node.status().wantToRetire() && node.history().event(History.Event.Type.wantToRetire) + .map(History.Event::agent) + .map(agent -> agent == Agent.operator) + .orElse(false); } /** @@ -597,7 +623,14 @@ public class NodeRepository extends AbstractComponent { * @throws NoSuchNodeException if the node is not found */ public Node park(String hostname, boolean keepAllocation, Agent agent, String reason) { - return move(hostname, keepAllocation, State.parked, agent, Optional.of(reason)); + NestedTransaction transaction = new NestedTransaction(); + Node parked = park(hostname, keepAllocation, agent, reason, transaction); + transaction.commit(); + return parked; + } + + public Node park(String hostname, boolean keepAllocation, Agent agent, String reason, NestedTransaction transaction) { + return move(hostname, keepAllocation, State.parked, agent, Optional.of(reason), transaction); } /** @@ -644,6 +677,14 @@ public class NodeRepository extends AbstractComponent { } private Node move(String hostname, boolean keepAllocation, State toState, Agent agent, Optional<String> reason) { + NestedTransaction transaction = new NestedTransaction(); + Node moved = move(hostname, keepAllocation, toState, agent, reason, transaction); + transaction.commit(); + return moved; + } + + private Node move(String hostname, boolean keepAllocation, State toState, Agent agent, Optional<String> reason, + NestedTransaction transaction) { Node node = getNode(hostname).orElseThrow(() -> new NoSuchNodeException("Could not move " + hostname + " to " + toState + ": Node not found")); @@ -651,10 +692,17 @@ public class NodeRepository extends AbstractComponent { node = node.withoutAllocation(); } - return move(node, toState, agent, reason); + return move(node, toState, agent, reason, transaction); } private Node move(Node node, State toState, Agent agent, Optional<String> reason) { + NestedTransaction transaction = new NestedTransaction(); + Node moved = move(node, toState, agent, reason, transaction); + transaction.commit(); + return moved; + } + + private Node move(Node node, State toState, Agent agent, Optional<String> reason, NestedTransaction transaction) { if (toState == Node.State.active && node.allocation().isEmpty()) illegal("Could not set " + node + " active. It has no allocation."); @@ -667,7 +715,7 @@ public class NodeRepository extends AbstractComponent { illegal("Could not set " + node + " active: Same cluster and index as " + currentActive); } } - return db.writeTo(toState, node, agent, reason); + return db.writeTo(toState, List.of(node), agent, reason, transaction).get(0); } } @@ -981,4 +1029,5 @@ public class NodeRepository extends AbstractComponent { private void illegal(String message) { throw new IllegalArgumentException(message); } + } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java index b55f49722cb..1a47af6b929 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java @@ -112,7 +112,7 @@ public class FailedExpirer extends NodeRepositoryMaintainer { nodesToRecycle.add(candidate); } } - nodeRepository.setDirty(nodesToRecycle, Agent.FailedExpirer, "Expired by FailedExpirer"); + nodeRepository.deallocate(nodesToRecycle, Agent.FailedExpirer, "Expired by FailedExpirer"); } /** Returns whether the current node fail count should be used as an indicator of hardware issue */ diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java index 392146f6ead..231d2ac08b1 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java @@ -14,14 +14,9 @@ import java.util.List; /** * Maintenance job which moves inactive nodes to dirty or parked after timeout. * - * The timeout is in place for two reasons: - * - * - To ensure that the new application configuration has time to - * propagate before the node is used for something else. - * - * - To provide a grace period in which nodes can be brought back to active - * if they were deactivated in error. As inactive nodes retain their state - * they can be brought back to active and correct state faster than a new node. + * The timeout is in place to provide a grace period in which nodes can be brought back to active + * if they were deactivated in error. As inactive nodes retain their state + * they can be brought back to active and correct state faster than a new node. * * Nodes with following flags set are not reusable and will be moved to parked * instead of dirty: @@ -44,11 +39,7 @@ public class InactiveExpirer extends Expirer { @Override protected void expire(List<Node> expired) { expired.forEach(node -> { - if (node.status().wantToDeprovision() || retiredByOperator(node)) { - nodeRepository.park(node.hostname(), false, Agent.InactiveExpirer, "Expired by InactiveExpirer"); - } else { - nodeRepository.setDirty(node, Agent.InactiveExpirer, "Expired by InactiveExpirer"); - } + nodeRepository.deallocate(node, Agent.InactiveExpirer, "Expired by InactiveExpirer"); }); } @@ -58,11 +49,4 @@ public class InactiveExpirer extends Expirer { || node.allocation().get().owner().instance().isTester(); } - private static boolean retiredByOperator(Node node) { - return node.status().wantToRetire() && node.history().event(History.Event.Type.wantToRetire) - .map(History.Event::agent) - .map(agent -> agent == Agent.operator) - .orElse(false); - } - } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceDeployment.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceDeployment.java index 66ddf7f9ffe..52c487c28cf 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceDeployment.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceDeployment.java @@ -186,7 +186,7 @@ class MaintenanceDeployment implements Closeable { // Immediately clean up if we reserved the node but could not activate or reserved a node on the wrong host expectedNewNode.flatMap(node -> nodeRepository.getNode(node.hostname(), Node.State.reserved)) - .ifPresent(node -> nodeRepository.setDirty(node, agent, "Expired by " + agent)); + .ifPresent(node -> nodeRepository.deallocate(node, agent, "Expired by " + agent)); } } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirer.java index c6427123d09..1967615de02 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirer.java @@ -25,6 +25,6 @@ public class ReservationExpirer extends Expirer { } @Override - protected void expire(List<Node> expired) { nodeRepository().setDirty(expired, Agent.ReservationExpirer, "Expired by ReservationExpirer"); } + protected void expire(List<Node> expired) { nodeRepository().deallocate(expired, Agent.ReservationExpirer, "Expired by ReservationExpirer"); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiHandler.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiHandler.java index 9b5dac49e32..08bfd104863 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiHandler.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesV2ApiHandler.java @@ -142,7 +142,7 @@ public class NodesV2ApiHandler extends LoggingRequestHandler { return new MessageResponse("Moved " + hostnamesAsString(parkedNodes) + " to parked"); } else if (path.startsWith("/nodes/v2/state/dirty/")) { - List<Node> dirtiedNodes = nodeRepository.dirtyRecursively(lastElement(path), Agent.operator, "Dirtied through the nodes/v2 API"); + List<Node> dirtiedNodes = nodeRepository.deallocateRecursively(lastElement(path), Agent.operator, "Dirtied through the nodes/v2 API"); return new MessageResponse("Moved " + hostnamesAsString(dirtiedNodes) + " to dirty"); } else if (path.startsWith("/nodes/v2/state/active/")) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java index 1f8f3d32043..cf1ccf9a052 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java @@ -142,11 +142,11 @@ public class MockNodeRepository extends NodeRepository { nodes = addNodes(nodes, Agent.system); nodes.remove(node7); nodes.remove(node55); - nodes = setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = deallocate(nodes, Agent.system, getClass().getSimpleName()); setReady(nodes, Agent.system, getClass().getSimpleName()); fail(node5.hostname(), Agent.system, getClass().getSimpleName()); - dirtyRecursively(node55.hostname(), Agent.system, getClass().getSimpleName()); + deallocateRecursively(node55.hostname(), Agent.system, getClass().getSimpleName()); fail("dockerhost6.yahoo.com", Agent.operator, getClass().getSimpleName()); removeRecursively("dockerhost6.yahoo.com"); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java index bcf53a07490..5f357b9e4b0 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java @@ -228,12 +228,12 @@ public class NodeRepositoryTest { assertEquals(6, tester.nodeRepository().getNodes().size()); // Should be OK to dirty host2 as it is in provisioned and its only child is in failed - tester.nodeRepository().dirtyRecursively("host2", Agent.system, NodeRepositoryTest.class.getSimpleName()); + tester.nodeRepository().deallocateRecursively("host2", Agent.system, NodeRepositoryTest.class.getSimpleName()); assertEquals(asSet("host2", "node20"), filterNodes(tester, node -> node.state() == Node.State.dirty)); // Cant dirty host1, node11 is ready and node12 is active try { - tester.nodeRepository().dirtyRecursively("host1", Agent.system, NodeRepositoryTest.class.getSimpleName()); + tester.nodeRepository().deallocateRecursively("host1", Agent.system, NodeRepositoryTest.class.getSimpleName()); fail("Should not be able to dirty host1"); } catch (IllegalArgumentException ignored) { } // Expected; diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java index 940404fa605..9f2f7541c91 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java @@ -312,7 +312,7 @@ public class FailedExpirerTest { List<Node> nodes = Stream.of(hostname) .map(this::get) .collect(Collectors.toList()); - nodes = nodeRepository.setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = nodeRepository.deallocate(nodes, Agent.system, getClass().getSimpleName()); nodeRepository.setReady(nodes, Agent.system, getClass().getSimpleName()); return this; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java index 8bcf1552204..60ca625d07e 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java @@ -60,12 +60,6 @@ public class LoadBalancerExpirerTest { // Expirer defers removal while nodes are still allocated to application expirer.maintain(); - assertEquals(3, tester.loadBalancerService().instances().size()); - assertEquals(2, tester.loadBalancerService().instances().get(lb1).reals().size()); - dirtyNodesOf(app1, cluster1); - - // Expirer prunes reals before expiration time of load balancer itself - expirer.maintain(); assertEquals(Set.of(), tester.loadBalancerService().instances().get(lb1).reals()); assertEquals(Set.of(), loadBalancers.get().get(lb1).instance().reals()); @@ -138,11 +132,11 @@ public class LoadBalancerExpirerTest { } private void dirtyNodesOf(ApplicationId application, ClusterSpec.Id cluster) { - tester.nodeRepository().setDirty(tester.nodeRepository().getNodes(application).stream() - .filter(node -> node.allocation().isPresent()) - .filter(node -> node.allocation().get().membership().cluster().id().equals(cluster)) - .collect(Collectors.toList()), - Agent.system, this.getClass().getSimpleName()); + tester.nodeRepository().deallocate(tester.nodeRepository().getNodes(application).stream() + .filter(node -> node.allocation().isPresent()) + .filter(node -> node.allocation().get().membership().cluster().id().equals(cluster)) + .collect(Collectors.toList()), + Agent.system, this.getClass().getSimpleName()); } private void deployApplication(ApplicationId application, ClusterSpec.Id... clusters) { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java index c03e489def2..9adba744101 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java @@ -163,7 +163,7 @@ public class MetricsReporterTest { Node dockerHost = Node.create("openStackId1", new IP.Config(Set.of("::1"), ipAddressPool), "dockerHost", nodeFlavors.getFlavorOrThrow("host"), NodeType.host).build(); nodeRepository.addNodes(List.of(dockerHost), Agent.system); - nodeRepository.dirtyRecursively("dockerHost", Agent.system, getClass().getSimpleName()); + nodeRepository.deallocateRecursively("dockerHost", Agent.system, getClass().getSimpleName()); nodeRepository.setReady("dockerHost", Agent.system, getClass().getSimpleName()); Node container1 = Node.createDockerNode(Set.of("::2"), "container1", @@ -225,7 +225,7 @@ public class MetricsReporterTest { ApplicationId application = ApplicationId.from("t1", "a1", "default"); Map<String, String> dimensions = Map.of("applicationId", application.toFullString()); NodeResources resources = new NodeResources(2, 8, 100, 1); - List<Node> activeNodes = tester.deploy(application, Capacity.from(new ClusterResources(4, 1, resources))); + List<Node> activeNodes = tester.deploy(application, ProvisioningTester.contentClusterSpec(), Capacity.from(new ClusterResources(4, 1, resources))); metricsReporter.maintain(); assertEquals(0D, getMetric("nodes.nonActiveFraction", metric, dimensions)); assertEquals(4, getMetric("nodes.active", metric, dimensions)); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java index 12b682ae26c..d0473d08ea2 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java @@ -259,7 +259,7 @@ public class NodeFailTester { } nodes = nodeRepository.addNodes(nodes, Agent.system); - nodes = nodeRepository.setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = nodeRepository.deallocate(nodes, Agent.system, getClass().getSimpleName()); return nodeRepository.setReady(nodes, Agent.system, getClass().getSimpleName()); } @@ -267,7 +267,7 @@ public class NodeFailTester { List<Node> nodes = tester.makeProvisionedNodes(count, (index) -> "parent" + index, hostFlavors.getFlavorOrThrow("default"), Optional.empty(), NodeType.host, 10, false); - nodes = nodeRepository.setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = nodeRepository.deallocate(nodes, Agent.system, getClass().getSimpleName()); tester.activateTenantHosts(); return nodeRepository.setReady(nodes, Agent.system, getClass().getSimpleName()); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java index cd188bc017f..ae1e36d73dd 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java @@ -96,10 +96,10 @@ public class PeriodicApplicationMaintainerTest { // Cause maintenance deployment which will update the applications with the re-activated nodes clock.advance(Duration.ofMinutes(35)); // Otherwise redeploys are inhibited fixture.runApplicationMaintainer(); - assertEquals("Superflous content nodes are retired", + assertEquals("Superfluous content nodes are retired", reactivatedInApp2, fixture.getNodes(Node.State.active).retired().size()); - assertEquals("Superflous container nodes are deactivated (this makes little point for container nodes)", - reactivatedInApp1, fixture.getNodes(Node.State.inactive).size()); + assertEquals("Superfluous container nodes are deallocated", + reactivatedInApp1, fixture.getNodes(Node.State.dirty).size()); } @Test(timeout = 60_000) diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java index 0246ec524e3..ef0e6d4ddc4 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.provisioning; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; +import com.yahoo.config.provision.ApplicationTransaction; import com.yahoo.config.provision.Capacity; import com.yahoo.config.provision.ClusterResources; import com.yahoo.config.provision.ClusterSpec; @@ -14,11 +15,14 @@ import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.OutOfCapacityException; +import com.yahoo.config.provision.ProvisionLock; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.Zone; +import com.yahoo.transaction.NestedTransaction; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; +import com.yahoo.vespa.hosted.provision.node.Agent; import org.junit.Test; import java.util.HashSet; @@ -356,11 +360,15 @@ public class DockerProvisioningTest { tester.makeReadyHosts(5, r).activateTenantHosts(); ApplicationId app1 = ProvisioningTester.applicationId("app1"); - ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.container, new ClusterSpec.Id("cluster1")).vespaVersion("7").build(); + ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build(); tester.activate(app1, cluster1, Capacity.from(new ClusterResources(5, 1, r))); tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, r))); + var tx = new ApplicationTransaction(new ProvisionLock(app1, tester.nodeRepository().lock(app1)), new NestedTransaction()); + tester.nodeRepository().deactivate(tester.nodeRepository().list(app1, Node.State.active).retired().asList(), tx); + tx.nested().commit(); + assertEquals(2, tester.getNodes(app1, Node.State.active).size()); assertEquals(3, tester.getNodes(app1, Node.State.inactive).size()); @@ -372,16 +380,16 @@ public class DockerProvisioningTest { } @Test - public void inactive_container_nodes_are_reused() { - assertInactiveReuse(ClusterSpec.Type.container); + public void inactive_container_nodes_are_not_reused() { + assertInactiveReuse(ClusterSpec.Type.container, false); } @Test public void inactive_content_nodes_are_reused() { - assertInactiveReuse(ClusterSpec.Type.content); + assertInactiveReuse(ClusterSpec.Type.content, true); } - private void assertInactiveReuse(ClusterSpec.Type clusterType) { + private void assertInactiveReuse(ClusterSpec.Type clusterType, boolean expectedReuse) { NodeResources r = new NodeResources(20, 40, 100, 4); ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))) .flavors(List.of(new Flavor(r))) @@ -398,9 +406,18 @@ public class DockerProvisioningTest { tester.nodeRepository().setRemovable(app1, tester.getNodes(app1).retired().asList()); tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, r))); - assertEquals(2, tester.getNodes(app1, Node.State.inactive).size()); - tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r))); - assertEquals(0, tester.getNodes(app1, Node.State.inactive).size()); + if (expectedReuse) { + assertEquals(2, tester.getNodes(app1, Node.State.inactive).size()); + tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r))); + assertEquals(0, tester.getNodes(app1, Node.State.inactive).size()); + } + else { + assertEquals(0, tester.getNodes(app1, Node.State.inactive).size()); + assertEquals(2, tester.nodeRepository().getNodes(Node.State.dirty).size()); + tester.nodeRepository().setReady(tester.nodeRepository().getNodes(Node.State.dirty), Agent.system, "test"); + tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r))); + } + } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java index eef342b527b..105f2122e0c 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java @@ -263,7 +263,7 @@ public class LoadBalancerProvisionerTest { } private void dirtyNodesOf(ApplicationId application) { - tester.nodeRepository().setDirty(tester.nodeRepository().getNodes(application), Agent.system, this.getClass().getSimpleName()); + tester.nodeRepository().deallocate(tester.nodeRepository().getNodes(application), Agent.system, this.getClass().getSimpleName()); } private Set<HostSpec> prepare(ApplicationId application, ClusterSpec... specs) { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java index acf7bda3cbf..bf2a6ba3627 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java @@ -153,7 +153,7 @@ public class NodeTypeProvisioningTest { assertEquals(10, nodes.size()); // Verify that the node is now inactive - assertEquals(Node.State.inactive, tester.nodeRepository().getNode(nodeToRetire.hostname()) + assertEquals(Node.State.dirty, tester.nodeRepository().getNode(nodeToRetire.hostname()) .orElseThrow(RuntimeException::new).state()); } } @@ -232,7 +232,7 @@ public class NodeTypeProvisioningTest { assertEquals(10, nodes.size()); // Verify the node we previously set to retire has finished retiring - assertEquals(Node.State.inactive, tester.nodeRepository().getNode(currentyRetiringHostname) + assertEquals(Node.State.dirty, tester.nodeRepository().getNode(currentyRetiringHostname) .orElseThrow(RuntimeException::new).state()); // Verify that a node is currently retiring @@ -263,9 +263,9 @@ public class NodeTypeProvisioningTest { .count(); assertEquals(11 - numNodesToRetire, numRetiredActiveProxyNodes); - // All the nodes that were marked with wantToRetire earlier are now inactive + // All the nodes that were marked with wantToRetire earlier are now dirty assertEquals(nodesToRetire.stream().map(Node::hostname).collect(Collectors.toSet()), - tester.nodeRepository().getNodes(Node.State.inactive).stream().map(Node::hostname).collect(Collectors.toSet())); + tester.nodeRepository().getNodes(Node.State.dirty).stream().map(Node::hostname).collect(Collectors.toSet())); } private List<HostSpec> deployProxies(ApplicationId application, ProvisioningTester tester) { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java index 611c3839f56..7778077110a 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java @@ -32,6 +32,7 @@ import com.yahoo.vespa.service.duper.InfraApplication; import org.junit.Test; import java.time.Duration; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -100,14 +101,15 @@ public class ProvisioningTest { SystemState state6 = prepare(application1, 0, 2, 0, 3, defaultResources, tester); tester.activate(application1, state6.allHosts); assertEquals(5, tester.getNodes(application1, Node.State.active).size()); - assertEquals(5, tester.getNodes(application1, Node.State.inactive).size()); + assertEquals(3, tester.getNodes(application1, Node.State.inactive).size()); // delete app NodeList previouslyActive = tester.getNodes(application1, Node.State.active); NodeList previouslyInactive = tester.getNodes(application1, Node.State.inactive); tester.remove(application1); - assertEquals(tester.toHostNames(previouslyActive.asList()), tester.toHostNames(tester.nodeRepository().getNodes(application1, Node.State.inactive))); - assertEquals(tester.toHostNames(previouslyInactive.asList()), tester.toHostNames(tester.nodeRepository().getNodes(Node.State.dirty))); + assertEquals(tester.toHostNames(previouslyActive.not().container().asList()), + tester.toHostNames(tester.nodeRepository().getNodes(application1, Node.State.inactive))); + assertTrue(tester.nodeRepository().getNodes(Node.State.dirty).containsAll(previouslyActive.container().asList())); assertEquals(0, tester.getNodes(application1, Node.State.active).size()); assertTrue(tester.nodeRepository().applications().get(application1).isEmpty()); @@ -203,7 +205,7 @@ public class ProvisioningTest { ApplicationId application1 = ProvisioningTester.applicationId(); - tester.makeReadyHosts(24, defaultResources); + tester.makeReadyHosts(30, defaultResources); tester.activateTenantHosts(); // deploy @@ -219,8 +221,8 @@ public class ProvisioningTest { // decrease again SystemState state3 = prepare(application1, 2, 2, 3, 3, defaultResources, tester); tester.activate(application1, state3.allHosts); - assertEquals("Superfluous container nodes are deactivated", - 3-2 + 4-2, tester.getNodes(application1, Node.State.inactive).size()); + assertEquals("Superfluous container nodes are dirtyed", + 3-2 + 4-2, tester.nodeRepository().getNodes(Node.State.dirty).size()); assertEquals("Superfluous content nodes are retired", 4-3 + 5-3, tester.getNodes(application1, Node.State.active).retired().size()); @@ -229,7 +231,6 @@ public class ProvisioningTest { assertEquals("Inactive nodes are reused", 0, tester.getNodes(application1, Node.State.inactive).size()); assertEquals("Earlier retired nodes are not unretired before activate", 4-3 + 5-3, tester.getNodes(application1, Node.State.active).retired().size()); - state4.assertExtends(state2); assertEquals("New and inactive nodes are reserved", 4 + 3, tester.getNodes(application1, Node.State.reserved).size()); // Remove a retired host from one of the content clusters (which one is random depending on host names) HostSpec removed = state4.removeHost(tester.getNodes(application1, Node.State.active).retired().asList().get(0).hostname()); @@ -243,8 +244,8 @@ public class ProvisioningTest { // decrease again SystemState state5 = prepare(application1, 2, 2, 3, 3, defaultResources, tester); tester.activate(application1, state5.allHosts); - assertEquals("Superfluous container nodes are also deactivated", - 4-2 + 5-2 + 1, tester.getNodes(application1, Node.State.inactive).size()); // + assertEquals("Superfluous container nodes are also dirtyed", + 4-2 + 5-2 + 1 + 4-2, tester.nodeRepository().getNodes(Node.State.dirty).size()); assertEquals("Superfluous content nodes are retired", 5-3 + 6-3 - 1, tester.getNodes(application1, Node.State.active).retired().size()); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java index ebd856e96a0..7c43a8d5859 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java @@ -456,7 +456,7 @@ public class ProvisioningTester { } nodes = nodeRepository.addNodes(nodes, Agent.system); - nodes = nodeRepository.setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = nodeRepository.deallocate(nodes, Agent.system, getClass().getSimpleName()); nodeRepository.setReady(nodes, Agent.system, getClass().getSimpleName()); ConfigServerApplication application = new ConfigServerApplication(); @@ -482,7 +482,7 @@ public class ProvisioningTester { } public List<Node> makeReadyNodes(int n, Flavor flavor, Optional<TenantName> reservedTo, NodeType type, int ipAddressPoolSize, boolean dualStack) { List<Node> nodes = makeProvisionedNodes(n, flavor, reservedTo, type, ipAddressPoolSize, dualStack); - nodes = nodeRepository.setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = nodeRepository.deallocate(nodes, Agent.system, getClass().getSimpleName()); return nodeRepository.setReady(nodes, Agent.system, getClass().getSimpleName()); } @@ -523,7 +523,7 @@ public class ProvisioningTester { nodes.add(builder.build()); } nodes = nodeRepository.addNodes(nodes, Agent.system); - nodes = nodeRepository.setDirty(nodes, Agent.system, getClass().getSimpleName()); + nodes = nodeRepository.deallocate(nodes, Agent.system, getClass().getSimpleName()); nodeRepository.setReady(nodes, Agent.system, getClass().getSimpleName()); return nodes; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/node55.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/node55.json index 3107311b792..0a2bf95c5bd 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/node55.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/node55.json @@ -1,14 +1,14 @@ { "url": "http://localhost:8080/nodes/v2/node/host55.yahoo.com", "id": "host55.yahoo.com", - "state": "dirty", + "state": "parked", "type": "tenant", "hostname": "host55.yahoo.com", "openStackId": "node55", "flavor": "[vcpu: 2.0, memory: 8.0 Gb, disk 50.0 Gb, bandwidth: 1.0 Gbps, storage type: local]", "resources":{"vcpu":2.0,"memoryGb":8.0,"diskGb":50.0,"bandwidthGbps":1.0,"diskSpeed":"fast","storageType":"local"}, "environment": "DOCKER_CONTAINER", - "rebootGeneration": 1, + "rebootGeneration": 0, "currentRebootGeneration": 0, "failCount": 0, "wantToRetire": true, @@ -20,7 +20,7 @@ "agent": "system" }, { - "event": "deallocated", + "event": "parked", "at": 123, "agent": "system" } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive-include-deprovisioned.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive-include-deprovisioned.json index 2b650bad39b..03df6c8e1dc 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive-include-deprovisioned.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive-include-deprovisioned.json @@ -17,8 +17,8 @@ @include(node2.json), @include(docker-node1.json), @include(node1.json), - @include(node55.json), @include(node5.json), + @include(node55.json), @include(dockerhost6.json) ] } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive.json index 55e216f454a..8835945dc92 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes-recursive.json @@ -17,7 +17,7 @@ @include(node2.json), @include(docker-node1.json), @include(node1.json), - @include(node55.json), - @include(node5.json) + @include(node5.json), + @include(node55.json) ] } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes.json index 54ff2bc232f..db4d0bb1682 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/nodes.json @@ -52,10 +52,10 @@ "url": "http://localhost:8080/nodes/v2/node/host1.yahoo.com" }, { - "url": "http://localhost:8080/nodes/v2/node/host55.yahoo.com" + "url": "http://localhost:8080/nodes/v2/node/host5.yahoo.com" }, { - "url": "http://localhost:8080/nodes/v2/node/host5.yahoo.com" + "url": "http://localhost:8080/nodes/v2/node/host55.yahoo.com" } ] } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/states-recursive.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/states-recursive.json index 27767be6315..68ff9fedc00 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/states-recursive.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/states-recursive.json @@ -45,7 +45,6 @@ "dirty": { "url": "http://localhost:8080/nodes/v2/state/dirty", "nodes": [ - @include(node55.json) ] }, "failed": { @@ -57,6 +56,7 @@ "parked": { "url": "http://localhost:8080/nodes/v2/state/parked", "nodes": [ + @include(node55.json) ] }, "deprovisioned": { diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java index e7f74161a8e..65744815ecf 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java @@ -55,6 +55,7 @@ import com.yahoo.jdisc.handler.ResponseHandler; import com.yahoo.jdisc.handler.UnsafeContentInputStream; import com.yahoo.jdisc.http.HttpRequest; import com.yahoo.jdisc.http.HttpRequest.Method; +import com.yahoo.messagebus.DynamicThrottlePolicy; import com.yahoo.messagebus.Message; import com.yahoo.messagebus.StaticThrottlePolicy; import com.yahoo.messagebus.Trace; @@ -148,10 +149,12 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { private static final String FIELD_SET = "fieldSet"; private static final String SELECTION = "selection"; private static final String CLUSTER = "cluster"; + private static final String DESTINATION_CLUSTER = "destinationCluster"; private static final String CONTINUATION = "continuation"; private static final String WANTED_DOCUMENT_COUNT = "wantedDocumentCount"; private static final String CONCURRENCY = "concurrency"; private static final String BUCKET_SPACE = "bucketSpace"; + private static final String TIME_CHUNK = "timeChunk"; private static final String TIMEOUT = "timeout"; private static final String TRACELEVEL = "tracelevel"; @@ -347,7 +350,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { private ContentChannel getDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) { enqueueAndDispatch(request, handler, () -> { - VisitorParameters parameters = parseParameters(request, path); + VisitorParameters parameters = parseGetParameters(request, path); return () -> { visitAndWrite(request, parameters, handler); return true; // VisitorSession has its own throttle handling. @@ -358,8 +361,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { private ContentChannel postDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) { enqueueAndDispatch(request, handler, () -> { + StorageCluster destination = resolveCluster(Optional.of(requireProperty(request, DESTINATION_CLUSTER)), clusters); VisitorParameters parameters = parseParameters(request, path); - parameters.setRemoteDataHandler(getProperty(request, ROUTE).orElseThrow(() -> new IllegalArgumentException("Missing required property '" + ROUTE + "'"))); + parameters.setRemoteDataHandler("[Content:cluster=" + destination.name() + "]"); // Bypass indexing. + parameters.setFieldSet(AllFields.NAME); return () -> { visitWithRemote(request, parameters, handler); return true; // VisitorSession has its own throttle handling. @@ -369,19 +374,17 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } private ContentChannel putDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) { - if (getProperty(request, SELECTION).isEmpty()) - throw new IllegalArgumentException("Missing required property '" + SELECTION + "'"); - return new ForwardingContentChannel(in -> { enqueueAndDispatch(request, handler, () -> { - String type = path.documentType().orElseThrow(() -> new IllegalStateException("Document type must be specified for mass updates")); - IdIdString dummyId = new IdIdString("dummy", type, "", ""); + StorageCluster cluster = resolveCluster(Optional.of(requireProperty(request, CLUSTER)), clusters); VisitorParameters parameters = parseParameters(request, path); parameters.setFieldSet(DocIdOnly.NAME); + String type = path.documentType().orElseThrow(() -> new IllegalStateException("Document type must be specified for mass updates")); + IdIdString dummyId = new IdIdString("dummy", type, "", ""); DocumentUpdate update = parser.parseUpdate(in, dummyId.toString()); - update.setCondition(new TestAndSetCondition(parameters.getDocumentSelection())); + update.setCondition(new TestAndSetCondition(requireProperty(request, SELECTION))); return () -> { - visitAndUpdate(request, parameters, handler, update, getProperty(request, ROUTE)); + visitAndUpdate(request, parameters, handler, update, cluster.name()); return true; // VisitorSession has its own throttle handling. }; }); @@ -389,15 +392,13 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } private ContentChannel deleteDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) { - if (getProperty(request, SELECTION).isEmpty()) - throw new IllegalArgumentException("Missing required property '" + SELECTION + "'"); - enqueueAndDispatch(request, handler, () -> { VisitorParameters parameters = parseParameters(request, path); parameters.setFieldSet(DocIdOnly.NAME); - TestAndSetCondition condition = new TestAndSetCondition(parameters.getDocumentSelection()); + TestAndSetCondition condition = new TestAndSetCondition(requireProperty(request, SELECTION)); + StorageCluster cluster = resolveCluster(Optional.of(requireProperty(request, CLUSTER)), clusters); return () -> { - visitAndDelete(request, parameters, handler, condition, getProperty(request, ROUTE)); + visitAndDelete(request, parameters, handler, condition, cluster.name()); return true; // VisitorSession has its own throttle handling. }; }); @@ -478,7 +479,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { .orElse(parameters()); for (String name : names) switch (name) { case CLUSTER: - parameters = getProperty(request, CLUSTER).map(cluster -> resolveCluster(Optional.of(cluster), clusters).route()) + parameters = getProperty(request, CLUSTER).map(cluster -> resolveCluster(Optional.of(cluster), clusters).name()) .map(parameters::withRoute) .orElse(parameters); break; @@ -644,6 +645,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { json.writeStringField("message", message); } + synchronized void writeDocumentCount(long count) throws IOException { + json.writeNumberField("documentCount", count); + } + synchronized void writeDocId(DocumentId id) throws IOException { json.writeStringField("id", id.toString()); } @@ -916,10 +921,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { // ------------------------------------------------- Visits ------------------------------------------------ - private VisitorParameters parseParameters(HttpRequest request, DocumentPath path) { + private VisitorParameters parseGetParameters(HttpRequest request, DocumentPath path) { int wantedDocumentCount = Math.min(1 << 10, getProperty(request, WANTED_DOCUMENT_COUNT, integerParser).orElse(1)); if (wantedDocumentCount <= 0) - throw new IllegalArgumentException("wantedDocumentCount must be positive"); + throw new IllegalArgumentException("wantedDocumentCount must be positive"); int concurrency = Math.min(100, getProperty(request, CONCURRENCY, integerParser).orElse(1)); if (concurrency <= 0) @@ -929,6 +934,25 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { if (cluster.isEmpty() && path.documentType().isEmpty()) throw new IllegalArgumentException("Must set 'cluster' parameter to a valid content cluster id when visiting at a root /document/v1/ level"); + VisitorParameters parameters = parseCommonParameters(request, path, cluster); + parameters.setFieldSet(getProperty(request, FIELD_SET).orElse(path.documentType().map(type -> type + ":[document]").orElse(AllFields.NAME))); + parameters.setMaxTotalHits(wantedDocumentCount); + parameters.setThrottlePolicy(new StaticThrottlePolicy().setMaxPendingCount(concurrency)); + parameters.visitInconsistentBuckets(true); + parameters.setSessionTimeoutMs(Math.max(1, request.getTimeout(TimeUnit.MILLISECONDS) - 5000)); + return parameters; + } + + private VisitorParameters parseParameters(HttpRequest request, DocumentPath path) { + disallow(request, CONCURRENCY, FIELD_SET, ROUTE, WANTED_DOCUMENT_COUNT); + requireProperty(request, SELECTION); + VisitorParameters parameters = parseCommonParameters(request, path, Optional.of(requireProperty(request, CLUSTER))); + parameters.setThrottlePolicy(new DynamicThrottlePolicy().setMinWindowSize(1).setWindowSizeIncrement(1)); + parameters.setSessionTimeoutMs(Math.max(1, getProperty(request, TIME_CHUNK, timeoutMillisParser).orElse(60_000L))); + return parameters; + } + + private VisitorParameters parseCommonParameters(HttpRequest request, DocumentPath path, Optional<String> cluster) { VisitorParameters parameters = new VisitorParameters(Stream.of(getProperty(request, SELECTION), path.documentType(), path.namespace().map(value -> "id.namespace=='" + value + "'"), @@ -940,15 +964,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { .toString()); getProperty(request, CONTINUATION).map(ProgressToken::fromSerializedString).ifPresent(parameters::setResumeToken); - parameters.setFieldSet(getProperty(request, FIELD_SET).orElse(path.documentType().map(type -> type + ":[document]").orElse(AllFields.NAME))); - parameters.setMaxTotalHits(wantedDocumentCount); - parameters.setThrottlePolicy(new StaticThrottlePolicy().setMaxPendingCount(concurrency)); - parameters.setSessionTimeoutMs(Math.max(1, request.getTimeout(TimeUnit.MILLISECONDS) - 5000)); - parameters.visitInconsistentBuckets(true); parameters.setPriority(DocumentProtocol.Priority.NORMAL_4); StorageCluster storageCluster = resolveCluster(cluster, clusters); - parameters.setRoute(storageCluster.route()); + parameters.setRoute(storageCluster.name()); parameters.setBucketSpace(resolveBucket(storageCluster, path.documentType(), List.of(FixedBucketSpaces.defaultSpace(), FixedBucketSpaces.globalSpace()), @@ -969,7 +988,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } private void visitAndDelete(HttpRequest request, VisitorParameters parameters, ResponseHandler handler, - TestAndSetCondition condition, Optional<String> route) { + TestAndSetCondition condition, String route) { visitAndProcess(request, parameters, handler, route, (id, operationParameters) -> { DocumentRemove remove = new DocumentRemove(id); remove.setCondition(condition); @@ -978,7 +997,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } private void visitAndUpdate(HttpRequest request, VisitorParameters parameters, ResponseHandler handler, - DocumentUpdate protoUpdate, Optional<String> route) { + DocumentUpdate protoUpdate, String route) { visitAndProcess(request, parameters, handler, route, (id, operationParameters) -> { DocumentUpdate update = new DocumentUpdate(protoUpdate); update.setId(id); @@ -987,11 +1006,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } private void visitAndProcess(HttpRequest request, VisitorParameters parameters, ResponseHandler handler, - Optional<String> route, BiFunction<DocumentId, DocumentOperationParameters, Result> operation) { + String route, BiFunction<DocumentId, DocumentOperationParameters, Result> operation) { visit(request, parameters, handler, new VisitCallback() { @Override public void onDocument(JsonResponse response, Document document, Runnable ack, Consumer<String> onError) { - DocumentOperationParameters operationParameters = (route.isEmpty() ? parameters() - : parameters().withRoute(route.get())) + DocumentOperationParameters operationParameters = parameters().withRoute(route) .withResponseHandler(operationResponse -> { outstanding.decrementAndGet(); switch (operationResponse.outcome()) { @@ -1057,7 +1075,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { callback.onEnd(response); switch (code) { case TIMEOUT: - if ( ! hasVisitedAnyBuckets()) { + if ( ! hasVisitedAnyBuckets() && parameters.getVisitInconsistentBuckets()) { response.writeMessage("No buckets visited within timeout of " + parameters.getSessionTimeoutMs() + "ms (request timeout -5s)"); response.respond(Response.Status.GATEWAY_TIMEOUT); @@ -1066,14 +1084,21 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { case SUCCESS: // Intentional fallthrough. case ABORTED: // Intentional fallthrough. if (error.get() == null) { - if (getProgress() != null && ! getProgress().isFinished()) - response.writeContinuation(getProgress().serializeToString()); + ProgressToken progress = getProgress() != null ? getProgress() : parameters.getResumeToken(); + if (progress != null && ! progress.isFinished()) + response.writeContinuation(progress.serializeToString()); + + if (getVisitorStatistics() != null) + response.writeDocumentCount(getVisitorStatistics().getDocumentsVisited()); response.respond(Response.Status.OK); break; } default: response.writeMessage(error.get() != null ? error.get() : message != null ? message : "Visiting failed"); + if (getVisitorStatistics() != null) + response.writeDocumentCount(getVisitorStatistics().getDocumentsReturned()); + response.respond(Response.Status.INTERNAL_SERVER_ERROR); } visitDispatcher.execute(() -> { @@ -1113,6 +1138,11 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { // ------------------------------------------------ Helpers ------------------------------------------------ + private static String requireProperty(HttpRequest request, String name) { + return getProperty(request, name) + .orElseThrow(() -> new IllegalArgumentException("Must specify '" + name + "' at '" + request.getUri().getRawPath() + "'")); + } + /** Returns the last property with the given name, if present, or throws if this is empty or blank. */ private static Optional<String> getProperty(HttpRequest request, String name) { if ( ! request.parameters().containsKey(name)) @@ -1130,6 +1160,12 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { return getProperty(request, name).map(parser::parse); } + private static void disallow(HttpRequest request, String... properties) { + for (String property : properties) + if (request.parameters().containsKey(property)) + throw new IllegalArgumentException("May not specify '" + property + "' at '" + request.getUri().getRawPath() + "'"); + } + @FunctionalInterface interface Parser<T> extends Function<String, T> { default T parse(String value) { @@ -1177,7 +1213,6 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler { } String name() { return name; } - String route() { return "[Content:cluster=" + name() + "]"; } Optional<String> bucketOf(String documentType) { return Optional.ofNullable(documentBuckets.get(documentType)); } } diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java index 1e3f3e13c2a..5d81e00cbcb 100644 --- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java +++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java @@ -142,8 +142,8 @@ public class DocumentV1ApiTest { public void testResolveCluster() { assertEquals("content", DocumentV1ApiHandler.resolveCluster(Optional.empty(), clusters).name()); - assertEquals("[Content:cluster=content]", - DocumentV1ApiHandler.resolveCluster(Optional.of("content"), clusters).route()); + assertEquals("content", + DocumentV1ApiHandler.resolveCluster(Optional.of("content"), clusters).name()); try { DocumentV1ApiHandler.resolveCluster(Optional.empty(), Map.of()); fail("Should fail without any clusters"); @@ -198,7 +198,7 @@ public class DocumentV1ApiTest { // GET at root is a visit. Numeric parameters have an upper bound. access.expect(tokens); access.expect(parameters -> { - assertEquals("[Content:cluster=content]", parameters.getRoute().toString()); + assertEquals("content", parameters.getRoute().toString()); assertEquals("default", parameters.getBucketSpace()); assertEquals(1024, parameters.getMaxTotalHits()); assertEquals(100, ((StaticThrottlePolicy) parameters.getThrottlePolicy()).getMaxPendingCount()); @@ -211,6 +211,7 @@ public class DocumentV1ApiTest { parameters.getLocalDataHandler().onMessage(new PutDocumentMessage(new DocumentPut(doc3)), tokens.get(2)); VisitorStatistics statistics = new VisitorStatistics(); statistics.setBucketsVisited(1); + statistics.setDocumentsVisited(3); parameters.getControlHandler().onVisitorStatistics(statistics); parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.TIMEOUT, "timeout is OK"); }); @@ -235,7 +236,8 @@ public class DocumentV1ApiTest { " \"id\": \"id:space:music:g=a:three\"," + " \"fields\": {}" + " }" + - " ]" + + " ]," + + " \"documentCount\": 3" + "}", response.readAll()); assertEquals(200, response.getStatus()); @@ -252,24 +254,24 @@ public class DocumentV1ApiTest { "}", response.readAll()); assertEquals(400, response.getStatus()); - // POST with namespace and document type is a restricted visit with a required remote data handler ("route") + // POST with namespace and document type is a restricted visit with a required destination cluster ("destinationCluster") access.expect(parameters -> { fail("Not supposed to run"); }); response = driver.sendRequest("http://localhost/document/v1/space/music/docid", POST); assertSameJson("{" + " \"pathId\": \"/document/v1/space/music/docid\"," + - " \"message\": \"Missing required property 'route'\"" + + " \"message\": \"Must specify 'destinationCluster' at '/document/v1/space/music/docid'\"" + "}", response.readAll()); assertEquals(400, response.getStatus()); - // POST with namespace and document type is a restricted visit with a require remote data handler ("route") + // POST with namespace and document type is a restricted visit with a required destination cluster ("destinationCluster") access.expect(parameters -> { - assertEquals("zero", parameters.getRemoteDataHandler()); - assertEquals("music:[document]", parameters.fieldSet()); + assertEquals("[Content:cluster=content]", parameters.getRemoteDataHandler()); + assertEquals("[all]", parameters.fieldSet()); parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.SUCCESS, "We made it!"); }); - response = driver.sendRequest("http://localhost/document/v1/space/music/docid?route=zero", POST); + response = driver.sendRequest("http://localhost/document/v1/space/music/docid?destinationCluster=content&selection=true&cluster=content", POST); assertSameJson("{" + " \"pathId\": \"/document/v1/space/music/docid\"" + "}", response.readAll()); @@ -280,19 +282,20 @@ public class DocumentV1ApiTest { access.expect(parameters -> { assertEquals("(true) and (music) and (id.namespace=='space')", parameters.getDocumentSelection()); assertEquals("[id]", parameters.fieldSet()); + assertEquals(10_000, parameters.getSessionTimeoutMs()); parameters.getLocalDataHandler().onMessage(new PutDocumentMessage(new DocumentPut(doc3)), tokens.get(2)); - parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.SUCCESS, "Huzzah!"); + parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.TIMEOUT, "Won't care"); }); access.session.expect((update, parameters) -> { DocumentUpdate expectedUpdate = new DocumentUpdate(doc3.getDataType(), doc3.getId()); expectedUpdate.addFieldUpdate(FieldUpdate.createAssign(doc3.getField("artist"), new StringFieldValue("Lisa Ekdahl"))); - expectedUpdate.setCondition(new TestAndSetCondition("(true) and (music) and (id.namespace=='space')")); + expectedUpdate.setCondition(new TestAndSetCondition("true")); assertEquals(expectedUpdate, update); parameters.responseHandler().get().handleResponse(new UpdateResponse(0, false)); - assertEquals(parameters().withRoute("zero"), parameters); + assertEquals(parameters().withRoute("content"), parameters); return new Result(Result.ResultType.SUCCESS, null); }); - response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=true&route=zero", PUT, + response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=true&cluster=content&timeChunk=10", PUT, "{" + " \"fields\": {" + " \"artist\": { \"assign\": \"Lisa Ekdahl\" }" + @@ -303,14 +306,25 @@ public class DocumentV1ApiTest { "}", response.readAll()); assertEquals(200, response.getStatus()); + // PUT with namespace, document type and group is also a restricted visit which requires a cluster. + access.expect(parameters -> { + fail("Not supposed to run"); + }); + response = driver.sendRequest("http://localhost/document/v1/space/music/group/troupe?selection=false", PUT); + assertSameJson("{" + + " \"pathId\": \"/document/v1/space/music/group/troupe\"," + + " \"message\": \"Must specify 'cluster' at '/document/v1/space/music/group/troupe'\"" + + "}", response.readAll()); + assertEquals(400, response.getStatus()); + // PUT with namespace, document type and group is also a restricted visit which requires a selection. access.expect(parameters -> { fail("Not supposed to run"); }); - response = driver.sendRequest("http://localhost/document/v1/space/music/group/troupe", PUT); + response = driver.sendRequest("http://localhost/document/v1/space/music/group/troupe?cluster=content", PUT); assertSameJson("{" + " \"pathId\": \"/document/v1/space/music/group/troupe\"," + - " \"message\": \"Missing required property 'selection'\"" + + " \"message\": \"Must specify 'selection' at '/document/v1/space/music/group/troupe'\"" + "}", response.readAll()); assertEquals(400, response.getStatus()); @@ -319,32 +333,44 @@ public class DocumentV1ApiTest { access.expect(parameters -> { assertEquals("(false) and (music) and (id.namespace=='space')", parameters.getDocumentSelection()); assertEquals("[id]", parameters.fieldSet()); + assertEquals(60_000, parameters.getSessionTimeoutMs()); parameters.getLocalDataHandler().onMessage(new PutDocumentMessage(new DocumentPut(doc2)), tokens.get(0)); parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.ABORTED, "Huzzah?"); }); access.session.expect((remove, parameters) -> { DocumentRemove expectedRemove = new DocumentRemove(doc2.getId()); - expectedRemove.setCondition(new TestAndSetCondition("(false) and (music) and (id.namespace=='space')")); - assertEquals(new DocumentRemove(doc2.getId()), remove); - assertEquals(parameters().withRoute("zero"), parameters); + expectedRemove.setCondition(new TestAndSetCondition("false")); + assertEquals(expectedRemove, remove); + assertEquals(parameters().withRoute("content"), parameters); parameters.responseHandler().get().handleResponse(new DocumentIdResponse(0, doc2.getId(), "boom", Response.Outcome.ERROR)); return new Result(Result.ResultType.SUCCESS, null); }); - response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=false&route=zero", DELETE); + response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=false&cluster=content", DELETE); assertSameJson("{" + " \"pathId\": \"/document/v1/space/music/docid\"," + " \"message\": \"boom\"" + "}", response.readAll()); assertEquals(500, response.getStatus()); - // DELETE at the root is also a deletion visit. These require a selection. + // DELETE at the root is also a deletion visit. These also require a selection. access.expect(parameters -> { fail("Not supposed to run"); }); - response = driver.sendRequest("http://localhost/document/v1/space/music/docid", DELETE); + response = driver.sendRequest("http://localhost/document/v1/", DELETE); assertSameJson("{" + - " \"pathId\": \"/document/v1/space/music/docid\"," + - " \"message\": \"Missing required property 'selection'\"" + + " \"pathId\": \"/document/v1/\"," + + " \"message\": \"Must specify 'selection' at '/document/v1/'\"" + + "}", response.readAll()); + assertEquals(400, response.getStatus()); + + // DELETE at the root is also a deletion visit. These also require a cluster. + access.expect(parameters -> { + fail("Not supposed to run"); + }); + response = driver.sendRequest("http://localhost/document/v1/?selection=true", DELETE); + assertSameJson("{" + + " \"pathId\": \"/document/v1/\"," + + " \"message\": \"Must specify 'cluster' at '/document/v1/'\"" + "}", response.readAll()); assertEquals(400, response.getStatus()); @@ -376,7 +402,7 @@ public class DocumentV1ApiTest { // GET with full document ID is a document get operation which returns 404 when no document is found access.session.expect((id, parameters) -> { assertEquals(doc1.getId(), id); - assertEquals(parameters().withRoute("[Content:cluster=content]").withFieldSet("go"), parameters); + assertEquals(parameters().withRoute("content").withFieldSet("go"), parameters); parameters.responseHandler().get().handleResponse(new DocumentResponse(0, null)); return new Result(Result.ResultType.SUCCESS, null); }); @@ -520,7 +546,7 @@ public class DocumentV1ApiTest { access.session.expect((remove, parameters) -> { DocumentRemove expectedRemove = new DocumentRemove(doc2.getId()); expectedRemove.setCondition(new TestAndSetCondition("false")); - assertEquals(new DocumentRemove(doc2.getId()), remove); + assertEquals(expectedRemove, remove); assertEquals(parameters().withRoute("route"), parameters); parameters.responseHandler().get().handleResponse(new DocumentIdResponse(0, doc2.getId())); return new Result(Result.ResultType.SUCCESS, null); |