Version checking: clean up code, add server flags

FB-1975
Cleaned up version checking, removed a race condition, and added error checking.
Added server flags for check-in endpoint and UUID storage file.
Incorporates Travis's changes to store UUID file in data directory
and unexport most of verchk.go.

Author: lorycloutier

ctl/server.go

@@ -49,6 +49,8 @@ func serverFlagSet(srv *server.Config, prefix string) *pflag.FlagSet {
 	flags.DurationVar((*time.Duration)(&srv.LongQueryTime), pre("long-query-time"), time.Duration(srv.LongQueryTime), "Duration that will trigger log and stat messages for slow queries. Zero to disable.")
 	flags.IntVar(&srv.QueryHistoryLength, pre("query-history-length"), srv.QueryHistoryLength, "Number of queries to remember in history.")
 	flags.Int64Var(&srv.MaxQueryMemory, pre("max-query-memory"), srv.MaxQueryMemory, "Maximum memory allowed per Extract() or SELECT query.")
+	flags.StringVar(&srv.VerChkAddress, pre("verchk-address"), srv.VerChkAddress, "Address to contact to check for latest version.")
+	flags.StringVar(&srv.UUIDFile, pre("uuid-file"), srv.UUIDFile, "File to store UUID used in checking latest version. If this is a relative path, the file will be stored in the server's data directory.")
 
 	// TLS
 	SetTLSConfig(flags, pre(""), &srv.TLS.CertificatePath, &srv.TLS.CertificateKeyPath, &srv.TLS.CACertPath, &srv.TLS.SkipVerify, &srv.TLS.EnableClientVerification)

ctl/server_test.go

@@ -20,4 +20,10 @@ func TestBuildServerFlags(t *testing.T) {
 	if cm.Flags().Lookup("log-path").Name == "" {
 		t.Fatal("log-path flag is required")
 	}
+	if cm.Flags().Lookup("verchk-address").Name == "" {
+		t.Fatal("verchk-address flag is required")
+	}
+	if cm.Flags().Lookup("uuid-file").Name == "" {
+		t.Fatal("uuid-file flag is required")
+	}
 }

server.go

@@ -89,6 +89,8 @@ type Server struct { // nolint: maligned
 
 	defaultClient *InternalClient
 	dataDir       string
+	verChkAddress string
+	uuidFile      string
 
 	// Threshold for logging long-running queries
 	longQueryTime      time.Duration
@@ -167,6 +169,24 @@ func OptServerDataDir(dir string) ServerOption {
 	}
 }
 
+// OptServerVerChkAddress is a functional option on Server
+// used to set the address to check for the current version.
+func OptServerVerChkAddress(addr string) ServerOption {
+	return func(s *Server) error {
+		s.verChkAddress = addr
+		return nil
+	}
+}
+
+// OptServerUUIDFile is a functional option on Server
+// used to set the file name for storing the checkin UUID.
+func OptServerUUIDFile(uf string) ServerOption {
+	return func(s *Server) error {
+		s.uuidFile = uf
+		return nil
+	}
+}
+
 // OptServerViewsRemovalInterval is a functional option on Server
 // used to set the ttl removal interval.
 func OptServerViewsRemovalInterval(interval time.Duration) ServerOption {
@@ -608,13 +628,17 @@ func (s *Server) Open() error {
 	// startup if the server endpoint is down/having issues.
 	go func() {
 		s.logger.Printf("Beginning featurebase version check-in")
-		vc := newVersionChecker(s.cluster.Path, "https://analytics.featurebase.com/v2/featurebase/metrics")
+		vc := newVersionChecker(s.cluster.Path, s.verChkAddress, s.uuidFile)
 		resp, err := vc.checkIn()
 		if err != nil {
 			s.logger.Errorf("doing version checkin. Error was %s", err)
 			return
 		}
-		s.logger.Printf("Version check-in complete. Latest version is %s", resp.Version)
+		if resp.Error != "" {
+			s.logger.Printf("Version check-in failed, endpoint response was %s", resp.Error)
+		} else {
+			s.logger.Printf("Version check-in complete. Latest version is %s", resp.Version)
+		}
 	}()
 
 	// Start DisCo.

server/config.go

@@ -152,6 +152,15 @@ type Config struct {
 	// Limits the total amount of memory to be used by Extract() & SELECT queries.
 	MaxQueryMemory int64 `toml:"max-query-memory"`
 
+	// On startup, featurebase server contacts a web server to check the latest version.
+	// This stores the address for that check
+	VerChkAddress string `toml:"verchk-address"`
+
+	// When checking version, server sends a UUID so that we can keep track of
+	// how many unique Featurebase installs are out there. The file is stored in
+	// the data directory; this stores the filename to use.
+	UUIDFile string `toml:"uuid-file"`
+
 	Cluster struct {
 		ReplicaN int    `toml:"replicas"`
 		Name     string `toml:"name"`
@@ -383,6 +392,8 @@ func NewConfig() *Config {
 		LongQueryTime: toml.Duration(-time.Minute),
 
 		CheckInInterval: 60 * time.Second,
+		VerChkAddress:   "https://analytics.featurebase.com/v2/featurebase/metrics",
+		UUIDFile:        ".client_id.txt",
 	}
 
 	// Cluster config.

server/server.go

@@ -596,6 +596,8 @@ func (m *Command) setupServer() error {
 		pilosa.OptServerServerlessStorage(m.serverlessStorage),
 		pilosa.OptServerIsDataframeEnabled(m.Config.Dataframe.Enable),
 		pilosa.OptServerDataframeUseParquet(m.Config.Dataframe.UseParquet),
+		pilosa.OptServerVerChkAddress(m.Config.VerChkAddress),
+		pilosa.OptServerUUIDFile(m.Config.UUIDFile),
 	}
 
 	if m.isComputeNode {

verchk.go

@@ -12,19 +12,17 @@ import (
 	"github.com/google/uuid"
 )
 
-const (
-	versionCheckerFilename = ".client_id.txt"
-)
-
 type versionChecker struct {
-	path string
-	url  string
+	path   string
+	url    string
+	idfile string
 }
 
-func newVersionChecker(path, endpoint string) *versionChecker {
+func newVersionChecker(path, endpoint, idfile string) *versionChecker {
 	v := versionChecker{
-		path: path,
-		url:  endpoint,
+		path:   path,
+		url:    endpoint,
+		idfile: idfile,
 	}
 	return &v
 }
@@ -62,7 +60,6 @@ func (v *versionChecker) checkIn() (*verCheckResponse, error) {
 
 		return nil, err
 	}
-
 	return &jsonResp, nil
 }
 
@@ -73,31 +70,15 @@ func (v *versionChecker) generateClientUUID() (string, error) {
 }
 
 func (v *versionChecker) writeClientUUID() (string, error) {
-	filename := filepath.Join(v.path, versionCheckerFilename)
-	if _, err := os.Stat(filename); err != nil {
-		if os.IsNotExist(err) {
-			fh, err := os.Create(filename)
-			if err != nil {
-				return "", err
-			}
-			defer fh.Close()
-
-			id, err := v.generateClientUUID()
-			if err != nil {
-				return "", err
-			}
-
-			if _, err := fh.WriteString(id); err != nil {
-				return "", err
-			}
-
-			return id, nil
-		} else {
-			return "", err
-		}
+	var filename string
+	// if v.idfile starts with a path separator then it's an absolute path
+	// otherwise it's a relative path and the file goes in the data directory
+	if v.idfile[0] == os.PathSeparator {
+		filename = v.idfile
+	} else {
+		filename = filepath.Join(v.path, v.idfile)
 	}
-
-	fh, err := os.Open(filename)
+	fh, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0744)
 	if err != nil {
 		return "", err
 	}
@@ -107,10 +88,24 @@ func (v *versionChecker) writeClientUUID() (string, error) {
 	if err != nil {
 		return "", err
 	}
+	// this just checks to see if there was anything at all in the file.
+	// we should probably check to make sure it's a valid UUID
+	if string(buf) == "" {
+		id, err := v.generateClientUUID()
+		if err != nil {
+			return "", err
+		}
+		_, err = fh.WriteString(id)
+		if err != nil {
+			return "", err
+		}
+		return id, nil
+	}
 
 	return string(buf), nil
 }
 
 type verCheckResponse struct {
 	Version string `json:"latest_version"`
+	Error   string `json:"error"`
 }