-
Notifications
You must be signed in to change notification settings - Fork 85
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
Extend bucket notification timeout and map to the test timeout #2135
base: development/2.6
Are you sure you want to change the base?
Conversation
williamlardier
commented
Aug 28, 2024
•
edited
Loading
edited
- Increase timeout for bucket notification tests, to match the current config (ensure the check timeouts before the step)
- Fixes an issue with kafka cleaner, when an error happens, we have no log and we clear the custer timeout, while the step timeout is -1, resulting in builds never completing (6h+), and improve the timeout.
- Add a custom timeout when checking the DMF volume, as the default 100s is too low, and we have this issue very frequently since months.
Hello williamlardier,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
9cecc0c
to
a2ec22a
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
An issue where the kafkacleaner never completed was observed in the CI; as a result, the build took 6h before being cancelled. THis fix ensures that we tolerate up to 6min. Note that the current value is 30s * (1000 + 5000) so 3 minutes. Issue: ZENKO-4806
In some cases, the platform will take some time to process the event, so the test will fail, although there is no error. This change also add a sleep between two checks, to reduce the impact on the CPU when multiple tests are running this step in parallel. Issue: ZENKO-4806
Issue: ZENKO-4806
tests/ctst/common/common.ts
Outdated
@@ -237,14 +237,15 @@ Then('i {string} be able to add user metadata to object {string}', | |||
} | |||
}); | |||
|
|||
Then('kafka consumed messages should not take too much place on disk', { timeout: -1 }, | |||
const kafkaCleanerTimeout = 1800000; // 30min |
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.
30 minutes seems verrrry long for kafka cleaner, which is setup to run every 30 second (or maybe 1-2 minutes) in CI...
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.
It's already 30min today, this was the config where we got the best results
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 am trying this now 0125893
@@ -307,6 +308,9 @@ Then('kafka consumed messages should not take too much place on disk', { timeout | |||
|
|||
// If a topic remains in this array, it means it has not been cleaned | |||
assert(topics.length === 0, `Topics ${topics.join(', ')} still have not been cleaned`); | |||
} catch (err: unknown) { |
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.
is this not done in cucumber/ctst?
should it not be?
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.
We have a "finally" that clears the timeout even if we got an error, which mean we never stop the test as its timeout was -1
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.
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.
what I mean is that (even with the catch) the exception should be propagated: and eventually reach cucumber/ctst where it shoudl be caught.... and immediately fail the test (even with no timeout)
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.
Maybe the timeout was not due to an error... At least this ensures we log the error with a message
Errors not catched by the tests are sent to cucumber directly, it'll mark the test as failed. But in the build above, it didn't work
/force_reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
Last try to reproduce the errors with fresh branches, if I cannot, I will close this PR, as the current changes are now already part of other PRs |