[aws/s3]: fix s3_input integration test #39648

pkoutsovasilis · 2024-05-21T14:56:42Z

Proposed commit message

This PR fixes the following errors captured https://github.com/elastic/ingest-dev/issues/3312

input/awss3/input_integration_test.go:138:46: undefined: s3Input
input/awss3/input_integration_test.go:144:19: undefined: s3Input
input/awss3/input_integration_test.go:238:14: undefined: s3Input

Initially these tests resulted as broken because of this PR #39353 which quoting from the description

This change is meant to have no functional effect, it is strictly a cleanup and reorganization in preparation for future changes. The hope is that the new layout makes initialization steps and logical dependencies clearer.

split up the pre-existing s3input to s3PollerInput and sqsReaderInput without reflecting this in the x-pack/filebeat/input/awss3/input_integration_test.go. However, even when applying the necessary changes in the former, another error surfaces. Specifically this one

| InternalErrorexception while calling sqs with unknown operation: Traceback (most recent call last):
| File "/opt/code/localstack/localstack/aws/chain.py", line 90, in handle
| handler(self, self.context, response)
| File "/opt/code/localstack/localstack/aws/handlers/service.py", line 64, in call
| return self.parse_and_enrich(context)
| File "/opt/code/localstack/localstack/aws/handlers/service.py", line 77, in parse_and_enrich
| operation, instance = parser.parse(context.request)
| File "/opt/code/localstack/localstack/aws/protocol/parser.py", line 172, in wrapper
| return func(*args, **kwargs)
| File "/opt/code/localstack/localstack/aws/protocol/parser.py", line 366, in parse
| raise ProtocolParserError(
| localstack.aws.protocol.parser.ProtocolParserError: Operation detection failed. Missing Action in request for query-protocol service ServiceModel(sqs).

I can trace the above only to the update of aws package dependencies which happened in this #39454 and it wasn't tested with aws label. To fix this error the image of localstack, spawned during the aws integration tests, had to be updated to 3.1.0

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

N/A

How to test this PR locally

Open a PR on beats CI and add the aws label

Related issues

Closes https://github.com/elastic/ingest-dev/issues/3312

Use cases

N/A

Screenshots

N/A

Logs

N/A

mergify · 2024-05-21T14:57:22Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @pkoutsovasilis? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

.ci/jobs/docker-compose.yml

elasticmachine · 2024-05-22T07:59:56Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

dliappis

LGTM

elasticmachine · 2024-05-22T08:23:03Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

bhapas · 2024-05-22T12:39:43Z

@pkoutsovasilis Thanks for taking this up... Can you attach the logs of test run locally.
Something like in this source PR

pkoutsovasilis · 2024-05-22T12:46:37Z

@pkoutsovasilis Thanks for taking this up... Can you attach the logs of test run locally. Something like in this source PR

I can attach them for sure @bhapas but why you need these particular logs attached here and not seeing that this test has run and passed from BK?

PS: not only TestInputRunSQSOnLocalstack was affected by #39353

bhapas · 2024-05-22T12:50:39Z

@pkoutsovasilis Thanks for taking this up... Can you attach the logs of test run locally. Something like in this source PR

I can attach them for sure @bhapas but why you need these particular logs attached here and not seeing that this test has run and passed from BK?

PS: not only TestInputRunSQSOnLocalstack was affected by #39353

Good Question.. I did not find the Cloud Module tests that failed before with -tags aws , integration . May be I got blind. Can you point me to the succesful run :-)

pkoutsovasilis · 2024-05-22T12:55:37Z

Good Question.. I did not find the Cloud Module tests that failed before with -tags aws , integration . May be I got blind. Can you point me to the succesful run :-)

I am getting mixed signals here, you want a failure or a success?! 😆

Either way, here you can see that TestInputRunSQSOnLocalstack PASSES now, and here you can see when it was failing, is that helpful @bhapas ?

bhapas · 2024-05-22T12:58:06Z

@pkoutsovasilis Thanks for taking this up... Can you attach the logs of test run locally. Something like in this source PR

