From d218b04fba8422049ec2a6f73bd646c55cd50c88 Mon Sep 17 00:00:00 2001 From: shiyer7474 Date: Wed, 28 Aug 2024 11:52:51 +0000 Subject: [PATCH 01/33] Fix test_role & test_keeper_s3_snapshot --- tests/integration/test_keeper_s3_snapshot/test.py | 2 +- tests/integration/test_role/test.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_keeper_s3_snapshot/test.py b/tests/integration/test_keeper_s3_snapshot/test.py index 84ffc964621..1e766cb974b 100644 --- a/tests/integration/test_keeper_s3_snapshot/test.py +++ b/tests/integration/test_keeper_s3_snapshot/test.py @@ -92,7 +92,7 @@ def test_s3_upload(started_cluster): # Keeper sends snapshots asynchornously, hence we need to retry. @retry(AssertionError, tries=10, delay=2) def _check_snapshots(): - assert set(get_saved_snapshots()) == set( + assert set(get_saved_snapshots()) >= set( [ "snapshot_50.bin.zstd", "snapshot_100.bin.zstd", diff --git a/tests/integration/test_role/test.py b/tests/integration/test_role/test.py index 225cab975ff..b746af56083 100644 --- a/tests/integration/test_role/test.py +++ b/tests/integration/test_role/test.py @@ -629,5 +629,6 @@ def test_roles_cache(): check() instance.query("DROP USER " + ", ".join(users)) - instance.query("DROP ROLE " + ", ".join(roles)) + if roles: + instance.query("DROP ROLE " + ", ".join(roles)) instance.query("DROP TABLE tbl") From d88aa3952d34eeecbca363a1a66df111c5b3e587 Mon Sep 17 00:00:00 2001 From: flynn Date: Thu, 29 Aug 2024 10:56:26 +0000 Subject: [PATCH 02/33] Disable alter table add vector similarity index if setting does not enabled --- src/Storages/AlterCommands.cpp | 10 ++++++++++ src/Storages/AlterCommands.h | 2 ++ src/Storages/MergeTree/MergeTreeData.cpp | 5 +++++ ...rbid_add_vector_similarity_index.reference | 0 ...231_forbid_add_vector_similarity_index.sql | 19 +++++++++++++++++++ 5 files changed, 36 insertions(+) create mode 100644 tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.reference create mode 100644 tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql diff --git a/src/Storages/AlterCommands.cpp b/src/Storages/AlterCommands.cpp index d92d8b59f6e..e0563f9f1c6 100644 --- a/src/Storages/AlterCommands.cpp +++ b/src/Storages/AlterCommands.cpp @@ -1142,6 +1142,16 @@ bool AlterCommands::hasFullTextIndex(const StorageInMemoryMetadata & metadata) return false; } +bool AlterCommands::hasVectorSimilarityIndex(const StorageInMemoryMetadata & metadata) +{ + for (const auto & index : metadata.secondary_indices) + { + if (index.type == "vector_similarity") + return true; + } + return false; +} + void AlterCommands::apply(StorageInMemoryMetadata & metadata, ContextPtr context) const { if (!prepared) diff --git a/src/Storages/AlterCommands.h b/src/Storages/AlterCommands.h index a91bac10214..b2f0f9a6abd 100644 --- a/src/Storages/AlterCommands.h +++ b/src/Storages/AlterCommands.h @@ -237,6 +237,8 @@ public: /// Check if commands have any full-text index static bool hasFullTextIndex(const StorageInMemoryMetadata & metadata); + + static bool hasVectorSimilarityIndex(const StorageInMemoryMetadata & metadata); }; } diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 94f6d196b99..8b12330c1a4 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -3230,6 +3230,11 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "Experimental full-text index feature is not enabled (turn on setting 'allow_experimental_full_text_index')"); + if (AlterCommands::hasVectorSimilarityIndex(new_metadata) && !settings.allow_experimental_vector_similarity_index) + throw Exception( + ErrorCodes::SUPPORT_IS_DISABLED, + "Experimental vector_similarity index feature is not enabled (turn on setting 'allow_experimental_vector_similarity_index')"); + for (const auto & disk : getDisks()) if (!disk->supportsHardLinks() && !commands.isSettingsAlter() && !commands.isCommentAlter()) throw Exception( diff --git a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.reference b/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql b/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql new file mode 100644 index 00000000000..a1e362e7bd1 --- /dev/null +++ b/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql @@ -0,0 +1,19 @@ +DROP TABLE IF EXISTS test_embedding; + +CREATE TABLE test_embedding +( + id UInt32, + embedding Array(Float32), +) +ENGINE = MergeTree +ORDER BY tuple(); + +SET allow_experimental_vector_similarity_index = 0; + +alter table test_embedding add INDEX idx embedding TYPE vector_similarity('hnsw', 'cosineDistance'); -- { serverError SUPPORT_IS_DISABLED } + +SET allow_experimental_vector_similarity_index = 1; + +alter table test_embedding add INDEX idx embedding TYPE vector_similarity('hnsw', 'cosineDistance'); + +DROP TABLE test_embedding; From 0400dcb03eefdb6604d04f5ed3c70c179032f84d Mon Sep 17 00:00:00 2001 From: flynn Date: Thu, 29 Aug 2024 12:34:58 +0000 Subject: [PATCH 03/33] no-fastest --- .../0_stateless/03231_forbid_add_vector_similarity_index.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql b/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql index a1e362e7bd1..e91d7c71eac 100644 --- a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql +++ b/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql @@ -1,3 +1,5 @@ +-- Tags: no-fasttest + DROP TABLE IF EXISTS test_embedding; CREATE TABLE test_embedding From 6ad8e5558a99b43e3452c057346b9c44e8e27517 Mon Sep 17 00:00:00 2001 From: flynn Date: Fri, 30 Aug 2024 07:25:25 +0000 Subject: [PATCH 04/33] Fix typo --- src/Storages/MergeTree/MergeTreeIndexVectorSimilarity.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/MergeTreeIndexVectorSimilarity.cpp b/src/Storages/MergeTree/MergeTreeIndexVectorSimilarity.cpp index ae183d74782..58892d0dbf2 100644 --- a/src/Storages/MergeTree/MergeTreeIndexVectorSimilarity.cpp +++ b/src/Storages/MergeTree/MergeTreeIndexVectorSimilarity.cpp @@ -195,7 +195,7 @@ void MergeTreeIndexGranuleVectorSimilarity::serializeBinary(WriteBuffer & ostr) LOG_TRACE(logger, "Start writing vector similarity index"); if (empty()) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Attempt to write empty minmax index {}", backQuote(index_name)); + throw Exception(ErrorCodes::LOGICAL_ERROR, "Attempt to write empty vector similarity index {}", backQuote(index_name)); writeIntBinary(FILE_FORMAT_VERSION, ostr); From c5b92413cac091c4e28acb5160b89fcabc37a853 Mon Sep 17 00:00:00 2001 From: flynn Date: Fri, 30 Aug 2024 08:28:31 +0000 Subject: [PATCH 05/33] Fix vector similarity index does not work for cosineDistance --- src/Storages/MergeTree/VectorSimilarityCondition.cpp | 2 ++ src/Storages/MergeTree/VectorSimilarityCondition.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Storages/MergeTree/VectorSimilarityCondition.cpp b/src/Storages/MergeTree/VectorSimilarityCondition.cpp index c8f33857640..251cdde65ab 100644 --- a/src/Storages/MergeTree/VectorSimilarityCondition.cpp +++ b/src/Storages/MergeTree/VectorSimilarityCondition.cpp @@ -44,6 +44,8 @@ VectorSimilarityCondition::Info::DistanceFunction stringToDistanceFunction(std:: { if (distance_function == "L2Distance") return VectorSimilarityCondition::Info::DistanceFunction::L2; + else if (distance_function == "cosineDistance") + return VectorSimilarityCondition::Info::DistanceFunction::Cosine; else return VectorSimilarityCondition::Info::DistanceFunction::Unknown; } diff --git a/src/Storages/MergeTree/VectorSimilarityCondition.h b/src/Storages/MergeTree/VectorSimilarityCondition.h index 2380f8f46b0..e2946222f49 100644 --- a/src/Storages/MergeTree/VectorSimilarityCondition.h +++ b/src/Storages/MergeTree/VectorSimilarityCondition.h @@ -57,7 +57,8 @@ public: enum class DistanceFunction : uint8_t { Unknown, - L2 + L2, + Cosine }; std::vector reference_vector; From af7adfe4b2f8e9b3b7354314a91d19a19b7955f9 Mon Sep 17 00:00:00 2001 From: flynn Date: Fri, 30 Aug 2024 09:03:23 +0000 Subject: [PATCH 06/33] Remove unused code --- src/Storages/MergeTree/VectorSimilarityCondition.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Storages/MergeTree/VectorSimilarityCondition.h b/src/Storages/MergeTree/VectorSimilarityCondition.h index e2946222f49..83ae1e19bfb 100644 --- a/src/Storages/MergeTree/VectorSimilarityCondition.h +++ b/src/Storages/MergeTree/VectorSimilarityCondition.h @@ -143,6 +143,7 @@ private: void traverseOrderByAST(const ASTPtr & node, RPN & rpn); /// Returns true and stores ANNExpr if the query has valid WHERE section + /// TODO NOT implemented, WHERE does not supported. static bool matchRPNWhere(RPN & rpn, Info & info); /// Returns true and stores ANNExpr if the query has valid ORDERBY section @@ -151,9 +152,6 @@ private: /// Returns true and stores Length if we have valid LIMIT clause in query static bool matchRPNLimit(RPNElement & rpn, UInt64 & limit); - /// Matches dist function, reference vector, column name - static bool matchMainParts(RPN::iterator & iter, const RPN::iterator & end, Info & info); - /// Gets float or int from AST node static float getFloatOrIntLiteralOrPanic(const RPN::iterator& iter); From 81d0a04ecbcb9ad95a875c94467ab588bc5eeb7c Mon Sep 17 00:00:00 2001 From: shiyer7474 Date: Sun, 1 Sep 2024 13:53:56 +0000 Subject: [PATCH 07/33] Added restart of node1 for running multiple iterations of test --- tests/integration/test_keeper_s3_snapshot/test.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/integration/test_keeper_s3_snapshot/test.py b/tests/integration/test_keeper_s3_snapshot/test.py index 1e766cb974b..5e2545ae0a7 100644 --- a/tests/integration/test_keeper_s3_snapshot/test.py +++ b/tests/integration/test_keeper_s3_snapshot/test.py @@ -2,6 +2,7 @@ import pytest from helpers.cluster import ClickHouseCluster from time import sleep from retry import retry +import helpers.keeper_utils as keeper_utils from kazoo.client import KazooClient @@ -125,3 +126,5 @@ def test_s3_upload(started_cluster): ) destroy_zk_client(node2_zk) + node1.start_clickhouse() # for next iteration + keeper_utils.wait_until_connected(cluster, node1) From 4f16797cd127a4dff7169908890b0010d82027ae Mon Sep 17 00:00:00 2001 From: shiyer7474 Date: Mon, 2 Sep 2024 00:35:59 +0000 Subject: [PATCH 08/33] Formatting --- tests/integration/test_keeper_s3_snapshot/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_keeper_s3_snapshot/test.py b/tests/integration/test_keeper_s3_snapshot/test.py index 5e2545ae0a7..f47a26a77ad 100644 --- a/tests/integration/test_keeper_s3_snapshot/test.py +++ b/tests/integration/test_keeper_s3_snapshot/test.py @@ -126,5 +126,5 @@ def test_s3_upload(started_cluster): ) destroy_zk_client(node2_zk) - node1.start_clickhouse() # for next iteration + node1.start_clickhouse() # for next iteration keeper_utils.wait_until_connected(cluster, node1) From 45556278c917a0053e902ae9ebcee60f98062fe5 Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 3 Sep 2024 11:06:28 +0000 Subject: [PATCH 09/33] Fix possible timeouts in `sh` tests with tsan, att. 2 --- src/Client/ClientBase.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/Client/ClientBase.cpp b/src/Client/ClientBase.cpp index e34e263beb5..edcb98be90f 100644 --- a/src/Client/ClientBase.cpp +++ b/src/Client/ClientBase.cpp @@ -1896,6 +1896,25 @@ void ClientBase::processParsedSingleQuery(const String & full_query, const Strin /// Temporarily apply query settings to context. std::optional old_settings; SCOPE_EXIT_SAFE({ + try + { + /// We need to park ParallelFormating threads, + /// because they can use settings from global context + /// and it can lead to data race with `setSettings` + if (output_format) + { + output_format->finalize(); + output_format.reset(); + } + } + catch (...) + { + if (!have_error) + { + client_exception = std::make_unique(getCurrentExceptionMessageAndPattern(print_stack_trace), getCurrentExceptionCode()); + have_error = true; + } + } if (old_settings) client_context->setSettings(*old_settings); }); From 0b1a0999e338325c2e5cd9ec25e9a5ecf3472d16 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 4 Sep 2024 08:29:55 +0000 Subject: [PATCH 10/33] Some fixups --- src/Interpreters/InterpreterCreateQuery.cpp | 4 +-- src/Storages/AlterCommands.h | 1 + src/Storages/MergeTree/MergeTreeData.cpp | 5 ++- ...tor_search_experimental_setting.reference} | 0 ...354_vector_search_experimental_setting.sql | 32 +++++++++++++++++++ ...231_forbid_add_vector_similarity_index.sql | 21 ------------ 6 files changed, 37 insertions(+), 26 deletions(-) rename tests/queries/0_stateless/{03231_forbid_add_vector_similarity_index.reference => 02354_vector_search_experimental_setting.reference} (100%) create mode 100644 tests/queries/0_stateless/02354_vector_search_experimental_setting.sql delete mode 100644 tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql diff --git a/src/Interpreters/InterpreterCreateQuery.cpp b/src/Interpreters/InterpreterCreateQuery.cpp index e9f40bdbaf5..c265154c9dd 100644 --- a/src/Interpreters/InterpreterCreateQuery.cpp +++ b/src/Interpreters/InterpreterCreateQuery.cpp @@ -781,14 +781,14 @@ InterpreterCreateQuery::TableProperties InterpreterCreateQuery::getTableProperti const auto & settings = getContext()->getSettingsRef(); if (index_desc.type == FULL_TEXT_INDEX_NAME && !settings.allow_experimental_full_text_index) - throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "Experimental full-text index feature is not enabled (the setting 'allow_experimental_full_text_index')"); + throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "Experimental full-text index feature is disabled. Turn on setting 'allow_experimental_full_text_index'"); /// ---- /// Temporary check during a transition period. Please remove at the end of 2024. if (index_desc.type == INVERTED_INDEX_NAME && !settings.allow_experimental_inverted_index) throw Exception(ErrorCodes::ILLEGAL_INDEX, "Please use index type 'full_text' instead of 'inverted'"); /// ---- if (index_desc.type == "vector_similarity" && !settings.allow_experimental_vector_similarity_index) - throw Exception(ErrorCodes::INCORRECT_QUERY, "Vector similarity index is disabled. Turn on allow_experimental_vector_similarity_index"); + throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "Experimental vector similarity index is disabled. Turn on setting 'allow_experimental_vector_similarity_index'"); properties.indices.push_back(index_desc); } diff --git a/src/Storages/AlterCommands.h b/src/Storages/AlterCommands.h index b2f0f9a6abd..c4c792e7dec 100644 --- a/src/Storages/AlterCommands.h +++ b/src/Storages/AlterCommands.h @@ -238,6 +238,7 @@ public: /// Check if commands have any full-text index static bool hasFullTextIndex(const StorageInMemoryMetadata & metadata); + /// Check if commands have any vector similarity index static bool hasVectorSimilarityIndex(const StorageInMemoryMetadata & metadata); }; diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 8b12330c1a4..ac39831b3df 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -3231,9 +3231,8 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context "Experimental full-text index feature is not enabled (turn on setting 'allow_experimental_full_text_index')"); if (AlterCommands::hasVectorSimilarityIndex(new_metadata) && !settings.allow_experimental_vector_similarity_index) - throw Exception( - ErrorCodes::SUPPORT_IS_DISABLED, - "Experimental vector_similarity index feature is not enabled (turn on setting 'allow_experimental_vector_similarity_index')"); + throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, + "Experimental vector similarity index is disabled (turn on setting 'allow_experimental_vector_similarity_index')"); for (const auto & disk : getDisks()) if (!disk->supportsHardLinks() && !commands.isSettingsAlter() && !commands.isCommentAlter()) diff --git a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.reference b/tests/queries/0_stateless/02354_vector_search_experimental_setting.reference similarity index 100% rename from tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.reference rename to tests/queries/0_stateless/02354_vector_search_experimental_setting.reference diff --git a/tests/queries/0_stateless/02354_vector_search_experimental_setting.sql b/tests/queries/0_stateless/02354_vector_search_experimental_setting.sql new file mode 100644 index 00000000000..cce838f8e02 --- /dev/null +++ b/tests/queries/0_stateless/02354_vector_search_experimental_setting.sql @@ -0,0 +1,32 @@ +-- Tags: no-fasttest, no-ordinary-database + +-- Tests that CREATE TABLE and ADD INDEX respect setting 'allow_experimental_vector_similarity_index'. + +DROP TABLE IF EXISTS tab; + +-- Test CREATE TABLE + +SET allow_experimental_vector_similarity_index = 0; +CREATE TABLE tab (id UInt32, vec Array(Float32), INDEX idx vec TYPE vector_similarity('hnsw', 'L2Distance')) ENGINE = MergeTree ORDER BY tuple(); -- { serverError SUPPORT_IS_DISABLED } + +SET allow_experimental_vector_similarity_index = 1; +CREATE TABLE tab (id UInt32, vec Array(Float32), INDEX idx vec TYPE vector_similarity('hnsw', 'L2Distance')) ENGINE = MergeTree ORDER BY tuple(); +DROP TABLE tab; + +-- Test ADD INDEX + +CREATE TABLE tab (id UInt32, vec Array(Float32)) ENGINE = MergeTree ORDER BY tuple(); + +SET allow_experimental_vector_similarity_index = 0; +ALTER TABLE tab ADD INDEX idx vec TYPE vector_similarity('hnsw', 'L2Distance'); -- { serverError SUPPORT_IS_DISABLED } + +SET allow_experimental_vector_similarity_index = 1; +ALTER TABLE tab ADD INDEX idx vec TYPE vector_similarity('hnsw', 'L2Distance'); + +-- Other index DDL must work regardless of the setting +SET allow_experimental_vector_similarity_index = 0; +ALTER TABLE tab MATERIALIZE INDEX idx; +-- ALTER TABLE tab CLEAR INDEX idx; -- <-- Should work but doesn't w/o enabled setting. Unexpected but not terrible. +ALTER TABLE tab DROP INDEX idx; + +DROP TABLE tab; diff --git a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql b/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql deleted file mode 100644 index e91d7c71eac..00000000000 --- a/tests/queries/0_stateless/03231_forbid_add_vector_similarity_index.sql +++ /dev/null @@ -1,21 +0,0 @@ --- Tags: no-fasttest - -DROP TABLE IF EXISTS test_embedding; - -CREATE TABLE test_embedding -( - id UInt32, - embedding Array(Float32), -) -ENGINE = MergeTree -ORDER BY tuple(); - -SET allow_experimental_vector_similarity_index = 0; - -alter table test_embedding add INDEX idx embedding TYPE vector_similarity('hnsw', 'cosineDistance'); -- { serverError SUPPORT_IS_DISABLED } - -SET allow_experimental_vector_similarity_index = 1; - -alter table test_embedding add INDEX idx embedding TYPE vector_similarity('hnsw', 'cosineDistance'); - -DROP TABLE test_embedding; From 2620325b303b9318fdd347372f66faa131ca9401 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 4 Sep 2024 08:59:52 +0000 Subject: [PATCH 11/33] Minor fixups + add a tests --- .../MergeTree/VectorSimilarityCondition.cpp | 4 ++-- .../MergeTree/VectorSimilarityCondition.h | 6 +----- .../02354_vector_search_queries.reference | 15 +++++++++++++++ .../0_stateless/02354_vector_search_queries.sql | 7 +++++++ 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/Storages/MergeTree/VectorSimilarityCondition.cpp b/src/Storages/MergeTree/VectorSimilarityCondition.cpp index 251cdde65ab..641b0037e7b 100644 --- a/src/Storages/MergeTree/VectorSimilarityCondition.cpp +++ b/src/Storages/MergeTree/VectorSimilarityCondition.cpp @@ -40,7 +40,7 @@ void extractReferenceVectorFromLiteral(std::vector & reference_vector, } } -VectorSimilarityCondition::Info::DistanceFunction stringToDistanceFunction(std::string_view distance_function) +VectorSimilarityCondition::Info::DistanceFunction stringToDistanceFunction(const String & distance_function) { if (distance_function == "L2Distance") return VectorSimilarityCondition::Info::DistanceFunction::L2; @@ -59,7 +59,7 @@ VectorSimilarityCondition::VectorSimilarityCondition(const SelectQueryInfo & que , index_is_useful(checkQueryStructure(query_info)) {} -bool VectorSimilarityCondition::alwaysUnknownOrTrue(String distance_function) const +bool VectorSimilarityCondition::alwaysUnknownOrTrue(const String & distance_function) const { if (!index_is_useful) return true; /// query isn't supported diff --git a/src/Storages/MergeTree/VectorSimilarityCondition.h b/src/Storages/MergeTree/VectorSimilarityCondition.h index 83ae1e19bfb..2e9e06a31d0 100644 --- a/src/Storages/MergeTree/VectorSimilarityCondition.h +++ b/src/Storages/MergeTree/VectorSimilarityCondition.h @@ -69,7 +69,7 @@ public: }; /// Returns false if query can be speeded up by an ANN index, true otherwise. - bool alwaysUnknownOrTrue(String distance_function) const; + bool alwaysUnknownOrTrue(const String & distance_function) const; std::vector getReferenceVector() const; size_t getDimensions() const; @@ -142,10 +142,6 @@ private: /// Traverses the AST of ORDERBY section void traverseOrderByAST(const ASTPtr & node, RPN & rpn); - /// Returns true and stores ANNExpr if the query has valid WHERE section - /// TODO NOT implemented, WHERE does not supported. - static bool matchRPNWhere(RPN & rpn, Info & info); - /// Returns true and stores ANNExpr if the query has valid ORDERBY section static bool matchRPNOrderBy(RPN & rpn, Info & info); diff --git a/tests/queries/0_stateless/02354_vector_search_queries.reference b/tests/queries/0_stateless/02354_vector_search_queries.reference index faff306ef60..e42f91d05dc 100644 --- a/tests/queries/0_stateless/02354_vector_search_queries.reference +++ b/tests/queries/0_stateless/02354_vector_search_queries.reference @@ -41,6 +41,21 @@ Special cases 6 [1,9.3] 0.005731362878640178 1 [2,3.2] 0.15200169244542905 7 [5.5,4.7] 0.3503476876550442 +Expression (Projection) + Limit (preliminary LIMIT (without OFFSET)) + Sorting (Sorting for ORDER BY) + Expression (Before ORDER BY) + ReadFromMergeTree (default.tab) + Indexes: + PrimaryKey + Condition: true + Parts: 1/1 + Granules: 4/4 + Skip + Name: idx + Description: vector_similarity GRANULARITY 2 + Parts: 1/1 + Granules: 2/4 -- Setting "max_limit_for_ann_queries" Expression (Projection) Limit (preliminary LIMIT (without OFFSET)) diff --git a/tests/queries/0_stateless/02354_vector_search_queries.sql b/tests/queries/0_stateless/02354_vector_search_queries.sql index 17939992165..8769e5c56bb 100644 --- a/tests/queries/0_stateless/02354_vector_search_queries.sql +++ b/tests/queries/0_stateless/02354_vector_search_queries.sql @@ -63,6 +63,13 @@ FROM tab ORDER BY cosineDistance(vec, reference_vec) LIMIT 3; +EXPLAIN indexes = 1 +WITH [0.0, 2.0] AS reference_vec +SELECT id, vec, cosineDistance(vec, reference_vec) +FROM tab +ORDER BY cosineDistance(vec, reference_vec) +LIMIT 3; + SELECT '-- Setting "max_limit_for_ann_queries"'; EXPLAIN indexes=1 WITH [0.0, 2.0] as reference_vec From 51464be3e1b53b839fd142ee6e7eed7de64f6c2e Mon Sep 17 00:00:00 2001 From: shiyer7474 Date: Wed, 4 Sep 2024 13:26:09 +0000 Subject: [PATCH 12/33] Set use_cluster=false --- .../test_keeper_s3_snapshot/configs/keeper_config1.xml | 1 + .../test_keeper_s3_snapshot/configs/keeper_config2.xml | 1 + .../test_keeper_s3_snapshot/configs/keeper_config3.xml | 1 + 3 files changed, 3 insertions(+) diff --git a/tests/integration/test_keeper_s3_snapshot/configs/keeper_config1.xml b/tests/integration/test_keeper_s3_snapshot/configs/keeper_config1.xml index 8459ea3e068..6af17946eec 100644 --- a/tests/integration/test_keeper_s3_snapshot/configs/keeper_config1.xml +++ b/tests/integration/test_keeper_s3_snapshot/configs/keeper_config1.xml @@ -5,6 +5,7 @@ minio minio123 + false 9181 1 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_s3_snapshot/configs/keeper_config2.xml b/tests/integration/test_keeper_s3_snapshot/configs/keeper_config2.xml index dfe73628f66..25f2b0de812 100644 --- a/tests/integration/test_keeper_s3_snapshot/configs/keeper_config2.xml +++ b/tests/integration/test_keeper_s3_snapshot/configs/keeper_config2.xml @@ -5,6 +5,7 @@ minio minio123 + false 9181 2 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_s3_snapshot/configs/keeper_config3.xml b/tests/integration/test_keeper_s3_snapshot/configs/keeper_config3.xml index 948d9527718..e274b5184f1 100644 --- a/tests/integration/test_keeper_s3_snapshot/configs/keeper_config3.xml +++ b/tests/integration/test_keeper_s3_snapshot/configs/keeper_config3.xml @@ -5,6 +5,7 @@ minio minio123 + false 9181 3 /var/lib/clickhouse/coordination/log From d38551a1788d2ebc1b7970cc912f894f87f3db1c Mon Sep 17 00:00:00 2001 From: vdimir Date: Wed, 4 Sep 2024 13:29:45 +0000 Subject: [PATCH 13/33] resetOutput --- src/Client/ClientBase.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Client/ClientBase.cpp b/src/Client/ClientBase.cpp index edcb98be90f..445500a5f0f 100644 --- a/src/Client/ClientBase.cpp +++ b/src/Client/ClientBase.cpp @@ -1901,11 +1901,7 @@ void ClientBase::processParsedSingleQuery(const String & full_query, const Strin /// We need to park ParallelFormating threads, /// because they can use settings from global context /// and it can lead to data race with `setSettings` - if (output_format) - { - output_format->finalize(); - output_format.reset(); - } + resetOutput(); } catch (...) { From ef177d2c9fff4e3594f0b11e3e72de3cb5d09055 Mon Sep 17 00:00:00 2001 From: shiyer7474 Date: Thu, 5 Sep 2024 11:37:57 +0000 Subject: [PATCH 14/33] Proper cleanup & restart for subsequent iterations --- .../test_keeper_s3_snapshot/test.py | 44 ++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_keeper_s3_snapshot/test.py b/tests/integration/test_keeper_s3_snapshot/test.py index f47a26a77ad..bcb9c7cbd13 100644 --- a/tests/integration/test_keeper_s3_snapshot/test.py +++ b/tests/integration/test_keeper_s3_snapshot/test.py @@ -2,7 +2,9 @@ import pytest from helpers.cluster import ClickHouseCluster from time import sleep from retry import retry +from multiprocessing.dummy import Pool import helpers.keeper_utils as keeper_utils +from minio.deleteobjects import DeleteObject from kazoo.client import KazooClient @@ -76,7 +78,18 @@ def wait_node(node): raise Exception("Can't wait node", node.name, "to become ready") +def delete_keeper_snapshots_logs(nodex): + nodex.exec_in_container( + [ + "bash", + "-c", + "rm -rf /var/lib/clickhouse/coordination/log /var/lib/clickhouse/coordination/snapshots" + ] + ) + + def test_s3_upload(started_cluster): + node1_zk = get_fake_zk(node1.name) # we defined in configs snapshot_distance as 50 @@ -90,10 +103,17 @@ def test_s3_upload(started_cluster): for obj in list(cluster.minio_client.list_objects("snapshots")) ] + def delete_s3_snapshots(): + snapshots = cluster.minio_client.list_objects("snapshots") + for s in snapshots: + cluster.minio_client.remove_object( + "snapshots", + s.object_name) + # Keeper sends snapshots asynchornously, hence we need to retry. @retry(AssertionError, tries=10, delay=2) def _check_snapshots(): - assert set(get_saved_snapshots()) >= set( + assert set(get_saved_snapshots()) == set( [ "snapshot_50.bin.zstd", "snapshot_100.bin.zstd", @@ -126,5 +146,25 @@ def test_s3_upload(started_cluster): ) destroy_zk_client(node2_zk) - node1.start_clickhouse() # for next iteration + node2.stop_clickhouse() + delete_keeper_snapshots_logs(node2) + node3.stop_clickhouse() + delete_keeper_snapshots_logs(node3) + delete_keeper_snapshots_logs(node1) + p = Pool(3) + waiters = [] + def start_clickhouse(node): + node.start_clickhouse() + + waiters.append(p.apply_async(start_clickhouse, args=(node1,))) + waiters.append(p.apply_async(start_clickhouse, args=(node2,))) + waiters.append(p.apply_async(start_clickhouse, args=(node3,))) + + delete_s3_snapshots() # for next iteration + + for waiter in waiters: + waiter.wait() + keeper_utils.wait_until_connected(cluster, node1) + keeper_utils.wait_until_connected(cluster, node2) + keeper_utils.wait_until_connected(cluster, node3) From 57f80473cc74a826bd11f96f9741f762ce639f82 Mon Sep 17 00:00:00 2001 From: Christoph Wurm Date: Thu, 5 Sep 2024 13:42:09 +0100 Subject: [PATCH 15/33] Docs: Add info on lightweight_deletes_sync --- docs/en/sql-reference/statements/delete.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/en/sql-reference/statements/delete.md b/docs/en/sql-reference/statements/delete.md index 8ddb5840f2e..ded0d7dacac 100644 --- a/docs/en/sql-reference/statements/delete.md +++ b/docs/en/sql-reference/statements/delete.md @@ -24,9 +24,15 @@ DELETE FROM hits WHERE Title LIKE '%hello%'; ## Lightweight `DELETE` does not delete data immediately -Lightweight `DELETE` is implemented as a [mutation](/en/sql-reference/statements/alter#mutations), which is executed asynchronously in the background by default. The statement is going to return almost immediately, but the data can still be visible to queries until the mutation is finished. +Lightweight `DELETE` is implemented as a [mutation](/en/sql-reference/statements/alter#mutations) that marks rows as deleted but does not immediately physically delete them. -The mutation marks rows as deleted, and at that point, they will no longer show up in query results. It does not physically delete the data, this will happen during the next merge. As a result, it is possible that for an unspecified period, data is not actually deleted from storage and is only marked as deleted. +By default, `DELETE` statements wait until marking the rows as deleted is completed before returning. This can take a long time if the amount of data is large. Alternatively, you can run it asynchronously in the background using the setting [`lightweight_deletes_sync`](/en/operations/settings/settings#lightweight_deletes_sync). If disabled, the `DELETE` statement is going to return immediately, but the data can still be visible to queries until the background mutation is finished. + +:::note +Before version 24.4, lightweight deletes were asynchronous by default. +::: + +The mutation does not physically delete the rows that have been marked as deleted, this will only happen during the next merge. As a result, it is possible that for an unspecified period, data is not actually deleted from storage and is only marked as deleted. If you need to guarantee that your data is deleted from storage in a predictable time, consider using the table setting [`min_age_to_force_merge_seconds`](https://clickhouse.com/docs/en/operations/settings/merge-tree-settings#min_age_to_force_merge_seconds). Or you can use the [ALTER TABLE ... DELETE](/en/sql-reference/statements/alter/delete) command. Note that deleting data using `ALTER TABLE ... DELETE` may consume significant resources as it recreates all affected parts. From 4d87e349a04173cff7dd0814a6425a1865cf74a8 Mon Sep 17 00:00:00 2001 From: shiyer7474 Date: Thu, 5 Sep 2024 13:57:46 +0000 Subject: [PATCH 16/33] Python formatting --- tests/integration/test_keeper_s3_snapshot/test.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_keeper_s3_snapshot/test.py b/tests/integration/test_keeper_s3_snapshot/test.py index bcb9c7cbd13..b6c25305aef 100644 --- a/tests/integration/test_keeper_s3_snapshot/test.py +++ b/tests/integration/test_keeper_s3_snapshot/test.py @@ -83,7 +83,7 @@ def delete_keeper_snapshots_logs(nodex): [ "bash", "-c", - "rm -rf /var/lib/clickhouse/coordination/log /var/lib/clickhouse/coordination/snapshots" + "rm -rf /var/lib/clickhouse/coordination/log /var/lib/clickhouse/coordination/snapshots", ] ) @@ -106,9 +106,7 @@ def test_s3_upload(started_cluster): def delete_s3_snapshots(): snapshots = cluster.minio_client.list_objects("snapshots") for s in snapshots: - cluster.minio_client.remove_object( - "snapshots", - s.object_name) + cluster.minio_client.remove_object("snapshots", s.object_name) # Keeper sends snapshots asynchornously, hence we need to retry. @retry(AssertionError, tries=10, delay=2) @@ -153,6 +151,7 @@ def test_s3_upload(started_cluster): delete_keeper_snapshots_logs(node1) p = Pool(3) waiters = [] + def start_clickhouse(node): node.start_clickhouse() From 18a6b970ebd512568f69203c169c69928e89e15b Mon Sep 17 00:00:00 2001 From: Pablo Marcos Date: Thu, 5 Sep 2024 13:58:21 +0000 Subject: [PATCH 17/33] Improve logical error trace This will provide meaningful information whenever the issue happens again. --- src/Client/ConnectionPoolWithFailover.h | 2 +- src/Common/PoolWithFailoverBase.h | 8 ++++++++ src/Storages/Distributed/DistributedAsyncInsertBatch.cpp | 9 ++------- .../Distributed/DistributedAsyncInsertDirectoryQueue.cpp | 4 +--- src/Storages/Distributed/DistributedSink.cpp | 4 +--- 5 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/Client/ConnectionPoolWithFailover.h b/src/Client/ConnectionPoolWithFailover.h index a2dc188eb7d..308644ebbdb 100644 --- a/src/Client/ConnectionPoolWithFailover.h +++ b/src/Client/ConnectionPoolWithFailover.h @@ -42,7 +42,7 @@ public: size_t max_error_cap = DBMS_CONNECTION_POOL_WITH_FAILOVER_MAX_ERROR_COUNT); using Entry = IConnectionPool::Entry; - using PoolWithFailoverBase::isTryResultInvalid; + using PoolWithFailoverBase::checkTryResultIsValid; /** Allocates connection to work. */ Entry get(const ConnectionTimeouts & timeouts) override; diff --git a/src/Common/PoolWithFailoverBase.h b/src/Common/PoolWithFailoverBase.h index c44ab7df53a..53a746c316e 100644 --- a/src/Common/PoolWithFailoverBase.h +++ b/src/Common/PoolWithFailoverBase.h @@ -122,6 +122,14 @@ public: return result.entry.isNull() || !result.is_usable || (skip_read_only_replicas && result.is_readonly); } + void checkTryResultIsValid(const TryResult & result, bool skip_read_only_replicas) const + { + if (isTryResultInvalid(result, skip_read_only_replicas)) + throw DB::Exception(DB::ErrorCodes::LOGICAL_ERROR, + "Got an invalid connection result: entry.isNull {}, is_usable {}, is_up_to_date {}, delay {}, is_readonly {}, skip_read_only_replicas {}", + result.entry.isNull(), result.is_usable, result.is_up_to_date, result.delay, result.is_readonly, skip_read_only_replicas); + } + size_t getPoolSize() const { return nested_pools.size(); } protected: diff --git a/src/Storages/Distributed/DistributedAsyncInsertBatch.cpp b/src/Storages/Distributed/DistributedAsyncInsertBatch.cpp index 2cf69b9f6b7..2db2bdf3981 100644 --- a/src/Storages/Distributed/DistributedAsyncInsertBatch.cpp +++ b/src/Storages/Distributed/DistributedAsyncInsertBatch.cpp @@ -28,7 +28,6 @@ namespace ErrorCodes extern const int TOO_MANY_PARTITIONS; extern const int DISTRIBUTED_TOO_MANY_PENDING_BYTES; extern const int ARGUMENT_OUT_OF_BOUND; - extern const int LOGICAL_ERROR; } /// Can the batch be split and send files from batch one-by-one instead? @@ -244,9 +243,7 @@ void DistributedAsyncInsertBatch::sendBatch(const SettingsChanges & settings_cha auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(insert_settings); auto results = parent.pool->getManyCheckedForInsert(timeouts, insert_settings, PoolMode::GET_ONE, parent.storage.remote_storage.getQualifiedName()); auto result = results.front(); - if (parent.pool->isTryResultInvalid(result, insert_settings.distributed_insert_skip_read_only_replicas)) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Got an invalid connection result"); - + parent.pool->checkTryResultIsValid(result, insert_settings.distributed_insert_skip_read_only_replicas); connection = std::move(result.entry); compression_expected = connection->getCompression() == Protocol::Compression::Enable; @@ -306,9 +303,7 @@ void DistributedAsyncInsertBatch::sendSeparateFiles(const SettingsChanges & sett auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(insert_settings); auto results = parent.pool->getManyCheckedForInsert(timeouts, insert_settings, PoolMode::GET_ONE, parent.storage.remote_storage.getQualifiedName()); auto result = results.front(); - if (parent.pool->isTryResultInvalid(result, insert_settings.distributed_insert_skip_read_only_replicas)) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Got an invalid connection result"); - + parent.pool->checkTryResultIsValid(result, insert_settings.distributed_insert_skip_read_only_replicas); auto connection = std::move(result.entry); bool compression_expected = connection->getCompression() == Protocol::Compression::Enable; diff --git a/src/Storages/Distributed/DistributedAsyncInsertDirectoryQueue.cpp b/src/Storages/Distributed/DistributedAsyncInsertDirectoryQueue.cpp index 7616b384860..2400de4c07c 100644 --- a/src/Storages/Distributed/DistributedAsyncInsertDirectoryQueue.cpp +++ b/src/Storages/Distributed/DistributedAsyncInsertDirectoryQueue.cpp @@ -416,9 +416,7 @@ void DistributedAsyncInsertDirectoryQueue::processFile(std::string & file_path, auto timeouts = ConnectionTimeouts::getTCPTimeoutsWithFailover(insert_settings); auto results = pool->getManyCheckedForInsert(timeouts, insert_settings, PoolMode::GET_ONE, storage.remote_storage.getQualifiedName()); auto result = results.front(); - if (pool->isTryResultInvalid(result, insert_settings.distributed_insert_skip_read_only_replicas)) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Got an invalid connection result"); - + pool->checkTryResultIsValid(result, insert_settings.distributed_insert_skip_read_only_replicas); auto connection = std::move(result.entry); LOG_DEBUG(log, "Sending `{}` to {} ({} rows, {} bytes)", diff --git a/src/Storages/Distributed/DistributedSink.cpp b/src/Storages/Distributed/DistributedSink.cpp index e3e73e42096..daccbb78f2c 100644 --- a/src/Storages/Distributed/DistributedSink.cpp +++ b/src/Storages/Distributed/DistributedSink.cpp @@ -378,9 +378,7 @@ DistributedSink::runWritingJob(JobReplica & job, const Block & current_block, si /// (anyway fallback_to_stale_replicas_for_distributed_queries=true by default) auto results = shard_info.pool->getManyCheckedForInsert(timeouts, settings, PoolMode::GET_ONE, storage.remote_storage.getQualifiedName()); auto result = results.front(); - if (shard_info.pool->isTryResultInvalid(result, settings.distributed_insert_skip_read_only_replicas)) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Got an invalid connection result"); - + shard_info.pool->checkTryResultIsValid(result, settings.distributed_insert_skip_read_only_replicas); job.connection_entry = std::move(result.entry); } else From 6621365526c4fc5072c254522a22088192f785b9 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Thu, 5 Sep 2024 16:40:44 +0200 Subject: [PATCH 18/33] Update 01287_max_execution_speed.sql --- tests/queries/0_stateless/01287_max_execution_speed.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01287_max_execution_speed.sql b/tests/queries/0_stateless/01287_max_execution_speed.sql index 35bc4e02d38..0d132999481 100644 --- a/tests/queries/0_stateless/01287_max_execution_speed.sql +++ b/tests/queries/0_stateless/01287_max_execution_speed.sql @@ -1,4 +1,4 @@ --- Tags: no-fasttest +-- Tags: no-fasttest, no-debug, no-tsan, no-msan, no-asan SET min_execution_speed = 100000000000, timeout_before_checking_execution_speed = 0; SELECT count() FROM system.numbers; -- { serverError TOO_SLOW } From 0996ed5246b2b48aab375ccc870bc529f969cdec Mon Sep 17 00:00:00 2001 From: Pablo Marcos Date: Thu, 5 Sep 2024 14:53:14 +0000 Subject: [PATCH 19/33] Create a deep copy of Settings to ensure they don't change --- src/Storages/Distributed/DistributedSink.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/Distributed/DistributedSink.cpp b/src/Storages/Distributed/DistributedSink.cpp index daccbb78f2c..39f75fd7d56 100644 --- a/src/Storages/Distributed/DistributedSink.cpp +++ b/src/Storages/Distributed/DistributedSink.cpp @@ -347,7 +347,7 @@ DistributedSink::runWritingJob(JobReplica & job, const Block & current_block, si } const Block & shard_block = (num_shards > 1) ? job.current_shard_block : current_block; - const Settings & settings = context->getSettingsRef(); + const Settings settings = context->getSettingsCopy(); size_t rows = shard_block.rows(); From 5e0673d207b409b8a93d56975e1d05ddb378f6be Mon Sep 17 00:00:00 2001 From: Christoph Wurm Date: Thu, 5 Sep 2024 16:07:33 +0100 Subject: [PATCH 20/33] Remove version note --- docs/en/sql-reference/statements/delete.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/en/sql-reference/statements/delete.md b/docs/en/sql-reference/statements/delete.md index ded0d7dacac..5733efbf8f2 100644 --- a/docs/en/sql-reference/statements/delete.md +++ b/docs/en/sql-reference/statements/delete.md @@ -28,10 +28,6 @@ Lightweight `DELETE` is implemented as a [mutation](/en/sql-reference/statements By default, `DELETE` statements wait until marking the rows as deleted is completed before returning. This can take a long time if the amount of data is large. Alternatively, you can run it asynchronously in the background using the setting [`lightweight_deletes_sync`](/en/operations/settings/settings#lightweight_deletes_sync). If disabled, the `DELETE` statement is going to return immediately, but the data can still be visible to queries until the background mutation is finished. -:::note -Before version 24.4, lightweight deletes were asynchronous by default. -::: - The mutation does not physically delete the rows that have been marked as deleted, this will only happen during the next merge. As a result, it is possible that for an unspecified period, data is not actually deleted from storage and is only marked as deleted. If you need to guarantee that your data is deleted from storage in a predictable time, consider using the table setting [`min_age_to_force_merge_seconds`](https://clickhouse.com/docs/en/operations/settings/merge-tree-settings#min_age_to_force_merge_seconds). Or you can use the [ALTER TABLE ... DELETE](/en/sql-reference/statements/alter/delete) command. Note that deleting data using `ALTER TABLE ... DELETE` may consume significant resources as it recreates all affected parts. From cedddf6fa48c8f84088cb3f3c98ec89fe9d7a849 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 26 Aug 2024 19:10:49 +0200 Subject: [PATCH 21/33] PoC --- .../integration/helper_container/Dockerfile | 4 +- tests/integration/helpers/network.py | 135 ++++++++++++++++-- 2 files changed, 129 insertions(+), 10 deletions(-) diff --git a/docker/test/integration/helper_container/Dockerfile b/docker/test/integration/helper_container/Dockerfile index 49a3d3cd84b..1084d087e53 100644 --- a/docker/test/integration/helper_container/Dockerfile +++ b/docker/test/integration/helper_container/Dockerfile @@ -3,6 +3,8 @@ FROM alpine:3.18 RUN apk add --no-cache -U iproute2 \ - && for bin in iptables iptables-restore iptables-save; \ + && for bin in \ + iptables iptables-restore iptables-save \ + ip6tables ip6tables-restore ip6tables-save; \ do ln -sf xtables-nft-multi "/sbin/$bin"; \ done diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index e6e79dc7947..5219ac22f71 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -3,6 +3,7 @@ import subprocess import time import logging import docker +import ipaddress class PartitionManager: @@ -26,25 +27,76 @@ class PartitionManager: self._check_instance(instance) self._add_rule( - {"source": instance.ip_address, "destination_port": 2181, "action": action} + { + "source": instance.ipv4_address, + "destination_port": 2181, + "action": action, + } ) self._add_rule( - {"destination": instance.ip_address, "source_port": 2181, "action": action} + { + "destination": instance.ipv4_address, + "source_port": 2181, + "action": action, + } ) + if instance.ipv6_address: + self._add_rule( + { + "source": instance.ipv6_address, + "destination_port": 2181, + "action": action, + } + ) + self._add_rule( + { + "destination": instance.ipv6_address, + "source_port": 2181, + "action": action, + } + ) + def dump_rules(self): - return _NetworkManager.get().dump_rules() + v4 = _NetworkManager.get().dump_rules() + v6 = _NetworkManager.get().dump_v6_rules() + + return v4 + v6 def restore_instance_zk_connections(self, instance, action="DROP"): self._check_instance(instance) self._delete_rule( - {"source": instance.ip_address, "destination_port": 2181, "action": action} + { + "source": instance.ipv4_address, + "destination_port": 2181, + "action": action, + } ) self._delete_rule( - {"destination": instance.ip_address, "source_port": 2181, "action": action} + { + "destination": instance.ipv4_address, + "source_port": 2181, + "action": action, + } ) + if instance.ipv6_address: + self._delete_rule( + { + "source": instance.ipv6_address, + "destination_port": 2181, + "action": action, + } + ) + self._delete_rule( + { + "destination": instance.ipv6_address, + "source_port": 2181, + "action": action, + } + ) + def partition_instances(self, left, right, port=None, action="DROP"): self._check_instance(left) self._check_instance(right) @@ -59,16 +111,36 @@ class PartitionManager: rule["destination_port"] = port return rule + def create_rule_v6(src, dst): + rule = { + "source": src.ipv6_address, + "destination": dst.ipv6_address, + "action": action, + } + if port is not None: + rule["destination_port"] = port + return rule + self._add_rule(create_rule(left, right)) self._add_rule(create_rule(right, left)) + if left.ipv6_address and right.ipv6_address: + self._add_rule(create_rule_v6(left, right)) + self._add_rule(create_rule_v6(right, left)) + def add_network_delay(self, instance, delay_ms): self._add_tc_netem_delay(instance, delay_ms) def heal_all(self): while self._iptables_rules: rule = self._iptables_rules.pop() - _NetworkManager.get().delete_iptables_rule(**rule) + + if self._is_ipv6_rule(rule): + _NetworkManager.get().delete_ip6tables_rule(**rule) + else: + _NetworkManager.get().delete_iptables_rule(**rule) + # _NetworkManager.get().delete_iptables_rule(**rule) + # _NetworkManager.get().delete_ip6tables_rule(**rule) while self._netem_delayed_instances: instance = self._netem_delayed_instances.pop() @@ -90,12 +162,29 @@ class PartitionManager: if instance.ip_address is None: raise Exception("Instance + " + instance.name + " is not launched!") + @staticmethod + def _is_ipv6_rule(rule): + is_ipv6 = False + + if "source" in rule: + is_ipv6 = ipaddress.ip_address(rule["source"]).version == 6 + if "destination" in rule: + is_ipv6 = ipaddress.ip_address(rule["source"]).version == 6 + + return is_ipv6 + def _add_rule(self, rule): - _NetworkManager.get().add_iptables_rule(**rule) + if self._is_ipv6_rule(rule): + _NetworkManager.get().add_ip6tables_rule(**rule) + else: + _NetworkManager.get().add_iptables_rule(**rule) self._iptables_rules.append(rule) def _delete_rule(self, rule): - _NetworkManager.get().delete_iptables_rule(**rule) + if self._is_ipv6_rule(rule): + _NetworkManager.get().delete_ip6tables_rule(**rule) + else: + _NetworkManager.get().delete_iptables_rule(**rule) self._iptables_rules.remove(rule) def _add_tc_netem_delay(self, instance, delay_ms): @@ -155,15 +244,29 @@ class _NetworkManager: cmd.extend(self._iptables_cmd_suffix(**kwargs)) self._exec_run(cmd, privileged=True) + def add_ip6tables_rule(self, **kwargs): + cmd = ["ip6tables-legacy", "--wait", "-I", "DOCKER-USER", "1"] + cmd.extend(self._iptables_cmd_suffix(**kwargs)) + self._exec_run(cmd, privileged=True) + def delete_iptables_rule(self, **kwargs): cmd = ["iptables", "--wait", "-D", "DOCKER-USER"] cmd.extend(self._iptables_cmd_suffix(**kwargs)) self._exec_run(cmd, privileged=True) + def delete_ip6tables_rule(self, **kwargs): + cmd = ["ip6tables-legacy", "--wait", "-D", "DOCKER-USER"] + cmd.extend(self._iptables_cmd_suffix(**kwargs)) + self._exec_run(cmd, privileged=True) + def dump_rules(self): cmd = ["iptables", "-L", "DOCKER-USER"] return self._exec_run(cmd, privileged=True) + def dump_v6_rules(self): + cmd = ["ip6tables-legacy", "-L", "DOCKER-USER"] + return self._exec_run(cmd, privileged=True) + @staticmethod def clean_all_user_iptables_rules(): for i in range(1000): @@ -178,6 +281,20 @@ class _NetworkManager: + " iterations, last error: " + str(res.stderr) ) + break + + for i in range(1000): + iptables_iter = i + # when rules will be empty, it will return error + res = subprocess.run("ip6tables-legacy --wait -D DOCKER-USER 1", shell=True) + + if res.returncode != 0: + logging.info( + "All ip6tables rules cleared, " + + str(iptables_iter) + + " iterations, last error: " + + str(res.stderr) + ) return @staticmethod @@ -244,7 +361,7 @@ class _NetworkManager: def _ensure_container(self): if self._container is None or self._container_expire_time <= time.time(): image_name = "clickhouse/integration-helper:" + os.getenv( - "DOCKER_HELPER_TAG", "latest" + "DOCKER_HELPER_TAG", "" ) for i in range(5): if self._container is not None: From fe42299928dddd9d2e705305524566ded756f6ec Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 26 Aug 2024 19:11:12 +0200 Subject: [PATCH 22/33] Typo --- tests/integration/helpers/network.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index 5219ac22f71..a92843a313b 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -361,7 +361,7 @@ class _NetworkManager: def _ensure_container(self): if self._container is None or self._container_expire_time <= time.time(): image_name = "clickhouse/integration-helper:" + os.getenv( - "DOCKER_HELPER_TAG", "" + "DOCKER_HELPER_TAG", "latest" ) for i in range(5): if self._container is not None: From d49d413c8d0ae7995bf4188ff8ce87ac85587c25 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 26 Aug 2024 19:11:48 +0200 Subject: [PATCH 23/33] No legacy --- tests/integration/helpers/network.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index a92843a313b..e62034a5104 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -245,7 +245,7 @@ class _NetworkManager: self._exec_run(cmd, privileged=True) def add_ip6tables_rule(self, **kwargs): - cmd = ["ip6tables-legacy", "--wait", "-I", "DOCKER-USER", "1"] + cmd = ["ip6tables", "--wait", "-I", "DOCKER-USER", "1"] cmd.extend(self._iptables_cmd_suffix(**kwargs)) self._exec_run(cmd, privileged=True) @@ -255,7 +255,7 @@ class _NetworkManager: self._exec_run(cmd, privileged=True) def delete_ip6tables_rule(self, **kwargs): - cmd = ["ip6tables-legacy", "--wait", "-D", "DOCKER-USER"] + cmd = ["ip6tables", "--wait", "-D", "DOCKER-USER"] cmd.extend(self._iptables_cmd_suffix(**kwargs)) self._exec_run(cmd, privileged=True) @@ -264,7 +264,7 @@ class _NetworkManager: return self._exec_run(cmd, privileged=True) def dump_v6_rules(self): - cmd = ["ip6tables-legacy", "-L", "DOCKER-USER"] + cmd = ["ip6tables", "-L", "DOCKER-USER"] return self._exec_run(cmd, privileged=True) @staticmethod @@ -286,7 +286,7 @@ class _NetworkManager: for i in range(1000): iptables_iter = i # when rules will be empty, it will return error - res = subprocess.run("ip6tables-legacy --wait -D DOCKER-USER 1", shell=True) + res = subprocess.run("ip6tables --wait -D DOCKER-USER 1", shell=True) if res.returncode != 0: logging.info( From 5ee5c8224ea715b9a78a9ee79f79419ae36db6f2 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Mon, 26 Aug 2024 22:16:05 +0200 Subject: [PATCH 24/33] Fix --- tests/integration/helpers/network.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index e62034a5104..c35ab65eef5 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -164,14 +164,10 @@ class PartitionManager: @staticmethod def _is_ipv6_rule(rule): - is_ipv6 = False - if "source" in rule: - is_ipv6 = ipaddress.ip_address(rule["source"]).version == 6 + return ipaddress.ip_address(rule["source"]).version == 6 if "destination" in rule: - is_ipv6 = ipaddress.ip_address(rule["source"]).version == 6 - - return is_ipv6 + return ipaddress.ip_address(rule["destination"]).version == 6 def _add_rule(self, rule): if self._is_ipv6_rule(rule): From 9be79614a3fd3d97ed1be76dddcd4182e75d3f8b Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Wed, 28 Aug 2024 15:20:56 +0200 Subject: [PATCH 25/33] Fix --- tests/integration/helpers/network.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index c35ab65eef5..d3de4660acb 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -164,11 +164,13 @@ class PartitionManager: @staticmethod def _is_ipv6_rule(rule): - if "source" in rule: + if rule.get("source"): return ipaddress.ip_address(rule["source"]).version == 6 - if "destination" in rule: + if rule.get("destination"): return ipaddress.ip_address(rule["destination"]).version == 6 + return False + def _add_rule(self, rule): if self._is_ipv6_rule(rule): _NetworkManager.get().add_ip6tables_rule(**rule) From 335ca75174329ecabb9d8de59c1cfd4f799842ba Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 3 Sep 2024 00:16:40 +0200 Subject: [PATCH 26/33] Lint --- tests/integration/helpers/network.py | 41 +++++++++------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index d3de4660acb..4b02f99876f 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -139,8 +139,6 @@ class PartitionManager: _NetworkManager.get().delete_ip6tables_rule(**rule) else: _NetworkManager.get().delete_iptables_rule(**rule) - # _NetworkManager.get().delete_iptables_rule(**rule) - # _NetworkManager.get().delete_ip6tables_rule(**rule) while self._netem_delayed_instances: instance = self._netem_delayed_instances.pop() @@ -267,33 +265,20 @@ class _NetworkManager: @staticmethod def clean_all_user_iptables_rules(): - for i in range(1000): - iptables_iter = i - # when rules will be empty, it will return error - res = subprocess.run("iptables --wait -D DOCKER-USER 1", shell=True) + for iptables in ("iptables", "ip6tables"): + for i in range(1000): + iptables_iter = i + # when rules will be empty, it will return error + res = subprocess.run(f"{iptables}--wait -D DOCKER-USER 1", shell=True) - if res.returncode != 0: - logging.info( - "All iptables rules cleared, " - + str(iptables_iter) - + " iterations, last error: " - + str(res.stderr) - ) - break - - for i in range(1000): - iptables_iter = i - # when rules will be empty, it will return error - res = subprocess.run("ip6tables --wait -D DOCKER-USER 1", shell=True) - - if res.returncode != 0: - logging.info( - "All ip6tables rules cleared, " - + str(iptables_iter) - + " iterations, last error: " - + str(res.stderr) - ) - return + if res.returncode != 0: + logging.info( + f"All {iptables} rules cleared, " + + str(iptables_iter) + + " iterations, last error: " + + str(res.stderr) + ) + break @staticmethod def _iptables_cmd_suffix( From d46065360b13c9dfbab11f642d54702610cd550d Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 3 Sep 2024 05:24:03 +0200 Subject: [PATCH 27/33] Fix --- tests/integration/helpers/network.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index 4b02f99876f..8d96add9501 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -28,14 +28,14 @@ class PartitionManager: self._add_rule( { - "source": instance.ipv4_address, + "source": instance.ip_address, "destination_port": 2181, "action": action, } ) self._add_rule( { - "destination": instance.ipv4_address, + "destination": instance.ip_address, "source_port": 2181, "action": action, } @@ -68,14 +68,14 @@ class PartitionManager: self._delete_rule( { - "source": instance.ipv4_address, + "source": instance.ip_address, "destination_port": 2181, "action": action, } ) self._delete_rule( { - "destination": instance.ipv4_address, + "destination": instance.ip_address, "source_port": 2181, "action": action, } From 58b84e9e3d75846428b6839aa03207a4ce8f5cbd Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 3 Sep 2024 17:29:44 +0200 Subject: [PATCH 28/33] Update network.py --- tests/integration/helpers/network.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index 8d96add9501..c9e2df0b2a0 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -269,7 +269,7 @@ class _NetworkManager: for i in range(1000): iptables_iter = i # when rules will be empty, it will return error - res = subprocess.run(f"{iptables}--wait -D DOCKER-USER 1", shell=True) + res = subprocess.run(f"{iptables} --wait -D DOCKER-USER 1", shell=True) if res.returncode != 0: logging.info( From 1f5eb2cd2c1de8639d017f769d04975d21cc590a Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 3 Sep 2024 18:36:32 +0200 Subject: [PATCH 29/33] Set IPv6 instance to the node object --- tests/integration/helpers/cluster.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/helpers/cluster.py b/tests/integration/helpers/cluster.py index 53f4f1e1f26..821bb887435 100644 --- a/tests/integration/helpers/cluster.py +++ b/tests/integration/helpers/cluster.py @@ -2112,6 +2112,7 @@ class ClickHouseCluster: self.base_cmd + ["up", "--force-recreate", "--no-deps", "-d", node.name] ) node.ip_address = self.get_instance_ip(node.name) + node.ipv6_address = self.get_instance_global_ipv6(node.name) node.client = Client(node.ip_address, command=self.client_bin_path) logging.info("Restart node with ip change") @@ -3182,6 +3183,7 @@ class ClickHouseCluster: for instance in self.instances.values(): instance.docker_client = self.docker_client instance.ip_address = self.get_instance_ip(instance.name) + instance.ipv6_address = self.get_instance_global_ipv6(instance.name) logging.debug( f"Waiting for ClickHouse start in {instance.name}, ip: {instance.ip_address}..." From 26add45c70cef4dd2c0fc18891c3db9bf90bb06e Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Tue, 3 Sep 2024 18:49:04 +0200 Subject: [PATCH 30/33] Poke CI From da3a7d069173b7508584a63190a5e24a9452e5bd Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Thu, 5 Sep 2024 06:23:20 +0200 Subject: [PATCH 31/33] Setup missing ip6tables chain --- tests/integration/helpers/network.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index c9e2df0b2a0..39f413cdf3b 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -235,12 +235,29 @@ class _NetworkManager: cls._instance = cls(**kwargs) return cls._instance + def setup_ip6tables_docker_user_chain(self): + _rules = subprocess.check_output( + f"ip6tables-save", shell=True + ) + if "DOCKER-USER" in _rules.decode("utf-8"): + return + + setup_cmds = [ + ["ip6tables", "--wait", "-N", "DOCKER-USER"], + ["ip6tables", "--wait", "-I", "FORWARD", "-j", "DOCKER-USER"], + ["ip6tables", "--wait", "-A", "DOCKER-USER", "-j", "RETURN"], + ] + for cmd in setup_cmds: + self._exec_run(cmd, privileged=True) + def add_iptables_rule(self, **kwargs): cmd = ["iptables", "--wait", "-I", "DOCKER-USER", "1"] cmd.extend(self._iptables_cmd_suffix(**kwargs)) self._exec_run(cmd, privileged=True) def add_ip6tables_rule(self, **kwargs): + self.setup_ip6tables_docker_user_chain() + cmd = ["ip6tables", "--wait", "-I", "DOCKER-USER", "1"] cmd.extend(self._iptables_cmd_suffix(**kwargs)) self._exec_run(cmd, privileged=True) From 798f4b4c3b4667b6cd09760449758b6b7e503e56 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 5 Sep 2024 04:35:04 +0000 Subject: [PATCH 32/33] Automatic style fix --- tests/integration/helpers/network.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/integration/helpers/network.py b/tests/integration/helpers/network.py index 39f413cdf3b..065836396f3 100644 --- a/tests/integration/helpers/network.py +++ b/tests/integration/helpers/network.py @@ -236,9 +236,7 @@ class _NetworkManager: return cls._instance def setup_ip6tables_docker_user_chain(self): - _rules = subprocess.check_output( - f"ip6tables-save", shell=True - ) + _rules = subprocess.check_output(f"ip6tables-save", shell=True) if "DOCKER-USER" in _rules.decode("utf-8"): return From a463b2b44c9162a9ee7cb3d830a3ad6a3b5a7ff6 Mon Sep 17 00:00:00 2001 From: Konstantin Bogdanov Date: Thu, 5 Sep 2024 15:59:21 +0200 Subject: [PATCH 33/33] Poke CI