Skip to content

test: Replace MetalLB with dummyLB and re-enable LoadBalancer test#12852

Merged
borkmann merged 5 commits intomasterfrom
pr/brb/test-dummylb
Aug 18, 2020
Merged

test: Replace MetalLB with dummyLB and re-enable LoadBalancer test#12852
borkmann merged 5 commits intomasterfrom
pr/brb/test-dummylb

Conversation

@brb
Copy link
Copy Markdown
Member

@brb brb commented Aug 12, 2020 •
edited
Loading

This PR re-enables LoadBalancer service tests, and switches from MetalLB to dummylb [1]. The main reason for switching is that the former was operating in L2 mode which interfered with the vbox VM bridge resulting in the test flakes.

The new lb only assigns LoadBalancerIP addr to a service (specified by a user in .spec.loadBalancerIP). Therefore, making the LB IP addr reachable from other nodes is up to a user. This can be achieved by installing a route on a client node (as we do in the test).

Also, refactor IP{Add,Del}Route and ExecIn{FirstPod,HostNetNS} helpers (see commit messages).

[1]: https://github.com/cilium/dummylb.

Fix #10763

@brb brb added the release-note/ci This PR makes changes to the CI. label Aug 12, 2020
@brb brb mentioned this pull request Aug 12, 2020
Merged
3 tasks
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 12, 2020

retest-net-next

brb added a commit to cilium/packer-ci-build that referenced this pull request Aug 12, 2020
@brb
Cache docker.io/cilium/dummylb:0.0.1
0b9d358 
The image replaces the MetalLB used in the Cilium's integration tests.
See cilium/cilium#12852 for more details.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb mentioned this pull request Aug 12, 2020
Merged
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 12, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 37.148% when pulling cbc617f on pr/brb/test-dummylb into 85600be on master.

@brb brb force-pushed the pr/brb/test-dummylb branch from b29d678 to 78549f3 Compare August 12, 2020 13:23
@brb
test: Re-enable LoadBalancer tests
be4e5dd 
This commit re-enables LoadBalancer service tests, and switches from
MetalLB to dummylb [1]. The main reason for switching is that the former
was operating in L2 mode which interfered with the vbox VM bridge
resulting in the test flakes.

The new lb only assigns LoadBalancerIP addr to a service (specified by a
user in .spec.loadBalancerIP). Therefore, making the LB IP addr reachable
from other nodes is up to a user. This can be achieved by installing a
route on a client node (as we do in the test).

[1]: https://github.com/cilium/dummylb.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/test-dummylb branch from 78549f3 to 467e9e9 Compare August 12, 2020 15:51
@brb brb changed the title test: Replace MetalLB with dummyLB test: Replace MetalLB with dummyLB and re-enable LoadBalancer test Aug 12, 2020
@brb brb added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow pending-review labels Aug 12, 2020
@brb brb force-pushed the pr/brb/test-dummylb branch from 467e9e9 to f19af88 Compare August 12, 2020 15:55
@brb brb marked this pull request as ready for review August 12, 2020 15:56
@brb brb requested a review from a team as a code owner August 12, 2020 15:56
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 12, 2020

test-me-please

christarazi
christarazi requested changes Aug 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@christarazi christarazi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few areas to look into wrt to CI semantics

Comment thread test/helpers/kubectl.go Outdated
Comment thread test/k8sT/Services.go Outdated
@brb brb force-pushed the pr/brb/test-dummylb branch 2 times, most recently from 070ba7f to a8f4a60 Compare August 13, 2020 10:49
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 13, 2020

test-me-please

@brb brb requested a review from christarazi August 13, 2020 12:09
@brb brb force-pushed the pr/brb/test-dummylb branch from a8f4a60 to dfb0b52 Compare August 13, 2020 13:39
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 13, 2020

test-me-please

@brb brb force-pushed the pr/brb/test-dummylb branch from dfb0b52 to e34d4f2 Compare August 13, 2020 14:00
aanm
aanm requested changes Aug 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

I didn't pay too much attention to the CI code.

Comment thread test/k8sT/manifests/demo_ds.yaml Outdated
@brb brb requested a review from aanm August 13, 2020 14:46
brb added a commit to cilium/packer-ci-build that referenced this pull request Aug 13, 2020
@brb
Cache docker.io/cilium/dummylb:0.0.1
9d83451 
The image replaces the MetalLB used in the Cilium's integration tests.
See cilium/cilium#12852 for more details.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 13, 2020

test-me-please

christarazi
christarazi reviewed Aug 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@christarazi christarazi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the improvement of consolidating the returns and making it concise. A few points below. I also assume you'll squash the related refactors into one commit?

Comment thread test/k8sT/DatapathConfiguration.go
Comment thread test/k8sT/DatapathConfiguration.go
@brb
test: Refactor IP{Add,Del}Route helpers
c38d3ca 
- Invoke cmds for a given node instead of returning cmd itself - reduces
  boilerplate by quite a bit.
- Changes method names form to verb + noun.

Suggested-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/test-dummylb branch from e34d4f2 to c38d3ca Compare August 14, 2020 12:44
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 14, 2020

I also assume you'll squash the related refactors into one commit?

I squashed two *IPRoute related commits.

@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 14, 2020

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 14, 2020
joestringer
joestringer requested changes Aug 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

GKE seems to have a few failures particularly in tests modified by this PR. How do we want to handle this?
https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/2220/#showFailuresLink

@maintainer-s-little-helper maintainer-s-little-helper Bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 14, 2020
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

retest-gke

@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

GKE seems to have a few failures particularly in tests modified by this PR. How do we want to handle this?

@joestringer Good catch, I didn't dare to look into the GKE failures considering that's broken. Anyway, I've added a new commit which should fix the failures introduced by this PR.

@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

retest-gke

@brb brb requested a review from joestringer August 17, 2020 09:56
@brb
test: Do not provision LB SVC with IP addr on GKE
9cfe7dc 
Otherwise, GKE LB might fail to assign the IP addr to other LB svcs
which results in the following failure:

  <*errors.errorString | 0xc0002a6410>: {
              s: "could not get service LoadBalancer IP addr: 30s
              timeout expired",
  }

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/test-dummylb branch from 5c0295e to 9cfe7dc Compare August 17, 2020 13:00
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

retest-gke

2 similar comments
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

retest-gke

@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

retest-gke

@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020 •
edited
Loading

GKE CI: all the tests executed after the Update test are failing. When I run them manually on my GKE cluster, they pass. Going to disable the Update test for now.

@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

retest-gke

@brb
test: Add K8sUpdate on GKE to the quarantine
cbc617f 
Currently, it's broken, and it prevents other tests from being invoked.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Copy Markdown
Member Author

brb commented Aug 17, 2020

test-me-please

@borkmann borkmann merged commit fef1a0d into master Aug 18, 2020
@borkmann borkmann deleted the pr/brb/test-dummylb branch August 18, 2020 06:45
@brb brb mentioned this pull request Aug 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@borkmann borkmann borkmann approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

@joestringer joestringer Awaiting requested review from joestringer

Assignees

No one assigned

Labels

area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests with MetalLB Connectivity to endpoint via LB

6 participants

@brb @coveralls @borkmann @joestringer @christarazi @aanm