I can attach them for sure @bhapas but why you need these particular logs attached here and not seeing that this test has run and passed from BK?
PS: not only TestInputRunSQSOnLocalstack was affected by #39353

Good Question.. I did not find the Cloud Module tests that failed before with -tags aws , integration . May be I got blind. Can you point me to the succesful run :-)

I am getting mixed signals here, you want a failure or a success?! 😆

Found a successful one though. Never mind :-)

belimawr · 2024-05-22T14:14:31Z

x-pack/filebeat/input/awss3/input_integration_test.go

@@ -353,7 +358,7 @@ func TestInputRunS3(t *testing.T) {
 	assert.EqualValues(t, s3Input.metrics.s3ObjectsRequestedTotal.Get(), 7)
 	assert.EqualValues(t, s3Input.metrics.s3ObjectsListedTotal.Get(), 8)
 	assert.EqualValues(t, s3Input.metrics.s3ObjectsProcessedTotal.Get(), 7)
-	assert.EqualValues(t, s3Input.metrics.s3ObjectsAckedTotal.Get(), 6)
+	assert.EqualValues(t, s3Input.metrics.s3ObjectsAckedTotal.Get(), 7)


Why did the number of acked objects changed?

first because it was failing but also this should be 7 to begin with, right???

"testdata/events-array.json",

"testdata/invalid.json",

"testdata/log.json",

"testdata/log.ndjson",

"testdata/multiline.json",

"testdata/multiline.json.gz",

"testdata/multiline.txt"

faec

Looks good, thanks!

it wasn't tested with aws label

Does this mean these tests are only run when the aws label is set? It looks like 2-3 unrelated errors accumulated here, if these tests aren't being run automatically on things that touch AWS code/libs then we should probably fix that.

faec · 2024-05-23T15:14:29Z

x-pack/filebeat/input/awss3/input_integration_test.go

@@ -402,7 +407,10 @@ func makeAWSConfig(t *testing.T, region string) aws.Config {

 func drainSQS(t *testing.T, region string, queueURL string, cfg aws.Config) {
 	sqs := &awsSQSAPI{
-		client:            sqs.NewFromConfig(cfg),
+		client: sqs.NewFromConfig(cfg, func(options *sqs.Options) {
+			//options.ClientLogMode = aws.LogResponseWithBody


Leftover comment?

comment so somebody can enable the client log mode and see what the client is responding with when tests are failing with an error like this one. you want it removed?

dliappis · 2024-05-23T15:24:31Z

Does this mean these tests are only run when the aws label is set? It looks like 2-3 unrelated errors accumulated here, if these tests aren't being run automatically on things that touch AWS code/libs then we should probably fix that.

Correct. This was the behavior for some time on Jenkins already (see https://github.com/elastic/observability-dev/blob/main/docs/dc/beats-ci/README.md#github-labels) and it was migrated over to Buildkite too. We had a discussion with @pierrehilbert and the platform ingest eng prod team about improvements and not having to rely on users to remember to use the right label on such PRs.

zmoog · 2024-05-24T08:20:46Z

Does this mean these tests are only run when the aws label is set? It looks like 2-3 unrelated errors accumulated here, if these tests aren't being run automatically on things that touch AWS code/libs then we should probably fix that.

Correct. This was the behavior for some time on Jenkins already (see https://github.com/elastic/observability-dev/blob/main/docs/dc/beats-ci/README.md#github-labels) and it was migrated over to Buildkite too. We had a discussion with @pierrehilbert and the platform ingest eng prod team about improvements and not having to rely on users to remember to use the right label on such PRs.

Interesting!

So, should we add the aws label when creating the PR to enable AWS SDK dependencies update with Dependabot?

    labels:
      - automation
      - dependabot
      - aws

I created the first PR for Azure, but PRs to manage AWS and GCP dependencies with Dependabot are coming.

pkoutsovasilis added the aws Enable builds in the CI for aws cloud testing label May 21, 2024

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 21, 2024

mergify bot assigned pkoutsovasilis May 21, 2024

pkoutsovasilis changed the title ~~fix: s3_input integration test~~ [aws/s3]: fix s3_input integration test May 21, 2024

dliappis mentioned this pull request May 21, 2024

[aws] Remove unused function newV2Context from input_test.go #39538

Merged

6 tasks

pkoutsovasilis force-pushed the pkoutsovasilis/fix_s3_input branch from 2122959 to aa33c4c Compare May 21, 2024 17:45

fix: s3_input integration tests

f0eb390

pkoutsovasilis force-pushed the pkoutsovasilis/fix_s3_input branch from e1deba5 to f0eb390 Compare May 22, 2024 06:07

dliappis reviewed May 22, 2024

View reviewed changes

.ci/jobs/docker-compose.yml Show resolved Hide resolved

pkoutsovasilis added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label May 22, 2024

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 22, 2024

pkoutsovasilis marked this pull request as ready for review May 22, 2024 07:59

pkoutsovasilis requested review from a team as code owners May 22, 2024 07:59

pkoutsovasilis requested review from belimawr and fearful-symmetry May 22, 2024 07:59

botelastic bot added the Team:Automation Label for the Observability productivity team label May 22, 2024

dliappis self-requested a review May 22, 2024 08:02

dliappis approved these changes May 22, 2024

View reviewed changes

pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label May 22, 2024

pierrehilbert requested a review from faec May 22, 2024 08:23

belimawr reviewed May 22, 2024

View reviewed changes

faec approved these changes May 23, 2024

View reviewed changes

zmoog approved these changes May 24, 2024

View reviewed changes

pkoutsovasilis merged commit 566d3bf into elastic:main May 24, 2024
18 checks passed

pkoutsovasilis deleted the pkoutsovasilis/fix_s3_input branch May 24, 2024 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aws/s3]: fix s3_input integration test #39648

[aws/s3]: fix s3_input integration test #39648

pkoutsovasilis commented May 21, 2024 •

edited

Loading

mergify bot commented May 21, 2024

elasticmachine commented May 22, 2024

dliappis left a comment

elasticmachine commented May 22, 2024

bhapas commented May 22, 2024

pkoutsovasilis commented May 22, 2024 •

edited

Loading

bhapas commented May 22, 2024 •

edited

Loading

pkoutsovasilis commented May 22, 2024 •

edited

Loading

bhapas commented May 22, 2024

belimawr May 22, 2024

pkoutsovasilis May 22, 2024 •

edited

Loading

faec left a comment

faec May 23, 2024

pkoutsovasilis May 23, 2024 •

edited

Loading

dliappis commented May 23, 2024

zmoog commented May 24, 2024 •

edited

Loading

[aws/s3]: fix s3_input integration test #39648

[aws/s3]: fix s3_input integration test #39648

Conversation

pkoutsovasilis commented May 21, 2024 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented May 21, 2024

elasticmachine commented May 22, 2024

dliappis left a comment

Choose a reason for hiding this comment

elasticmachine commented May 22, 2024

bhapas commented May 22, 2024

pkoutsovasilis commented May 22, 2024 • edited Loading

bhapas commented May 22, 2024 • edited Loading

pkoutsovasilis commented May 22, 2024 • edited Loading

bhapas commented May 22, 2024

belimawr May 22, 2024

Choose a reason for hiding this comment

pkoutsovasilis May 22, 2024 • edited Loading

Choose a reason for hiding this comment

faec left a comment

Choose a reason for hiding this comment

faec May 23, 2024

Choose a reason for hiding this comment

pkoutsovasilis May 23, 2024 • edited Loading

Choose a reason for hiding this comment

dliappis commented May 23, 2024

zmoog commented May 24, 2024 • edited Loading

pkoutsovasilis commented May 21, 2024 •

edited

Loading

pkoutsovasilis commented May 22, 2024 •

edited

Loading

bhapas commented May 22, 2024 •

edited

Loading

pkoutsovasilis commented May 22, 2024 •

edited

Loading

pkoutsovasilis May 22, 2024 •

edited

Loading

pkoutsovasilis May 23, 2024 •

edited

Loading

zmoog commented May 24, 2024 •

edited

Loading