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

Fix race in DeltaFIFO #32125

Merged
merged 1 commit into from Sep 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/client/cache/delta_fifo.go
Expand Up @@ -522,6 +522,18 @@ func (f *DeltaFIFO) syncKey(key string) error {
return nil
}

// If we are doing Resync() and there is already an event queued for that object,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we pass "sync bool" to queueActionLocked()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, I think that we should introduce a new type of event "Replace" (next to Added, Updated, Deleted and Sync), but maybe not in 1.4 timeframe?

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm - isn't replace Sync?

Copy link
Member Author

@wojtek-t wojtek-t Sep 6, 2016

Choose a reason for hiding this comment

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

It is treated as Sync now. But those two are different in fact:

  • Sync is supposed to be purely "process my with whatever value I have now"
  • Replace is "replace me with that value no matter what value I had before"

Conceptually they are different, but in the code they are treated the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried about all the places that will now get a new event type that didn't before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is only one such place (except from DeltaFIFO). But that's the main reason why I don't want to change it for 1.4.

Copy link
Member

Choose a reason for hiding this comment

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

"replace" should not be a new event type, just compute the corresponding add/update/delete object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand your comment. Basically we are calling Replace e.g. when we are doing "relist", if we are not able to get all the deltas from in-between. This is effectively an "update", but this is handled in the code as "Sync".

Copy link
Member

Choose a reason for hiding this comment

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

That's probably wrong. If it changes a value "sync" is probably wrong. Scary to change, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - we can try changing it in 1.5, but definitely not now.

// we ignore the Resync for it. This is to avoid the race, in which the resync
// comes with the previous value of object (since queueing an event for the object
// doesn't trigger changing the underlying store <knownObjects>.
id, err := f.KeyOf(obj)
if err != nil {
return KeyError{obj, err}
}
if len(f.items[id]) > 0 {
return nil
}

if err := f.queueActionLocked(Sync, obj); err != nil {
return fmt.Errorf("couldn't queue object: %v", err)
}
Expand Down
23 changes: 23 additions & 0 deletions pkg/client/cache/delta_fifo_test.go
Expand Up @@ -336,6 +336,29 @@ func TestDeltaFIFO_ReplaceMakesDeletions(t *testing.T) {
}
}

func TestDeltaFIFO_UpdateResyncRace(t *testing.T) {
f := NewDeltaFIFO(
testFifoObjectKeyFunc,
nil,
keyLookupFunc(func() []testFifoObject {
return []testFifoObject{mkFifoObj("foo", 5)}
}),
)
f.Update(mkFifoObj("foo", 6))
f.Resync()

expectedList := []Deltas{
{{Updated, mkFifoObj("foo", 6)}},
}

for _, expected := range expectedList {
cur := Pop(f).(Deltas)
if e, a := expected, cur; !reflect.DeepEqual(e, a) {
t.Errorf("Expected %#v, got %#v", e, a)
}
}
}

func TestDeltaFIFO_detectLineJumpers(t *testing.T) {
f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)

Expand Down