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

Enable NAP in 'aaa' cluster #1604

Merged
merged 1 commit into from
May 4, 2021
Merged

Enable NAP in 'aaa' cluster #1604

merged 1 commit into from
May 4, 2021

Conversation

mborsz
Copy link
Member

@mborsz mborsz commented Feb 3, 2021

Context:

Currently perf-dash has cpu request/limit set to 3 cores and it still insufficient:
image

Also is being continuously OOMKilled with memory limit set to 8GiB.

The node pool added in #659 for perfdash is using n1-standard-4 which provides 4 cores and 12 GiB of allocatable memory.

I would like to increase perf-dash's requests to at least 6 cpus and 16 GiB (to give some room for growth), but I'm not able to do that due to the used machine type.

Instead of adding 'pool-3' node pool with larger machine type, I would like to propose enabling Node auto-provisioning on this cluster that will be adding such node pools as needed, reducing maintenance burden.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. wg/k8s-infra labels Feb 3, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 3, 2021
@mborsz
Copy link
Member Author

mborsz commented Feb 3, 2021

I would like to hear feedback on this change. I understand that there may be some reasons why we may want to avoid enabling this, but I would like to be sure that this is a conscious that we do not enable this here and continue manually adding larger and larger node pools.

Base automatically changed from master to main February 9, 2021 00:35
@spiffxp
Copy link
Member

spiffxp commented Feb 11, 2021

/uncc @mikedanese @nikhita
/cc @spiffxp @BenTheElder
/assign @thockin @dims @ameukam
I'd like to hear your opinions. I have very little experience managing aaa specifically, or clusters using this feature.

@k8s-ci-robot k8s-ci-robot requested review from BenTheElder and spiffxp and removed request for mikedanese and nikhita February 11, 2021 07:17
@dims
Copy link
Member

dims commented Feb 11, 2021

we forgot @cblecker :)

@ameukam
Copy link
Member

ameukam commented Feb 11, 2021

@mborsz Did you consider the possibility we can change the instance type to n1-standard-8 or another instance type for the node-pool pool-2 instead of enable the auto-scaling at the cluster level ?

@thockin
Copy link
Member

thockin commented Feb 11, 2021

I'm skeptical of why perf-dash needs that much memory - the trajectory is problematic. That said, NAP seems like the rightest answer. This is sort of exactly what it is for. So I am +1

Isn't there an overall cluster limit on resources, too?

@mborsz
Copy link
Member Author

mborsz commented Feb 12, 2021

I'm skeptical of why perf-dash needs that much memory - the trajectory is problematic. That said, NAP seems like the rightest answer. This is sort of exactly what it is for. So I am +1

perf-dash resource usage: perf-dash keeps all the data in memory and periodically refreshes state from GCS. We are continuously adding new tests or change existing ones (e.g. kubernetes/test-infra#20448), this will cause memory growth for sure.

Most of the cpu and memory is used when perf-dash either initializes or refreshes state, most likely for network buffers and json deserialization. For most of the time resource usage is around ~700MiB and cpu usage jumps between 0 and 3 cores (the current limit) when it refreshes state.

I'm think we can optimize perf-dash to reduce resource usage, but I'm not sure if this is a good area to invest time of our team there. For comparison: perf dash consumes currently 3 cores and 10Gi of memory, while e.g. our 5000 node test consumes ~5000 cores and ~20TiB of memory. I think that e.g. optimizing our 5000 node test has better ROI and we have some achievements there (e.g. kubernetes/perf-tests#561).

@mborsz
Copy link
Member Author

mborsz commented Feb 12, 2021

Maciej Borsz Did you consider the possibility we can change the instance type to n1-standard-8 or another instance type for the node-pool pool-2 instead of enable the auto-scaling at the cluster level ?

Yes, it's not possible to change instance type, we need to create a new node pool with larger machine. Technically we can do that, but I'm afraid that some day we will need to add even larger node pool. This is why I suggest starting using NAP.

@BenTheElder
Copy link
Member

I have no experience with NAP.

If we review the configuration for requested resources though then this seems fine. Instead of debating how big the node pool config should be set to we can debate how big the pod requests should be 🙃

@thockin
Copy link
Member

thockin commented Feb 12, 2021 via email

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mborsz, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021
@thockin thockin removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021
@thockin
Copy link
Member

thockin commented Feb 12, 2021

I didn't mean to unilaterally approve - looking for consensus :)

// Enable NAP
cluster_autoscaling {
enabled = true
resource_limits = [
Copy link
Member

Choose a reason for hiding this comment

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

I may be wrong, but I think resource_limits is a sub-block of cluster_autoscaling. You'll need to split the resource types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 23, 2021
@ameukam
Copy link
Member

ameukam commented Feb 23, 2021

/lgtm
/hold
If others want to comment.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 23, 2021
@mm4tt
Copy link
Contributor

mm4tt commented Apr 16, 2021

Can we merge this? Or are we waiting for someone else approval?

mm4tt added a commit to mm4tt/perf-tests that referenced this pull request Apr 16, 2021
Version 2.33 requires more than 10GB of memory and we cannot bump the mem limit without kubernetes/k8s.io#1604
@thockin
Copy link
Member

thockin commented Apr 16, 2021

I think we can enable this but someone needs to run terraform on it ASAP, so whomever has time should remove the hold and do it. Not me, at least not today.

@mborsz
Copy link
Member Author

mborsz commented Apr 29, 2021

@thockin can you suggest anyone who will be able to run terraform on this in the next couple of days?

@mborsz
Copy link
Member Author

mborsz commented Apr 30, 2021

@BenTheElder @ameukam @spiffxp can you run terraform on this?

@ameukam
Copy link
Member

ameukam commented Apr 30, 2021

@mborsz Will run this Monday if nobody is faster than me.

@spiffxp NAP will create instances of the E2 family for any new workload.

@spiffxp
Copy link
Member

spiffxp commented May 4, 2021

I have no objections but can't commit to running this just now, if @ameukam wants to take it, go for it

@ameukam
Copy link
Member

ameukam commented May 4, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit 04ddf8d into kubernetes:main May 4, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 4, 2021
@ameukam
Copy link
Member

ameukam commented May 4, 2021

Running terraform apply -auto-approve -target google_container_cluster.cluster

@ameukam
Copy link
Member

ameukam commented May 4, 2021

Change applied successfully :

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants