Merge pull request #295 from kubecost/AjayTripathy-getcustompricing-safety

more locking safety, getcustompricing safety

cloud/awsprovider.go

@@ -256,7 +256,7 @@ func (aws *AWS) GetManagementPlatform() (string, error) {
 }
 
 func (aws *AWS) GetConfig() (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("aws.json")
+	c, err := GetCustomPricingData("aws.json")
 	if c.Discount == "" {
 		c.Discount = "0%"
 	}
@@ -269,19 +269,16 @@ func (aws *AWS) GetConfig() (*CustomPricing, error) {
 	return c, nil
 }
 func (aws *AWS) UpdateConfigFromConfigMap(a map[string]string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("aws.json")
+	c, err := GetCustomPricingData("aws.json")
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
-	configPath := path + "aws.json"
-	return configmapUpdate(c, configPath, a)
+
+	return configmapUpdate(c, configPathFor("aws.json"), a)
 }
+
 func (aws *AWS) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("aws.json")
+	c, err := GetCustomPricingData("aws.json")
 	if err != nil {
 		return nil, err
 	}
@@ -345,11 +342,9 @@ func (aws *AWS) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, er
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
-	path += "aws.json"
+
+	path := configPathFor("aws.json")
+
 	remoteEnabled := os.Getenv(remoteEnabled)
 	if remoteEnabled == "true" {
 		err = UpdateClusterMeta(os.Getenv(clusterIDKey), c.ClusterName)
@@ -357,7 +352,11 @@ func (aws *AWS) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, er
 			return nil, err
 		}
 	}
+
+	configLock.Lock()
 	err = ioutil.WriteFile(path, cj, 0644)
+	configLock.Unlock()
+
 	if err != nil {
 		return nil, err
 	}
