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

Ensure that init containers are no longer tailed after they stop #14394

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

vjsamuel
Copy link
Contributor

@vjsamuel vjsamuel commented Nov 5, 2019

Init containers are killed off immediately after their work is done. So, ideally we should stop polling their endpoints and tailing their log files. The lack of this check impacted one of our builders that didnt have container in the log file path. Which caused the same path to be tailed multiple times.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@odacremolbap
Copy link
Contributor

Thanks @vjsamuel

Can you add a changelog entry?
Let me spend some time to manually test

@odacremolbap odacremolbap added Team:Integrations Label for the Integrations team containers Related to containers use case labels Nov 5, 2019
@vjsamuel vjsamuel force-pushed the initcontainer_stop branch 2 times, most recently from 90c2494 to 2adbe58 Compare November 5, 2019 22:35
@vjsamuel
Copy link
Contributor Author

vjsamuel commented Nov 5, 2019

thanks @odacremolbap. I have done the needful.

@exekias exekias added bug needs_backport PR is waiting to be backported to other branches. labels Nov 8, 2019
@odacremolbap
Copy link
Contributor

@vjsamuel sorry for the multi-step review.

tests are failing because container status are not being informed at a "start" pod test.

Adding ContainerStateRunning to that test solved the issue.
Would you mind adding a fix for the test along this lines:

diff --git a/libbeat/autodiscover/providers/kubernetes/kubernetes_test.go b/libbeat/autodiscover/providers/kubernetes/kubernetes_test.go
index fdd71e799..59e69038f 100644
--- a/libbeat/autodiscover/providers/kubernetes/kubernetes_test.go
+++ b/libbeat/autodiscover/providers/kubernetes/kubernetes_test.go
@@ -23,7 +23,7 @@ import (

        "github.com/gofrs/uuid"
        "github.com/stretchr/testify/assert"
-       "k8s.io/api/core/v1"
+       v1 "k8s.io/api/core/v1"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        "k8s.io/apimachinery/pkg/types"

@@ -198,6 +198,9 @@ func TestEmitEvent(t *testing.T) {
                                                {
                                                        Name:        name,
                                                        ContainerID: containerID,
+                                                       State: v1.ContainerState{
+                                                               Running: &v1.ContainerStateRunning{},
+                                                       },
                                                },
                                        },
                                },

(note: that "v1" import at my example depends on goimports configured at my IDE, no need to change that)

Copy link
Contributor

@odacremolbap odacremolbap left a comment

Choose a reason for hiding this comment

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

can you fix tests as depicted here?

@vjsamuel
Copy link
Contributor Author

@odacremolbap done. thanks for debugging the build failure.

@odacremolbap odacremolbap merged commit aaca48b into elastic:master Nov 12, 2019
@odacremolbap
Copy link
Contributor

@vjsamuel LGTM
thanks a ton for your involvement into beats

@odacremolbap odacremolbap removed the needs_backport PR is waiting to be backported to other branches. label Nov 12, 2019
odacremolbap pushed a commit to odacremolbap/beats that referenced this pull request Nov 12, 2019
odacremolbap pushed a commit to odacremolbap/beats that referenced this pull request Nov 12, 2019
odacremolbap pushed a commit that referenced this pull request Nov 12, 2019
odacremolbap pushed a commit that referenced this pull request Nov 12, 2019
…tailed after they stop (#14476)

* Ensure that init containers are no longer tailed after they stop (#14394)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…longer tailed after they stop (elastic#14476)

* Ensure that init containers are no longer tailed after they stop (elastic#14394)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug containers Related to containers use case Team:Integrations Label for the Integrations team v7.5.0 v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants