FIX PR #98 -- advanced node taint querying #113

isaacnboyd · 2024-01-19T14:57:09Z

PR #98 required:

Rebasing Add support for node taints #98 branch on main and resolving numerous conflicts, especially in the capacity tests (please review)
Cleaning lint (no return value on splitTaint() and goformatting)
Testing new feature and adding instructions in README

resolves #91
closes #98

robscott

robscott · 2024-01-22T05:01:40Z

pkg/capacity/capacity.go

@@ -59,7 +60,103 @@ func FetchAndPrint(showContainers, showPods, showUtil, showPodCount, excludeTain
 	printList(&cm, showContainers, showPods, showUtil, showPodCount, showNamespace, output, sortBy, availableFormat)
 }

-func getPodsAndNodes(clientset kubernetes.Interface, excludeTainted bool, podLabels, nodeLabels, namespaceLabels, namespace string) (*corev1.PodList, *corev1.NodeList) {
+// Taints can have two possible formats, key1:effect or key1=value1:effect. This function handles both formats and returns each part as a separate string for comparison.


Nit: I'm used to function comments starting with the name of the function in golang, would recommend using that pattern throughout. Would also slightly prefer wrapping comment lines at 80 chars, but I don't think I've done great at following that here, it's just a common pattern in k8s projects.

robscott · 2024-01-22T05:03:01Z

README.md

@@ -131,6 +131,27 @@ kube-capacity --namespace-labels team=api
 kube-capacity --node-labels kubernetes.io/role=node
 ```

+### Filtering By Taints


Thanks for adding docs to the readme, this is really great! One tiny suggestion would be to use "Node Taints" instead of just "Taints"

Suggested change

### Filtering By Taints

### Filtering By Node Taints

robscott · 2024-01-22T05:16:24Z

pkg/capacity/capacity.go

@@ -59,7 +60,103 @@ func FetchAndPrint(showContainers, showPods, showUtil, showPodCount, excludeTain
 	printList(&cm, showContainers, showPods, showUtil, showPodCount, showNamespace, output, sortBy, availableFormat)
 }

-func getPodsAndNodes(clientset kubernetes.Interface, excludeTainted bool, podLabels, nodeLabels, namespaceLabels, namespace string) (*corev1.PodList, *corev1.NodeList) {
+// Taints can have two possible formats, key1:effect or key1=value1:effect. This function handles both formats and returns each part as a separate string for comparison.
+func splitTaint(taint string) (string, string, string) {


Can you use one of the the k8s helper functions instead here? Maybe something like this https://github.com/kubernetes/kubernetes/blob/a07b1aaa5b39b351ec8586de800baa5715304a3f/pkg/apis/core/helper/helpers.go#L428 or https://github.com/kubernetes/kubernetes/blob/a07b1aaa5b39b351ec8586de800baa5715304a3f/pkg/util/taints/taints.go#L91. I know those might not be quite a perfect fit, but maybe there's something along those lines that could help. Also in case it's ever helpful cs.k8s.io is a k8s-wide codesearch that helps find if helper functions like this already exist - that's what I used here fwiw.

ooh cs.k8s.io is nifty! I'll have to do some digging there.

Apparently you can create node taints that have no value. Like kubectl taint nodes foo bar:NoSchedule
as opposed to a k/v taint like kubectl taint nodes foo dedicated=special-user:NoSchedule.
That's what this function is handling. Those 2 possible taint expressions.

I'll look around and see what's been built.

I think you're right ParseTaints() helper function can be a replacement for splitTaint(), it will change the user syntax from kube-capacity --node-taints !node.kubernetes.io/unschedulable:NoSchedule to a more familiar syntax kube-capacity --node-taints node.kubernetes.io/unschedulable:NoSchedule- with the - denoting removing a taint instead of !.

I think that's a valuable addition I'll try to get this added in the next few days.

splitTaint() has been replaced with k8s helper function ParseTaint(). See 903bc98

robscott · 2024-01-22T05:18:56Z

pkg/capacity/capacity.go

+
+	if nodeTaints != "" {
+		taintedNodeList := *nodeList
+		taintedNmList := removeNodeMetricsWithTaints(nmList, taintedNodeList)


Maybe a dumb question, but if we're just doing a join with the nodes later, and we're already removing the nodes that are tainted separately, do we need to actually remove the node metrics? I'd thought that might just automatically happen if corresponding nodes didn't exist, but it's been awhile since I've worked with this code.

I believe you are correct. I tested kube-capactiy -t <taint key>=<taint value>:<taint effect> --util with and without this block of code and the results are the same.

See 903bc98

robscott · 2024-01-26T06:05:11Z

Hey @isaacnboyd, let me know if you think you'll have some time to work on this in the next week or so. Largely thanks to all your great improvements, I think it's probably close to time to cut another release (probably v0.8.0), but don't want this to miss the cut if you've got plans to work on it sometime soon.

isaacnboyd · 2024-01-26T12:13:44Z

Hey @isaacnboyd, let me know if you think you'll have some time to work on this in the next week or so. Largely thanks to all your great improvements, I think it's probably close to time to cut another release (probably v0.8.0), but don't want this to miss the cut if you've got plans to work on it sometime soon.

Hey howdy Rob, yes I'd like to get this one into the release. I should be able to find some time to work on it here in the next few days.

Lint is preventing the merge of PR robscott#98 Linter picked up no return value on splitTaint() And goformatting whitespace errors in capacity_test

robscott · 2024-01-27T04:53:47Z

Thanks @isaacnboyd! Just let me know whenever it's ready for another round of review (or if it already is).

Testing with --util shows additional removal of tainted node metrics to be unnecessary

Use k8s helper function to parse taints with familiar syntax.

In order to parse a node, kube-capacity requires at least a key and effect. It will also handle key,value, and effect taints.

taints can be referenced by key=value:effect and by key:effect

isaacnboyd · 2024-02-03T21:00:28Z

Thanks @isaacnboyd! Just let me know whenever it's ready for another round of review (or if it already is).

Hey @robscott this is ready for another review.

robscott · 2024-02-07T05:16:06Z

Thanks @isaacnboyd!

isaacnboyd force-pushed the FIX-PR-#98 branch from 833790e to f71e65a Compare January 19, 2024 15:02

robscott reviewed Jan 22, 2024

View reviewed changes

isaacnboyd changed the title ~~FIX PR #98~~ FIX PR #98 -- advanced node taint querying Jan 22, 2024

isaacnboyd force-pushed the FIX-PR-#98 branch 3 times, most recently from 10e1325 to 557f5cd Compare January 22, 2024 15:24

robscott mentioned this pull request Jan 23, 2024

Add support for node taints #98

Closed

OperationalDev and others added 5 commits January 26, 2024 06:26

Initial commit of taint filter

c5afa26

Add support for including and excluding node taints

027763c

Add comments and remove a return from splitTaint that is not required

bea3def

Clean Lint

c652b08

Lint is preventing the merge of PR robscott#98 Linter picked up no return value on splitTaint() And goformatting whitespace errors in capacity_test

Update README instructions on taint filter

7bc9c28

isaacnboyd force-pushed the FIX-PR-#98 branch 3 times, most recently from 7605696 to 05cdc41 Compare January 26, 2024 19:57

isaacnboyd added 2 commits January 30, 2024 07:46

Fix rebase issues and documentation

7618434

Remove duplicate logic to remove nodes with taints from metric server

903bc98

Testing with --util shows additional removal of tainted node metrics to be unnecessary

isaacnboyd force-pushed the FIX-PR-#98 branch 2 times, most recently from 3900024 to 903bc98 Compare January 31, 2024 14:57

isaacnboyd added 3 commits February 3, 2024 14:36

Refactor taint parsing

25e1a92

Use k8s helper function to parse taints with familiar syntax.

Fix documentation

09d189e

In order to parse a node, kube-capacity requires at least a key and effect. It will also handle key,value, and effect taints.

Fix capacity errors

6def879

taints can be referenced by key=value:effect and by key:effect

robscott merged commit 5e9ad1b into robscott:main Feb 7, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX PR #98 -- advanced node taint querying #113

FIX PR #98 -- advanced node taint querying #113

isaacnboyd commented Jan 19, 2024

robscott left a comment

robscott Jan 22, 2024

robscott Jan 22, 2024

robscott Jan 22, 2024

isaacnboyd Jan 26, 2024

isaacnboyd Jan 30, 2024 •

edited

Loading

isaacnboyd Feb 3, 2024

robscott Jan 22, 2024

isaacnboyd Jan 26, 2024 •

edited

Loading

robscott commented Jan 26, 2024

isaacnboyd commented Jan 26, 2024

robscott commented Jan 27, 2024

isaacnboyd commented Feb 3, 2024

robscott commented Feb 7, 2024

FIX PR #98 -- advanced node taint querying #113

FIX PR #98 -- advanced node taint querying #113

Conversation

isaacnboyd commented Jan 19, 2024

robscott left a comment

Choose a reason for hiding this comment

robscott Jan 22, 2024

Choose a reason for hiding this comment

robscott Jan 22, 2024

Choose a reason for hiding this comment

robscott Jan 22, 2024

Choose a reason for hiding this comment

isaacnboyd Jan 26, 2024

Choose a reason for hiding this comment

isaacnboyd Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

isaacnboyd Feb 3, 2024

Choose a reason for hiding this comment

robscott Jan 22, 2024

Choose a reason for hiding this comment

isaacnboyd Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

robscott commented Jan 26, 2024

isaacnboyd commented Jan 26, 2024

robscott commented Jan 27, 2024

isaacnboyd commented Feb 3, 2024

robscott commented Feb 7, 2024

isaacnboyd Jan 30, 2024 •

edited

Loading

isaacnboyd Jan 26, 2024 •

edited

Loading