Taint nodes on spot and scheduled events #162

diversario · 2020-05-15T14:00:34Z

Issue #, if available: #160

Description of changes:

Upon receiving an event, taint the affect node with the appropriate taint. Remove the taint if event is cancelled or completed.

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

Fixes aws#160. Signed-off-by: Ilya Shaisultanov <[email protected]>

diversario · 2020-05-15T14:24:15Z

@bwagner5 I see the build failed but not all of the tests – unsure if related to my changes here or not. Do you know how I could debug those?

bwagner5 · 2020-05-15T14:39:37Z

@diversario I believe it's because the cluster role does not have taint permissions. https://github.com/aws/aws-node-termination-handler/blob/master/config/helm/aws-node-termination-handler/templates/clusterrole.yaml

You can run the e2e tests locally:

make e2e-test

or all tests:

make test

Also, looks like some small formatting issues:

❌ goimports found a problem in go source files. See above for the files with problems.
Grade: A+ (92.6%)
Files: 24
Issues: 5
gofmt: 83%
	pkg/interruptionevent/spot-itn-event_internal_test.go
		Line 1: warning: file is not gofmted with -s (gofmt)
	pkg/node/node.go
		Line 1: warning: file is not gofmted with -s (gofmt)
	pkg/interruptionevent/scheduled-event_internal_test.go
		Line 1: warning: file is not gofmted with -s (gofmt)
	pkg/interruptionevent/spot-itn-event.go
		Line 1: warning: file is not gofmted with -s (gofmt)
go_vet: 100%
gocyclo: 95%
	test/ec2-metadata-test-proxy/cmd/ec2-metadata-test-proxy.go
		Line 100: warning: cyclomatic complexity 25 of function handleRequest() is high (> 15) (gocyclo)
golint: 95%
	pkg/node/node.go
		Line 272: warning: exported method Node.TaintSpotItn should have comment or be unexported (golint)
		Line 281: warning: exported method Node.TaintScheduledMaintenance should have comment or be unexported (golint)
		Line 290: warning: exported method Node.CleanAllTaints should have comment or be unexported (golint)
ineffassign: 91%
	pkg/interruptionevent/scheduled-event_internal_test.go
		Line 82: warning: ineffectual assignment to err (ineffassign)
	pkg/interruptionevent/spot-itn-event_internal_test.go
		Line 59: warning: ineffectual assignment to err (ineffassign)
license: 100%
misspell: 100%

bwagner5

add permissions to cluster role and run goimports -w ./ && gofmt -s -w ./

If all the tests except helm-sync passes, I can take the action to fix that. It's just a tedious task of syncing the helm chart to the aws/eks-charts repo.

diversario · 2020-05-15T15:59:35Z

Is it possible for me to see the build logs? I don't think it's a permissions issue because NTH can already add labels to nodes. I checked cluster-autoscaler clusterrole and it looks like this:

- apiGroups:
  - ""
  resources:
  - nodes
  verbs:
  - watch
  - list
  - get
  - update

which looks pretty much like the NTH's one:

- apiGroups:
    - ""
  resources:
    - nodes
  verbs:
    - get
    - patch
    - update

I'm currently trying to get some view into what's failing from the e2e run.

For scheduled event cancellation test, add tolerations to the proxy to allow it to schedule to emit the cancellation event.

codecov-io · 2020-05-17T09:54:17Z

Codecov Report

Merging #162 into master will decrease coverage by 10.42%.
The diff coverage is 31.14%.

@@             Coverage Diff             @@
##           master     #162       +/-   ##
===========================================
- Coverage   94.64%   84.22%   -10.43%     
===========================================
  Files           8        8               
  Lines         635      748      +113     
===========================================
+ Hits          601      630       +29     
- Misses         22       99       +77     
- Partials       12       19        +7

