-
Notifications
You must be signed in to change notification settings - Fork 532
functionaltests: Add 7x to 8x standalone to managed test #16503
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
Conversation
This pull request does not have a backport label. Could you fix it @ericywl? 🙏
|
472a2d7
to
e3a5884
Compare
f2283ee
to
e7ca71c
Compare
This pull request is now in conflicts. Could you fix it @ericywl? 🙏
|
85cc1dd
to
b198e3a
Compare
b198e3a
to
002e828
Compare
This pull request is now in conflicts. Could you fix it @ericywl? 🙏
|
002e828
to
e4af556
Compare
810fcac
to
22a570a
Compare
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.
I see that the tests are still failing for 8.x, maybe it needs to ignore 7.x data stream as well?
As the above issue needs to be addressed, I'd suggest to extract some small refactoring into separate PRs to focus on the 7 to 8 test that is already quite sizeable.
The code for the 7 to 8 test looks good to me, so if tests are green we can move forward merging this :)
This pull request is now in conflicts. Could you fix it @ericywl? 🙏
|
f8b5976
to
b5ccd1a
Compare
c364715
to
4143bdf
Compare
This pull request is now in conflicts. Could you fix it @ericywl? 🙏
|
b5ccd1a
to
ef29ad0
Compare
This pull request is now in conflicts. Could you fix it @ericywl? 🙏
|
Co-authored-by: Edoardo Tenani <[email protected]>
100e2df
to
2de3459
Compare
The last run had 2 failures A failure in A failure in
Inspecting the logs it seems the error "recovers" to become a 503. I'm thinking this may be due to the fleet server bootstrapping the apm server. I'm not 100% confident this is the right choice but it feels safe to ignore (probably worth checking with the Fleet team though). |
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.
minor comments for the PR but there are test failures to address.
IndicesManagedBy: []string{managedByILM, managedByILM}, | ||
} | ||
|
||
check := map[string]asserts.CheckDataStreamIndividualWant{ |
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.
The next step here would be to refactor all data stream check to follow a similar pattern: instead of a list checked in a order sensitive manner, use a map where the checker can match keys to data streams and then use the specific check value for assertions. Not to be addressed in this PR.
functionaltests/steps_legacy_test.go
Outdated
if !e.integrations { | ||
idxDocCount := getDocCountPerIndexV7(t, ctx, e.esc) | ||
asserts.CheckDocCountV7(t, idxDocCount, previousRes.IndicesDocCount, | ||
expectedIndicesIngest()) | ||
return testStepResult{IndicesDocCount: idxDocCount} | ||
} | ||
|
||
// Managed, check data streams | ||
dsDocCount := getDocCountPerDSV7(t, ctx, e.esc, e.dsNamespace) | ||
asserts.CheckDocCount(t, dsDocCount, previousRes.DSDocCount, | ||
expectedDataStreamsIngestV7(e.dsNamespace)) | ||
return testStepResult{DSDocCount: dsDocCount} |
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.
Why are we also checking non standalone DS here?
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.
Not sure what you mean by this? When we ingest in managed mode, we check the data streams.
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.
My confusion stemmed from using legacy
here. In this case this step is used for 7.x tests in both standalone and managed modes, so it's correct that is checking data streams. Thanks for clarifying that.
Updated and ran new passing run: https://github.com/elastic/apm-server/actions/runs/14436234632. |
Motivation/summary
Add tests for 7.x -> 8.x:
Note: Requires #16537 to be merged first.
How to test these changes
Run functionaltests with this branch: https://github.com/elastic/apm-server/actions/runs/14436234632.