Skip to content

Commit 44c8f8e

Browse files
sebastienvassebastienvas
committed
Fix data race when updating user-data
1 parent 5bc7b15 commit 44c8f8e

13 files changed

+166
-69
lines changed

‎boskos/boskos.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ func handleUpdate(r *ranch.Ranch) http.HandlerFunc {
366366
}
367367
}
368368

369-
if err := r.Update(name, owner, state, userData); err != nil {
369+
if err := r.Update(name, owner, state, &userData); err != nil {
370370
logrus.WithError(err).Errorf("Update failed: %v - %v (%v)", name, state, owner)
371371
http.Error(res, err.Error(), ErrorToStatus(err))
372372
return

‎boskos/client/client.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ func (c *Client) SyncAll() error {
182182
}
183183

184184
// UpdateOne signals update for one of the resources hold by the client.
185-
func (c *Client) UpdateOne(name, state string, userData common.UserData) error {
185+
func (c *Client) UpdateOne(name, state string, userData *common.UserData) error {
186186
c.lock.Lock()
187187
defer c.lock.Unlock()
188188

@@ -216,7 +216,7 @@ func (c *Client) HasResource() bool {
216216

217217
// private methods
218218

219-
func (c *Client) updateLocalResource(i common.Item, state string, data common.UserData) error {
219+
func (c *Client) updateLocalResource(i common.Item, state string, data *common.UserData) error {
220220
res, err := common.ItemToResource(i)
221221
if err != nil {
222222
return err
@@ -227,6 +227,7 @@ func (c *Client) updateLocalResource(i common.Item, state string, data common.Us
227227
} else {
228228
res.UserData.Update(data)
229229
}
230+
230231
return c.storage.Update(res)
231232
}
232233

@@ -297,7 +298,7 @@ func (c *Client) release(name, dest string) error {
297298
return nil
298299
}
299300

300-
func (c *Client) update(name, state string, userData common.UserData) error {
301+
func (c *Client) update(name, state string, userData *common.UserData) error {
301302
var body io.Reader
302303
if userData != nil {
303304
b := new(bytes.Buffer)

‎boskos/common/BUILD.bazel

+1-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ go_library(
1313
"mason_config.go",
1414
],
1515
importpath = "k8s.io/test-infra/boskos/common",
16-
deps = ["//vendor/gopkg.in/yaml.v2:go_default_library"],
16+
deps = ["//vendor/github.com/ghodss/yaml:go_default_library"],
1717
)
1818

1919
filegroup(

‎boskos/common/common.go

+69-17
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,13 @@ package common
1919
import (
2020
"errors"
2121
"fmt"
22-
"gopkg.in/yaml.v2"
2322
"strings"
2423
"time"
24+
25+
"encoding/json"
26+
"sync"
27+
28+
"github.com/ghodss/yaml"
2529
)
2630

2731
const (
@@ -38,7 +42,12 @@ const (
3842
)
3943

4044
// UserData is a map of Name to user defined interface, serialized into a string
41-
type UserData map[string]string
45+
type UserData struct {
46+
sync.Map
47+
}
48+
49+
// UserDataMap is the standard Map version of UserMap, it is used to ease UserMap creation.
50+
type UserDataMap map[string]string
4251

4352
// LeasedResources is a list of resources name that used in order to create another resource by Mason
4453
type LeasedResources []string
@@ -56,7 +65,7 @@ type Resource struct {
5665
Owner string `json:"owner"`
5766
LastUpdate time.Time `json:"lastupdate"`
5867
// Customized UserData
59-
UserData UserData `json:"userdata"`
68+
UserData *UserData `json:"userdata"`
6069
}
6170

6271
// ResourceEntry is resource config format defined from config.yaml
@@ -87,7 +96,7 @@ func NewResource(name, rtype, state, owner string, t time.Time) Resource {
8796
State: state,
8897
Owner: owner,
8998
LastUpdate: t,
90-
UserData: map[string]string{},
99+
UserData: &UserData{},
91100
}
92101
}
93102

@@ -100,6 +109,15 @@ func NewResourcesFromConfig(e ResourceEntry) []Resource {
100109
return resources
101110
}
102111

112+
// UserDataFromMap returns a UserData from a map
113+
func UserDataFromMap(m UserDataMap) *UserData {
114+
ud := &UserData{}
115+
for k, v := range m {
116+
ud.Store(k, v)
117+
}
118+
return ud
119+
}
120+
103121
// UserDataNotFound will be returned if requested resource does not exist.
104122
type UserDataNotFound struct {
105123
ID string
@@ -144,40 +162,74 @@ func (r *ResTypes) Set(value string) error {
144162
// GetName implements the Item interface used for storage
145163
func (res Resource) GetName() string { return res.Name }
146164

165+
// UnmarshalJSON implements JSON Unmarshaler interface
166+
func (ud *UserData) UnmarshalJSON(data []byte) error {
167+
tmpMap := UserDataMap{}
168+
if err := json.Unmarshal(data, &tmpMap); err != nil {
169+
return err
170+
}
171+
ud.FromMap(tmpMap)
172+
return nil
173+
}
174+
175+
// MarshalJSON implents JSON Marshaler interface
176+
func (ud *UserData) MarshalJSON() ([]byte, error) {
177+
return json.Marshal(ud.ToMap())
178+
}
179+
147180
// Extract unmarshalls a string a given struct if it exists
148-
func (ud UserData) Extract(id string, out interface{}) error {
149-
content, ok := ud[id]
181+
func (ud *UserData) Extract(id string, out interface{}) error {
182+
content, ok := ud.Load(id)
150183
if !ok {
151184
return &UserDataNotFound{id}
152185
}
153-
return yaml.Unmarshal([]byte(content), out)
186+
return yaml.Unmarshal([]byte(content.(string)), out)
154187
}
155188

156189
// User Data are used to store custom information mainly by Mason and Masonable implementation.
157190
// Mason used a LeasedResource keys to store information about other resources that used to
158191
// create the given resource.
159192

160193
// Set marshalls a struct to a string into the UserData
161-
func (ud UserData) Set(id string, in interface{}) error {
194+
func (ud *UserData) Set(id string, in interface{}) error {
162195
b, err := yaml.Marshal(in)
163196
if err != nil {
164197
return err
165198
}
166-
ud[id] = string(b)
199+
ud.Store(id, string(b))
167200
return nil
168201
}
169202

170203
// Update updates existing UserData with new UserData.
171204
// If a key as an empty string, the key will be deleted
172-
func (ud UserData) Update(new UserData) {
173-
if new != nil {
174-
for id, content := range new {
175-
if content != "" {
176-
ud[id] = content
177-
} else {
178-
delete(ud, id)
179-
}
205+
func (ud *UserData) Update(new *UserData) {
206+
if new == nil {
207+
return
208+
}
209+
new.Range(func(key, value interface{}) bool {
210+
if value.(string) != "" {
211+
ud.Store(key, value)
212+
} else {
213+
ud.Delete(key)
180214
}
215+
return true
216+
})
217+
}
218+
219+
// ToMap converts a UserData to UserDataMap
220+
func (ud *UserData) ToMap() UserDataMap {
221+
m := UserDataMap{}
222+
ud.Range(func(key, value interface{}) bool {
223+
m[key.(string)] = value.(string)
224+
return true
225+
})
226+
return m
227+
}
228+
229+
// FromMap feels updates user data from a map
230+
func (ud *UserData) FromMap(m UserDataMap) {
231+
for key, value := range m {
232+
ud.Store(key, value)
181233
}
182234
}
183235

‎boskos/common/common_test.go

+43-7
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ limitations under the License.
1717
package common
1818

1919
import (
20+
"bytes"
21+
"encoding/json"
2022
"reflect"
2123
"testing"
2224
)
@@ -28,7 +30,9 @@ type fakeStruct struct {
2830
func TestUserData_Extract(t *testing.T) {
2931
ud := UserData{}
3032
fs := fakeStruct{"value"}
31-
ud.Set("test", &fs)
33+
if err := ud.Set("test", &fs); err != nil {
34+
t.Errorf("unable to set data")
35+
}
3236
var rfs fakeStruct
3337
if err := ud.Extract("test", &rfs); err != nil {
3438
t.Error("unable to extract struct")
@@ -39,16 +43,48 @@ func TestUserData_Extract(t *testing.T) {
3943
}
4044

4145
func TestUserData_Update(t *testing.T) {
42-
ud1 := UserData{"0": "0"}
43-
ud2 := UserData{"1": "1", "2": "2"}
44-
ud3 := UserData{"0": "0", "1": "1", "2": "2"}
46+
ud1 := UserDataFromMap(UserDataMap{"0": "0"})
47+
ud2 := UserDataFromMap(UserDataMap{"1": "1", "2": "2"})
48+
ud3 := UserDataFromMap(UserDataMap{"0": "0", "1": "1", "2": "2"})
4549
ud1.Update(ud2)
46-
if !reflect.DeepEqual(ud1, ud3) {
50+
if !reflect.DeepEqual(ud1.ToMap(), ud3.ToMap()) {
4751
t.Errorf("%v does not match expected %v", ud1, ud3)
4852
}
4953
// Testing delete
50-
ud3.Update(UserData{"0": ""})
51-
if !reflect.DeepEqual(ud3, ud2) {
54+
ud3.Update(UserDataFromMap(UserDataMap{"0": ""}))
55+
if !reflect.DeepEqual(ud3.ToMap(), ud2.ToMap()) {
5256
t.Errorf("%v does not match expected %v", ud3, ud2)
5357
}
5458
}
59+
60+
func TestUserData_Marshall(t *testing.T) {
61+
ud := UserDataFromMap(UserDataMap{"0": "0", "1": "1", "2": "2"})
62+
b, err := ud.MarshalJSON()
63+
if err != nil {
64+
t.Errorf("unable to marshall %v", ud.ToMap())
65+
}
66+
var udFromJSON UserData
67+
if err := udFromJSON.UnmarshalJSON(b); err != nil {
68+
t.Errorf("unable to unmarshall %v", string(b))
69+
}
70+
if !reflect.DeepEqual(ud.ToMap(), udFromJSON.ToMap()) {
71+
t.Errorf("src %v does not match %v", ud.ToMap(), udFromJSON.ToMap())
72+
}
73+
}
74+
75+
func TestUserData_JSON(t *testing.T) {
76+
ud := UserDataFromMap(UserDataMap{"0": "0", "1": "1", "2": "2"})
77+
b := new(bytes.Buffer)
78+
if err := json.NewEncoder(b).Encode(ud); err != nil {
79+
t.Errorf("unable to marshall %v", ud.ToMap())
80+
}
81+
var decodedUD UserData
82+
if err := json.NewDecoder(b).Decode(&decodedUD); err != nil {
83+
t.Errorf("unable to unmarshall %v", b.String())
84+
}
85+
86+
if !reflect.DeepEqual(ud.ToMap(), decodedUD.ToMap()) {
87+
t.Errorf("src %v does not match %v", ud.ToMap(), decodedUD.ToMap())
88+
}
89+
90+
}

‎boskos/crds/resource_crd.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,10 @@ type ResourceSpec struct {
6969

7070
// ResourceStatus holds information that are likely to change
7171
type ResourceStatus struct {
72-
State string `json:"state,omitempty"`
73-
Owner string `json:"owner"`
74-
LastUpdate time.Time `json:"lastUpdate,omitempty"`
75-
UserData common.UserData `json:"userData,omitempty"`
72+
State string `json:"state,omitempty"`
73+
Owner string `json:"owner"`
74+
LastUpdate time.Time `json:"lastUpdate,omitempty"`
75+
UserData *common.UserData `json:"userData,omitempty"`
7676
}
7777

7878
// GetName returns a unique identifier for a given resource

‎boskos/mason/mason.go

+6-5
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ const (
3939

4040
// Masonable should be implemented by all configurations
4141
type Masonable interface {
42-
Construct(*common.Resource, common.TypeToResources) (common.UserData, error)
42+
Construct(*common.Resource, common.TypeToResources) (*common.UserData, error)
4343
}
4444

4545
// ConfigConverter converts a string into a Masonable
@@ -49,7 +49,7 @@ type boskosClient interface {
4949
Acquire(rtype, state, dest string) (*common.Resource, error)
5050
AcquireByState(state, dest string, names []string) ([]common.Resource, error)
5151
ReleaseOne(name, dest string) error
52-
UpdateOne(name, state string, userData common.UserData) error
52+
UpdateOne(name, state string, userData *common.UserData) error
5353
SyncAll() error
5454
UpdateAll(dest string) error
5555
ReleaseAll(dest string) error
@@ -379,8 +379,8 @@ func (m *Mason) recycleOne(res *common.Resource) (*requirements, error) {
379379
}
380380
}
381381
// Deleting Leased Resources
382-
delete(res.UserData, LeasedResources)
383-
if err := m.client.UpdateOne(res.Name, res.State, common.UserData{LeasedResources: ""}); err != nil {
382+
res.UserData.Delete(LeasedResources)
383+
if err := m.client.UpdateOne(res.Name, res.State, common.UserDataFromMap(map[string]string{LeasedResources: ""})); err != nil {
384384
logrus.WithError(err).Errorf("could not update resource %s with freed leased resources", res.Name)
385385
}
386386
}
@@ -467,7 +467,7 @@ func (m *Mason) fulfillOne(ctx context.Context, req *requirements) error {
467467
leasedResources = append(leasedResources, r.Name)
468468
}
469469
}
470-
userData := common.UserData{}
470+
userData := &common.UserData{}
471471
if err := userData.Set(LeasedResources, &leasedResources); err != nil {
472472
logrus.WithError(err).Errorf("failed to add %s user data", LeasedResources)
473473
return err
@@ -481,6 +481,7 @@ func (m *Mason) fulfillOne(ctx context.Context, req *requirements) error {
481481
} else {
482482
req.resource.UserData.Update(userData)
483483
}
484+
484485
logrus.Infof("requirements for release %s is fulfilled", req.resource.Name)
485486
return nil
486487
}

‎boskos/mason/mason_test.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,8 @@ func fakeConfigConverter(in string) (Masonable, error) {
5454
return &fakeConfig{}, nil
5555
}
5656

57-
func (fc *fakeConfig) Construct(res *common.Resource, typeToRes common.TypeToResources) (common.UserData, error) {
58-
return common.UserData{"fakeConfig": "unused"}, nil
57+
func (fc *fakeConfig) Construct(res *common.Resource, typeToRes common.TypeToResources) (*common.UserData, error) {
58+
return common.UserDataFromMap(map[string]string{"fakeConfig": "unused"}), nil
5959
}
6060

6161
// Create a fake client
@@ -71,7 +71,7 @@ func createFakeBoskos(tc testConfig) (*ranch.Storage, *Client, []common.Resource
7171
Type: rtype,
7272
Name: fmt.Sprintf("%s_%d", rtype, i),
7373
State: common.Free,
74-
UserData: common.UserData{},
74+
UserData: &common.UserData{},
7575
}
7676
if c.resourceNeeds != nil {
7777
res.State = common.Dirty
@@ -105,7 +105,7 @@ func (fb *fakeBoskos) ReleaseOne(name, dest string) error {
105105
return fb.ranch.Release(name, dest, owner)
106106
}
107107

108-
func (fb *fakeBoskos) UpdateOne(name, state string, userData common.UserData) error {
108+
func (fb *fakeBoskos) UpdateOne(name, state string, userData *common.UserData) error {
109109
return fb.ranch.Update(name, owner, state, userData)
110110
}
111111

@@ -249,10 +249,10 @@ func TestFulfillOne(t *testing.T) {
249249
if res.UserData.Extract(LeasedResources, &leasedResources); err != nil {
250250
t.Errorf("unable to extract %s", LeasedResources)
251251
}
252-
if res.UserData[LeasedResources] != req.resource.UserData[LeasedResources] {
252+
if res.UserData.ToMap()[LeasedResources] != req.resource.UserData.ToMap()[LeasedResources] {
253253
t.Errorf(
254254
"resource user data from requirement %v should be the same as the one received %v",
255-
req.resource.UserData[LeasedResources], res.UserData[LeasedResources])
255+
req.resource.UserData.ToMap()[LeasedResources], res.UserData.ToMap()[LeasedResources])
256256
}
257257
if len(leasedResources) != 1 {
258258
t.Errorf("there should be one leased resource, found %d", len(leasedResources))

‎boskos/ranch/BUILD.bazel

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ go_library(
2626
deps = [
2727
"//boskos/common:go_default_library",
2828
"//boskos/storage:go_default_library",
29+
"//vendor/github.com/ghodss/yaml:go_default_library",
2930
"//vendor/github.com/hashicorp/go-multierror:go_default_library",
3031
"//vendor/github.com/sirupsen/logrus:go_default_library",
31-
"//vendor/gopkg.in/yaml.v2:go_default_library",
3232
],
3333
)
3434

0 commit comments

Comments
 (0)