diff --git a/src/ray/common/scheduling/label_selector.h b/src/ray/common/scheduling/label_selector.h index df9a0133dcd2..76715129893a 100644 --- a/src/ray/common/scheduling/label_selector.h +++ b/src/ray/common/scheduling/label_selector.h @@ -126,18 +126,18 @@ inline bool operator==(const LabelSelector &lhs, const LabelSelector &rhs) { } template -H AbslHashValue(H h, const LabelSelector &label_selector) { - h = H::combine(std::move(h), label_selector.GetConstraints().size()); - for (const auto &constraint : label_selector.GetConstraints()) { - h = H::combine(std::move(h), - constraint.GetLabelKey(), - static_cast(constraint.GetOperator())); +H AbslHashValue(H h, const LabelConstraint &constraint) { + // Hash the values set as a set, the way operator== above compares it. absl hashes its + // unordered containers order-independently and mixes in the size. + return H::combine(std::move(h), + constraint.GetLabelKey(), + static_cast(constraint.GetOperator()), + constraint.GetLabelValues()); +} - for (const auto &value : constraint.GetLabelValues()) { - h = H::combine(std::move(h), value); - } - } - return h; +template +H AbslHashValue(H h, const LabelSelector &label_selector) { + return H::combine(std::move(h), label_selector.GetConstraints()); } inline std::optional> GetHardNodeAffinityValues( diff --git a/src/ray/common/scheduling/tests/label_selector_test.cc b/src/ray/common/scheduling/tests/label_selector_test.cc index 73c018171673..7a8e6223d490 100644 --- a/src/ray/common/scheduling/tests/label_selector_test.cc +++ b/src/ray/common/scheduling/tests/label_selector_test.cc @@ -15,11 +15,15 @@ #include "ray/common/scheduling/label_selector.h" #include +#include #include +#include #include #include #include +#include "absl/container/flat_hash_set.h" +#include "absl/hash/hash.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -208,4 +212,74 @@ TEST(LabelSelectorTest, Deduplication) { ASSERT_EQ(selector.GetConstraints().size(), 4); } +namespace { + +LabelSelector OneConstraint(std::string key, + LabelSelectorOperator op, + absl::flat_hash_set values) { + LabelSelector selector; + selector.AddConstraint(LabelConstraint(std::move(key), op, std::move(values))); + return selector; +} + +LabelSelector RegionSelector(absl::flat_hash_set values) { + return OneConstraint("region", LabelSelectorOperator::LABEL_IN, std::move(values)); +} + +} // namespace + +// LabelConstraint values form an unordered set, so their hash must be order-independent +// to match operator==. We shuffle inputs to force diverse internal memory layouts +// (caused by insertion order and internal salting) and verify hash stability. +TEST(LabelSelectorTest, EqualSelectorsHashEquallyWhateverTheValueOrder) { + std::vector values; + values.reserve(64); + for (int i = 0; i < 64; i++) { + values.push_back("region-" + std::to_string(i)); + } + const LabelSelector reference = + RegionSelector(absl::flat_hash_set(values.begin(), values.end())); + + absl::flat_hash_set hashes; + absl::flat_hash_set> layouts; + std::mt19937 rng(20260826); + for (int round = 0; round < 16; round++) { + std::shuffle(values.begin(), values.end(), rng); + absl::flat_hash_set set(values.begin(), values.end()); + const LabelSelector selector = RegionSelector(std::move(set)); + // operator== compares the constraint vectors, so it also passes when both sides are + // empty; the count needs its own assertion, which also guards the [0] below. + ASSERT_EQ(selector.GetConstraints().size(), 1u); + ASSERT_EQ(selector, reference); + const auto &stored = selector.GetConstraints()[0].GetLabelValues(); + layouts.insert(std::vector(stored.begin(), stored.end())); + hashes.insert(absl::HashOf(selector)); + } + + // Without more than one layout the hash assertion below holds for an order-dependent + // hash too, so the test would pass while guarding nothing. + ASSERT_GT(layouts.size(), 1u); + EXPECT_EQ(hashes.size(), 1u); +} + +// Verify that the values are mixed into the hash. +TEST(LabelSelectorTest, SelectorsWithDifferentValuesHashDifferently) { + EXPECT_NE(absl::HashOf(RegionSelector({"us-east", "us-west"})), + absl::HashOf(RegionSelector({"eu-central", "ap-south"}))); + EXPECT_NE(absl::HashOf(RegionSelector({"us-east", "us-west"})), + absl::HashOf(RegionSelector({"us-east", "us-west", "eu-central"}))); + EXPECT_NE(absl::HashOf(RegionSelector({})), absl::HashOf(RegionSelector({"us-east"}))); +} + +// Verify that both the key and the operator are mixed into the hash. +TEST(LabelSelectorTest, SelectorsDifferingOnlyInKeyOrOperatorHashDifferently) { + const absl::flat_hash_set values = {"us-east"}; + EXPECT_NE( + absl::HashOf(OneConstraint("region", LabelSelectorOperator::LABEL_IN, values)), + absl::HashOf(OneConstraint("zone", LabelSelectorOperator::LABEL_IN, values))); + EXPECT_NE( + absl::HashOf(OneConstraint("region", LabelSelectorOperator::LABEL_IN, values)), + absl::HashOf(OneConstraint("region", LabelSelectorOperator::LABEL_NOT_IN, values))); +} + } // namespace ray