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

add more scheduling unit tests #1565

Merged
merged 3 commits into from
Mar 24, 2022
Merged

Conversation

tzneal
Copy link
Contributor

@tzneal tzneal commented Mar 23, 2022

1. Issue, if available:

N/A

2. Description of changes:

Adds additional unit tests to scheduling:

  • ensures that all instance types passed to the cloud provider meet
    the required constraints
  • verify prov/pod constraints on OS
  • verify prov/pod constraints on zone/capacity type
  • verify mixed prov/pod constraints

3. How was this change tested?

Unit test, no functional changes were made.

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Mar 23, 2022

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 3bd4f18
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/623cdc59dba0f80009f77f55

@tzneal tzneal force-pushed the add-more-scheduling-tests branch from dc522b0 to b823dce Compare March 24, 2022 00:30
- ensures that all instance types passed to the cloud provider meet
  the required constraints
- verify prov/pod constraints on OS
- verify prov/pod constraints on zone/capacity type
- verify mixed prov/pod constraints
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

I like this! Do you think there would be value in making a script that loads the real AWS instance types in the AWS cloud provider?

@tzneal
Copy link
Contributor Author

tzneal commented Mar 24, 2022

I like this! Do you think there would be value in making a script that loads the real AWS instance types in the AWS cloud provider?

Yeah, that would be great! I don't think ginkgo supports table driven tests, but we could make a test helper that lets you pass in the list of instance types. One could be this unique set and the other are a snapshot of what real instance types were. That would be good to hook into the AWS CP EC2 Instance Type -> cloudprovider.InstanceType translation as well to get more testing on that.

Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

just a couple questions, nothing mandatory

pkg/cloudprovider/fake/instancetype.go Outdated Show resolved Hide resolved
pkg/cloudprovider/fake/instancetype.go Outdated Show resolved Hide resolved
@tzneal tzneal force-pushed the add-more-scheduling-tests branch from b823dce to f33a1b7 Compare March 24, 2022 20:23
}
}
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, the above cases can all find compatible instance type for the give pod. Can we add some cases with multiple pods that will create incompatibility and we should see them been put on separated nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set of tests was geared at two recent issues that caused point releases where we were passing a set of instance types to the cloudprovider where some, but not all of the instance types matched the constraints. We have some individual tests for that behavior:

  • "should launch pods with resources that aren't on any single instance type on different instances"
  • "should schedule incompatible pods to the different node"

Copy link
Contributor

@felix-zhe-huang felix-zhe-huang left a comment

Choose a reason for hiding this comment

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

LGTM!

@tzneal tzneal merged commit 72f8c56 into aws:main Mar 24, 2022
@tzneal tzneal deleted the add-more-scheduling-tests branch March 24, 2022 23:56
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.

4 participants