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

[processor/k8sattributes] Wait for ReplicaSet informer before starting pod informer #37138

Merged
merged 29 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
de3aad2
wait for replica set informer before starting pod informer
bacherfl Jan 10, 2025
b4d7636
re-enable failing test
bacherfl Jan 10, 2025
2d0334e
wait for other informers before starting pod informer, add changelog
bacherfl Jan 13, 2025
94bf7ac
fix linting
bacherfl Jan 13, 2025
ec1c895
trigger CI checks
bacherfl Jan 13, 2025
3c4e6c4
trigger CI checks
bacherfl Jan 13, 2025
02ad7bb
trigger CI checks
bacherfl Jan 13, 2025
041defd
trigger CI checks
bacherfl Jan 13, 2025
5fc8ca9
trigger CI checks
bacherfl Jan 13, 2025
3101bd8
trigger CI checks
bacherfl Jan 13, 2025
002adad
trigger CI checks
bacherfl Jan 13, 2025
f2c8e91
trigger CI checks
bacherfl Jan 14, 2025
284d8ce
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 14, 2025
a3b97e9
trigger CI checks
bacherfl Jan 14, 2025
383bf0d
Merge branch 'fix/37056/deployment-name' of https://github.com/bacher…
bacherfl Jan 14, 2025
76a042f
trigger CI checks
bacherfl Jan 14, 2025
4ba0a32
trigger CI checks
bacherfl Jan 14, 2025
4de34d4
trigger CI checks
bacherfl Jan 14, 2025
c86c80b
adapt to pr review
bacherfl Jan 14, 2025
f64e13a
add comment to explain changes to waitForMetadata logic
bacherfl Jan 14, 2025
0cbf681
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 14, 2025
711d559
adapt to pr review
bacherfl Jan 14, 2025
2696602
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 15, 2025
9a5fdc7
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 15, 2025
a2f90c4
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 15, 2025
3ddf4d9
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 16, 2025
ff7fb2d
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 20, 2025
9664201
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 21, 2025
a0ae518
Merge branch 'main' into fix/37056/deployment-name
bacherfl Jan 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .chloggen/k8sattributes-wait-for-informers.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: k8sattributesprocessor

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Wait for the other informers to complete their initial sync before starting the pod informers

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [37056]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
2 changes: 0 additions & 2 deletions processor/k8sattributesprocessor/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1099,8 +1099,6 @@ func TestE2E_NamespacedRBACNoPodIP(t *testing.T) {
// make docker-otelcontribcol
// KUBECONFIG=/tmp/kube-config-otelcol-e2e-testing kind load docker-image otelcontribcol:latest
func TestE2E_ClusterRBACCollectorStartAfterTelemetryGen(t *testing.T) {
// TODO: Re-enable this test when the issue being tested here is fully solved: https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/37056
t.Skip("Skipping test as https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/37056 is not fully solved yet")
testDir := filepath.Join("testdata", "e2e", "clusterrbac")

k8sClient, err := k8stest.NewK8sClient(testKubeConfig)
Expand Down
47 changes: 33 additions & 14 deletions processor/k8sattributesprocessor/internal/kube/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ func New(
// Start registers pod event handlers and starts watching the kubernetes cluster for pod changes.
func (c *WatchClient) Start() error {
synced := make([]cache.InformerSynced, 0)

// start the replicaSet informer first, as the replica sets need to be
// present at the time the pods are handled, to correctly establish the connection between pods and deployments
if c.Rules.DeploymentName || c.Rules.DeploymentUID {
Expand All @@ -225,18 +224,7 @@ func (c *WatchClient) Start() error {
go c.replicasetInformer.Run(c.stopCh)
}

reg, err := c.informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: c.handlePodAdd,
UpdateFunc: c.handlePodUpdate,
DeleteFunc: c.handlePodDelete,
})
if err != nil {
return err
}
synced = append(synced, reg.HasSynced)
go c.informer.Run(c.stopCh)

reg, err = c.namespaceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
reg, err := c.namespaceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: c.handleNamespaceAdd,
UpdateFunc: c.handleNamespaceUpdate,
DeleteFunc: c.handleNamespaceDelete,
Expand All @@ -260,13 +248,28 @@ func (c *WatchClient) Start() error {
go c.nodeInformer.Run(c.stopCh)
}

reg, err = c.informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: c.handlePodAdd,
UpdateFunc: c.handlePodUpdate,
DeleteFunc: c.handlePodDelete,
})
if err != nil {
return err
}

fatsheep9146 marked this conversation as resolved.
Show resolved Hide resolved
// start the podInformer with the prerequisite of the other informers to be finished first
go c.runInformerWithDependencies(c.informer, synced)

if c.waitForMetadata {
timeoutCh := make(chan struct{})
t := time.AfterFunc(c.waitForMetadataTimeout, func() {
close(timeoutCh)
})
defer t.Stop()
if !cache.WaitForCacheSync(timeoutCh, synced...) {
// Wait for the Pod informer to be completed.
// The other informers will already be finished at this point, as the pod informer
// waits for them be finished before it can run
if !cache.WaitForCacheSync(timeoutCh, reg.HasSynced) {
return errors.New("failed to wait for caches to sync")
}
}
Expand Down Expand Up @@ -1123,6 +1126,22 @@ func (c *WatchClient) getReplicaSet(uid string) (*ReplicaSet, bool) {
return nil, false
}

// runInformerWithDependencies starts the given informer. The second argument is a list of other informers that should complete
// before the informer is started. This is necessary e.g. for the pod informer which requires the replica set informer
// to be finished to correctly establish the connection to the replicaset/deployment it belongs to.
func (c *WatchClient) runInformerWithDependencies(informer cache.SharedInformer, dependencies []cache.InformerSynced) {
if len(dependencies) > 0 {
timeoutCh := make(chan struct{})
// TODO hard coding the timeout for now, check if we should make this configurable
t := time.AfterFunc(5*time.Second, func() {
close(timeoutCh)
})
defer t.Stop()
cache.WaitForCacheSync(timeoutCh, dependencies...)
}
informer.Run(c.stopCh)
}

// ignoreDeletedFinalStateUnknown returns the object wrapped in
// DeletedFinalStateUnknown. Useful in OnDelete resource event handlers that do
// not need the additional context.
Expand Down
Loading