Impacted Files	Coverage Δ
pkg/node/node.go	`66.77% <23.07%> (-22.45%)`	⬇️
pkg/interruptionevent/scheduled-event.go	`90.47% <71.42%> (-2.86%)`	⬇️
pkg/interruptionevent/spot-itn-event.go	`93.75% <81.81%> (-6.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b647783...1011bd2. Read the comment docs.

diversario · 2020-05-17T10:14:49Z

pkg/node/node.go

@@ -363,3 +434,112 @@ func jsonPatchEscape(value string) string {
 	value = strings.Replace(value, "~", "~0", -1)
 	return strings.Replace(value, "/", "~1", -1)
 }
+
+func addTaint(node *corev1.Node, nth Node, taintKey string, taintValue string, effect corev1.TaintEffect) error {


This, addTaintToSpec and cleanTaint are lifted from cluster-autoscaler with minor modifications.

diversario · 2020-05-17T10:17:45Z

test/e2e/maintenance-event-cancellation-test

@@ -80,7 +94,9 @@ helm upgrade --install $CLUSTER_NAME-emtp $SCRIPTPATH/../../config/helm/ec2-meta
  --set ec2MetadataTestProxy.enableScheduledMaintenanceEvents="true" \
  --set ec2MetadataTestProxy.enableSpotITN="false" \
  --set ec2MetadataTestProxy.scheduledEventStatus="canceled" \
-  --set ec2MetadataTestProxy.port="$IMDS_PORT"    
+  --set ec2MetadataTestProxy.port="$IMDS_PORT" \


Since the worker node is now tainted, the proxy won't schedule there unless it tolerates the taint.

bwagner5

nice work on this! I just had a few comments in-line.

pkg/node/node.go

bwagner5

LGTM, thanks @diversario ! 🚀

Taint nodes on spot and scheduled events

77ff34a

Fixes aws#160. Signed-off-by: Ilya Shaisultanov <[email protected]>

bwagner5 self-requested a review May 15, 2020 14:39

bwagner5 requested changes May 15, 2020

View reviewed changes

format

3f0778c

diversario added 8 commits May 15, 2020 18:42

Trim taint value to max 63 chars

62c623d

Do not actually taint nodes in dryRun

e59f7ab

fmt

e7a7aac

attempting to add taint removal in e2e tests

5403794

still trying to fix da tests

5343653

typo

1180ef2

Update ec2-metadata-test-proxy to support tolerations

a21215c

For scheduled event cancellation test, add tolerations to the proxy to allow it to schedule to emit the cancellation event.

fix unit test

1011bd2

diversario marked this pull request as ready for review May 17, 2020 10:08

diversario requested a review from bwagner5 May 17, 2020 10:08

diversario commented May 17, 2020

View reviewed changes

bwagner5 requested changes May 17, 2020

View reviewed changes

pkg/node/node.go Outdated Show resolved Hide resolved

pkg/node/node.go Show resolved Hide resolved

diversario added 2 commits May 18, 2020 10:30

Default taintNode to false

3c47458

default taintNode to false in the helm chart

b4f6e60

diversario requested a review from bwagner5 May 18, 2020 12:03

bwagner5 approved these changes May 18, 2020

View reviewed changes

bwagner5 merged commit 0d528d6 into aws:master May 18, 2020

diversario deleted the diversario-160-taint-nodes-on-events branch May 19, 2020 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taint nodes on spot and scheduled events #162

Taint nodes on spot and scheduled events #162

diversario commented May 15, 2020

diversario commented May 15, 2020

bwagner5 commented May 15, 2020

bwagner5 left a comment •

edited

Loading

diversario commented May 15, 2020

codecov-io commented May 17, 2020

diversario May 17, 2020 •

edited

Loading

diversario May 17, 2020

bwagner5 left a comment

bwagner5 left a comment

Taint nodes on spot and scheduled events #162

Taint nodes on spot and scheduled events #162

Conversation

diversario commented May 15, 2020

diversario commented May 15, 2020

bwagner5 commented May 15, 2020

bwagner5 left a comment • edited Loading

Choose a reason for hiding this comment

diversario commented May 15, 2020

codecov-io commented May 17, 2020

Codecov Report

diversario May 17, 2020 • edited Loading

Choose a reason for hiding this comment

diversario May 17, 2020

Choose a reason for hiding this comment

bwagner5 left a comment

Choose a reason for hiding this comment

bwagner5 left a comment

Choose a reason for hiding this comment

bwagner5 left a comment •

edited

Loading

diversario May 17, 2020 •

edited

Loading