-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add multitenant support for search workloads #13713
Changes from all commits
661e4f8
c8cde73
7bf8b57
bc6f2b9
330ca66
f94c4f9
823e9a6
01fda32
da39ae9
db05a10
4c6b46a
92f2620
675a105
c8f262b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.search; | ||
|
||
import org.opensearch.core.common.io.stream.StreamInput; | ||
import org.opensearch.core.common.io.stream.StreamOutput; | ||
import org.opensearch.core.common.io.stream.Writeable; | ||
|
||
import java.io.IOException; | ||
|
||
/** | ||
* Enum to hold all multitenant labels in workloads | ||
*/ | ||
public enum MultiTenantLabel implements Writeable { | ||
// This label is basically used to define tenancy for multiple features e,g; Query Sandboxing, Query Insights | ||
TENANT("tenant"); | ||
|
||
private final String value; | ||
|
||
MultiTenantLabel(String name) { | ||
this.value = name; | ||
} | ||
|
||
public String getValue() { | ||
return value; | ||
} | ||
|
||
public static MultiTenantLabel fromName(String name) { | ||
for (MultiTenantLabel label : values()) { | ||
if (label.getValue().equalsIgnoreCase(name)) { | ||
return label; | ||
} | ||
} | ||
throw new IllegalArgumentException("Illegal name + " + name); | ||
} | ||
|
||
public static MultiTenantLabel fromName(StreamInput in) throws IOException { | ||
return fromName(in.readString()); | ||
} | ||
|
||
/** | ||
* Write this into the {@linkplain StreamOutput}. | ||
* | ||
* @param out | ||
*/ | ||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
out.writeString(value); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,6 +158,7 @@ | |
import java.util.concurrent.atomic.AtomicLong; | ||
import java.util.function.LongSupplier; | ||
|
||
import static org.opensearch.action.search.TransportSearchAction.NOT_PROVIDED; | ||
import static org.opensearch.common.unit.TimeValue.timeValueHours; | ||
import static org.opensearch.common.unit.TimeValue.timeValueMillis; | ||
import static org.opensearch.common.unit.TimeValue.timeValueMinutes; | ||
|
@@ -568,6 +569,7 @@ public void executeQueryPhase( | |
assert request.canReturnNullResponseIfMatchNoDocs() == false || request.numberOfShards() > 1 | ||
: "empty responses require more than one shard"; | ||
final IndexShard shard = getShard(request); | ||
setTenantInTask(task, request); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not also instrument this tenant logic in task for other phases like fetch etc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do populate the tenancy info at the coordinator level but it would not suffice because of following reasons
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. But should we also instrument this tenant logic for other phases as well like fetch etc? |
||
rewriteAndFetchShardRequest(shard, request, new ActionListener<ShardSearchRequest>() { | ||
@Override | ||
public void onResponse(ShardSearchRequest orig) { | ||
|
@@ -598,6 +600,14 @@ public void onFailure(Exception exc) { | |
}); | ||
} | ||
|
||
private void setTenantInTask(SearchShardTask task, ShardSearchRequest request) { | ||
String tenant = NOT_PROVIDED; | ||
if (request.source() != null && request.source().multiTenantLabels() != null) { | ||
tenant = (String) request.source().multiTenantLabels().get(MultiTenantLabel.TENANT.name()); | ||
} | ||
Comment on lines
+604
to
+607
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NIT] Can be done inline like below:
|
||
task.setResourceLimitGroupName(tenant); | ||
} | ||
|
||
private IndexShard getShard(ShardSearchRequest request) { | ||
if (request.readerId() != null) { | ||
return findReaderContext(request.readerId(), request).indexShard(); | ||
|
@@ -676,6 +686,7 @@ public void executeQueryPhase( | |
} | ||
runAsync(getExecutor(readerContext.indexShard()), () -> { | ||
final ShardSearchRequest shardSearchRequest = readerContext.getShardSearchRequest(null); | ||
setTenantInTask(task, shardSearchRequest); | ||
try ( | ||
SearchContext searchContext = createContext(readerContext, shardSearchRequest, task, false); | ||
SearchOperationListenerExecutor executor = new SearchOperationListenerExecutor(searchContext) | ||
|
@@ -778,6 +789,7 @@ public void executeFetchPhase( | |
public void executeFetchPhase(ShardFetchRequest request, SearchShardTask task, ActionListener<FetchSearchResult> listener) { | ||
final ReaderContext readerContext = findReaderContext(request.contextId(), request); | ||
final ShardSearchRequest shardSearchRequest = readerContext.getShardSearchRequest(request.getShardSearchRequest()); | ||
setTenantInTask(task, shardSearchRequest); | ||
final Releasable markAsUsed = readerContext.markAsUsed(getKeepAlive(shardSearchRequest)); | ||
runAsync(getExecutor(readerContext.indexShard()), () -> { | ||
try (SearchContext searchContext = createContext(readerContext, shardSearchRequest, task, false)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,7 @@ | |
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
|
@@ -136,6 +137,7 @@ public final class SearchSourceBuilder implements Writeable, ToXContentObject, R | |
public static final ParseField SLICE = new ParseField("slice"); | ||
public static final ParseField POINT_IN_TIME = new ParseField("pit"); | ||
public static final ParseField SEARCH_PIPELINE = new ParseField("search_pipeline"); | ||
public static final ParseField MULTI_TENANT_LABELS = new ParseField("multitenant_attrs"); | ||
|
||
public static SearchSourceBuilder fromXContent(XContentParser parser) throws IOException { | ||
return fromXContent(parser, true); | ||
|
@@ -223,6 +225,7 @@ public static HighlightBuilder highlight() { | |
private PointInTimeBuilder pointInTimeBuilder = null; | ||
|
||
private Map<String, Object> searchPipelineSource = null; | ||
private Map<String, Object> multiTenantLabels = new HashMap<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just define it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping it generic if want to provide some complex objects as part of tenancy definition. |
||
|
||
/** | ||
* Constructs a new search source builder. | ||
|
@@ -297,6 +300,10 @@ public SearchSourceBuilder(StreamInput in) throws IOException { | |
derivedFields = in.readList(DerivedField::new); | ||
} | ||
} | ||
|
||
if (in.getVersion().onOrAfter(Version.V_2_14_0)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You will need to define this to Version.V_3_0_0 initially in main branch, after backporting to 2.x, you will then need to change it to Version.V_2_14_0 |
||
multiTenantLabels = in.readMap(); | ||
} | ||
} | ||
|
||
@Override | ||
|
@@ -377,6 +384,10 @@ public void writeTo(StreamOutput out) throws IOException { | |
out.writeList(derivedFields); | ||
} | ||
} | ||
|
||
if (out.getVersion().onOrAfter(Version.V_2_14_0)) { | ||
out.writeMap(multiTenantLabels); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -1088,6 +1099,14 @@ public SearchSourceBuilder searchPipelineSource(Map<String, Object> searchPipeli | |
return this; | ||
} | ||
|
||
/** | ||
* | ||
* @return {@code <String, String>} pairs | ||
*/ | ||
public Map<String, Object> multiTenantLabels() { | ||
return multiTenantLabels; | ||
} | ||
|
||
/** | ||
* Rewrites this search source builder into its primitive form. e.g. by | ||
* rewriting the QueryBuilder. If the builder did not change the identity | ||
|
@@ -1334,6 +1353,8 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th | |
searchPipelineSource = parser.mapOrdered(); | ||
} else if (DERIVED_FIELDS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { | ||
derivedFieldsObject = parser.map(); | ||
} else if (MULTI_TENANT_LABELS.match(currentFieldName, parser.getDeprecationHandler())) { | ||
multiTenantLabels = parser.map(); | ||
} else { | ||
throw new ParsingException( | ||
parser.getTokenLocation(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.tasks; | ||
|
||
/** | ||
* Tasks which should be grouped | ||
*/ | ||
public interface ResourceLimitGroupTask { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are essentially grouping by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming part we are still discussing. |
||
void setResourceLimitGroupName(String name); | ||
|
||
String getResourceLimitGroupName(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.search; | ||
|
||
import org.opensearch.core.common.io.stream.StreamInput; | ||
|
||
import java.io.IOException; | ||
|
||
import static org.mockito.Mockito.doReturn; | ||
import static org.mockito.Mockito.mock; | ||
|
||
public class MultiTenantLabelTests extends AbstractSearchTestCase { | ||
|
||
public void testValidMultiTenantLabel() { | ||
MultiTenantLabel label = MultiTenantLabel.fromName("tenant"); | ||
assertEquals(label.getValue(), "tenant"); | ||
} | ||
|
||
public void testInvalidMultiTenantLabel() { | ||
assertThrows(IllegalArgumentException.class, () -> MultiTenantLabel.fromName("foo")); | ||
} | ||
|
||
public void testValidMultiTenantLabelWithStreamInput() throws IOException { | ||
StreamInput streamInput = mock(StreamInput.class); | ||
doReturn("tenant").when(streamInput).readString(); | ||
|
||
MultiTenantLabel label = MultiTenantLabel.fromName(streamInput); | ||
assertEquals(label.getValue(), "tenant"); | ||
} | ||
|
||
public void testInvalidMultiTenantLabelWithStreamInput() throws IOException { | ||
StreamInput streamInput = mock(StreamInput.class); | ||
doReturn("foo").when(streamInput).readString(); | ||
|
||
assertThrows(IllegalArgumentException.class, () -> MultiTenantLabel.fromName(streamInput)); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this logic can also be extended to other places like indexing etc, defining this here doesn't make sense.
Either
OR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! good catch. I have defined a global string just forgot to use that here.