[GarbageCollector] Increase log verbosity for Garbage collector tests #30316
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@gmarek the change bloats the stdout output a little bit, but I think it's worth. (see https://console.cloud.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/30316/kubernetes-pull-test-unit-integration/38513/artifacts/) |
|
On top of RBAC tests there's also a race: |
126fee8 to
d0ec359
Compare
1 similar comment
|
unit test out of memory. @k8s-bot unit test this, issue #IGNORE |
d0ec359 to
c2e01ee
Compare
|
GKE test endpoint is down... |
| setObjectTypeMeta(oldObj) | ||
| // oldObj is obtained from the informer's cache, so it should | ||
| // have typeMeta set. Setting it again here causes data race. | ||
| // setObjectTypeMeta(oldObj) |
There was a problem hiding this comment.
@gmarek could you be more specific? Is it the setObjectTypeMeta looking sketchy or not setting typemeta for oldObj sketchy? For the former, I've convince @lavalamp there is no better way in #28480 (comment). For the latter, the oldObj is not even used by the GC, so it's safe.
There was a problem hiding this comment.
The fact that this causes race is surprising. The fact that the same method for newObj doesn't is even more surprising.
There was a problem hiding this comment.
It's because the oldObj is get from the controller cache (https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/framework/controller.go#L238), so garbage collector's processEvent may be processing the object already. The newObj is obtained through list or watch and is fresh, so it's not causing race.
There was a problem hiding this comment.
Sorry I didn't catch this in review. We can't modify objects coming from a shared informer cache. I thought this wasn't a shared informer cache though. Where is the code that this races with?
There was a problem hiding this comment.
So what's the verdict on this. I take it it's unneeded since it's already been set, but is it actually fixing a data race?
There was a problem hiding this comment.
Because we increase the log level, the log function will read the data and causes the race. We need this fix.
There was a problem hiding this comment.
Can you delete the line instead of commenting the line out?
|
Adding @lavalamp to give final LGTM if the informer thing is correct. |
c2e01ee to
e125fc0
Compare
e125fc0 to
9ac91e5
Compare
|
GCE e2e build/test passed for commit 9ac91e5. |
| gc.propagator.eventQueue.Add(&workqueue.TimedWorkQueueItem{StartTime: gc.clock.Now(), Object: event}) | ||
| }, | ||
| UpdateFunc: func(oldObj, newObj interface{}) { | ||
| setObjectTypeMeta(newObj) |
There was a problem hiding this comment.
Can you open an issue so we'll revisit whether it's OK for garbage collector to mutate these objects?
|
Automatic merge from submit-queue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I cannot reproduce the flake of GC locally, see #28713 (comment), so I increased the log verbosity for Garbage collector tests.
This change is