Skip to content

Commit

Permalink
Rework status locking to ensure locked over file write
Browse files Browse the repository at this point in the history
  • Loading branch information
Rob Brockbank committed Aug 6, 2018
1 parent ca56a14 commit bd86816
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 69 deletions.
41 changes: 19 additions & 22 deletions pkg/status/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
)

const (
DefaultStatusFile string = "status.json"
DefaultStatusFile = "status.json"
)

type ConditionStatus struct {
Expand All @@ -46,20 +46,14 @@ func New(file string) *Status {
return &st
}

// SetReady sets the status of one ready key and the reason associated with
// that status.
func (s *Status) SetReady(key string, val bool, reason string) {
needUpdate := false
// SetReady sets the status of one ready key and the reason associated with that status.
func (s *Status) SetReady(key string, ready bool, reason string) {
s.readyMutex.Lock()
if prev, ok := s.Readiness[key]; !ok {
needUpdate = true
} else if prev.Ready != val || prev.Reason != reason {
needUpdate = true
}
s.Readiness[key] = ConditionStatus{Ready: val, Reason: reason}
s.readyMutex.Unlock()
if needUpdate {
_ = s.WriteStatus()
defer s.readyMutex.Unlock()

if prev, ok := s.Readiness[key]; !ok || prev.Ready != ready || prev.Reason != reason {
s.Readiness[key] = ConditionStatus{Ready: ready, Reason: reason}
_ = s.writeStatus()
}
}

Expand All @@ -68,6 +62,7 @@ func (s *Status) SetReady(key string, val bool, reason string) {
func (s *Status) GetReady(key string) bool {
s.readyMutex.Lock()
defer s.readyMutex.Unlock()

v, ok := s.Readiness[key]
if !ok {
return false
Expand Down Expand Up @@ -97,9 +92,10 @@ func (s *Status) GetReadiness() bool {
// are not ready the reasons are combined and returned.
// The output format is '<reason 1>; <reason 2>'.
func (s *Status) GetNotReadyConditions() string {
var unready []string
s.readyMutex.Lock()
defer s.readyMutex.Unlock()

var unready []string
for _, v := range s.Readiness {
if !v.Ready {
unready = append(unready, v.Reason)
Expand All @@ -109,15 +105,16 @@ func (s *Status) GetNotReadyConditions() string {

}

// WriteStatus writes out the status in json format.
func (c *Status) WriteStatus() error {
b, err := json.Marshal(c)
// writeStatus writes out the status in json format.
// Lock should be held by caller.
func (s *Status) writeStatus() error {
b, err := json.Marshal(s)
if err != nil {
logrus.Errorf("Failed to marshal readiness: %s", err)
return err
}

err = ioutil.WriteFile(c.statusFile, b, 0644)
err = ioutil.WriteFile(s.statusFile, b, 0644)
if err != nil {
logrus.Errorf("Failed to write readiness file: %s", err)
return err
Expand All @@ -128,15 +125,15 @@ func (c *Status) WriteStatus() error {

// ReadStatusFile reads in the status file as written by WriteStatus.
func ReadStatusFile(file string) (*Status, error) {
st := Status{}
st := &Status{}
contents, err := ioutil.ReadFile(file)
if err != nil {
return nil, err
}
err = json.Unmarshal(contents, &st)
err = json.Unmarshal(contents, st)
if err != nil {
return nil, err
}

return &st, nil
return st, nil
}
10 changes: 9 additions & 1 deletion pkg/status/status_suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,20 @@
package status_test

import (
"testing"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

"testing"
"github.com/projectcalico/libcalico-go/lib/testutils"
"github.com/sirupsen/logrus"
)

func init() {
testutils.HookLogrusForGinkgo()
logrus.SetLevel(logrus.InfoLevel)
}

func TestConfig(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "pkg/status suite")
Expand Down
119 changes: 73 additions & 46 deletions pkg/status/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,44 +17,17 @@ package status
import (
"io/ioutil"
"os"

"github.com/sirupsen/logrus"
"strconv"
"sync"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
)

var log = logrus.WithField("UT", "true")

// This section contains unit tests for the Status pkg.
var _ = Describe("Status pkg UTs", func() {

It("should correctly read and write the status file", func() {
f, err := ioutil.TempFile("", "test")
Expect(err).NotTo(HaveOccurred())
defer os.Remove(f.Name())
st := New(f.Name())

st.SetReady("anykey", false, "the-reason")
err = st.WriteStatus()
By("writing the readiness file", func() {
Expect(err).NotTo(HaveOccurred())
_, err = os.Stat(f.Name())
Expect(err).NotTo(HaveOccurred())
})

readSt, err := ReadStatusFile(f.Name())
By("reading the readiness file", func() {
Expect(err).NotTo(HaveOccurred())
})
By("read status file should return proper Status object", func() {
Expect(readSt.Readiness).Should(HaveKeyWithValue("anykey", ConditionStatus{Ready: false, Reason: "the-reason"}))
})
})

It("should report failed readiness with no condition statuses", func() {
st := New("no-needed")

Expect(st.GetReadiness()).To(BeFalse())
})

Expand All @@ -64,65 +37,119 @@ var _ = Describe("Status pkg UTs", func() {
defer os.Remove(f.Name())
st := New(f.Name())

st.SetReady("anykey", false, "reason1")

By("status file should return proper Status object", func() {
st.SetReady("anykey", false, "reason1")

readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())

Expect(readSt.Readiness).Should(HaveKeyWithValue("anykey",
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey",
ConditionStatus{Ready: false, Reason: "reason1"}))
Expect(readSt.GetReadiness()).Should(Equal(false))
Expect(readSt.GetReadiness()).To(Equal(false))
})

By("status file should be updated when reason changes", func() {
st.SetReady("anykey", false, "reason2")

// File should be updated, check the data.
readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())

Expect(readSt.Readiness).Should(HaveKeyWithValue("anykey",
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey",
ConditionStatus{Ready: false, Reason: "reason2"}))
Expect(readSt.GetReadiness()).Should(Equal(false))
Expect(readSt.GetReadiness()).To(Equal(false))
})

By("status file should be updated when set ready", func() {
st.SetReady("anykey", true, "")

// File should be updated, check the data.
readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())

Expect(readSt.Readiness).Should(HaveKeyWithValue("anykey",
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey",
ConditionStatus{Ready: true, Reason: ""}))
Expect(readSt.GetReadiness()).Should(Equal(true))
Expect(readSt.GetReadiness()).To(Equal(true))
})

By("status file should not be updated when set ready a 2nd time", func() {
prevStat, err := os.Stat(f.Name())
Expect(err).NotTo(HaveOccurred())

// Set ready again
// Set read again.
st.SetReady("anykey", true, "")

// Check previous modification time to the time reported now
// The file should remain unchanged.
nowStat, err := os.Stat(f.Name())
Expect(err).NotTo(HaveOccurred())
Expect(nowStat.ModTime()).To(Equal(prevStat.ModTime()))

// Make sure the status value has not changed
readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())
Expect(readSt.Readiness).Should(HaveKeyWithValue("anykey",
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey",
ConditionStatus{Ready: true, Reason: ""}))
Expect(readSt.GetReadiness()).Should(Equal(true))
Expect(readSt.GetReadiness()).To(Equal(true))
})

By("status file should be updated to not ready after being ready", func() {
st.SetReady("anykey", false, "reason3")

// File should be updated, check the data.
readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())

Expect(readSt.Readiness).Should(HaveKeyWithValue("anykey",
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey",
ConditionStatus{Ready: false, Reason: "reason3"}))
Expect(readSt.GetReadiness()).Should(Equal(false))
Expect(readSt.GetReadiness()).To(Equal(false))
})

By("status file should handle a lot of concurrent not-ready updates for a lot of keys", func() {
wg := sync.WaitGroup{}
wg.Add(100)
for i := 0; i < 100; i++ {
go func(j int) {
st.SetReady("anykey"+strconv.Itoa(j), false, "reason"+strconv.Itoa(j))
wg.Done()
}(i)
}
wg.Wait()
Expect(st.Readiness).To(HaveLen(101))

// File should be updated, check the data.
readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())
Expect(readSt.Readiness).To(HaveLen(101))
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey99",
ConditionStatus{Ready: false, Reason: "reason99"}))
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey50",
ConditionStatus{Ready: false, Reason: "reason50"}))
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey1",
ConditionStatus{Ready: false, Reason: "reason1"}))
Expect(readSt.GetReadiness()).To(Equal(false))
})

By("status file should handle a lot of concurrent ready updates for all of the keys (plus more)", func() {
wg := sync.WaitGroup{}
wg.Add(200)
go st.SetReady("anykey", true, "reason")
for i := 0; i < 200; i++ {
go func(j int) {
st.SetReady("anykey"+strconv.Itoa(j), true, "reason"+strconv.Itoa(j))
wg.Done()
}(i)
}
wg.Wait()
Expect(st.Readiness).To(HaveLen(201))

// File should be updated, check the data.
readSt, err := ReadStatusFile(f.Name())
Expect(err).NotTo(HaveOccurred())
Expect(readSt.Readiness).To(HaveLen(201))
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey199",
ConditionStatus{Ready: true, Reason: "reason199"}))
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey150",
ConditionStatus{Ready: true, Reason: "reason150"}))
Expect(readSt.Readiness).To(HaveKeyWithValue("anykey100",
ConditionStatus{Ready: true, Reason: "reason100"}))
Expect(readSt.GetReadiness()).To(Equal(true))
})
})
})

0 comments on commit bd86816

Please sign in to comment.