Feat/pacing #480
Feat/pacing #480
Conversation
There was a problem hiding this comment.
Great to finally have the pacing feature on core. It'll be a very important core building block for NApps. Overall, looks good to me, left some minor comments though other than the ones you already mentioned regarding elastic window and changelog.
1 - Also, can you also document here the local stress test with wireshark flowmods IO graphs? It's great to know that they're passing, but let's take the opportunity to doc on this PR, when limits need to be upgraded in the future, someone can run the same stress test again and confirm it still works and if the charts look similar.
2 - Also, let's document in the PR why we decided not to support elastic window, if someone ask in the future again, we can point out to this PR, until upstream fixes or enhances it (no need to prove what the root cause issue was, but just document what was the unexpected behavior), fixed_window is certainly sufficient for now, but let's not lose that information for future reference.
|
@viniarck Not sure why scrutinizer is failing, but otherwise, I think your concerns here should be addressed. |
There was a problem hiding this comment.
I'll leave it pre-approved @Ktmi, great work, functionality-wise looks solid, and I'll rely on what you reported here. Before merging though, please take a look on the log.warning issue and the tests marked to be skipped.
Finally, it's up to you if you'll document the flowmod charts here or on other PRs, but eventually, I'd like to see it for also historic reasons for us to look back at it one day. Let me know on which PR I should expect to see it in the future.
Implements EP0038
Summary
Adds in the pacer to the controller, which is intended to be used by NApps. The pacer allows for pacing actions to a set rate, using a specified strategy.
Local Tests
Everything seems to be working fine. I experimented with having a pace set for sending flow mods, and everything seemed fine.
End-to-End Tests
Can't really do in depth E2E tests, due to the lack of implementation. From what I've seen, it seems to work correctly here.