Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add and improve the e2e use case of the NIC where the default route is located #2425

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

ty-dc
Copy link
Collaborator

@ty-dc ty-dc commented Oct 16, 2023

What this PR does / why we need it:

The changes are as follows:

  1. Add a new case. Without setting any annotations or PodDefaultRouteNIC (default situation), the default route should be on net1.

  2. Change the existing use case, specify PodDefaultRouteNIC as eth0, and check that the default route should be on eth0

Which issue(s) this PR fixes:

NONE

make sure your commit is signed off
Signed-off-by: ty-dc [email protected]

@ty-dc ty-dc added the release/none no release note label Oct 16, 2023
@ty-dc ty-dc requested a review from cyclinder October 16, 2023 07:34
@ty-dc
Copy link
Collaborator Author

ty-dc commented Oct 16, 2023

debug log:

  exec dep-name-c177c54e8b-5ff8f49c55-q48hx -n ns-6414376efd -- ip r get '8.8.8.8' 
  Execute command result:  8.8.8.8 via 10.220.144.1 dev net1  src 10.220.144.2 

  [FAILED] Expected NIC eth0 mismatch
  Expected
      <string>: 8.8.8.8 via 10.220.144.1 dev net1  src 10.220.144.2 
      
  to contain substring
      <string>: eth0
  In [It] at: /root/code/spiderpool/test/e2e/coordinator/macvlan-overlay-one/macvlan_overlay_one_test.go:302 

Expect(err).NotTo(HaveOccurred(), "failed to execute command, error is: %v ", err)
Expect(string(executeCommandResult)).Should(ContainSubstring(common.NIC1), "Expected NIC %v mismatch", common.NIC1)

// ip r get <IP in net1 subnet>, should flow out from net1
Copy link
Collaborator

Choose a reason for hiding this comment

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

是否还可以加个 ip r get <service_subnet>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #2425 (6fdf4fb) into main (39eb7b4) will not change coverage.
Report is 6 commits behind head on main.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2425   +/-   ##
=======================================
  Coverage   80.97%   80.97%           
=======================================
  Files          49       49           
  Lines        5251     5251           
=======================================
  Hits         4252     4252           
  Misses        842      842           
  Partials      157      157           
Flag Coverage Δ
unittests 80.97% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@ty-dc ty-dc force-pushed the e2e/new-3 branch 2 times, most recently from dc36928 to 00dffd8 Compare October 16, 2023 10:12
@weizhoublue
Copy link
Collaborator

is this ready for merge ?

@weizhoublue
Copy link
Collaborator

it could add commit together to pr for CI

@ty-dc ty-dc force-pushed the e2e/new-3 branch 3 times, most recently from bdb43b7 to 67728fe Compare October 18, 2023 03:33
@ty-dc ty-dc requested a review from Icarus9913 as a code owner October 18, 2023 03:33
@ty-dc ty-dc force-pushed the e2e/new-3 branch 6 times, most recently from 5652af4 to f24ed02 Compare October 18, 2023 06:31
if defaultInterfaceRoutes[idx].Family == netlink.FAMILY_V6 {
zeroIPAddress = net.IPv6zero
}
if route.Dst != nil || !route.Dst.IP.Equal(zeroIPAddress) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if !route.Dst.IP.Equal(zeroIPAddress) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

if defaultInterfaceRoutes[idx].Family == netlink.FAMILY_V6 {
zeroIPAddress = net.IPv6zero
}
if route.Dst != nil || !route.Dst.IP.Equal(zeroIPAddress) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if !route.Dst.IP.Equal(zeroIPAddress) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -179,6 +179,15 @@ setup_spiderpool:
fi ; \
if [ "$(E2E_SPIDERPOOL_ENABLE_COORDINATOR)" == "true" ] ; then \
HELM_OPTION+=" --set coordinator.enabled=true " ; \
if [ "$(INSTALL_CILIUM)" == "true" ] ; then \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, This is the same problem as #2433. It was fixed by #2434. it's OK to merge this part of the code for me. But we should revert these changes until #2434 has been merged.

Copy link
Collaborator Author

@ty-dc ty-dc Oct 18, 2023

Choose a reason for hiding this comment

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

OK, restoring this part of the code after the #2434 merge will also help verify the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/none no release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants