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

[testing/integration]: fix TestLogIngestionFleetManaged #5648

Merged
merged 11 commits into from
Oct 10, 2024

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Oct 1, 2024

Looking at diagnostics of #5631 failure, it was clear that the monitoring units were not healthy. This is a known issue and it takes some time for them to be healthy.

This PR aims to fix that by making sure the components are healthy before proceeding with the test case.

@VihasMakwana VihasMakwana added Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team skip-changelog labels Oct 1, 2024
Copy link
Contributor

mergify bot commented Oct 1, 2024

This pull request does not have a backport label. Could you fix it @VihasMakwana? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Oct 1, 2024

backport-v8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 1, 2024
@VihasMakwana VihasMakwana force-pushed the fix-log-ingestion-tests branch from b20e2f9 to c3f7b0e Compare October 1, 2024 11:42
@VihasMakwana
Copy link
Contributor Author

/test

@VihasMakwana VihasMakwana marked this pull request as ready for review October 1, 2024 13:49
@VihasMakwana VihasMakwana requested a review from a team as a code owner October 1, 2024 13:49
@VihasMakwana
Copy link
Contributor Author

Will run this 4-5 times and observer the CI.

@VihasMakwana
Copy link
Contributor Author

/test

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Oct 1, 2024

succeeded 2/2 times. Triggered 3rd time

@VihasMakwana
Copy link
Contributor Author

TestLogIngestionFleetManaged succeeded 3/3

@VihasMakwana
Copy link
Contributor Author

/test

@cmacknz
Copy link
Member

cmacknz commented Oct 1, 2024

I think we should also decrease the metrics interval used in the test to 1s, this will effectively be speeding up and allowing for retries if we hit this failure on the first attempt to collect metrics.

#5631 (comment)

@ycombinator ycombinator requested review from pchila and removed request for pkoutsovasilis and andrzej-stencel October 1, 2024 23:55
@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Oct 3, 2024

I think we should also decrease the metrics interval used in the test to 1s, this will effectively be speeding up and allowing for retries if we hit this failure on the first attempt to collect metrics.

#5631 (comment)

This particular test creates policy in fleet and the policy schema doesn't really have any metrics_period field.

EDIT: I think we can do that. I'll try that and update the PR.

EDIT2: We can override elastic-agent.yml, but doing so would contradict the purpose of this fleet-managed test.

Co-authored-by: Paolo Chilà <paolo.chila@elastic.co>
@VihasMakwana
Copy link
Contributor Author

I think we should also decrease the metrics interval used in the test to 1s, this will effectively be speeding up and allowing for retries if we hit this failure on the first attempt to collect metrics.

#5631 (comment)

This was the first thing I wanted to do, as we've done the same with Monitoring* integration test cases by overriding the elastic-agent.yml

@VihasMakwana VihasMakwana requested a review from swiatekm October 3, 2024 14:12
@VihasMakwana VihasMakwana requested a review from pchila October 3, 2024 14:12
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good.

@cmacknz
Copy link
Member

cmacknz commented Oct 3, 2024

EDIT2: We can override elastic-agent.yml, but doing so would contradict the purpose of this fleet-managed test.

This is fine, if you want to properly do it through Fleet you can use the policy overrides API. See https://support.elastic.dev/knowledge/view/06b69893 for an example.

@cmacknz
Copy link
Member

cmacknz commented Oct 3, 2024

(I don't think it matters for the purpose of what we are trying to test here whether you change the initial local agent configuration or use the overrides API to do this).

Checking for health at startup makes sure we identify the right error, but it roughly equivalent to just waiting longer for us to get healthy. If we increase the metrics interval the whole test should execute faster.

VihasMakwana added a commit to elastic/elastic-agent-libs that referenced this pull request Oct 4, 2024
go.mod Outdated
@@ -14,7 +14,7 @@ require (
github.com/dolmen-go/contextio v0.0.0-20200217195037-68fc5150bcd5
github.com/elastic/elastic-agent-autodiscover v0.8.2
github.com/elastic/elastic-agent-client/v7 v7.16.0
github.com/elastic/elastic-agent-libs v0.10.1
github.com/elastic/elastic-agent-libs v0.12.1-0.20241004133534-196dace11604
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a minor release once the CI is green on this commit.

Copy link
Contributor

mergify bot commented Oct 4, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-log-ingestion-tests upstream/fix-log-ingestion-tests
git merge upstream/main
git push upstream fix-log-ingestion-tests

@VihasMakwana
Copy link
Contributor Author

VihasMakwana commented Oct 8, 2024

CI is red.
BLOCKED on #5542

@AndersonQ
Copy link
Member

AndersonQ commented Oct 8, 2024

CI is red. BLOCKED on #5542

I'm fixing it with #5741
the issue is with the test, not with github.com/elastic/elastic-agent-libs v0.12.0

@blakerouse blakerouse enabled auto-merge (squash) October 10, 2024 00:00
Copy link

@blakerouse blakerouse merged commit 26f3d59 into elastic:main Oct 10, 2024
14 checks passed
mergify bot pushed a commit that referenced this pull request Oct 10, 2024
fix TestLogIngestionFleetManaged

---------

Co-authored-by: Paolo Chilà <paolo.chila@elastic.co>
Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
(cherry picked from commit 26f3d59)

# Conflicts:
#	go.mod
#	go.sum
blakerouse added a commit that referenced this pull request Oct 10, 2024
…tManaged (#5751)

* [testing/integration]: fix TestLogIngestionFleetManaged (#5648)

fix TestLogIngestionFleetManaged

---------

Co-authored-by: Paolo Chilà <paolo.chila@elastic.co>
Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
(cherry picked from commit 26f3d59)

# Conflicts:
#	go.mod
#	go.sum

* Update go.mod

* Update go.sum

* Fix notice.

---------

Co-authored-by: VihasMakwana <121151420+VihasMakwana@users.noreply.github.com>
Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co>
Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify skip-changelog Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flaky Test]: TestLogIngestionFleetManaged – Condition never satisfied
6 participants