-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[processor/k8sattributes] Wait for ReplicaSet informer before starting pod informer #37138
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
…fl/opentelemetry-collector-contrib into fix/37056/deployment-name
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
after running the tests a number of times, it looks like the e2e tests are passing consistently now, so this should be ready for review |
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 left some suggestions based on the following logic:
Step 1: wait for all Informers to get synced before unblocking the Pod's informer start
Step 2. Based on the waitForMetadata
setting block on Pod's informer sync which is already dependent to the sync of the rest of the informers.
In this, the Pod's informer sync comes as a task chord which is the only one we need to block on from the main flow since it's already blocked on the rest "internally".
Thanks for the review! Those suggestions make sense, I will look into updating the PR accordingly now |
Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Florian Bacher <[email protected]>
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.
Thank's!
Signed-off-by: Florian Bacher <[email protected]>
@fatsheep9146 @TylerHelmuth @dmitryax could you take a look as well? |
if err != nil { | ||
return err | ||
} | ||
|
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.
Why not add podInformer's HasSynced into the slice synced
Description
This PR is an alternative approach to the previous fix made for #37056, which did not fully solve the issue of the deployment name not being added to a pod after the initial informer sync
Link to tracking issue
Fixes #37056
Testing
Re-enabled the flaky E2E test