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

Update go-discover to support ECS discovery #13782

Merged
merged 18 commits into from Jan 12, 2023

Conversation

fdr2
Copy link
Contributor

@fdr2 fdr2 commented Jul 15, 2022

Description

Go-Discover now supports ECS container instance discovery. This change updates the go-discover dependency and corresponding documentation outlining new options.

Testing & Reproduction steps

test/load-ecs has been added with load testing capabilities for 25 vus at 10 minutes. see test/load-ecs/README.md for more info. test/load-ecs has been moved to terraform-aws-consul-ecs PR:139

Links

Issue: GH-6802
Dependency: PR-197

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@github-actions github-actions bot added pr/dependencies PR specifically updates dependencies of project type/docs Documentation needs to be created/updated/clarified labels Jul 15, 2022
@Amier3
Copy link
Contributor

Amier3 commented Jul 18, 2022

Hey @fdr2

Thanks for making this, we're in a bit of crunch-time right now gearing up for the next consul release, so i'd expect us to get to this after the release in early august.

@fdr2
Copy link
Contributor Author

fdr2 commented Jul 18, 2022

Sounds great @Amier3 thanks for the feedback. I'm hoping to investigate the performance concerns raised regarding efs, currently working out load tests with k6 and datadog in a similar fashion to the ec2 load test

@fdr2 fdr2 marked this pull request as ready for review August 11, 2022 00:38
@fdr2 fdr2 requested a review from a team as a code owner August 11, 2022 00:38
Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Added some suggestions for the docs -- mostly edits for consistency with the style guide.

website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
website/content/docs/install/cloud-auto-join.mdx Outdated Show resolved Hide resolved
fdr2 and others added 3 commits September 1, 2022 08:24
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
add datadog api key
improve makefile
@fdr2
Copy link
Contributor Author

fdr2 commented Sep 1, 2022

Added some suggestions for the docs -- mostly edits for consistency with the style guide.

Thank you! These are excellent improvements. I've incorporated them all.

@pglass pglass self-requested a review September 13, 2022 19:37
@pglass
Copy link

pglass commented Sep 13, 2022

Hi @fdr2, this is great work! Thank you.

We'd prefer to put the load test code for ECS into our ECS-focused repository: https://github.com/hashicorp/terraform-aws-consul-ecs/

The path forward I'd suggest is:

  1. Trim down this PR to only include the version bump of the go-discover dependency and the associated documentation changes in the website directory. This should enable us to get the change in prior to our upcoming code freeze for the next release of Consul.
  2. Then, open PR(s) to https://github.com/hashicorp/terraform-aws-consul-ecs/ containing the load test code, and add me as a reviewer. I'd like to potentially extract your Terraform code for deploying the Consul servers on ECS into a new Terraform module there, and include your load test code over there as well.

I know this is a lot to ask, so thanks for your patience 🙂. Let me know if you have any questions.

fdr2 and others added 6 commits November 1, 2022 09:08
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
add datadog api key
improve makefile
@fdr2 fdr2 requested review from a team as code owners November 8, 2022 14:14
@fdr2 fdr2 requested review from sarahethompson and modrake and removed request for a team November 8, 2022 14:14
go.mod Outdated Show resolved Hide resolved
@pglass pglass added backport/1.13 Changes are backported to 1.13 backport/1.12 Changes are backported to 1.12 labels Nov 17, 2022
@pglass
Copy link

pglass commented Nov 17, 2022

Hi @fdr2, thanks for your patience on this!

Can you please add a changelog entry that indicates the support for ECS discovery? There are instructions here for including a changelog entry

We were in code freeze at the time, so I wasn't able to include this in Consul 1.14.0, but this can go out in the next patch release(s) of 1.12, 1.13, and 1.14 after this is merged.

@pglass pglass added the backport/1.14 Changes are backported to 1.14 label Nov 18, 2022
@pglass pglass merged commit 59a3a07 into hashicorp:main Jan 12, 2023
pglass pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
pglass pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
pglass pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
pglass pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: Frank DiRocco <160608+fdr2@users.noreply.github.com>
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
pglass pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: Frank DiRocco <160608+fdr2@users.noreply.github.com>
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
pglass pushed a commit that referenced this pull request Jan 12, 2023
Co-authored-by: Frank DiRocco <160608+fdr2@users.noreply.github.com>
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
skpratt pushed a commit that referenced this pull request Jan 25, 2023
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.12 Changes are backported to 1.12 backport/1.13 Changes are backported to 1.13 backport/1.14 Changes are backported to 1.14 pr/dependencies PR specifically updates dependencies of project type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants