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

OSS pattern for EKS Fargate new cluster #106

Merged
merged 14 commits into from
Nov 10, 2023

Conversation

ratnopamc
Copy link
Contributor

Issue #, if available:
#51
Description of changes:
This PR contains the code for single new eks cluster on AWS Fargate.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@ratnopamc Thanks for the draft PR. Docs is missing, mkdocs change is missing, and other feedback listed below. Also fix all lint errors

@elamaran11
Copy link
Contributor

@ratnopamc Any progress on this PR?

@ratnopamc
Copy link
Contributor Author

@elamaran11, The PR is ready for review. Added the documentation with steps for testing with sample java workload.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@ratnopamc AWSome work, thankyou for working on this much needed pattern. Have some tactical comments and feedback which can be addressed, overall feedback

  1. Doc needs a beefup about what is fargate, why we need this pattern, more details on AMP, AMG services uses and monitoring about Fargate. Use the Observability best practices guide for more info.
  2. Architecture Diagram is must.
  3. Since we are updating the collector config, please make sure you run OSS and Graviton/GPU pattern once and show evidence it works and you can see metrics in AMG.

@@ -0,0 +1,75 @@
import 'source-map-support/register';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove this file and reuse the file from OSS pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not taken care?

Copy link
Contributor

Choose a reason for hiding this comment

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

done

constructor(scope: Construct, id: string) {
const stackId = `${id}-observability-accelerator`;


Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra space

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not taken care.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

values: { webhook: { port: 9443 } }
}),
new blueprints.addons.GrafanaOperatorAddon({
version: 'v5.0.0-rc3'
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this, we have upgrade GO to latest versionin blueprints

Copy link
Contributor

Choose a reason for hiding this comment

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

Not taken care?

mkdocs.yml Outdated
@@ -42,6 +42,7 @@ nav:
- OSS GPU: patterns/single-new-eks-observability-accelerators/single-new-eks-gpu-opensource-observability.md
- OSS Java Mon : patterns/single-new-eks-observability-accelerators/single-new-eks-java-opensource-observability.md
- OSS Nginx Mon : patterns/single-new-eks-observability-accelerators/single-new-eks-nginx-opensource-observability.md
- EKS Fargate OSS: patterns/single-new-eks-observability-accelerators/single-new-eks-fargate-opensource-observability.md
Copy link
Contributor

Choose a reason for hiding this comment

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

lets create a Separate folder for EKS Fargate and under that add this. Also move AWS Native Fargate pattern too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not taken care

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@elamaran11 elamaran11 linked an issue Oct 23, 2023 that may be closed by this pull request
@ratnopamc
Copy link
Contributor Author

/do-e2e-test

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla @ratnopamc Great work, i see lot of improvement and i love the Architecture and documentation updates. Still following needs to be taken care

  1. This PR is dependent on this PR in GitOps repo which has GitHub Action errors
  2. The PR still has code comments which is not take care, please take care of those
  3. The PR still has questions which needs responses

once you take care of all of these, we can move to e2e and merge.

@@ -0,0 +1,75 @@
import 'source-map-support/register';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not taken care?

constructor(scope: Construct, id: string) {
const stackId = `${id}-observability-accelerator`;


Copy link
Contributor

Choose a reason for hiding this comment

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

This is not taken care.

values: { webhook: { port: 9443 } }
}),
new blueprints.addons.GrafanaOperatorAddon({
version: 'v5.0.0-rc3'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not taken care?

mkdocs.yml Outdated
@@ -42,6 +42,7 @@ nav:
- OSS GPU: patterns/single-new-eks-observability-accelerators/single-new-eks-gpu-opensource-observability.md
- OSS Java Mon : patterns/single-new-eks-observability-accelerators/single-new-eks-java-opensource-observability.md
- OSS Nginx Mon : patterns/single-new-eks-observability-accelerators/single-new-eks-nginx-opensource-observability.md
- EKS Fargate OSS: patterns/single-new-eks-observability-accelerators/single-new-eks-fargate-opensource-observability.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Not taken care

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@Howlla Looks great just one feedback. Remove the -new.yaml collector config file always from checkin or add it to gitignore. Running e2e but this needs to be fixed and also this PR to be fixed and merged before merge.

Also replace XRAYADOT to XRAY Addon in AWS Native Fargate pattern

lib/common/resources/otel-collector-config-new.yml Outdated Show resolved Hide resolved
@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-fargate-opensource-observability deploy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-opensource-observability deploy

@Howlla
Copy link
Contributor

Howlla commented Nov 10, 2023

updated gitignore

@elamaran11
Copy link
Contributor

@Howlla AWS Native Fargate pattern should be updated to replace XrayAdotAddOn with XrayAddOn thats a bug in the previous pattern. Im running e2e, but this needs to be fixed before merge.

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-fargate-opensource-observability deploy

@elamaran11
Copy link
Contributor

/do-e2e-test single-new-eks-fargate-opensource-observability destroy

@elamaran11
Copy link
Contributor

e2e error, no issues with the pattern, merging the pattern

@elamaran11 elamaran11 merged commit e8afb45 into aws-observability:main Nov 10, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Source EKS on Fargate Monitoring
3 participants