Fix the insufficient memory instance type bug #1080
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: ec6ef9d 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61d60fc157c7d8000767376c |
felix-zhe-huang
force-pushed
the
issue1034
branch
from
089c388 to
ddd310c
Compare
Happy to do so. |
felix-zhe-huang
force-pushed
the
issue1034
branch
from
bca6c40 to
a30dd08
Compare
felix-zhe-huang
force-pushed
the
issue1034
branch
from
a30dd08 to
4ade204
Compare
pkg/utils/injection/injection.go
Outdated
| @@ -79,3 +79,17 @@ func GetControllerName(ctx context.Context) string { | |||
| } | |||
| return name.(string) | |||
| } | |||
|
|
|||
| type testInstanceKey struct{} | |||
There was a problem hiding this comment.
This injection pattern is useful to keep interfaces in production code clean. I'm wary of coupling test implementation details to this injection package. There are some other examples of injecting data into the EC2 fake. Happy to help offline if you need.
There was a problem hiding this comment.
Yeah this may not be ideal. I am removing it and implements a function to switch on and off those special instance types instead.
felix-zhe-huang
force-pushed
the
issue1034
branch
2 times, most recently
from
1b8e510 to
ce043cd
Compare
| test.PodOptions{ | ||
| ResourceRequirements: v1.ResourceRequirements{Requests: v1.ResourceList{v1.ResourceCPU: resource.MustParse("3"), v1.ResourceMemory: resource.MustParse("3Gi")}}, | ||
| }, | ||
| ))[0] |
There was a problem hiding this comment.
I'm not sure what the output is if ExpectedProvisioned returns 0 pods, but I'm guessing it's a not very graceful crash. Would it be more informative to the user to add an explicit:
Expect(len(pods)).To(Equal(1))
node := ExpectScheduled(ctx, env.Client, pod[0])
| @@ -39,6 +39,17 @@ func (c *CloudProvider) Create(_ context.Context, constraints *v1alpha5.Constrai | |||
| for i := 0; i < quantity; i++ { | |||
| name := strings.ToLower(randomdata.SillyName()) | |||
| instance := instanceTypes[0] | |||
| // To create error test cases, give preferences to the illy constructed instance types | |||
There was a problem hiding this comment.
I'd be confused looking at just the tests on how the bad instance types are guaranteed to cause a test failure. I think it would be more apparent if the fake cloudprovider was more generic and the setup of a bad set of instance types is set in the test.
You could use the exported InstanceTypes field in the CloudProvider struct to set a specific set of bad instance types that would be apparent in the actual test that it is guaranteed to fail if the binpacking short circuit logic is not working. Wdyt?
| }, | ||
| ))[0] | ||
| node := ExpectScheduled(ctx, env.Client, pod) | ||
| Expect(*node.Status.Allocatable.Cpu()).To(Equal(resource.MustParse("4"))) |
There was a problem hiding this comment.
This seems a bit fragile if we change the fake instance type data, the functionality of not choosing an instance type may still work properly but the test could fail if the fake instance type doesn't have 4 vcpus and 4 GBs of allocatable RAM.
felix-zhe-huang
force-pushed
the
issue1034
branch
from
ce043cd to
ec6ef9d
Compare
1. Issue, if available:
resolve issue #1034
2. Description of changes:
3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.