-
Notifications
You must be signed in to change notification settings - Fork 260
Exposed a new API to get number of CPU Cores on a Node #379
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #379 +/- ##
=======================================
Coverage 40.01% 40.01%
=======================================
Files 25 25
Lines 3546 3546
=======================================
Hits 1419 1419
Misses 1927 1927
Partials 200 200Continue to review full report at Codecov.
|
cns/api.go
Outdated
| } | ||
|
|
||
| // getNumberOfCPUCores describes reponse that returns the host local IP Address. | ||
| // getNumberOfCPUCores describes reponse that returns num of cpu cores present on host. |
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.
Shouldn't the comment say - NumOfCPUCoresResponse describes ... ?
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.
yes this should be NumOfCPUCoresResponse
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.
why did you resolve this?
cns/restserver/restserver.go
Outdated
| log.Printf("[Azure-CNS] getNumberOfCPUCores") | ||
| log.Request(service.Name, "getNumberOfCPUCores", nil) | ||
|
|
||
| var num 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.
Can you combine the vars together ?
var (
num int
returnCode int
errMsg string
)
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 being done anywhere in similar context?
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 agree its not done elsewhere but whatever ashvin suggested looks better. lets do this from now on
cns/api.go
Outdated
| } | ||
|
|
||
| // getNumberOfCPUCores describes reponse that returns the host local IP Address. | ||
| // getNumberOfCPUCores describes reponse that returns num of cpu cores present on host. |
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.
yes this should be NumOfCPUCoresResponse
cns/api.go
Outdated
| } | ||
|
|
||
| // getNumberOfCPUCores describes reponse that returns the host local IP Address. | ||
| // getNumberOfCPUCores describes reponse that returns num of cpu cores present on host. |
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.
why did you resolve this?
cns/restserver/restserver.go
Outdated
| log.Printf("[Azure-CNS] getNumberOfCPUCores") | ||
| log.Request(service.Name, "getNumberOfCPUCores", nil) | ||
|
|
||
| var num 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.
I agree its not done elsewhere but whatever ashvin suggested looks better. lets do this from now on
Exposed a new API to get number of CPU Cores on a Node. This will be used by DNC eventually to enforce per node NIC limit which is dependent on number of cores.
DNC before allocating a NIC, will get query a node for number of cores and see if the limit ha already been achieved,