Merge branch 'master' of https://github.com/gluster/glusterd2 into user_guide

Author: rishubhjain

.gitignore

@@ -22,4 +22,4 @@ VERSION
 endpoints.json
 generate-doc
 # Ignore vi backup files
-*~
+*~

doc/coding.md

@@ -2,12 +2,15 @@
 
 Please follow coding conventions and guidelines described in the following documents:
 
+* [Go proverbs](https://go-proverbs.github.io/) - highly recommended read
 * [CodeReviewComments](https://github.com/golang/go/wiki/CodeReviewComments)
 * [Effective Go](https://golang.org/doc/effective_go.html)
+* [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/)
 
-### Some more conventions
+Here's a list of some more specific conventions that are often followed in
+the code and will be pointed out in the review process:
 
-**General:**
+### General
 * Keep variable names short for variables that are local to the function.
 * Do not export a function or variable name outside the package until you
   have an external consumer for it.
@@ -16,7 +19,7 @@ Please follow coding conventions and guidelines described in the following docum
 * Do not use named return values in function definitions. Use only the type.
   Exception: defer()'d functions.
 
-**Imports:**
+### Imports
 
 We use the following convention for specifying imports:
 
@@ -48,22 +51,22 @@ import (
 )
 ```
 
-**Error Handling:**
+### Error Handling
 
 * Use variable name `err` to denote error variable during a function call.
 * Reuse the previously declared `err` variable as long as it is in scope.
   For example, do not use `errWrite` or `errRead`.
-* Do not panic().
+* Do not panic() for errors that can be bubbled up back to user. Use panic()
+  only for fatal errors which shouldn't occur.
 * Do not ignore errors using `_` variable unless you know what you're doing.
 * Error strings should not start with a capital letter.
 * If error requires passing of extra information, you can define a new type
-* Error types should end in `Error` and error variables should have `Err` as
-  prefix.
+* Error types should end with `Error`.
 
-**Logging:**
+### Logging
 
 * If a function is only invoked as part of a transaction step, always use the
-  transaction's logger.
+  transaction's logger to ensure propagation of request ID and transaction ID.
 * The inner-most utility functions should never log. Logging must almost always
   be done by the caller on receiving an `error`.
 * Always use log level `DEBUG` to provide useful **diagnostic information** to
@@ -75,3 +78,56 @@ import (
   or retry for it and/or is fully recoverable.
 * Use log level `ERROR` when something occurs which is fatal to the operation,
   but not to the service or application.
+
+### Use of goto
+
+Use of `goto` is generally frowned up on in higher level languages. We use
+`goto` statements for the following specific uses:
+* Ensure RPCs always return a reply to caller
+* Getting out of nested loops
+* Auto-generated code
+
+Please use `defer()` for ensuring that relevant resource cleanups happen when a
+function/method exits. Also use `defer()` to revert something on a later
+failure.
+
+Developers with significant experience in C should be careful not to
+excessively use `goto` just to ensure single exit point to a function. Unlike
+C programs, there is no memory to be free()d here. Care must be taken when one
+ports code from glusterd1 (c) to glusterd2 (go).
+
+### glusterd2 specific conventions
+
+**Do not log at the caller of `txn.*` methods:**
+
+Certain patterns repeat very often in codebase. For reducing clutter, we have
+moved some of the logging at the caller to inside the function. One such
+instance are the methods of `transaction.Context` interface, which are used at
+so many places. They log internally and caller shouldn't be logging. For
+example:
+
+```go
+if err := txn.Ctx.Set("req", &req); err != nil {
+        // do NOT log here
+        restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
+        return
+}
+```
+
+**Use log.WithError() to log errors**
+
+It is common pattern to log the error received as a field in the log entry.
+Please use `WithError` for the same:
+
+Do this:
+```go
+log.WithError(err).WithField("path", path).Error("Failed to delete path")
+```
+
+Do NOT do this:
+```go
+        log.WithFields(log.Fields{
+                "error": err,
+                "path":  path,
+        }).Error("Failed to delete path")
+```

doc/endpoints.md

@@ -49,12 +49,26 @@ func handleGlusterdConnectFailure(msg, endpoints string, err error, errcode int)
 }
 
 func failure(msg string, err error, errcode int) {
+
 	handleGlusterdConnectFailure(msg, flagEndpoints[0], err, errcode)
 
-	// If different error
-	os.Stderr.WriteString(msg + "\n")
+	w := os.Stderr
+
+	w.WriteString(msg + "\n")
+
 	if err != nil {
-		os.Stderr.WriteString("\nError: " + err.Error() + "\n")
+		resp := client.LastErrorResponse()
+
+		w.WriteString("\nResponse headers:\n")
+		for k, v := range resp.Header {
+			if strings.HasSuffix(k, "-Id") {
+				w.WriteString(fmt.Sprintf("%s: %s\n", k, v[0]))
+			}
+		}
+
+		w.WriteString("\nResponse body:\n")
+		w.WriteString(fmt.Sprintf("%s\n", err.Error()))
 	}
+
 	os.Exit(errcode)
 }

glustercli/cmd/common.go

@@ -24,6 +24,7 @@ func volumeListHandler(w http.ResponseWriter, r *http.Request) {
 	volumes, err := volume.GetVolumes(filterParams)
 	if err != nil {
 		restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
+		return
 	}
 	resp := createVolumeListResp(volumes)
 	restutils.SendHTTPResponse(ctx, w, http.StatusOK, resp)

glusterd2/commands/volumes/volume-list.go

@@ -1,6 +1,7 @@
 package volumecommands
 
 import (
+	"io"
 	"net/http"
 
 	"github.com/gluster/glusterd2/glusterd2/events"
@@ -76,7 +77,8 @@ func volumeStartHandler(w http.ResponseWriter, r *http.Request) {
 	volname := mux.Vars(r)["volname"]
 	var req api.VolumeStartReq
 
-	if err := restutils.UnmarshalRequest(r, &req); err != nil {
+	// request body is optional
+	if err := restutils.UnmarshalRequest(r, &req); err != nil && err != io.EOF {
 		restutils.SendHTTPError(ctx, w, http.StatusBadRequest, err)
 		return
 	}

glusterd2/commands/volumes/volume-start.go

@@ -1,6 +1,7 @@
 package events
 
 import (
+	"errors"
 	"strings"
 	"sync"
 
@@ -26,7 +27,6 @@ var lWatcher *livenessWatcher
 // broadcasts this information locally.
 func (l *livenessWatcher) Watch() {
 	defer l.wg.Done()
-
 	wch := store.Store.Watch(store.Store.Ctx(), store.LivenessKeyPrefix,
 		clientv3.WithPrefix(), clientv3.WithKeysOnly())
 	for {
@@ -65,15 +65,28 @@ func (l *livenessWatcher) Watch() {
 	}
 }
 
+//Stop will stop the livenessWatcher if it is running and waits for
+//it to exit.
+func (l *livenessWatcher) Stop() error {
+	if l.stopCh == nil {
+		return errors.New("livness watcher has not been started")
+	}
+	close(l.stopCh)
+	l.stopCh = nil
+	l.wg.Wait()
+	return nil
+}
+
 func startLivenessWatcher() {
 	lWatcher = &livenessWatcher{
-		stopCh: make(chan struct{}, 0),
+		stopCh: make(chan struct{}),
 	}
 	lWatcher.wg.Add(1)
 	go lWatcher.Watch()
 }
 
 func stopLivenessWatcher() {
-	close(lWatcher.stopCh)
-	lWatcher.wg.Wait()
+	if err := lWatcher.Stop(); err != nil {
+		log.WithError(err).Errorf("got error in stopping liveness watcher")
+	}
 }

glusterd2/events/liveness.go

@@ -162,7 +162,7 @@ func GetBrickMountInfo(mountRoot string) (*Mntent, error) {
 
 }
 
-//CreateSubvolInfo parses subvol  information for response
+//CreateSubvolInfo parses subvol information for response
 func CreateSubvolInfo(sv *[]Subvol) []api.Subvol {
 	var subvols []api.Subvol
 
@@ -173,20 +173,21 @@ func CreateSubvolInfo(sv *[]Subvol) []api.Subvol {
 		}
 
 		subvols = append(subvols, api.Subvol{
-			Name:         subvol.Name,
-			Type:         api.SubvolType(subvol.Type),
-			Bricks:       blist,
-			ReplicaCount: subvol.ReplicaCount,
-			ArbiterCount: subvol.ArbiterCount,
+			Name:          subvol.Name,
+			Type:          api.SubvolType(subvol.Type),
+			Bricks:        blist,
+			ReplicaCount:  subvol.ReplicaCount,
+			ArbiterCount:  subvol.ArbiterCount,
+			DisperseCount: subvol.DisperseCount,
 		})
 	}
 	return subvols
 }
 
-//CreateVolumeInfoResp parses volume  information for response
+//CreateVolumeInfoResp parses volume information for response
 func CreateVolumeInfoResp(v *Volinfo) *api.VolumeInfo {
 
-	return &api.VolumeInfo{
+	resp := &api.VolumeInfo{
 		ID:        v.ID,
 		Name:      v.Name,
 		Type:      api.VolType(v.Type),
@@ -198,4 +199,13 @@ func CreateVolumeInfoResp(v *Volinfo) *api.VolumeInfo {
 		Metadata:  v.Metadata,
 		SnapList:  v.SnapList,
 	}
+
+	// for common use cases, replica count of the volume is usually the
+	// replica count of any one of the subvols and we take replica count
+	// from the first subvol
+	resp.ReplicaCount = resp.Subvols[0].ReplicaCount
+	resp.ArbiterCount = resp.Subvols[0].ArbiterCount
+	resp.DisperseCount = resp.Subvols[0].DisperseCount
+
+	return resp
 }

glusterd2/volume/volume-utils.go

@@ -16,12 +16,13 @@ type BrickInfo struct {
 
 // Subvol contains static information about sub volume
 type Subvol struct {
-	Name         string      `json:"name"`
-	Type         SubvolType  `json:"type"`
-	Bricks       []BrickInfo `json:"bricks"`
-	Subvols      []Subvol    `json:"subvols,omitempty"`
-	ReplicaCount int         `json:"replica-count"`
-	ArbiterCount int         `json:"arbiter-count"`
+	Name          string      `json:"name"`
+	Type          SubvolType  `json:"type"`
+	Bricks        []BrickInfo `json:"bricks"`
+	Subvols       []Subvol    `json:"subvols,omitempty"`
+	ReplicaCount  int         `json:"replica-count"`
+	ArbiterCount  int         `json:"arbiter-count"`
+	DisperseCount int         `json:"disperse-count"`
 }
 
 // SizeInfo represents sizing information.
@@ -52,18 +53,19 @@ type BricksStatusResp []BrickStatus
 // VolumeInfo contains static information about the volume.
 // Clients should NOT use this struct directly.
 type VolumeInfo struct {
-	ID           uuid.UUID         `json:"id"`
-	Name         string            `json:"name"`
-	Type         VolType           `json:"type"`
-	Transport    string            `json:"transport"`
-	DistCount    int               `json:"distribute-count"`
-	ReplicaCount int               `json:"replica-count"`
-	ArbiterCount int               `json:"arbiter-count"`
-	Options      map[string]string `json:"options"`
-	State        VolState          `json:"state"`
-	Subvols      []Subvol          `json:"subvols"`
-	Metadata     map[string]string `json:"metadata"`
-	SnapList     []string          `json:"snap-list"`
+	ID            uuid.UUID         `json:"id"`
+	Name          string            `json:"name"`
+	Type          VolType           `json:"type"`
+	Transport     string            `json:"transport"`
+	DistCount     int               `json:"distribute-count"`
+	ReplicaCount  int               `json:"replica-count"`
+	ArbiterCount  int               `json:"arbiter-count,omitempty"`
+	DisperseCount int               `json:"disperse-count,omitempty"`
+	Options       map[string]string `json:"options"`
+	State         VolState          `json:"state"`
+	Subvols       []Subvol          `json:"subvols"`
+	Metadata      map[string]string `json:"metadata"`
+	SnapList      []string          `json:"snap-list"`
 }
 
 // VolumeStatusResp response contains the statuses of all bricks of the volume.