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

Avoid using hardcoded value for max pod per node #1

Closed
tanmng opened this issue Jun 7, 2018 · 5 comments
Closed

Avoid using hardcoded value for max pod per node #1

tanmng opened this issue Jun 7, 2018 · 5 comments
Assignees

Comments

@tanmng
Copy link

tanmng commented Jun 7, 2018

Right now in the user-data script we have

sed -i s,MAX_PODS,20,g /etc/systemd/system/kubelet.service

The value 20 is hardcoded right now. Since AWS released the numbers in their CloudFormation template, I think we can extract the value and use a lookup function to get the proper value.

A proposal:

locals {
  # Mapping from the node type that we selected and the max number of pods that it can run
  # Taken from https://amazon-eks.s3-us-west-2.amazonaws.com/1.10.3/2018-06-05/amazon-eks-nodegroup.yaml
  max_pod_per_node = {
    c4.large    = 29
    c4.xlarge   = 58
    c4.2xlarge  = 58
    c4.4xlarge  = 234
    c4.8xlarge  = 234
    c5.large    = 29
    c5.xlarge   = 58
    c5.2xlarge  = 58
    c5.4xlarge  = 234
    c5.9xlarge  = 234
    c5.18xlarge = 737
    i3.large    = 29
    i3.xlarge   = 58
    i3.2xlarge  = 58
    i3.4xlarge  = 234
    i3.8xlarge  = 234
    i3.16xlarge = 737
    m3.medium   = 12
    m3.large    = 29
    m3.xlarge   = 58
    m3.2xlarge  = 118
    m4.large    = 20
    m4.xlarge   = 58
    m4.2xlarge  = 58
    m4.4xlarge  = 234
    m4.10xlarge = 234
    m5.large    = 29
    m5.xlarge   = 58
    m5.2xlarge  = 58
    m5.4xlarge  = 234
    m5.12xlarge = 234
    m5.24xlarge = 737
    p2.xlarge   = 58
    p2.8xlarge  = 234
    p2.16xlarge = 234
    p3.2xlarge  = 58
    p3.8xlarge  = 234
    p3.16xlarge = 234
    r3.xlarge   = 58
    r3.2xlarge  = 58
    r3.4xlarge  = 234
    r3.8xlarge  = 234
    r4.large    = 29
    r4.xlarge   = 58
    r4.2xlarge  = 58
    r4.4xlarge  = 234
    r4.8xlarge  = 234
    r4.16xlarge = 737
    t2.small    = 8
    t2.medium   = 17
    t2.large    = 35
    t2.xlarge   = 44
    t2.2xlarge  = 44
    x1.16xlarge = 234
    x1.32xlarge = 234
  }

  workers_userdata = <<USERDATA
#!/bin/bash -xe
CA_CERTIFICATE_DIRECTORY=/etc/kubernetes/pki
CA_CERTIFICATE_FILE_PATH=$CA_CERTIFICATE_DIRECTORY/ca.crt
mkdir -p $CA_CERTIFICATE_DIRECTORY
echo "${aws_eks_cluster.this.certificate_authority.0.data}" | base64 -d >  $CA_CERTIFICATE_FILE_PATH
INTERNAL_IP=$(curl -s http://169.254.169.254/latest/meta-data/local-ipv4)
sed -i s,MASTER_ENDPOINT,${aws_eks_cluster.this.endpoint},g /var/lib/kubelet/kubeconfig
sed -i s,CLUSTER_NAME,${var.cluster_name},g /var/lib/kubelet/kubeconfig
sed -i s,REGION,${data.aws_region.current.name},g /etc/systemd/system/kubelet.service
sed -i s,MAX_PODS,${lookup(local.max_pod_per_node, var. workers_instance_type)},g /etc/systemd/system/kubelet.service
sed -i s,MASTER_ENDPOINT,${aws_eks_cluster.this.endpoint},g /etc/systemd/system/kubelet.service
sed -i s,INTERNAL_IP,$INTERNAL_IP,g /etc/systemd/system/kubelet.service
DNS_CLUSTER_IP=10.100.0.10
if [[ $INTERNAL_IP == 10.* ]] ; then DNS_CLUSTER_IP=172.20.0.10; fi
sed -i s,DNS_CLUSTER_IP,$DNS_CLUSTER_IP,g /etc/systemd/system/kubelet.service
sed -i s,CERTIFICATE_AUTHORITY_FILE,$CA_CERTIFICATE_FILE_PATH,g /var/lib/kubelet/kubeconfig
sed -i s,CLIENT_CA_FILE,$CA_CERTIFICATE_FILE_PATH,g  /etc/systemd/system/kubelet.service
systemctl daemon-reload
systemctl restart kubelet kube-proxy
USERDATA
}

@brandoconnor Please let me know if this is OK, I'll create a fork and a pull request later

@brandonjbjelland brandonjbjelland self-assigned this Jun 7, 2018
@brandonjbjelland
Copy link
Contributor

Hey @tanmng ,

Thanks and good catch! Absolutely follow through with a PR (in the way you've suggested) when you have a chance. Admittedly, I hadn't gone through the userdata script with a fine-tooth comb just yet. I'll keep an eye out for your PR and cut the first release of the project once these low-hanging bugs are wrapped up. 👍

@brandonjbjelland
Copy link
Contributor

Thanks again! I shipped this change just now and am in the midst of releasing 0.1.0.

@antonbabenko
Copy link
Member

@brandoconnor Great work with starting EKS module!

@tanmng
Copy link
Author

tanmng commented Jun 7, 2018

@brandoconnor sorry it was a bit late so I didn't catch your message earlier.

I'm opening several PRs right now. Most of the content are taken from my hack last night

Cheers,

RothAndrew added a commit to saic-devsecops/terraform-aws-eks that referenced this issue Dec 10, 2018
…x_#187_for_windows_compatibility

Feature/fix terraform-aws-modules#187 for windows compatibility
max-rocket-internet pushed a commit that referenced this issue Dec 13, 2018
* Added map_roles_count and user_roles_count (#1)

* Update readme for new vars

* updated tests to include count

* fix syntax error

* updated changelog

* Added map_accounts_count variable for consistency

* Fix counts in example and user latest terraform-docs to generate readme
max-rocket-internet pushed a commit that referenced this issue Jun 19, 2019
* run terraform upgrade tool

* fix post upgrade TODOs

* use strict typing for variables

* upgrade examples, point them at VPC module tf 0.12 PR

* remove unnecessary `coalesce()` calls

coalesce(lookup(map, key, ""), default) -> lookup(map, key, default)

* Fix autoscaling_enabled broken (#1)

* always set a value for tags, fix coalescelist calls

* always set a value for these tags

* fix tag value

* fix tag value

* default element available

* added default value

* added a general default

without this default - TF is throwing an error when running a destroy

* Fix CI

* Change vpc module back to `terraform-aws-modules/vpc/aws` in example

* Update CHANGELOG.md

* Change type of variable `cluster_log_retention_in_days` to number

* Remove `xx_count` variables

* Actual lists instead of strings with commas

* Remove `xx_count` variable from docs

* Replace element with list indexing

* Change variable `worker_group_tags` to a attribute of worker_group

* Fix workers_launch_template_mixed tags

* Change override_instance_type_x variables to list.

* Update CHANGELOG.md
max-rocket-internet pushed a commit that referenced this issue Jan 27, 2020
…701)

* Configurable local exec command for waiting until cluster is healthy

* readme

* line feeds

* format

* fix readme

* fix readme

* Configurable local exec command for waiting until cluster is healthy (#1)

* Configurable local exec command for waiting until cluster is healthy

* readme

* line feeds

* format

* fix readme

* fix readme

* change log

* Configurable local exec wait 4 cluster op (#2)

* Configurable local exec command for waiting until cluster is healthy

* readme

* line feeds

* format

* fix readme

* fix readme

* change log

* changelog (#3)

* Changelog (#4)

* changelog

* changelog

* simplify wait_for_cluster command

* readme

* no op for manage auth false

* formatting

* docs? not sure

* linter

* specify dependency to wait for cluster more accurately
bhargavsutapalli added a commit to bhargavsutapalli/terraform-aws-eks that referenced this issue May 26, 2021
slillibri pushed a commit to slillibri/terraform-aws-eks that referenced this issue Oct 26, 2021
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants