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

MetalLB: Fix regressions of tolerations and nodeSelectors #927

Merged
merged 5 commits into from
Sep 10, 2020
Merged

MetalLB: Fix regressions of tolerations and nodeSelectors #927

merged 5 commits into from
Sep 10, 2020

Conversation

surajssd
Copy link
Member

@surajssd surajssd commented Sep 9, 2020

  • Remove the tolerations.
  • Remove the nodeSelector from yaml and move the golang templated block down.

@surajssd surajssd requested a review from ipochi as a code owner September 9, 2020 11:07
@surajssd surajssd removed the request for review from ipochi September 9, 2020 11:07
johananl
johananl reviewed Sep 9, 2020
View reviewed changes
pkg/components/metallb/manifests.go Outdated
@@ -371,13 +365,19 @@ spec:
- ALL
readOnlyRootFilesystem: true
hostNetwork: true
# XXX: Lokomotive spcific change.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's worth worrying about, but I've used a different phrasing here:

lokomotive/pkg/components/metallb/manifests.go

Line 289 in fa7f06a

// Divergence from upstream: disable fast dead node detection for layer 2. We use BGP and therefore

I'll leave this up to you.

@surajssd surajssd force-pushed the surajssd/fix-metallb-toleration-regression branch from fa7f06a to d98d857 Compare September 9, 2020 11:11
@johananl
Copy link
Member

johananl commented Sep 9, 2020

I've tested the following manually:

  • Speaker node selector is set to match a label which exists only on one node. A speaker is scheduled only on that node as expected.
  • A node is tainted. No tolerations are configured. A speaker isn't scheduled on the tainted node as expected.
  • A speaker toleration is added. A speaker is now scheduled on the tainted node as expected.

johananl
johananl previously approved these changes Sep 9, 2020
View reviewed changes
@niels-s
Copy link
Contributor

niels-s commented Sep 9, 2020

I'm not familiar with the testing strategy for the lokomotive repo, but will you also provide a regression test to check the custom tolerations/node selectors are added, just to prevent this from happening in the future?

invidian
invidian previously approved these changes Sep 9, 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.

Looks good, but yeah, some regression tests for it would be nice. Even to render the manifests and scan them for certain strings.

@surajssd surajssd dismissed stale reviews from invidian and johananl via 60312a5 September 9, 2020 14:39
@johananl johananl mentioned this pull request Sep 9, 2020
Merged
@surajssd surajssd force-pushed the surajssd/fix-metallb-toleration-regression branch 2 times, most recently from dbf89dd to 12c491e Compare September 9, 2020 16:03
@johananl johananl added this to the v0.4.1 milestone Sep 9, 2020
@surajssd surajssd force-pushed the surajssd/fix-metallb-toleration-regression branch 2 times, most recently from eb9aefd to 0bff235 Compare September 9, 2020 17:09
invidian
invidian reviewed Sep 10, 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.

Temporarily tests seems OK, but I'd definitely prefer to have e2e tests for it, so we can test the functionality, not the implementation.

pkg/components/metallb/component_test.go Show resolved Hide resolved
pkg/components/metallb/component_test.go Outdated Show resolved Hide resolved
pkg/components/metallb/component_test.go Show resolved Hide resolved
@surajssd surajssd requested a review from invidian September 10, 2020 13:17
@surajssd
metallb: Add speaker tolerations removed by mistake
b305811 
This commit fixes the regression introduced in the previous update of
metallb.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
metallb: Remove additional nodeSelector in daemonset
4651ffa 
- Fix the regression added in the previous update by removing the
  extraneous nodeSelector field.

- Move the nodeSelector golang template block down so that in the next
  update cycle we don't encounter this issue.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
metallb: Divide helper function to return config
b84b94d 
This commit divides the functionality of `testRenderManifest` that
generates yaml files into a separate function `renderManifest`. This
will be helpful in subsequent test additions.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
metallb: Add tests to verify conversion
df6e01e 
This commit adds tests that check if the specified component attributes
in hcl configs are correctly converted to corresponding Kubernetes yaml
objects.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
metallb: Remove additional nodeSelector in deployment
0a452fd 
- Fix the regression added in the previous update by removing the
  extraneous nodeSelector field.

- Move the nodeSelector golang template block down so that in the next
  update cycle we don't encounter this issue.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd surajssd force-pushed the surajssd/fix-metallb-toleration-regression branch from 0bff235 to 0a452fd Compare September 10, 2020 13:45
invidian
invidian approved these changes Sep 10, 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.

OK

@surajssd surajssd merged commit 2c42f34 into master Sep 10, 2020
@surajssd surajssd deleted the surajssd/fix-metallb-toleration-regression branch September 10, 2020 15:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@invidian invidian invidian approved these changes

@johananl johananl Awaiting requested review from johananl

@rata rata Awaiting requested review from rata

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4.1
Development

Successfully merging this pull request may close these issues.
5 participants
@surajssd @johananl @niels-s @rata @invidian