Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Commit

Permalink
bitrot plugin code clean up
Browse files Browse the repository at this point in the history 
done code clean up where its required in this plugin.

updated code for missing error handling,

if called function is returning an error,
instead of logging with the logger.WithFileds() log with
logger.Error()

Signed-off-by: Madhu Rajanna <mrajanna@redhat.com>
  • Loading branch information
@Madhu-1 @prashanthpai
Madhu-1 authored and prashanthpai committed Aug 8, 2018
1 parent 660b029 commit a848a87
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 136 deletions.
21 changes: 12 additions & 9 deletions plugins/bitrot/bitd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,24 +65,27 @@ func newBitd() (*Bitd, error) {
return nil, e
}

b := &Bitd{binarypath: binarypath}
b.volfileID = gdctx.MyUUID.String() + "-gluster/bitd"
b.logfile = path.Join(config.GetString("logdir"), "glusterfs", "bitd.log")

// Create pidFiledir dir
pidFileDir := fmt.Sprintf("%s/bitd", config.GetString("rundir"))
e = os.MkdirAll(pidFileDir, os.ModeDir|os.ModePerm)
if e = os.MkdirAll(pidFileDir, os.ModeDir|os.ModePerm); e != nil {
return nil, e
}
shost, _, e := net.SplitHostPort(config.GetString("clientaddress"))
if e != nil {
return nil, e
}
b.pidfilepath = fmt.Sprintf("%s/bitd.pid", pidFileDir)
b.socketfilepath = b.SocketFile()

shost, _, _ := net.SplitHostPort(config.GetString("clientaddress"))
if shost == "" {
shost = "localhost"
}

b := &Bitd{
binarypath: binarypath,
volfileID: gdctx.MyUUID.String() + "-gluster/bitd",
logfile: path.Join(config.GetString("logdir"), "glusterfs", "bitd.log"),
pidfilepath: fmt.Sprintf("%s/bitd.pid", pidFileDir),
}

b.socketfilepath = b.SocketFile()
b.args = []string{}
b.args = append(b.args, "-s", shost)
b.args = append(b.args, "--volfile-id", b.volfileID)
Expand Down
118 changes: 50 additions & 68 deletions plugins/bitrot/rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,18 @@ import (
"github.com/gluster/glusterd2/pkg/errors"
bitrotapi "github.com/gluster/glusterd2/plugins/bitrot/api"
"github.com/gorilla/mux"
log "github.com/sirupsen/logrus"

"github.com/pborman/uuid"
)

func bitrotEnableHandler(w http.ResponseWriter, r *http.Request) {
// Collect inputs from URL
p := mux.Vars(r)
volName := p["volname"]
volname := mux.Vars(r)["volname"]

ctx := r.Context()
logger := gdctx.GetReqLogger(ctx)

txn, err := transaction.NewTxnWithLocks(ctx, volName)
txn, err := transaction.NewTxnWithLocks(ctx, volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -34,7 +32,7 @@ func bitrotEnableHandler(w http.ResponseWriter, r *http.Request) {
defer txn.Done()

// Validate volume existence
volinfo, err := volume.GetVolume(volName)
volinfo, err := volume.GetVolume(volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -54,7 +52,6 @@ func bitrotEnableHandler(w http.ResponseWriter, r *http.Request) {
}
//save volume information for transaction failure scenario
if err := txn.Ctx.Set("oldvolinfo", volinfo); err != nil {
logger.WithError(err).Error("failed to set oldvolinfo in transaction context")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}
Expand All @@ -66,7 +63,6 @@ func bitrotEnableHandler(w http.ResponseWriter, r *http.Request) {
volinfo.Options[keyFeaturesScrub] = "true"

if err := txn.Ctx.Set("volinfo", volinfo); err != nil {
logger.WithError(err).Error("failed to set volinfo in transaction context")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}
Expand All @@ -89,31 +85,27 @@ func bitrotEnableHandler(w http.ResponseWriter, r *http.Request) {
},
}

err = txn.Do()
if err != nil {
if err = txn.Do(); err != nil {
/* TODO: Need to handle failure case. Unlike other daemons,
* bitrot daemon is one per node and depends on volfile change.
* Need to handle scenarios where bitrot enable is succeeded in
* few nodes and failed in few others */
logger.WithFields(log.Fields{
"error": err.Error(),
"volname": volName,
}).Error("failed to enable bitrot")
logger.WithError(err).WithField("volname",
volinfo.Name).Error("failed to enable bitrot")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}
restutils.SendHTTPResponse(ctx, w, http.StatusOK, "Bitrot enabled successfully")
restutils.SendHTTPResponse(ctx, w, http.StatusOK, nil)
}

func bitrotDisableHandler(w http.ResponseWriter, r *http.Request) {
// Collect inputs from URL
p := mux.Vars(r)
volName := p["volname"]
volname := mux.Vars(r)["volname"]

ctx := r.Context()
logger := gdctx.GetReqLogger(ctx)

txn, err := transaction.NewTxnWithLocks(ctx, volName)
txn, err := transaction.NewTxnWithLocks(ctx, volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -122,7 +114,7 @@ func bitrotDisableHandler(w http.ResponseWriter, r *http.Request) {
defer txn.Done()

// Validate volume existence
volinfo, err := volume.GetVolume(volName)
volinfo, err := volume.GetVolume(volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -141,7 +133,6 @@ func bitrotDisableHandler(w http.ResponseWriter, r *http.Request) {
volinfo.Options[keyFeaturesScrub] = "false"

if err := txn.Ctx.Set("volinfo", volinfo); err != nil {
logger.WithError(err).Error("failed to set volinfo in transaction context")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}
Expand All @@ -163,29 +154,24 @@ func bitrotDisableHandler(w http.ResponseWriter, r *http.Request) {
},
}

err = txn.Do()
if err != nil {
if err = txn.Do(); err != nil {
// TODO: Handle rollback
logger.WithFields(log.Fields{
"error": err.Error(),
"volname": volName,
}).Error("failed to disable bitrot")
logger.WithError(err).WithField("volname",
volinfo.Name).Error("failed to disable bitrot")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}

restutils.SendHTTPResponse(ctx, w, http.StatusOK, "Bitrot disabled successfully")
restutils.SendHTTPResponse(ctx, w, http.StatusOK, nil)
}

func bitrotScrubOndemandHandler(w http.ResponseWriter, r *http.Request) {
// Collect inputs from URL
p := mux.Vars(r)
volName := p["volname"]
volname := mux.Vars(r)["volname"]

ctx := r.Context()
logger := gdctx.GetReqLogger(ctx)

txn, err := transaction.NewTxnWithLocks(ctx, volName)
txn, err := transaction.NewTxnWithLocks(ctx, volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -194,7 +180,7 @@ func bitrotScrubOndemandHandler(w http.ResponseWriter, r *http.Request) {
defer txn.Done()

// Validate volume existence
volinfo, err := volume.GetVolume(volName)
volinfo, err := volume.GetVolume(volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -220,29 +206,28 @@ func bitrotScrubOndemandHandler(w http.ResponseWriter, r *http.Request) {
Nodes: txn.Nodes,
},
}
txn.Ctx.Set("volname", volName)
if err = txn.Ctx.Set("volname", volname); err != nil {
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}

err = txn.Do()
if err != nil {
logger.WithFields(log.Fields{
"error": err.Error(),
"volname": volName,
}).Error("failed to start scrubber")
if err = txn.Do(); err != nil {
logger.WithError(err).WithField("volname",
volinfo.Name).Error("failed to start scrubber")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}
restutils.SendHTTPResponse(ctx, w, http.StatusOK, "Scrubber started successfully")
restutils.SendHTTPResponse(ctx, w, http.StatusOK, nil)
}

func bitrotScrubStatusHandler(w http.ResponseWriter, r *http.Request) {
// Collect inputs from URL
p := mux.Vars(r)
volName := p["volname"]
volname := mux.Vars(r)["volname"]

ctx := r.Context()
logger := gdctx.GetReqLogger(ctx)

txn, err := transaction.NewTxnWithLocks(ctx, volName)
txn, err := transaction.NewTxnWithLocks(ctx, volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand All @@ -251,7 +236,7 @@ func bitrotScrubStatusHandler(w http.ResponseWriter, r *http.Request) {
defer txn.Done()

// Validate volume existence
volinfo, err := volume.GetVolume(volName)
volinfo, err := volume.GetVolume(volname)
if err != nil {
status, err := restutils.ErrToStatusCode(err)
restutils.SendHTTPError(ctx, w, status, err)
Expand Down Expand Up @@ -283,25 +268,24 @@ func bitrotScrubStatusHandler(w http.ResponseWriter, r *http.Request) {
Nodes: txn.Nodes,
},
}
txn.Ctx.Set("volname", volName)
if err = txn.Ctx.Set("volname", volname); err != nil {
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}

err = txn.Do()
if err != nil {
logger.WithFields(log.Fields{
"error": err.Error(),
"volname": volName,
}).Error("failed to get scrubber status")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError,
err)
if err = txn.Do(); err != nil {
logger.WithError(err).WithField("volname",
volinfo.Name).Error("failed to get scrubber status")
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err)
return
}

result, err := createScrubStatusResp(txn.Ctx, volinfo)
if err != nil {
errMsg := "Failed to aggregate scrub status results from multiple nodes."
logger.WithField("error", err.Error()).Error("bitrotScrubStatusHandler:" + errMsg)
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError,
errMsg)
errMsg := "failed to aggregate scrub status results from multiple nodes"
logger.WithError(err).WithField("volname",
volinfo.Name).Error("bitrotScrubStatusHandler:" + errMsg)
restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, errMsg)
return
}
restutils.SendHTTPResponse(ctx, w, http.StatusOK, result)
Expand All @@ -311,7 +295,6 @@ func createScrubStatusResp(ctx transaction.TxnCtx, volinfo *volume.Volinfo) (*bi

var resp bitrotapi.ScrubStatus
var exists bool

// Fill generic info which are same for each node
resp.Volume = volinfo.Name
resp.State = "Active (Idle)"
Expand All @@ -320,10 +303,8 @@ func createScrubStatusResp(ctx transaction.TxnCtx, volinfo *volume.Volinfo) (*bi
// If not available in Options, it's not set. Use default value
opt, err := xlator.FindOption(keyScrubFrequency)
if err != nil {
log.WithFields(log.Fields{
"error": err.Error(),
"volname": volinfo.Name,
}).Error("failed to get scrub-freq option")
ctx.Logger().WithError(err).WithField("volname",
volinfo.Name).Error("failed to get scrub-freq option")
return &resp, err
}
resp.Frequency = opt.DefaultValue
Expand All @@ -334,26 +315,26 @@ func createScrubStatusResp(ctx transaction.TxnCtx, volinfo *volume.Volinfo) (*bi
// If not available in Options, it's not set. Use default value
opt, err := xlator.FindOption(keyScrubThrottle)
if err != nil {
log.WithFields(log.Fields{
"error": err.Error(),
"volname": volinfo.Name,
}).Error("failed to get scrub-throttle option")
ctx.Logger().WithError(err).WithField("volname",
volinfo.Name).Error("failed to get scrub-throttle option")
return &resp, err
}
resp.Throttle = opt.DefaultValue
}
//Bitd log file
bitrotDaemon, err := newBitd()
if err != nil {
log.WithError(err).Error("Failed to create Bitd instance")
ctx.Logger().WithError(err).WithField("volname",
volinfo.Name).Error("Failed to create Bitd instance")
return &resp, err
}
resp.BitdLogFile = bitrotDaemon.logfile

//Scrub log file
scrubDaemon, err := newScrubd()
if err != nil {
log.WithError(err).Error("Failed to create Scrubd instance")
ctx.Logger().WithError(err).WithField("volname",
volinfo.Name).Error("Failed to create Scrubd instance")
return &resp, err
}
resp.ScrubLogFile = scrubDaemon.logfile
Expand All @@ -370,7 +351,8 @@ func createScrubStatusResp(ctx transaction.TxnCtx, volinfo *volume.Volinfo) (*bi

scrubRunning, err := strconv.Atoi(tmp.ScrubRunning)
if err != nil {
log.WithError(err).Error("strconv of ScrubRunning failed")
ctx.Logger().WithError(err).WithField("volname",
volinfo.Name).Error("strconv of ScrubRunning failed")
return &resp, err
}
if scrubRunning == 1 {
Expand Down
22 changes: 12 additions & 10 deletions plugins/bitrot/scrubd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,27 @@ func newScrubd() (*Scrubd, error) {
if e != nil {
return nil, e
}

s := &Scrubd{binarypath: binarypath}
s.volfileID = gdctx.MyUUID.String() + "-gluster/scrub"
s.logfile = path.Join(config.GetString("logdir"), "glusterfs", "scrub.log")

// Create pidFiledir dir
pidFileDir := fmt.Sprintf("%s/scrub", config.GetString("rundir"))
e = os.MkdirAll(pidFileDir, os.ModeDir|os.ModePerm)
if e = os.MkdirAll(pidFileDir, os.ModeDir|os.ModePerm); e != nil {
return nil, e
}
shost, _, e := net.SplitHostPort(config.GetString("clientaddress"))
if e != nil {
return nil, e
}
s.pidfilepath = fmt.Sprintf("%s/scrub.pid", pidFileDir)
s.socketfilepath = s.SocketFile()

shost, _, _ := net.SplitHostPort(config.GetString("clientaddress"))
if shost == "" {
shost = "localhost"
}

s := &Scrubd{
binarypath: binarypath,
volfileID: gdctx.MyUUID.String() + "-gluster/scrub",
logfile: path.Join(config.GetString("logdir"), "glusterfs", "scrub.log"),
pidfilepath: fmt.Sprintf("%s/scrub.pid", pidFileDir),
}

s.socketfilepath = s.SocketFile()
s.args = []string{}
s.args = append(s.args, "-s", shost)
s.args = append(s.args, "--volfile-id", s.volfileID)
Expand Down
Loading

0 comments on commit a848a87

Please sign in to comment.