-
Notifications
You must be signed in to change notification settings - Fork 34
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
Adjust defaults for container0 (build container) as it is too frugal #361
Conversation
8087ae5
to
6d7258e
Compare
Requests: corev1.ResourceList{ | ||
"cpu": resource.MustParse("100m"), | ||
"mem": resource.MustParse("50Mi"), | ||
}, | ||
Limits: corev1.ResourceList{ | ||
"mem": resource.MustParse("1Gi"), | ||
"cpu": resource.MustParse("1000m"), | ||
"mem": resource.MustParse("4Gi"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During CI, we run our test controller in the same k8s cluster with our production k8s controller (for bk). So setting a limit on RAM might be wise.
Setting namespace resource limit is another option, I don't think I've set it when I create the namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am only modifying the podspec for container 0, sadly this isn't visible in the test diff or YAML diff...
# This will be applied to the job's podSpec as a strategic merge patch
# See https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch
pod-spec-patch:
serviceAccountName: buildkite-agent-sa
automountServiceAccountToken: true
containers:
- name: container-0
env:
- name: GITHUB_TOKEN
valueFrom:
secretKeyRef:
name: github-secrets
key: github-token
resources:
requests:
cpu: 1000m # one core
mem: 4Gi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I see it's container-0 being changed. I may have confused you in my comment I think (sorry). What I mean is that it might be safer to do this on container-0 spec:
resources:
requests:
cpu: 1000m # one core
mem: 4Gi
limits:
mem: 4Gi # This will prevent one bad test case impacting other tenants on the cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhming0 yeah I am definitely going to dig into how this works 🙏🏻
So very good raising these sorts of questions!
As discovered while debugging some build issues, we could probably be a bit more generous with CPU and memory. Locally release builds are taking ~20 seconds, in this container they are taking over ~5 minutes threshold for jobs..
Currently the build container only has 1/10th of a CPU and 50mb of RAM.