Skip to content
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 event batch processing results and rerun reconfig test #1186

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented Oct 23, 2023

Proposed changes

Add event batch processing metrics to reconfiguration test and rerun the test. Added some clarification and other small adjustments to test.

Problem: We already measured how long it took for NGINX to reload, but didn't measure for how long it took for NGF to update statuses of a resource.

Solution: Add event batch processing metrics to test.

Testing: Reran the manual test.

Closes #1127

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@bjee19 bjee19 requested a review from a team as a code owner October 23, 2023 22:16
@github-actions github-actions bot added documentation Improvements or additions to documentation tests Pull requests that update tests labels Oct 23, 2023
@bjee19
Copy link
Contributor Author

bjee19 commented Oct 23, 2023

Note the quite big changes in results of the TimeToReadyTotal and TimeToReadyAvgSingle, I checked with @ciarams87 with a couple of them and she said that they seemed reasonable. What do you all think?

@ciarams87
Copy link
Member

ciarams87 commented Oct 24, 2023

Note the quite big changes in results of the TimeToReadyTotal and TimeToReadyAvgSingle, I checked with @ciarams87 with a couple of them and she said that they seemed reasonable. What do you all think?

The big deviation is mostly in test 2. I believe this expected behaviour after the status updater improvements (only updating the status when there is a change). My understanding is that there are actually less changes being processed, which then leads to more efficient handling of each individual resource update. This means we are doing less batching (that is, there are less items being added to each batch), and therefore calling a reload for almost every config change. Because we are reloading more, I think this is leading to the increased TTR.

Unfortunately we don't have the event batch processing results from the first round, but I checked the logs from the run and there are less batches with more resources in each (cumulatively increasing with each batch) and correspondingly less reloads. The largest batch resulting in a config change in the old runs logs was 310.

Does that make sense?

@bjee19 bjee19 requested a review from ciarams87 October 24, 2023 15:59
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@bjee19 bjee19 merged commit 95e6ba6 into nginxinc:main Oct 24, 2023
23 checks passed
bjee19 added a commit to bjee19/nginx-gateway-fabric that referenced this pull request Oct 24, 2023
…1186)

Add event batch processing metrics to reconfiguration test and rerun the test. Added some clarification and other small adjustments to test.

Problem: We already measured how long it took for NGINX to reload, but didn't measure for how long it took for NGF to update statuses of a resource.

Solution: Add event batch processing metrics to test.
bjee19 added a commit that referenced this pull request Oct 24, 2023
@bjee19 bjee19 deleted the test/time-to-update-status branch November 20, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation tests Pull requests that update tests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Measure time to update statuses of resources in reconfig tests
4 participants