-
Notifications
You must be signed in to change notification settings - Fork 240
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
fix: add legacy IPAM metrics back to IPAMv2 #2970
Merged
Merged
+240
−62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rbtr
added
cns
Related to CNS.
swift
Related to SWIFT networking.
fix
Fixes something.
release/latest
Change affects latest release train
labels
Aug 28, 2024
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
nddq
reviewed
Aug 29, 2024
nddq
approved these changes
Sep 3, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sep 3, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sep 3, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sep 4, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sep 4, 2024
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
rbtr
force-pushed
the
fix/ipam-metrics
branch
from
September 4, 2024 15:45
25a85a8
to
fa69c71
Compare
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
cns
Related to CNS.
fix
Fixes something.
release/latest
Change affects latest release train
swift
Related to SWIFT networking.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reason for Change:
IPAM metrics that were set by the old IPAM PoolMonitor were not set in the v2 monitor. This breaks the AMA workbooks for subnet usage monitoring on AKS 1.30+ where CNS 1.6 with v2 IPAM is deployed.
The metrics were missing because the v2 PoolMonitor does not use most of that source data anymore - it depends only on Pod count and Batch size, not the full IPAM utilization buckets and previous NNC state that the v1 implementation needed to scale up/down.
This fix creates a "metrics observer" which functionally performs what the IPAM v1 monitor used to do WRT the metrics in a portable closed scope. It is injected into the v2 monitor so that the v2 monitor does not need to be polluted with the data pulls that the v1 monitor needed and can still trigger the metrics updates.
TODO: As the v1 monitor is phased-out (hopefully in the 1.7 release of CNS), these metrics should be removed to more appropriate locations, such as moving the Allocated/Available/Assigned/etc IP state metrics in to the CNS core datastore IPConfig paths where those IP states are mutated.
Issue Fixed:
Requirements:
Notes: