-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Don't store no-op updates in etcd. #20897
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,24 @@ func (h *etcdHelper) Set(ctx context.Context, key string, obj, out runtime.Objec | |
if ctx == nil { | ||
glog.Errorf("Context is nil") | ||
} | ||
|
||
version := uint64(0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW - Set() method is used only in tests. I'm happy to remove this method from the interface if you agree it's not needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's open a follow up issue post 1.3 - I don't think we have to remove it now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #20963 for it. |
||
if h.versioner != nil { | ||
var err error | ||
if version, err = h.versioner.ObjectResourceVersion(obj); err != nil { | ||
return errors.New("couldn't get resourceVersion from object") | ||
} | ||
if version != 0 { | ||
// We cannot store object with resourceVersion in etcd, we need to clear it here. | ||
if err := h.versioner.UpdateObject(obj, nil, 0); err != nil { | ||
return errors.New("resourceVersion cannot be set on objects store in etcd") | ||
} | ||
} | ||
} | ||
// TODO: If versioner is nil, then we may end up with having ResourceVersion set | ||
// in the object and this will be incorrect ResourceVersion. We should fix it by | ||
// requiring "versioner != nil" at the constructor level for 1.3 milestone. | ||
|
||
var response *etcd.Response | ||
data, err := runtime.Encode(h.codec, obj) | ||
if err != nil { | ||
|
@@ -200,7 +218,7 @@ func (h *etcdHelper) Set(ctx context.Context, key string, obj, out runtime.Objec | |
|
||
create := true | ||
if h.versioner != nil { | ||
if version, err := h.versioner.ObjectResourceVersion(obj); err == nil && version != 0 { | ||
if version != 0 { | ||
create = false | ||
startTime := time.Now() | ||
opts := etcd.SetOptions{ | ||
|
@@ -558,6 +576,16 @@ func (h *etcdHelper) GuaranteedUpdate(ctx context.Context, key string, ptrToType | |
ttl = *newTTL | ||
} | ||
|
||
// Since update object may have a resourceVersion set, we need to clear it here. | ||
if h.versioner != nil { | ||
if err := h.versioner.UpdateObject(ret, meta.Expiration, 0); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is RV the only field like this? How about SelfLink, can we clear that too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can, but then we can't use versioner for it. And since we will need to use meta.Accessor, then there is no need to use versioner at all. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a really big change right now....
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @smarterclayton - sorry, I'm afraid I don't understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was suggesting making the smallest change possible for 1.2 - using
versioner and not resetting self link (I don't think we have evidence
self link is causing updates to not be no-op, but if we do then yes,
we have to fix it)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with punting on a self link fix. That'd only be a problem with objects available from multiple URLs, and we won't have many for 1.2. |
||
return errors.New("resourceVersion cannot be set on objects store in etcd") | ||
} | ||
} | ||
// TODO: If versioner is nil, then we may end up with having ResourceVersion set | ||
// in the object and this will be incorrect ResourceVersion. We should fix it by | ||
// requiring "versioner != nil" at the constructor level for 1.3 milestone. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @smarterclayton - I browsed through the code, and basically there is no easy way to construct etcdHelper with nil versioner (all constructors set it to ApiObjectVersioner{}) and we don't have setters to change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fine
|
||
data, err := runtime.Encode(h.codec, ret) | ||
if err != nil { | ||
return err | ||
|
@@ -580,7 +608,10 @@ func (h *etcdHelper) GuaranteedUpdate(ctx context.Context, key string, ptrToType | |
} | ||
|
||
if string(data) == origBody { | ||
return nil | ||
// If we don't send an update, we simply return the currently existing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yes. Can you add a test case for this condition so we don't regress? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't tell if you added one or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I didn't. I will add tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, a test for this would be good--subtle problem. |
||
// version of the object. | ||
_, _, err := h.extractObj(res, nil, ptrToType, ignoreNotFound, false) | ||
return err | ||
} | ||
|
||
startTime := time.Now() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if possible, can we also have a separate test that inserts an old incorrect record (containing resourceVersion) into etcd, makes sure a no-op update is ignored, and an actual update is persisted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - I'm afraid I don't understand:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other tests are testing updates against new records made with the current version which will not set resourceVersion in etcd. I want to make sure when this fix runs against an etcd containing old incorrect records which contain resourceVersion, we get correct update behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it - makes sense (although in that case no-op update is not no-op so it will be performed). But having such test makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add such test tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - that should be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - it seems that there is no easy way to store object with ResourceVersion set in etcd after my fixes.
I manually checked that it works as expected and added a TODO for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but we don't have such, so this would make this change much bigger (it will take some time to implement it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.