Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

AWS: Add support for custom taints and labels #832

Merged
merged 3 commits into from
Aug 24, 2020
Merged

AWS: Add support for custom taints and labels #832

merged 3 commits into from
Aug 24, 2020

Conversation

knrt10
Copy link
Member

@knrt10 knrt10 commented Aug 24, 2020

fixes #4

Signed-off-by: knrt10 kautilya@kinvolk.io

@knrt10 knrt10 requested review from surajssd, ipochi and invidian August 24, 2020 07:20
@knrt10 knrt10 requested a review from johananl as a code owner August 24, 2020 07:20
invidian
invidian reviewed Aug 24, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just some nits, so LGTM.

I wonder, perhaps we should also add tests for that, to make sure the labels and taints are properly applied and retained.

pkg/platform/aws/template.go Outdated Show resolved Hide resolved
pkg/platform/aws/template.go Outdated Show resolved Hide resolved
@knrt10
Copy link
Member Author

knrt10 commented Aug 24, 2020

Yes, I was thinking to add tests, but was confused where to add them. Can you suggest @invidian ?

knrt10 added 2 commits August 24, 2020 13:47
@knrt10
AWS: Add support for custom taints and labels
8fa0afc 
User can now use custom labels and taints as part of cluster creation.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10
docs/aws: Add custom labels and taints
9b0ed9b 
Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10 knrt10 force-pushed the knrt10/aws-taints-and-labels branch from fa7f1a5 to 9b0ed9b Compare August 24, 2020 08:17
@invidian
Copy link
Member

Yes, I was thinking to add tests, but was confused where to add them. Can you suggest @invidian ?

Sure. I'd do the following:

  • add extra worker pool with some very small instance type and spot price, so we don't pay much for it, with labels and taints set.
  • Modify test/components/kubernetes/kubelet_labels_test.go and test/components/kubernetes/kubelet_disruptive_test.go to also include AWS
  • in test/components/kubernetes add a test to verify that the node has taint applied
  • modify test/components/kubernetes/kubelet_disruptive_test.go to also check, that the taint has been retained during node re-registration.

@knrt10 knrt10 requested a review from pothos as a code owner August 24, 2020 11:10
@knrt10 knrt10 requested a review from invidian August 24, 2020 11:10
@knrt10 knrt10 force-pushed the knrt10/aws-taints-and-labels branch from cfcfbb3 to 8a6ef6c Compare August 24, 2020 11:20
invidian
invidian suggested changes Aug 24, 2020
View reviewed changes
test/components/kubernetes/kubelet_taint_test.go Outdated Show resolved Hide resolved
test/components/kubernetes/kubelet_taint_test.go Outdated Show resolved Hide resolved
@knrt10 knrt10 force-pushed the knrt10/aws-taints-and-labels branch from 8a6ef6c to 874099f Compare August 24, 2020 12:43
@knrt10 knrt10 requested a review from invidian August 24, 2020 12:44
@knrt10 knrt10 force-pushed the knrt10/aws-taints-and-labels branch 2 times, most recently from ed8b026 to e5bac44 Compare August 24, 2020 13:36
invidian
invidian suggested changes Aug 24, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just one nit, otherwise LGTM.

ci/aws/aws-cluster.lokocfg.envsubst Show resolved Hide resolved
@knrt10 knrt10 force-pushed the knrt10/aws-taints-and-labels branch from e5bac44 to 029ea6a Compare August 24, 2020 13:48
@knrt10 knrt10 requested a review from invidian August 24, 2020 13:49
invidian
invidian previously approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM

@knrt10
aws: Add test for taints and labels
8726796 
This checks whether taints are labels get applied succesfully after
cluster creation.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10 knrt10 force-pushed the knrt10/aws-taints-and-labels branch from 029ea6a to 8726796 Compare August 24, 2020 14:03
@knrt10 knrt10 merged commit 5eaf0b2 into master Aug 24, 2020
@knrt10 knrt10 deleted the knrt10/aws-taints-and-labels branch August 24, 2020 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@invidian invidian invidian approved these changes

@surajssd surajssd Awaiting requested review from surajssd

@ipochi ipochi Awaiting requested review from ipochi

@johananl johananl Awaiting requested review from johananl

@pothos pothos Awaiting requested review from pothos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set custom labels and taints for AWS platform
2 participants
@knrt10 @invidian