@@ -478,7 +477,7 @@ func (aws *AWS) isPreemptible(key string) bool {
 func (aws *AWS) DownloadPricingData() error {
 	aws.DownloadPricingDataLock.Lock()
 	defer aws.DownloadPricingDataLock.Unlock()
-	c, err := GetDefaultPricingData("aws.json")
+	c, err := GetCustomPricingData("aws.json")
 	if err != nil {
 		klog.V(1).Infof("Error downloading default pricing data: %s", err.Error())
 	}
@@ -702,7 +701,7 @@ func (aws *AWS) DownloadPricingData() error {
 
 // Stubbed NetworkPricing for AWS. Pull directly from aws.json for now
 func (c *AWS) NetworkPricing() (*Network, error) {
-	cpricing, err := GetDefaultPricingData("aws.json")
+	cpricing, err := GetCustomPricingData("aws.json")
 	if err != nil {
 		return nil, err
 	}

cloud/azureprovider.go

@@ -430,7 +430,7 @@ func (az *Azure) NodePricing(key Key) (*Node, error) {
 
 // Stubbed NetworkPricing for Azure. Pull directly from azure.json for now
 func (c *Azure) NetworkPricing() (*Network, error) {
-	cpricing, err := GetDefaultPricingData("azure.json")
+	cpricing, err := GetCustomPricingData("azure.json")
 	if err != nil {
 		return nil, err
 	}
@@ -514,28 +514,22 @@ func (az *Azure) AddServiceKey(url url.Values) error {
 }
 
 func (az *Azure) UpdateConfigFromConfigMap(a map[string]string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("azure.json")
+	c, err := GetCustomPricingData("azure.json")
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
-	configPath := path + "azure.json"
-	return configmapUpdate(c, configPath, a)
+
+	return configmapUpdate(c, configPathFor("azure.json"), a)
 }
 
 func (az *Azure) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, error) {
 	defer az.DownloadPricingData()
-	c, err := GetDefaultPricingData("azure.json")
+
+	c, err := GetCustomPricingData("azure.json")
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
+
 	a := make(map[string]interface{})
 	err = json.NewDecoder(r).Decode(&a)
 	if err != nil {
@@ -558,10 +552,12 @@ func (az *Azure) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, e
 			c.SharedCosts = sc //todo: support reflection/multiple map fields
 		}
 	}
+
 	cj, err := json.Marshal(c)
 	if err != nil {
 		return nil, err
 	}
+
 	remoteEnabled := os.Getenv(remoteEnabled)
 	if remoteEnabled == "true" {
 		err = UpdateClusterMeta(os.Getenv(clusterIDKey), c.ClusterName)
@@ -570,8 +566,12 @@ func (az *Azure) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, e
 		}
 	}
 
-	configPath := path + "azure.json"
+	configPath := configPathFor("azure.json")
+
+	configLock.Lock()
 	err = ioutil.WriteFile(configPath, cj, 0644)
+	configLock.Unlock()
+
 	if err != nil {
 		return nil, err
 	}
@@ -579,7 +579,7 @@ func (az *Azure) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, e
 	return c, nil
 }
 func (az *Azure) GetConfig() (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("azure.json")
+	c, err := GetCustomPricingData("azure.json")
 	if c.Discount == "" {
 		c.Discount = "0%"
 	}

cloud/customprovider.go

@@ -5,7 +5,6 @@ import (
 	"io"
 	"io/ioutil"
 	"net/url"
-	"os"
 	"strconv"
 	"strings"
 	"sync"
@@ -43,7 +42,7 @@ func (*CustomProvider) GetLocalStorageQuery(offset string) (string, error) {
 }
 
 func (*CustomProvider) GetConfig() (*CustomPricing, error) {
-	return GetDefaultPricingData("default.json")
+	return GetCustomPricingData("default.json")
 }
 
 func (*CustomProvider) GetManagementPlatform() (string, error) {
@@ -55,27 +54,20 @@ func (*CustomProvider) ApplyReservedInstancePricing(nodes map[string]*Node) {
 }
 
 func (cp *CustomProvider) UpdateConfigFromConfigMap(a map[string]string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("default.json")
+	c, err := GetCustomPricingData("default.json")
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
-	configPath := path + "default.json"
-	return configmapUpdate(c, configPath, a)
+
+	return configmapUpdate(c, configPathFor("default.json"), a)
 }
 
 func (cp *CustomProvider) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("default.json")
+	c, err := GetCustomPricingData("default.json")
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
+
 	a := make(map[string]interface{})
 	err = json.NewDecoder(r).Decode(&a)
 	if err != nil {
@@ -104,8 +96,12 @@ func (cp *CustomProvider) UpdateConfig(r io.Reader, updateType string) (*CustomP
 		return nil, err
 	}
 
-	configPath := path + "default.json"
+	configPath := configPathFor("default.json")
+
+	configLock.Lock()
 	err = ioutil.WriteFile(configPath, cj, 0644)
+	configLock.Unlock()
+
 	if err != nil {
 		return nil, err
 	}
@@ -172,7 +168,7 @@ func (cp *CustomProvider) DownloadPricingData() error {
 		m := make(map[string]*NodePrice)
 		cp.Pricing = m
 	}
-	p, err := GetDefaultPricingData("default.json")
+	p, err := GetCustomPricingData("default.json")
 	if err != nil {
 		return err
 	}
@@ -218,7 +214,7 @@ func (*CustomProvider) QuerySQL(query string) ([]byte, error) {
 }
 
 func (*CustomProvider) PVPricing(pvk PVKey) (*PV, error) {
-	cpricing, err := GetDefaultPricingData("default")
+	cpricing, err := GetCustomPricingData("default.json")
 	if err != nil {
 		return nil, err
 	}
@@ -228,7 +224,7 @@ func (*CustomProvider) PVPricing(pvk PVKey) (*PV, error) {
 }
 
 func (*CustomProvider) NetworkPricing() (*Network, error) {
-	cpricing, err := GetDefaultPricingData("default")
+	cpricing, err := GetCustomPricingData("default.json")
 	if err != nil {
 		return nil, err
 	}

cloud/gcpprovider.go

@@ -86,7 +86,7 @@ func (gcp *GCP) GetLocalStorageQuery(offset string) (string, error) {
 }
 
 func (gcp *GCP) GetConfig() (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("gcp.json")
+	c, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		return nil, err
 	}
@@ -119,20 +119,16 @@ func (gcp *GCP) GetManagementPlatform() (string, error) {
 }
 
 func (gcp *GCP) UpdateConfigFromConfigMap(a map[string]string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("gcp.json")
+	c, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		return nil, err
 	}
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
-	}
-	configPath := path + "gcp.json"
-	return configmapUpdate(c, configPath, a)
+
+	return configmapUpdate(c, configPathFor("gcp.json"), a)
 }
 
 func (gcp *GCP) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, error) {
-	c, err := GetDefaultPricingData("gcp.json")
+	c, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		return nil, err
 	}
@@ -184,6 +180,7 @@ func (gcp *GCP) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, er
 			}
 		}
 	}
+
 	cj, err := json.Marshal(c)
 	if err != nil {
 		return nil, err
@@ -197,20 +194,22 @@ func (gcp *GCP) UpdateConfig(r io.Reader, updateType string) (*CustomPricing, er
 	}
 
 	configPath := path + "gcp.json"
+
+	configLock.Lock()
 	err = ioutil.WriteFile(configPath, cj, 0644)
+	configLock.Unlock()
 	if err != nil {
 		return nil, err
 	}
 
 	return c, nil
-
 }
 
 // ExternalAllocations represents tagged assets outside the scope of kubernetes.
 // "start" and "end" are dates of the format YYYY-MM-DD
 // "aggregator" is the tag used to determine how to allocate those assets, ie namespace, pod, etc.
 func (gcp *GCP) ExternalAllocations(start string, end string, aggregator string, filterType string, filterValue string) ([]*OutOfClusterAllocation, error) {
-	c, err := GetDefaultPricingData("gcp.json")
+	c, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		return nil, err
 	}
@@ -235,7 +234,7 @@ func (gcp *GCP) ExternalAllocations(start string, end string, aggregator string,
 
 // QuerySQL should query BigQuery for billing data for out of cluster costs.
 func (gcp *GCP) QuerySQL(query string) ([]*OutOfClusterAllocation, error) {
-	c, err := GetDefaultPricingData("gcp.json")
+	c, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		return nil, err
 	}
@@ -688,7 +687,7 @@ func (gcp *GCP) parsePages(inputKeys map[string]Key, pvKeys map[string]PVKey) (m
 func (gcp *GCP) DownloadPricingData() error {
 	gcp.DownloadPricingDataLock.Lock()
 	defer gcp.DownloadPricingDataLock.Unlock()
-	c, err := GetDefaultPricingData("gcp.json")
+	c, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		klog.V(2).Infof("Error downloading default pricing data: %s", err.Error())
 		return err
@@ -762,7 +761,7 @@ func (gcp *GCP) PVPricing(pvk PVKey) (*PV, error) {
 
 // Stubbed NetworkPricing for GCP. Pull directly from gcp.json for now
 func (c *GCP) NetworkPricing() (*Network, error) {
-	cpricing, err := GetDefaultPricingData("gcp.json")
+	cpricing, err := GetCustomPricingData("gcp.json")
 	if err != nil {
 		return nil, err
 	}

cloud/provider.go

@@ -210,60 +210,71 @@ func CustomPricesEnabled(p Provider) bool {
 	return config.CustomPricesEnabled == "true"
 }
 
+// DefaultPricing should be returned so we can do computation even if no file is supplied.
+func DefaultPricing() *CustomPricing {
+	return &CustomPricing{
+		Provider:              "base",
+		Description:           "Default prices based on GCP us-central1",
+		CPU:                   "0.031611",
+		SpotCPU:               "0.006655",
+		RAM:                   "0.004237",
+		SpotRAM:               "0.000892",
+		GPU:                   "0.95",
+		Storage:               "0.00005479452",
+		ZoneNetworkEgress:     "0.01",
+		RegionNetworkEgress:   "0.01",
+		InternetNetworkEgress: "0.12",
+		CustomPricesEnabled:   "false",
+	}
+}
+
 // GetDefaultPricingData will search for a json file representing pricing data in /models/ and use it for base pricing info.
-func GetDefaultPricingData(fname string) (*CustomPricing, error) {
+func GetCustomPricingData(fname string) (*CustomPricing, error) {
 	configLock.Lock()
 	defer configLock.Unlock()
 
-	path := os.Getenv("CONFIG_PATH")
-	if path == "" {
-		path = "/models/"
+	path := configPathFor(fname)
+
+	exists, err := fileExists(path)
+	// File Error other than NotExists
+	if err != nil {
+		klog.Infof("Custom Pricing file at path '%s' read error: '%s'", path, err.Error())
+		return DefaultPricing(), err
 	}
-	path += fname
-	if _, err := os.Stat(path); err == nil {
-		jsonFile, err := os.Open(path)
-		if err != nil {
-			return nil, err
-		}
-		defer jsonFile.Close()
-		byteValue, err := ioutil.ReadAll(jsonFile)
-		if err != nil {
-			return nil, err
-		}
-		var customPricing = &CustomPricing{}
-		err = json.Unmarshal([]byte(byteValue), customPricing)
-		if err != nil {
-			return nil, err
-		}
-		return customPricing, nil
-	} else if os.IsNotExist(err) {
-		c := &CustomPricing{
-			Provider:              fname,
-			Description:           "Default prices based on GCP us-central1",
-			CPU:                   "0.031611",
-			SpotCPU:               "0.006655",
-			RAM:                   "0.004237",
-			SpotRAM:               "0.000892",
-			GPU:                   "0.95",
-			Storage:               "0.00005479452",
-			ZoneNetworkEgress:     "0.01",
-			RegionNetworkEgress:   "0.01",
-			InternetNetworkEgress: "0.12",
-			CustomPricesEnabled:   "false",
-		}
+
+	// File Doesn't Exist
+	if !exists {
+		klog.Infof("Could not find Custom Pricing file at path '%s'", path)
+		c := DefaultPricing()
 		cj, err := json.Marshal(c)
 		if err != nil {
-			return nil, err
+			return c, err
 		}
 
 		err = ioutil.WriteFile(path, cj, 0644)
 		if err != nil {
-			return nil, err
+			klog.Infof("Could not write Custom Pricing file to path '%s'", path)
+			return c, err
 		}
+
 		return c, nil
-	} else {
-		return nil, err
 	}
+
+	// File Exists - Read all contents of file, unmarshal json
+	byteValue, err := ioutil.ReadFile(path)
+	if err != nil {
+		klog.Infof("Could not read Custom Pricing file at path %s", path)
+		return DefaultPricing(), err
+	}
+
+	var customPricing CustomPricing
+	err = json.Unmarshal(byteValue, &customPricing)
+	if err != nil {
+		klog.Infof("Could not decode Custom Pricing file at path %s", path)
+		return DefaultPricing(), err
+	}
+
+	return &customPricing, nil
 }
 
 func configmapUpdate(c *CustomPricing, path string, a map[string]string) (*CustomPricing, error) {
@@ -275,17 +286,19 @@ func configmapUpdate(c *CustomPricing, path string, a map[string]string) (*Custo
 		}
 	}
 
-	configLock.Lock()
-	defer configLock.Unlock()
-
 	cj, err := json.Marshal(c)
 	if err != nil {
-		return nil, err
+		return c, err
 	}
+
+	configLock.Lock()
 	err = ioutil.WriteFile(path, cj, 0644)
+	configLock.Unlock()
+
 	if err != nil {
-		return nil, err
+		return c, err
 	}
+
 	return c, nil
 }
 
@@ -433,3 +446,32 @@ func GetOrCreateClusterMeta(cluster_id, cluster_name string) (string, string, er
 
 	return id, name, nil
 }
+
+// File exists has three different return cases that should be handled:
+//   1. File exists and is not a directory (true, nil)
+//   2. File does not exist (false, nil)
+//   3. File may or may not exist. Error occurred during stat (false, error)
+// The third case represents the scenario where the stat returns an error,
+// but the error isn't relevant to the path. This can happen when the current
+// user doesn't have permission to access the file.
+func fileExists(filename string) (bool, error) {
+	info, err := os.Stat(filename)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+
+		return false, err
+	}
+
+	return !info.IsDir(), nil
+}
+
+// Returns the configuration directory concatenated with a specific config file name
+func configPathFor(filename string) string {
+	path := os.Getenv("CONFIG_PATH")
+	if path == "" {
+		path = "/models/"
+	}
+	return path + filename
+}