Merge branch 'master' into user_guide

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")
+```

glustercli/cmd/common.go

@@ -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)
 }

glusterd2/commands/volumes/volume-start.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/volume/volume-utils.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
 }

pkg/api/volume_resp.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.

pkg/restclient/common.go

@@ -1,7 +1,6 @@
 package restclient
 
 import (
- "bytes"
  "crypto/tls"
  "crypto/x509"
  "encoding/json"
@@ -12,8 +11,6 @@ import (
  "strings"
  "time"
 
- "github.com/gluster/glusterd2/pkg/api"
-
  "github.com/dgrijalva/jwt-go"
 )
 
@@ -24,12 +21,20 @@ const (
 
 // Client represents Glusterd2 REST Client
 type Client struct {
- baseURL string
- username string
- password string
- cacert string
- insecure bool
- timeout time.Duration
+ baseURL string
+ username string
+ password string
+ cacert string
+ insecure bool
+ timeout time.Duration
+ lastRespErr *http.Response
+}
+
+// LastErrorResponse returns the last error response received by this
+// client from glusterd2. Please note that the Body of the response has
+// been read and drained.
+func (c *Client) LastErrorResponse() *http.Response {
+ return c.lastRespErr
 }
 
 // SetTimeout sets the overall client timeout which includes the time taken
@@ -51,28 +56,6 @@ func New(baseURL string, username string, password string, cacert string, insecu
  }
 }
 
-func parseHTTPError(jsonData []byte) string {
- var errResp api.ErrorResp
- err := json.Unmarshal(jsonData, &errResp)
- if err != nil {
- return err.Error()
- }
-
- var buffer bytes.Buffer
- for _, apiErr := range errResp.Errors {
- switch api.ErrorCode(apiErr.Code) {
- case api.ErrTxnStepFailed:
- buffer.WriteString(fmt.Sprintf(
- "Transaction step %s failed on peer %s with error: %s\n",
- apiErr.Fields["step"], apiErr.Fields["peer-id"], apiErr.Fields["error"]))
- default:
- buffer.WriteString(apiErr.Message)
- }
- }
-
- return buffer.String()
-}
-
 func getAuthToken(username string, password string) string {
  // Create the Claims
  claims := &jwt.StandardClaims{
@@ -104,12 +87,12 @@ func (c *Client) del(url string, data interface{}, expectStatusCode int, output
  return c.do("DELETE", url, data, expectStatusCode, output)
 }
 
-func (c *Client) do(method string, url string, data interface{}, expectStatusCode int, output interface{}) error {
+func (c *Client) do(method string, url string, input interface{}, expectStatusCode int, output interface{}) error {
  url = fmt.Sprintf("%s%s", c.baseURL, url)
 
  var body io.Reader
- if data != nil {
- reqBody, marshalErr := json.Marshal(data)
+ if input != nil {
+ reqBody, marshalErr := json.Marshal(input)
  if marshalErr != nil {
  return marshalErr
  }
@@ -151,24 +134,32 @@ func (c *Client) do(method string, url string, data interface{}, expectStatusCod
 
  client := &http.Client{
  Transport: tr,
- Timeout: c.timeout}
+ Timeout: c.timeout,
+ }
 
  resp, err := client.Do(req)
  if err != nil {
  return err
  }
-
  defer resp.Body.Close()
- outputRaw, err := ioutil.ReadAll(resp.Body)
+
+ if resp.StatusCode != expectStatusCode {
+ // FIXME: We should may be rather look for 4xx or 5xx series
+ // to determine that we got an error response instead of
+ // comparing to what's expected ?
+ c.lastRespErr = resp
+ return newHTTPErrorResponse(resp)
+ }
+
+ b, err := ioutil.ReadAll(resp.Body)
  if err != nil {
  return err
  }
- if resp.StatusCode != expectStatusCode {
- return &UnexpectedStatusError{"Unexpected Status", expectStatusCode, resp.StatusCode, parseHTTPError(outputRaw)}
- }
 
+ // If a response struct is specified, unmarshall the json response
+ // body into the response struct provided.
  if output != nil {
- return json.Unmarshal(outputRaw, output)
+ return json.Unmarshal(b, output)
  }
 
  return nil