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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kafka on EKS - EKS Bluprints V5 Mitigation #169

Merged
merged 17 commits into from
Jun 20, 2023

Conversation

AlyIbrahim
Copy link
Contributor

What does this PR do?

馃洃 Please open an issue first to discuss any significant work and flesh out details/direction - we would hate for your time to be wasted.
Consult the CONTRIBUTING guide for submitting pull-requests.

Updating Kafka-on-EKS patterns by:

  • Removing EKS Blueprints dependencies
  • Updating Strimzi Operator to v0.34
  • Update Kafka to v3.4

Motivation

EKS Blueprints v5 upgrade

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Mandatory for new blueprints. Yes, I have added a example to support my blueprint PR
  • Mandatory for new blueprints. Yes, I have updated the website/docs or website/blog section for this feature
  • Yes, I ran pre-commit run -a with this PR. Link for installing pre-commit locally

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@AlyIbrahim AlyIbrahim requested a review from a team as a code owner March 31, 2023 16:17
@AlyIbrahim AlyIbrahim temporarily deployed to DoEKS Test March 31, 2023 16:17 — with GitHub Actions Inactive
@AlyIbrahim AlyIbrahim changed the title Kafka v5 Kafka on EKS - EKS Bluprints V5 Mitigation Mar 31, 2023
resource "aws_eks_addon" "vpc_cni" {
cluster_name = module.eks.cluster_name
addon_name = "vpc-cni"
service_account_role_arn = module.vpc_cni_irsa.iam_role_arn
Copy link
Contributor

Choose a reason for hiding this comment

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

add preserve=true to ensure VPC CNI is not fully deleted before the EKS Cluster. VPC CNI sometimes required to cleanup the existing add-ons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

Thanks @AlyIbrahim ! PR Looks good. few minor comment.
In addition.
1/ Please add install and cleanup scripts like other blueprints
2/ Raise an issue to create a Architecture diagram for this pattern

Thanks 馃憤馃徏


resource "aws_eks_addon" "coredns" {
cluster_name = module.eks.cluster_name
addon_name = "coredns"
Copy link
Contributor

Choose a reason for hiding this comment

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

add preserve=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


resource "aws_eks_addon" "kube_proxy" {
cluster_name = module.eks.cluster_name
addon_name = "kube-proxy"
Copy link
Contributor

Choose a reason for hiding this comment

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

add preserve=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


#---------------------------------------------------------------
# Cluster Autoscaler
#---------------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

nit:remove extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -169,85 +114,115 @@ provisioner: ebs.csi.aws.com
reclaimPolicy: Delete
volumeBindingMode: WaitForFirstConsumer
YAML
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a dependency to on EKS module for this storage class resource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 196 to 197
yaml_body = file("./monitoring-manifests/podmonitor-cluster-operator-metrics.yml")
depends_on = [kubectl_manifest.kafka_namespace]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting. add two spaces. Pre-commit didn't pickup thisone?
same for all the resources below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 52 to 68
# ingress_fsx1 = {
# description = "Allows Lustre traffic between Lustre clients"
# cidr_blocks = module.vpc.private_subnets_cidr_blocks
# from_port = 1021
# to_port = 1023
# protocol = "tcp"
# type = "ingress"
# }

# ingress_fsx2 = {
# description = "Allows Lustre traffic between Lustre clients"
# cidr_blocks = module.vpc.private_subnets_cidr_blocks
# from_port = 988
# to_port = 988
# protocol = "tcp"
# type = "ingress"
# }
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the commented lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@AlyIbrahim AlyIbrahim temporarily deployed to DoEKS Test April 16, 2023 15:17 — with GitHub Actions Inactive
@AlyIbrahim AlyIbrahim temporarily deployed to DoEKS Test April 16, 2023 15:37 — with GitHub Actions Inactive
@AlyIbrahim AlyIbrahim temporarily deployed to DoEKS Test April 16, 2023 16:28 — with GitHub Actions Inactive
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @AlyIbrahim ! LGTM 馃憤馃徏

@AlyIbrahim AlyIbrahim temporarily deployed to DoEKS Test June 20, 2023 15:35 — with GitHub Actions Inactive
Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

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

LGTM 馃憤馃徏 Thanks for the PR 馃殌

@vara-bonthu vara-bonthu merged commit fb43ea6 into awslabs:main Jun 20, 2023
52 of 55 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.

None yet

2 participants