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

test(e2e): add in chaos E2E test suite #61

Merged
merged 7 commits into from
Mar 25, 2024

Conversation

charliedmcb
Copy link
Collaborator

@charliedmcb charliedmcb commented Dec 13, 2023

Fixes #216

Description

How was this change tested?
9/10 successful runs required as the benchmark for adding a new test suite

Last 10 runs (10/10 successful):

Previous 10 runs (8/10 successful):

Older passing E2E test:
https://github.com/Azure/karpenter/actions/runs/7202151191

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

NONE

Copy link
Collaborator Author

@charliedmcb charliedmcb left a comment

Choose a reason for hiding this comment

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

/test

@coveralls
Copy link

coveralls commented Mar 11, 2024

Pull Request Test Coverage Report for Build 8427608579

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.716%

Totals Coverage Status
Change from base Build 8397094800: 0.0%
Covered Lines: 35591
Relevant Lines: 36423

💛 - Coveralls

Copy link
Collaborator Author

@charliedmcb charliedmcb left a comment

Choose a reason for hiding this comment

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

/test

Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

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

Can we do 5 runs showing its not flakey

Copy link
Collaborator Author

@charliedmcb charliedmcb left a comment

Choose a reason for hiding this comment

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

/test

@charliedmcb
Copy link
Collaborator Author

Can we do 5 runs showing its not flakey

My current benchmark is 9/10

@charliedmcb charliedmcb marked this pull request as ready for review March 20, 2024 21:30
Copy link
Contributor

@rakechill rakechill left a comment

Choose a reason for hiding this comment

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

Have a few questions to understand the goals of these tests

test/suites/chaos/suite_test.go Show resolved Hide resolved
test/suites/chaos/suite_test.go Show resolved Hide resolved
@Bryce-Soghigian
Copy link
Collaborator

Bryce-Soghigian commented Mar 22, 2024

Have a few questions to understand the goals of these tests

Look at the original pr for the chaos suite maybe? This might give us better insights as to the why behind these tests.

@jonathan-innis any insights as I see you authored the original pr

@rakechill
Copy link
Contributor

rakechill commented Mar 22, 2024

The extra information from the original PR:

constantly tainting a node after creation

makes sense why we'd get > 1 node for a 1 pod deployment.

35 still feels like a magic number to me though :)

@rakechill
Copy link
Contributor

Also found this comment interesting for our consideration:

It's worth noting that without enabling either ttlSecondsAfterEmpty or consolidation.enabled, this issue persists for nodes that would receive taints that Karpenter is unaware of after node launch

@charliedmcb
Copy link
Collaborator Author

35 still feels like a magic number to me though :)

Yea, I'm not sure on the 35. Agree seems magic number-ish

@rakechill
Copy link
Contributor

Checked with the AWS folks, and the < 35 nodes check was added due to testing as a typical upper limit with disruption. This is good enough for me right now, so I'll approve for now. Moreso for posterity's sake in case we want to adjust this limit in the future and how we might reason about doing so.

@charliedmcb charliedmcb merged commit 64e5128 into main Mar 25, 2024
12 checks passed
@charliedmcb charliedmcb deleted the charliedmcb/addChaosE2ETestSuite branch March 25, 2024 22: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.

Add in karpenter chaos e2e test
5 participants