-
Notifications
You must be signed in to change notification settings - Fork 260
[CNS] Debug - Expose In-memory data from HTTPRestService #825
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
[CNS] Debug - Expose In-memory data from HTTPRestService #825
Conversation
Codecov Report
@@ Coverage Diff @@
## master #825 +/- ##
==========================================
+ Coverage 42.10% 42.13% +0.02%
==========================================
Files 158 158
Lines 15025 15089 +64
==========================================
+ Hits 6327 6358 +31
- Misses 7926 7954 +28
- Partials 772 777 +5 |
ramiro-gamarra
left a comment
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.
Some comments
8b8f623 to
6652dba
Compare
6652dba to
da5071c
Compare
ramiro-gamarra
left a comment
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 think this is fine to merge, but let's discuss the comments left first.
| pm.mu.Lock() | ||
|
|
||
| return cns.IpamPoolMonitorStateSnapshot { | ||
| MinimumFreeIps: pm.MinimumFreeIps, |
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.
Can you also return
cachedNNC nnc.NodeNetworkConfig
updatingIpsNotInUseCount int
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.
@neaggarwMS yes I have added them too to IPAMPoolMonitor in the latest commit, please review if that looks good and the CLI command/end-point we should name for this debug API.
neaggarwMS
left a comment
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.
🕐
c8bb282 to
3425da9
Compare
neaggarwMS
left a comment
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.
![]()
This commit has following changes - 1. Removed AllocatedIPCount field from HTTPRestService struct in restserver.go as it is not used in project. 2. Added debug command getInMemory and API to expose fields-HTTPRestService and 2 fields from IPAMPoolMonitor. Please review the naming of the command, handler and end point. 3. Added Test function to test the new api response. 4. Added changes as per review comments - Get request for Test endpoint.
3425da9 to
a53848e
Compare
cns/cnsclient/cnsclient_test.go
Outdated
| desiredIpAddress := "10.0.0.5" | ||
|
|
||
| secondaryIps := make([]string, 0) | ||
| secondaryIps = append(secondaryIps, desiredIpAddress) |
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.
can just
secondaryIps := []string{desiredIpAddress}In-memory data API - adding 2 more fields to IPAMPoolMonitor
a53848e to
9aa3781
Compare
Feat: [CNS] Debug API to expose In-Memory Data HTTPRestService
This commit has following changes -
Reason for Change: Helping in debug commands for CNS service.
Notes: Please review and suggest the naming of the command, handler and end point.