refactor(datalayer): key AttributeMap by DataKey instead of string - #2190
refactor(datalayer): key AttributeMap by DataKey instead of string#2190satyamg1620 wants to merge 6 commits into
Conversation
2b0e757 to
80aadb0
Compare
f1948ca to
e11ce4e
Compare
elevran
left a comment
There was a problem hiding this comment.
The datascope package adds actual runtime enforcement that a plugin's endpoint reads/writes match its declared Produces()/Consumes()
Two things worth addressing before merge:
- The PR description says enforcement against a plugin's own declaration is "not covered... worth a separate issue", but commits 2 and 4 (enforce(requestcontrol), enforce(framework)) add exactly that for endpoint attributes. Please update the description so it matches what's shipped.
- InferenceRequest.PutAttribute/GetAttribute (the request-side attribute store) got the DataKey type change but is not wrapped by any datascope-equivalent enforcement, unlike the endpoint side. If that asymmetry is intentional (e.g. deferred to a follow-up), a note in the description or a linked issue would help; if not, it is a real gap in what this PR claims to fix.
| // The write violation is dropped: Filter has no error return, and Scope | ||
| // has already logged and rejected the write. Producers run under | ||
| // executePluginsAsDAG, which does fail the request on one. | ||
| scoped, _ := datascope.Scope(logger, filter, filteredEndpoints) |
There was a problem hiding this comment.
nit:
a filter's or scorer's undeclared write only surfaces as a dropped log line here and in runScorer below, since Filter/Score have no error return. Worth confirming there is a metric or otherwise-visible signal so a misbehaving plugin does not fail silently in production.
There was a problem hiding this comment.
Added. Rejections now increment llm_d_epp_plugin_data_scope_violations_total, labelled by extension point, plugin type, plugin name, and access kind (read/write), so a misdeclared filter or scorer is visible in production rather than only at log verbosity.
Logging is deduplicated per invocation: the first offence of each kind logs at Error, the rest at DEBUG, so a plugin that misreaches on every candidate does not emit one error line per endpoint per request. The metric counts every rejection.
There was a problem hiding this comment.
datascopeis a plausible name but it's vague: "scope" could mean many things in Go code (variable scope, request scope, tracing scope). Better options, given what it actually does (wraps an Endpoint and enforces the plugin's own Produces()/Consumes() declaration) might beconfine.
The use ifconfinematches the doc comment's own language ("confines a plugin's attribute access"), is short, verb-based, and reads naturally asconfine.Scope(...). It doesn't collide with other Go-ecosystem naming conventions the way "scope" does oraclwould. This is a naming preference, not a blocking issue.- Instead of new package, consider
pkg/epp/datalayer/endpoint_scope.go(or similar) alongsidedata_graph.goinstead of a newframework/*package. The new code here and existing code in datalayer enforce the sameProduces()/Consumes()contract, one statically and one at runtime, and no import cycle blocks it.Usingframework/datascope(orframework/confine) as its own package is defensible too (keeps runtime enforcement separate from static DAG validation), but I would lean towards reusing the datalayer package..
There was a problem hiding this comment.
Took the placement suggestion, which moots the naming one. This file is gone — it's now pkg/epp/datalayer/endpoint_scope.go, in package datalayer beside data_graph.go, so the static and runtime halves sit together. Type is ScopedEndpoint; call sites read datalayer.Scope(...).
Confirmed no cycle: pkg/epp/datalayer already depends on the three framework/interface/* packages and on pkg/epp/metrics, with nothing importing back.
|
@satyamg1620 please address conflict |
Thanks @elevran for the review. I will update the PR description and code accordingly |
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
Move runtime confinement alongside the static dependency validation it mirrors, so both halves of the Produces()/Consumes() contract live in one package. A rejected access surfaced only as a log line, and a rejected read logged one per endpoint per request. Count every rejection under llm_d_epp_plugin_data_scope_violations_total and log the first offence of each kind per invocation, so a misdeclared plugin stays visible in production without depending on log verbosity. Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
b4fb23f to
7d96382
Compare
|
Resolved and rebased PR |
The attribute name and its producer are separate parameters, but they were once a single "Attribute/Producer" string. A key built from that spelling matches nothing: the read resolves as absent, so the filter keeps every endpoint and the scorer returns zero, with no error anywhere. Reject it at construction and name the split in the message. A producer pointer tells an omitted producer from one set to the empty string, so an attribute whose own name contains a slash stays reachable. Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
Plugins declare the data they exchange as
DataKeyvalues, butAttributeMapaccess took a raw string. Nothing tied a plugin's reads and writes to itsProduces()/Consumes()declaration, so a plugin could declare one key and use another.This closes that in two layers:
Typing.
AttributeMapandscheduling.Endpointare keyed byDataKey, which removes the raw-string path into the attribute map. It does not enforce the declaration on its own:plugin.NewDataKeyis exported, so a plugin can still construct a key it never declared. Runtime confinement is what makes the declaration binding. Custom scalar metrics derive their key from configuration viaattrmetrics.ResolveConfiguredKey.Runtime confinement.
datalayer.Scopewraps the endpoints handed to a plugin so writes outsideProduces()are dropped and reads outsideConsumes()resolve as absent. It covers filters, scorers, andDataProducer.Produce. Every rejection incrementsllm_d_epp_plugin_data_scope_violations_total, labelled by extension point, plugin type, plugin name, and access kind. A rejected write additionally fails the request where the extension point has an error return, which today meansDataProducer.Runtime confinement lives next to
ValidateAndOrderDataDependencies, which enforces the same contract statically.The
endpoint-attribute-filterandendpoint-attribute-scorerplugins previously declaredConsumes() map[string]any, which never satisfiedConsumerPlugin, so they participated in no dependency ordering at all. They now declareplugin.DataDependenciesand name their producer through a newproducerparameter.Not covered
InferenceRequest.PutAttribute/GetAttribute) is typed byDataKeybut not confined:p2psourceanddisagg/topologyaffinitycurrently exchange request attributes they do not declare.Endpoint.GetAttributes()rather than a scoped endpoint.Admitter,Screener, and profile handlers receive endpoints unscoped. This is not only theoretical:LatencyAdmissiondeclares aRequireddependency inConsumes()and reads it off the endpoint outside the scope, and the disaggPrefixBasedPDDeciderreadsPrefixCacheMatchInfoDataKeythe same way.Admithas an error return, so it could be scoped on the same terms asDataProducerin a follow-up.Which issue(s) this PR fixes:
Fixes #1223
Release note: