Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement sleep failpoint status #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions runtime/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"net"
"net/http"
"sort"
"strconv"
"strings"
)

Expand Down Expand Up @@ -87,12 +88,24 @@ func (*httpHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
sort.Strings(fps)
lines := make([]string, len(fps))
for i := range lines {
s, _ := status(fps[i])
s, _, _ := status(fps[i])
lines[i] = fps[i] + "=" + s
}
w.Write([]byte(strings.Join(lines, "\n") + "\n"))
} else if strings.HasSuffix(key, "/count") {
fp := key[:len(key)-len("/count")]
_, count, err := status(fp)
if err != nil {
if err == ErrNoExist {
http.Error(w, "failed to GET: "+err.Error(), http.StatusNotFound)
} else {
http.Error(w, "failed to GET: "+err.Error(), http.StatusInternalServerError)
}
return
}
w.Write([]byte(strconv.Itoa(count)))
} else {
status, err := status(key)
status, _, err := status(key)
if err != nil {
http.Error(w, "failed to GET: "+err.Error(), http.StatusNotFound)
}
Expand Down
12 changes: 6 additions & 6 deletions runtime/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,25 +112,25 @@ func disable(name string) error {
return nil
}

// Status gives the current setting for the failpoint
func Status(failpath string) (string, error) {
// Status gives the current setting and execution count for the failpoint
func Status(failpath string) (string, int, error) {
failpointsMu.Lock()
defer failpointsMu.Unlock()
return status(failpath)
}

func status(failpath string) (string, error) {
func status(failpath string) (string, int, error) {
fp := failpoints[failpath]
if fp == nil {
return "", ErrNoExist
return "", 0, ErrNoExist
}

t := fp.t
if t == nil {
return "", ErrDisabled
return "", 0, ErrDisabled
}

return t.desc, nil
return t.desc, t.counter, nil
}

func List() []string {
Expand Down
3 changes: 3 additions & 0 deletions runtime/terms.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ type terms struct {

// mu protects the state of the terms chain
mu sync.Mutex
// tracks executions count of terms that are actually evaluated
counter int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be a field/attribute of Failpoint, and we need to clearly define what does counter exactly mean. There are two options:

  • The count of the failpoint execution, no matter the terms are evaluated or not.
  • The count of the failpoint execution, only include the case that the terms are really evaluated. For example, 40.0%return(true) means 40% possibility to return true. If it returns false (falls into the other 60% possibility), then we don't increment the counter.

Your current implementation should be the second option.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the definition of counter. Regarding making it a field of Failpoint, I have the following concerns

  1. There are number of similar fields that are defined in terms rather than in failpoint (e.g., fpath, desc), should we standardize all, rather than moving just one field into Failpoint ?
  2. This will involve changes to parsing code, get Failpoint lock and reference in terms.go.

Should these be addressed as a refactor in a separate PR ?

}

// term is an executable unit of the failpoint terms chain
Expand Down Expand Up @@ -102,6 +104,7 @@ func (t *terms) eval() (interface{}, error) {
defer t.mu.Unlock()
for _, term := range t.chain {
if term.mods.allow() {
t.counter++
return term.do(), nil
}
}
Expand Down
158 changes: 158 additions & 0 deletions runtime/termscounter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
package runtime_test

import (
"strings"
"testing"

"go.etcd.io/gofail/runtime"
)

// This variable mimics the code generated by gofail code package.
// This works in tandem with exampleFunc function.
var __fp_ExampleString *runtime.Failpoint //nolint:stylecheck

// check if failpoint is initialized as gofail
// tests can clear global variables of runtime packages
func initFP() {
fps := runtime.List()
if fps != nil {
s_fps := strings.Join(fps, " ")
if strings.Contains(s_fps, "ExampleString") {
return
}
}
__fp_ExampleString = runtime.NewFailpoint("ExampleString") //nolint:stylecheck
}

func TestTermsCounter(t *testing.T) {
testcases := []struct {
name string
fp string
failpointTerm string
runBeforeEnabling int
runAfterEnabling int
wantCount int
}{
{
name: "Terms limit Failpoint",
fp: "ExampleString",
failpointTerm: `10*sleep(10)->1*return("abc")`,
runAfterEnabling: 12,
// This example tests mods which allows users to restrict the
// number of failpoint actions as against their callsite executions.
// This is the reason why wantCount < runAfterEnabling
// In a real world example you can hit a code spot a million times but
// using mods restrict the associated fallpoint actions to run twice.
wantCount: 11,
},
{
name: "Inbetween Enabling Failpoint",
fp: "ExampleString",
failpointTerm: `10*sleep(10)->1*return("abc")`,
runBeforeEnabling: 2,
runAfterEnabling: 3,
wantCount: 3,
},
{
name: "Before Enabling Failpoint",
fp: "ExampleString",
failpointTerm: `10*sleep(10)->1*return("abc")`,
runBeforeEnabling: 2,
runAfterEnabling: 0,
wantCount: 0,
},
}

initFP()
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {

for i := 0; i < tc.runBeforeEnabling; i++ {
exampleFunc()
}

err := runtime.Enable(tc.fp, tc.failpointTerm)
if err != nil {
t.Fatal(err)
}
defer runtime.Disable(tc.fp)
for i := 0; i < tc.runAfterEnabling; i++ {
exampleFunc()
}
_, count, err := runtime.Status(tc.fp)
if err != nil {
t.Fatal(err)
}
if tc.wantCount != count {
t.Fatal("counter is not properly incremented")
}
})
}
}

func TestEnablingNewTermResetsCount(t *testing.T) {
testcases := []struct {
name string
fp string
oldTerm string
newTerm string
runOldTerm int
runNewTerm int
wantCount int
}{
{
name: "Change and Reset Counter",
fp: "ExampleString",
oldTerm: `10*sleep(10)->1*return("abc")`,
newTerm: "sleep(10)",
runOldTerm: 2,
runNewTerm: 3,
wantCount: 3,
},
}

initFP()
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
err := runtime.Enable(tc.fp, tc.oldTerm)
if err != nil {
t.Fatal(err)
}

for i := 0; i < tc.runOldTerm; i++ {
exampleFunc()
}
err = runtime.Enable(tc.fp, tc.newTerm)
if err != nil {
t.Fatal(err)
}
defer runtime.Disable(tc.fp)

for i := 0; i < tc.runNewTerm; i++ {
exampleFunc()
}
_, count, err := runtime.Status(tc.fp)
if err != nil {
t.Fatal(err)
}
if tc.wantCount != count {
t.Fatal("counter is not properly incremented")
}
})
}

}

// This function mimics a customized code that is generated by gofail code package.
func exampleFunc() string {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you add some comments about this test setup. My understanding is that exampleFunc mimics code generated by gofail and __fp_ExampleString mimics fail.go file. This way you don't need to actually run gofail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment for this function and the variable that together mimics the actions of code package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need exampleFunc in this test file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It simulates the triggering of the failpoint. We can achieve the same thing by a combination of "envTerms" and "Acquire()" as seen in failpoint_test.go. I can remove and replace if that is the preferred way.

if vExampleString, __fpErr := __fp_ExampleString.Acquire(); __fpErr == nil { //nolint:stylecheck
ExampleString, __fpTypeOK := vExampleString.(string) //nolint:stylecheck
if !__fpTypeOK { //nolint:stylecheck
goto __badTypeExampleString //nolint:stylecheck
}
return ExampleString
__badTypeExampleString: //nolint:stylecheck
__fp_ExampleString.BadType(vExampleString, "string") //nolint:stylecheck
}
return "example"
}