Fix Peter's comments

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

Author: Ganesh Vernekar

pkg/ring/lifecycler.go

@@ -261,8 +261,10 @@ func (i *Lifecycler) setTokens(tokens Tokens) {
 	defer i.stateMtx.Unlock()
 
 	i.tokens = tokens
-	if err := i.tokens.StoreToFile(i.cfg.TokensFilePath); err != nil {
-		level.Error(util.Logger).Log("msg", "error storing tokens to disk", "path", i.cfg.TokensFilePath, "err", err)
+	if i.cfg.TokensFilePath != "" {
+		if err := i.tokens.StoreToFile(i.cfg.TokensFilePath); err != nil {
+			level.Error(util.Logger).Log("msg", "error storing tokens to disk", "path", i.cfg.TokensFilePath, "err", err)
+		}
 	}
 }
 
@@ -451,9 +453,22 @@ heartbeatLoop:
 // - add an ingester entry to the ring
 // - copies out our state and tokens if they exist
 func (i *Lifecycler) initRing(ctx context.Context) error {
-	var ringDesc *Desc
+	var (
+		ringDesc       *Desc
+		tokensFromFile Tokens
+		err            error
+	)
+
+	if i.cfg.TokensFilePath != "" {
+		tokensFromFile, err = LoadTokensFromFile(i.cfg.TokensFilePath)
+		if err != nil {
+			level.Error(util.Logger).Log("msg", "error in getting tokens from file", "err", err)
+		}
+	} else {
+		level.Warn(util.Logger).Log("msg", "not loading tokens from file, tokens file path is empty")
+	}
 
-	err := i.KVStore.CAS(ctx, ConsulKey, func(in interface{}) (out interface{}, retry bool, err error) {
+	err = i.KVStore.CAS(ctx, ConsulKey, func(in interface{}) (out interface{}, retry bool, err error) {
 		if in == nil {
 			ringDesc = NewDesc()
 		} else {
@@ -462,18 +477,14 @@ func (i *Lifecycler) initRing(ctx context.Context) error {
 
 		ingesterDesc, ok := ringDesc.Ingesters[i.ID]
 		if !ok {
-			var tokens Tokens
-			// We load the tokens from the file only if it does not exist in the ring yet.
-			err := tokens.LoadFromFile(i.cfg.TokensFilePath)
-			if err != nil {
-				level.Error(util.Logger).Log("msg", "error in getting tokens from file", "err", err)
-			} else if len(tokens) > 0 {
-				level.Info(util.Logger).Log("msg", "adding tokens from file", "num_tokens", len(tokens))
-				if len(tokens) == i.cfg.NumTokens {
+			// We use the tokens from the file only if it does not exist in the ring yet.
+			if len(tokensFromFile) > 0 {
+				level.Info(util.Logger).Log("msg", "adding tokens from file", "num_tokens", len(tokensFromFile))
+				if len(tokensFromFile) == i.cfg.NumTokens {
 					i.setState(ACTIVE)
 				}
-				ringDesc.AddIngester(i.ID, i.Addr, tokens, i.GetState(), i.cfg.NormaliseTokens)
-				i.setTokens(tokens)
+				ringDesc.AddIngester(i.ID, i.Addr, tokensFromFile, i.GetState(), i.cfg.NormaliseTokens)
+				i.setTokens(tokensFromFile)
 				return ringDesc, true, nil
 			}
 

pkg/ring/lifecycler_test.go

@@ -289,7 +289,7 @@ func TestTokensOnDisk(t *testing.T) {
 
 	lifecyclerConfig := testLifecyclerConfig(ringConfig, "ing1")
 	lifecyclerConfig.NumTokens = 512
-	lifecyclerConfig.TokensFilePath = tokenDir
+	lifecyclerConfig.TokensFilePath = tokenDir + "/tokens"
 	lifecyclerConfig.NormaliseTokens = true
 
 	// Start first ingester.

pkg/ring/tokens.go

@@ -5,15 +5,6 @@ import (
 	"errors"
 	"io/ioutil"
 	"os"
-
-	"github.com/cortexproject/cortex/pkg/util"
-	"github.com/go-kit/kit/log/level"
-	"github.com/prometheus/prometheus/tsdb/fileutil"
-)
-
-const (
-	// TokensVersion1 is the version is a simple list of tokens.
-	TokensVersion1 = 1
 )
 
 // Tokens is a simple list of tokens.
@@ -25,6 +16,8 @@ func (t Tokens) StoreToFile(tokenFilePath string) error {
 		return errors.New("path is empty")
 	}
 
+	// If any operations failed further in the function, we keep the temporary
+	// file hanging around for debugging.
 	f, err := ioutil.TempFile(os.TempDir(), "tokens")
 	if err != nil {
 		return err
@@ -35,10 +28,6 @@ func (t Tokens) StoreToFile(tokenFilePath string) error {
 		// the error (if any) from f.Close(). If the file was already closed, then
 		// we would ignore the error in that case too.
 		f.Close()
-		// RemoveAll returns no error when tmp doesn't exist so it is safe to always run it.
-		if err := os.RemoveAll(f.Name()); err != nil {
-			level.Warn(util.Logger).Log("msg", "error deleting temporary file", "err", err)
-		}
 	}()
 
 	b, err := t.Marshal()
@@ -53,29 +42,27 @@ func (t Tokens) StoreToFile(tokenFilePath string) error {
 		return err
 	}
 
-	// Block successfully written, make visible and remove old ones.
-	return fileutil.Replace(f.Name(), tokenFilePath)
+	// Tokens successfully written, replace the temporary file with the actual file path.
+	return os.Rename(f.Name(), tokenFilePath)
 }
 
-// LoadFromFile loads tokens from given directory.
-func (t *Tokens) LoadFromFile(tokenFilePath string) error {
+// LoadTokensFromFile loads tokens from given file path.
+func LoadTokensFromFile(tokenFilePath string) (Tokens, error) {
 	b, err := ioutil.ReadFile(tokenFilePath)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return nil
+			return nil, nil
 		}
-		return err
+		return nil, err
 	}
-	return t.Unmarshal(b)
+	var t Tokens
+	err = t.Unmarshal(b)
+	return t, err
 }
 
 // Marshal encodes the tokens into JSON.
 func (t Tokens) Marshal() ([]byte, error) {
-	data := tokensJSON{
-		Version: TokensVersion1,
-		Tokens:  t,
-	}
-	return json.Marshal(data)
+	return json.Marshal(tokensJSON{Tokens: t})
 }
 
 // Unmarshal reads the tokens from JSON byte stream.
@@ -84,16 +71,10 @@ func (t *Tokens) Unmarshal(b []byte) error {
 	if err := json.Unmarshal(b, &tj); err != nil {
 		return err
 	}
-	switch tj.Version {
-	case TokensVersion1:
-		*t = Tokens(tj.Tokens)
-		return nil
-	default:
-		return errors.New("invalid token version")
-	}
+	*t = Tokens(tj.Tokens)
+	return nil
 }
 
 type tokensJSON struct {
-	Version int      `json:"version"`
-	Tokens  []uint32 `json:"tokens"`
+	Tokens []uint32 `json:"tokens"`
 }