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

[Multicast] Add multicast e2e tests #2986

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

ceclinux
Copy link
Contributor

@ceclinux ceclinux commented Nov 5, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2021

Codecov Report

Merging #2986 (ab40a6c) into main (1b08a61) will decrease coverage by 8.16%.
The diff coverage is n/a.

❗ Current head ab40a6c differs from pull request most recent head e00e7aa. Consider uploading reports for the commit e00e7aa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2986      +/-   ##
==========================================
- Coverage   59.79%   51.63%   -8.17%     
==========================================
  Files         306      300       -6     
  Lines       26178    35724    +9546     
==========================================
+ Hits        15654    18447    +2793     
- Misses       8807    15527    +6720     
- Partials     1717     1750      +33     
Flag Coverage Δ
e2e-tests 51.63% <ø> (?)
kind-e2e-tests ?
unit-tests ?

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

Impacted Files Coverage Δ
pkg/controller/egress/controller.go 0.00% <0.00%> (-88.45%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 4.58% <0.00%> (-86.85%) ⬇️
pkg/agent/util/iptables/lock.go 0.00% <0.00%> (-81.82%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 0.00% <0.00%> (-80.29%) ⬇️
pkg/controller/ipam/validate.go 0.00% <0.00%> (-80.00%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go 0.00% <0.00%> (-79.77%) ⬇️
...kg/agent/flowexporter/connections/conntrack_ovs.go 0.00% <0.00%> (-77.58%) ⬇️
pkg/controller/externalippool/validate.go 0.00% <0.00%> (-76.20%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go 3.47% <0.00%> (-75.70%) ⬇️
pkg/cni/client.go 0.00% <0.00%> (-75.52%) ⬇️
... and 296 more

@XinShuYang
Copy link
Contributor

/test-multicast-e2e

3 similar comments
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

1 similar comment
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux ceclinux force-pushed the add_multicast_e2e_test branch 3 times, most recently from 9791ccd to 7312995 Compare January 20, 2022 02:08
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

return []string{}, err
}
agentConfData := configMap.Data["antrea-agent.conf"]
for _, line := range strings.Split(agentConfData, "\n") {
Copy link
Member

Choose a reason for hiding this comment

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

var agentConf agentconfig.AgentConfig
if err := yaml.Unmarshal([]byte(agentConfData, &agentConf); err != nil {
	return nil, fmt.Errorf("failed to unmarshal Agent config from ConfigMap: %v", err)
}
return agentConfData.MulticastInterfaces, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

if err != nil {
return nil, err
}
multicastInterfaces, err := data.GetMulticastInterfaces(antreaNamespace)
Copy link
Member

Choose a reason for hiding this comment

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

Move it to out of the loop as it's same for all Nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

} else {
nodeIdx = workerIdx
workerIdx++
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just iterate clusterInfo.nodes, all above code can be removed.

for i, node := range clusterInfo.nodes {
        _, localInterfacesStr, _, err := RunCommandOnNode(node.name, fmt.Sprintf("ls /sys/class/net"))
        ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}

var nodeMulticastInterfaces [][]string
var transportInterface string
Copy link
Member

Choose a reason for hiding this comment

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

avoid to define such gloabl variables, it should not be hard to pass them via arugments. They are easy to conflict in this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated. removed

break
}
}
if !multipleInterfacesTested {
Copy link
Member

Choose a reason for hiding this comment

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

t.Run("runTestMulticastForwardToMultipleInterfaces", func(t *testing.T) {
	multipleInterfacesFound := false
	var nodeIdx int
	for i, ifaces := range nodeMulticastInterfaces {
		if len(ifaces) >= 2 {
			multipleInterfacesFound = true
			nodeIdx = i
			break
		}
	}
	if !multipleInterfacesFound {
		t.Skip("Skipping test because none of the Nodes has more than one multicast enabled interface")
	}
	runTestMulticastForwardToMultipleInterfaces(t, data, nodeIdx, 3464, "224.3.4.13")
})

To leverage the Skip method so that the test will be executed anyway and marked as skipped like other skipped tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

2 similar comments
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

test/e2e/multicast_test.go Outdated Show resolved Hide resolved
test/e2e/multicast_test.go Show resolved Hide resolved
test/e2e/multicast_test.go Show resolved Hide resolved
}
return true, nil
}); err != nil {
t.Fatalf("Error when waiting for multicast routes and stats: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't wait for multicast routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Signed-off-by: Ruochen Shen <src655@gmail.com>
@ceclinux
Copy link
Contributor Author

/test-multicast-e2e

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Jan 20, 2022

/test-multicast-e2e
/test-e2e
/test-conformance
/test-networkpolicy

@tnqn
Copy link
Member

tnqn commented Jan 20, 2022

/test-integration

2 similar comments
@tnqn
Copy link
Member

tnqn commented Jan 20, 2022

/test-integration

@tnqn
Copy link
Member

tnqn commented Jan 20, 2022

/test-integration

@tnqn
Copy link
Member

tnqn commented Jan 20, 2022

/skip-integration This PR has nothing to do with integration test but the check kept failing. I have ran the test manually and created #3213 to track the test issue.

@tnqn tnqn merged commit 839dc5c into antrea-io:main Jan 20, 2022
@wenyingd wenyingd mentioned this pull request Jan 21, 2022
12 tasks
yanjunz97 pushed a commit to yanjunz97/antrea that referenced this pull request Feb 14, 2022
Signed-off-by: Ruochen Shen <src655@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants