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

Implement Force Removal Taint Logic #246

Merged
merged 10 commits into from
Sep 27, 2024

Conversation

jackcasey-visier
Copy link
Contributor

@jackcasey-visier jackcasey-visier commented Sep 18, 2024

closes #245

Summary

This PR is a WIP, TODOs:

  • Unit Tests
  • Documentation

Opening in progress to ensure maintainers are happy with the direction and patterns implemented #201

These changes implement a new taint: atlassian.com/escalator-force

Nodes marked with this taint will be removed as soon as all non daemonset pods are completed. This is checked during the Escalator loop

In the current design, this taint is not expected to be added by Escalator to any nodes. The expected usage is for an external system or administrator to implement the taint, and have Escalator perform the safe scale down.

Thank you!

@jackcasey-visier jackcasey-visier changed the title #245 Implement Force Removal Taint Logic (WIP) Implement Force Removal Taint Logic (WIP) Sep 18, 2024
@jackcasey-visier jackcasey-visier marked this pull request as draft September 18, 2024 22:28
@jackcasey-visier jackcasey-visier changed the title Implement Force Removal Taint Logic (WIP) Implement Force Removal Taint Logic Sep 20, 2024
@awprice awprice assigned awprice and unassigned awprice Sep 20, 2024
@awprice
Copy link
Member

awprice commented Sep 23, 2024

Thank you for contributing this @jackcasey-visier!

Overall approach looks good to me - would be good to see the docs and unit tests explicitly testing this new functionality implemented.

I'm keen to test this out internally

@jackcasey-visier
Copy link
Contributor Author

Agreed! I'll add new tests in the next day or two. So far I've just patched the existing tests with new changes implemented :)

@jackcasey-visier
Copy link
Contributor Author

@awprice I believe we have the logic covered by unit tests and I've added a cheeky docs update :) Please let me know what you think! Thanks

@jackcasey-visier jackcasey-visier marked this pull request as ready for review September 25, 2024 17:44
Copy link
Member

@awprice awprice left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work for a first contribution

@jackcasey-visier
Copy link
Contributor Author

@awprice Amazing, thank you!

Copy link
Member

@Jacobious52 Jacobious52 left a comment

Choose a reason for hiding this comment

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

Nice! Awesome first contribution. Especially for not being a Go shop!

I think I'm happy to merge, then we can test internally before we cut a git release for a new minor version.

@awprice awprice merged commit 650dda7 into atlassian:master Sep 27, 2024
3 checks passed
@awprice
Copy link
Member

awprice commented Sep 27, 2024

@jackcasey-visier If you're keen on using your change before we've cut a release, you can grab the latest build of master, including your change here - https://github.com/atlassian/escalator/pkgs/container/escalator/280253350?tag=650dda7

@jackcasey-visier
Copy link
Contributor Author

Cheers, thanks again @awprice!

Come have a 🍻 if you're ever in Vancouver, BC :)

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.

[Feature Request] Force Removal Taint
3 participants