test: refactor integration test harness to support data layer testing#2664
Conversation
|
@Mohamedma96: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @Mohamedma96. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
LukeAVanDrie
left a comment
There was a problem hiding this comment.
This looks great, thanks!
4f401f2 to
5ec0154
Compare
| fwkplugin.Register(mockType, func(_ string, _ json.RawMessage, _ fwkplugin.Handle) (fwkplugin.Plugin, error) { | ||
| return mockDataSource, nil | ||
| }) | ||
| defer delete(fwkplugin.Registry, mockType) |
There was a problem hiding this comment.
this might be unavoidable as there is no Unregister, but seems odd.
There was a problem hiding this comment.
There is no lazy initialization, so this is currently safe, but it is a bit brittle.
There was a problem hiding this comment.
I agree it seems odd, yet I find no way to prevents registry pollution between different tests, and right, this assumes that registry map is read only during setup function and not any stage later (which is true for now), so for now this will work.
I can remove the defer delete, since only datalayer tests "know" about the mock-metrics-plugin and in case it's not deleted the tests would just overwrite or re-register the plugin which has no actual difference (re-register with same factory func).
What do you suggest?
| mockDataSourceType = "mock-metrics-source" | ||
|
|
||
| // testDLConfig configures the data layer using the mock data source type. | ||
| testDLConfig = ` |
There was a problem hiding this comment.
nit: might be something you want to pull in via //go:embed from an external file
|
|
||
| // mockDataSourceBackend wraps the mock DataSource to implement metricsBackend. | ||
| // It also sets metrics on the legacy FakePodMetricsClient because Prometheus metrics | ||
| // (e.g. inference_pool_ready_pods) are still populated by the legacy refresh loop. |
There was a problem hiding this comment.
Q: is this functionality missing in the data layer or just not configured by the test? If missing, please open an issue to complement.
There was a problem hiding this comment.
I think setting metrics functionality is missing in datalayer
There was a problem hiding this comment.
can you please review pkg/epp/datalayer/logger/logger.go and create an issue to add missing metrics?
Specifically, I think ready pods are already set on L114.
There was a problem hiding this comment.
true, changing the comment, and actually the two loggers are at full parity — both record the same three metrics. The gap is not between datalayer and backend loggers, but between what fwkdl.Metrics has and what either logger publishes. RunningRequestsSize is collected per-pod but neither logger records it created this issue to handle missing logs:
#2689
| Name: mockDataSourceType, | ||
| }) | ||
| mgr, dataStore, err = eppRunner.NewTestRunnerSetup(ctx, testEnv.Config, eppOptions, fakePmc, mockDataSource) | ||
| require.NoError(t, err, "failed to create manager") |
There was a problem hiding this comment.
Q: is it common practice to use testify/require in files that are _test.go? I realize this decision was not made in the PR - seeking recommendation for future use.
It makes sense in this context since it is effectively a test file, just not named so (which is another question...)
There was a problem hiding this comment.
- we can change the name to harness_test.go. harness.go is only called from test code.
- I can see files that has require.NoError() from before which are not _test.go like test/integration/util.go, not.
There was a problem hiding this comment.
@LukeAVanDrie is in a better position to comment on the file name change as #2084 dropped the extension
There was a problem hiding this comment.
Here is the original discussion thread that resulted in #2084. #2022 (comment)
I actually started with the _test suffix but was advised to remove it.
There was a problem hiding this comment.
then keeping it as harness.go
Thanks
| // non-nil, its plugin type is registered as a factory that returns the provided instance, so the | ||
| // YAML config can reference it by type name and the runner wires it into the endpoint factory | ||
| // automatically. Pass nil to fall back to the legacy metrics system with pmc. | ||
| func NewTestRunnerSetup( |
There was a problem hiding this comment.
nit: please do not break parameter list across multiple lines unless necessary (e.g., see deleted L30 for acceptable length)
| Name: mockDataSourceType, | ||
| }) | ||
| mgr, dataStore, err = eppRunner.NewTestRunnerSetup(ctx, testEnv.Config, eppOptions, fakePmc, mockDataSource) | ||
| require.NoError(t, err, "failed to create manager") |
There was a problem hiding this comment.
@LukeAVanDrie is in a better position to comment on the file name change as #2084 dropped the extension
9168677 to
1fa4a5e
Compare
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
db142e3 to
7ba5b05
Compare
|
/lgtm |
ahg-g
left a comment
There was a problem hiding this comment.
Thanks, I left a couple of questions not blocking to this PR.
/approve
| mockDataSourceType = "mock-metrics-source" | ||
| ) | ||
|
|
||
| //go:embed testdata/default-config.yaml |
There was a problem hiding this comment.
this is very cool, I didn't know about it!
| ) | ||
|
|
||
| // TestFullDuplexStreamed_DataLayer runs integration tests through the datalayer metrics pipeline. | ||
| // It mirrors a representative subset of hermetic_test.go test cases but uses the data layer |
There was a problem hiding this comment.
Will we be able to run the rest of the tests using the datalayer once the legacy metrics collection code is removed?
There was a problem hiding this comment.
yes, there are common tests that are shared among the legacy metrics collection and datalayer, those common test can be used to run the tests for datalayer, the other tests in harness.go (not common) are suited for the legacy metrics collection and I would drop them when the code is removed.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, Mohamedma96 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…kubernetes-sigs/gateway-api-inference-extension#2664) * Create integration tests for datalayer metrics Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * formatting Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * review comments and code enhancements Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * use go:embed, some code lint fixes Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * use CommonTest and code cleanup Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * remove unnecessary comment Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> --------- Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
…kubernetes-sigs/gateway-api-inference-extension#2664) * Create integration tests for datalayer metrics Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * formatting Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * review comments and code enhancements Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * use go:embed, some code lint fixes Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * use CommonTest and code cleanup Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> * remove unnecessary comment Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com> --------- Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
Summary
Refactors the integration test harness to support testing both the legacy backend/metrics and the new data layer architecture. This enables comprehensive validation of the data layer metrics pipeline using mock data sources.
What type of PR is this?
/kind test
What this PR does / why we need it:
The data layer architecture introduces a new way to collect and process metrics from model servers. We need integration tests that validate this new pipeline works correctly while maintaining test coverage for the existing legacy metrics approach.
Which issue(s) this PR fixes:
Partially Fixes #2638
Does this PR introduce a user-facing change?:
Key changes:
Pull Request Title
test: refactor integration test harness to support data layer testing
Pull Request Description
Summary
Refactors the integration test harness to support testing both the standard metrics client and the new data layer architecture. This enables comprehensive validation of the data layer metrics pipeline using mock data sources.
Motivation
The data layer architecture introduces a new way to collect and process metrics from model servers. We need integration tests that validate this new pipeline works correctly while maintaining test coverage for the existing standard metrics client approach.
Changes
Test Infrastructure
test/integration/epp/util.go: Extracted common test cases intocommonTestCases()function that can be shared across test suitestest/integration/epp/harness.go:WithDataLayer()option to configure test harness with mock data sourcesmockDataSourceType,testDLConfig)New Test Suite
test/integration/epp/datalayer_integration_test.go: New integration test suite that:Test Refactoring
test/integration/epp/hermetic_test.go: Updated to use sharedcommonTestCases()function