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.

Sign up for GitHub

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

Jump to bottom

[observer/ecs] Don't report EC2 tasks with unassigned container instances #23279

Merged
merged 1 commit into from
Jul 12, 2023
Merged

[observer/ecs] Don't report EC2 tasks with unassigned container instances #23279

merged 1 commit into from
Jul 12, 2023

Conversation

shchuko
Copy link
Contributor

@shchuko shchuko commented Jun 12, 2023

When ECS task is in state Provisioning/Pending, it can contain container(s) which don't have EC2 instance yet. Such containers have nil instance arn.

This change fixes service discovery error:

describe container instanced failed offset=0: ecs.DescribeContainerInstance
failed: InvalidParameterException: Container instance can not be blank.
{"kind": "extension", "name": "ecs_observer", "ErrScope": "Unknown"}

Testing: Related unit test is added.

@shchuko shchuko requested review from a team and dmitryax as code owners June 12, 2023 16:44
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 12, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: shchuko / name: Vladislav Yaroshchuk (3459788)

@shchuko
Copy link
Contributor Author

shchuko commented Jun 12, 2023 •
edited
Loading

❌ - login: @shchuko / name: Vladislav Yaroshchuk . The commit (f58a9b8) is not authorized under a signed CLA.

I'm in progress consulting with my employer, trying to fix ASAP

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 27, 2023
@shchuko
Copy link
Contributor Author

shchuko commented Jun 28, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

Signing CLA takes a bit longer than expected, but PR is not forgotten

@dmitryax dmitryax removed the Stale label Jun 28, 2023
@dmitryax
Copy link
Member

@shchuko, thank you. please sign the CLA

@shchuko
Copy link
Contributor Author

shchuko commented Jun 29, 2023

Thanks for your review!

please sign the CLA

Hope to sign it next week, waiting for our legal department response

@dmitryax
Copy link
Member

Also, there are some linter issues. PTAL

@shchuko
Copy link
Contributor Author

shchuko commented Jun 29, 2023

Linter reported a problem:

Error: ecsobserver/fetcher_test.go:108:3: commentFormatting: put a space between `//` and comment text (gocritic)
		//Expect 2 tasks, with LaunchType Fargate and EC2 with non-nil ContainerInstanceArn

Fixed, force-pushed

@shchuko
Copy link
Contributor Author

shchuko commented Jul 7, 2023

@dmitryax I've signed the CLA, but build-and-test / publish-check (pull_request) check has failed. Could I ask you to re-run checks, please?

@shchuko
Copy link
Contributor Author

shchuko commented Jul 11, 2023

I don't have sufficient permissions to merge, seems it should be done by authorized person

You’re not authorized to merge this pull request.

@TylerHelmuth
Copy link
Member

@shchuko we had some CI issue recently that are now fixed, please update your branch with the latest from main.

@shchuko
[observer/ecs] Don't report tasks if EC2 launch type and missing cont…
3459788 
…ainer instances

When ECS task of EC2 launch type is in state Provisioning/Pending,
it may not have EC2 instance.
Such tasks have `nil` instance arn and the attachContainerInstance
call will fail.

This change fixes an error:
```error [email protected]/error.go:77 attachContainerInstance failed:
describe container instanced failed offset=0: ecs.DescribeContainerInstance
failed: InvalidParameterException: Container instance can not be blank.
{"kind": "extension", "name": "ecs_observer", "ErrScope": "Unknown"}
```
@shchuko
Copy link
Contributor Author

shchuko commented Jul 12, 2023

Rebased on main, typo fix is squashed with original commit

@shchuko
Copy link
Contributor Author

shchuko commented Jul 12, 2023 •
edited
Loading

I see new linter check problem, but can't reproduce it on my machine with make -j2 golint GROUP=other. Make exits without errors with code 0.

@TylerHelmuth TylerHelmuth merged commit da15e5f into open-telemetry:main Jul 12, 2023
91 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 12, 2023
@shchuko shchuko deleted the yaroshchuk/ecsobserver-nil-container-arn-fix branch July 12, 2023 22:51
fyuan1316 pushed a commit to fyuan1316/opentelemetry-collector-contrib that referenced this pull request Jul 13, 2023
@shchuko @fyuan1316
[observer/ecs] Don't report EC2 tasks with unassigned container insta…
a733845 
…nces (open-telemetry#23279)

When ECS task is in state Provisioning/Pending, it can contain
container(s) which don't have EC2 instance yet. Such containers have
`nil` instance arn.

This change fixes service discovery error:
```error [email protected]/error.go:77 attachContainerInstance failed:
describe container instanced failed offset=0: ecs.DescribeContainerInstance
failed: InvalidParameterException: Container instance can not be blank.
{"kind": "extension", "name": "ecs_observer", "ErrScope": "Unknown"}
```


**Testing:** Related unit test is added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax approved these changes

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

Assignees

@TylerHelmuth TylerHelmuth

Labels
extension/observer
Projects
None yet
Milestone
v0.82.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shchuko @dmitryax @TylerHelmuth