-
Notifications
You must be signed in to change notification settings - Fork 520
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
Ajay tripathy fix historical #67
Conversation
cloud/gcpprovider.go
Outdated
for k, key := range inputKeys { | ||
if key.GPUType() == gpuType { | ||
if region == strings.Split(k, ",")[0] { | ||
klog.V(3).Infof("MATCHED GPU TO NODE in region " + region) |
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.
.Infof("...", region)
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.
Done
main.go
Outdated
w.Write(wrapData(data, err)) | ||
err = p.Cloud.DownloadPricingData() | ||
if err != nil { | ||
klog.V(1).Infof("Error redownloading data on config update") |
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.
Please don't swallow errors -- the first thing anyone who sees this message will want to know is what was the error while redownloading ...
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.
Done
provIdRx := regexp.MustCompile("(Nvidia Tesla [^ ]+) ") | ||
for matchnum, group := range provIdRx.FindStringSubmatch(product.Description) { | ||
if matchnum == 1 { | ||
gpuType = strings.ToLower(strings.Join(strings.Split(group, " "), "-")) |
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.
for your consideration, it's quite strange to use a regex, with a grouping construct, and then use some split-join magic to extract the thing one actually cares about from that string
cloud/gcpprovider.go
Outdated
@@ -435,6 +520,11 @@ func (gcp *gcpKey) Features() string { | |||
} else { | |||
usageType = "ondemand" | |||
} | |||
|
|||
if _, ok := gcp.Labels["cloud.google.com/gke-accelerator"]; ok { |
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.
Making this a constant would likely go a long way toward avoiding updated-in-one-place-but-not-the-other bugs
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.
Done
cloud/awsprovider.go
Outdated
@@ -76,6 +76,8 @@ type AWSProductAttributes struct { | |||
UsageType string `json:"usagetype"` | |||
OperatingSystem string `json:"operatingSystem"` | |||
PreInstalledSw string `json:"preInstalledSw"` | |||
InstanceFamily string `json:"instanceFamily"` | |||
GPU string `json:gpu` |
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.
It would be outstanding if this carried some godoc, since GPU
of type string
could be anything
Same thing fro its GPU string
friend in the GCP version below
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.
Done
Fixes #62 -- mostly. If the node has disappeared, you would need to examine the pricing data from the emitted /metrics endpoint.
Also adds better GPU support.