-
Notifications
You must be signed in to change notification settings - Fork 373
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
Antrea native secondary network feature e2e test framework. #4252
Antrea native secondary network feature e2e test framework. #4252
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4252 +/- ##
==========================================
- Coverage 74.38% 67.21% -7.18%
==========================================
Files 414 404 -10
Lines 62884 60512 -2372
==========================================
- Hits 46777 40672 -6105
- Misses 13129 16992 +3863
+ Partials 2978 2848 -130
*This pull request uses carry forward flags. Click here to find out more. |
test/e2e/secondary_network_test.go
Outdated
) | ||
|
||
// The function ensureAntreaRunningWithSecondaryNetworkOption ensures that the antrea is enabled and running with sriov and whereabouts secondary network options | ||
func (data *TestData) ensureAntreaRunningWithSecondaryNetworkOption() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this doesn't belong in the test files. The scripts in #4097 already take care (or should take care) of deploying Antrea with the correct options for SecondaryNetwork testing.
Based on our Slack discussion, this is to counteract
Line 122 in e7486d5
err = ensureAntreaRunning(testData) |
from the main function, which overwrites the Antrea deployment.
Which means that:
- we have scripts that deploy Antrea with the correct configuration
- the deployment is overwritten by
ensureAntreaRunning
- we have to run
ensureAntreaRunningWithSecondaryNetworkOption
to go back to the correct depoloyment
That seems wasteful.
I think the best option to move forward is to move these tests to test/e2e-secondary-network/
and let them have their own main
function. I still feel that these tests don't really fit well within the main suite of tests, which supports things like code coverage measurement.
test/e2e/secondary_network_test.go
Outdated
|
||
// The function getSecondaryInterface shows up the secondary interfaces created for the specific pod and extracts the IP address for the same. | ||
func (data *TestData) getSecondaryInterface(targetPod int, targetInterface int) (string, error) { | ||
respCode, output, _, err := data.RunCommandOnNode(controlPlaneNodeName(), fmt.Sprintf("kubectl exec -it %s -n kube-system -- ip addr show %s | grep \"inet\" | awk '{print $2}' | cut -d/ -f1", podData[targetPod].nameOfPods, podData[targetPod].nameOfInterfacePerPod[targetInterface])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you need to run a command on a Pod, use RunCommandOnPod
. RunCommandOnNode
should be avoided as much as possible, as it makes assumptions as to which software is available on each Node.
ced1dce
to
950d93b
Compare
The PR includes two commits. Is this one a mistake: "Merge branch 'antrea-io:main' into antrea-secondary-network-test-sep22"? |
@jianjuns yes, the 2nd commit was a mistake during rebase and move my local repo to a different workspace. I will get it address. please review the other commit on this PR. |
aa2da53
to
950d93b
Compare
e2eTestData *antreae2e.TestData | ||
kubeConfig *restclient.Config | ||
clientset kubernetes.Interface | ||
aggregatorClient aggregatorclientset.Interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't seem to be using this clientset?
busyboxImage = "projects.registry.vmware.com/antrea/busybox" | ||
) | ||
|
||
var SNtestData *SNTestData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just rename it to testData
/ TestData
, there is no risk of name collision since this is in its own package
return nil | ||
} | ||
|
||
func (data *SNTestData) RunPingCommandFromTestPod(podInfo podInfo, ns string, targetPodIPs *PodIPs, ctrName string, count int, size int) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to redefine these functions?
could you just export the existing function in the other e2e package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed to update on this comment. runPingCommandFromTestPod at test/e2e was not exported. exporting this would lead to changes all the other test functions which is using it already. If we are good with it, then great. I can goahead and make that change today and update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took care of these changes couple of weeks back. sorry, missed to follow-up on this PR. Please review when you get sometime.
type PodConfig struct { | ||
InterfaceType struct { | ||
InterfaceType string `yaml:"interfacetype"` | ||
} `yaml:"interface_type"` | ||
SriovConf struct { | ||
Networkinterface string `yaml:"networkinterface"` | ||
Numberofvfs int `yaml:"numberofvfs"` | ||
} `yaml:"sriov_conf"` | ||
VirNet struct { | ||
TotalNumberOfVirtualNetworks int `yaml:"totalnumberofvirtualnetworks"` | ||
Virtualnetworknames []string `yaml:"virtualnetworknames"` | ||
} `yaml:"vir_net"` | ||
CreatePod struct { | ||
Numberofpods int `yaml:"numberofpods"` | ||
Describe [][]interface{} `yaml:"describe"` | ||
} `yaml:"create_pod"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use camelCase consistently for yaml field names
if err := data.extractPodsInfo(); err != nil { | ||
tb.Errorf("Error in extracting Pods info from secondary_network_configuration.yml : %v", err) | ||
return nil, err | ||
} | ||
// Set log directory for test execution. | ||
if err := data.setupLogDirectoryForTest(tb.Name()); err != nil { | ||
tb.Errorf("Error creating logs directory '%s': %v", data.logsDirForTestCase, err) | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you return the error, I don't think you should be calling tb.Error
var totalNumberOfPods int | ||
var interfaceType string | ||
|
||
const secondary_network_config_yaml = "../e2e/infra/secondary_network/secondary_network_configuration.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use camelCase for variable names
|
||
for podCheckForSubnet := 0; podCheckForSubnet < podData[sourcePod].countOfVirtualNetworksPerPod; podCheckForSubnet++ { | ||
if podData[sourcePod].nameOfVirtualNetworkPerPod[podCheckForSubnet] == podData[targetPod].nameOfVirtualNetworkPerPod[targetInterface] { | ||
time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't use sleeps in tests as much as possible as it leads to flaky tests or tests that take longer to run than they should
we typically use wait.Poll
950d93b
to
21f1e65
Compare
f71fd40
to
92e2fc8
Compare
7746f3a
to
f6f2c63
Compare
@arunvelayutham : could you check these two comments from @antoninbas ? |
d950dc5
to
afec677
Compare
@jianjuns @antoninbas addressed the pending comments as well and updated the PR couple of days back. Please review further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of CI jobs failing
"time" | ||
|
||
corev1 "k8s.io/api/core/v1" | ||
//restclient "k8s.io/client-go/rest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove if no longer needed
InterfaceType struct { | ||
InterfaceType string `yaml:"interfacetype"` | ||
} `yaml:"interface_type"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the nesting here for a simple string value
const secondaryNetworkConfigYaml = "../e2e/infra/secondary_network/secondary_network_configuration.yml" | ||
const nameSpace = "kube-system" | ||
const ctrName = "busyboxpod" | ||
const testPodName = "testsecpod" | ||
const osType = "linux" | ||
const count = 5 | ||
const size = 40 | ||
const defaultTimeout = 10 * time.Second | ||
const reqName = "intel.com/intel_sriov_netdevice" | ||
const resNum = 3 | ||
const ( | ||
podName = iota | ||
podVNsCount | ||
podVirtualNetwork | ||
podInterfaceName | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
group const
s as one block?
} `yaml:"vir_net"` | ||
CreatePod struct { | ||
NumberOfPods int `yaml:"numberofpods"` | ||
Describe [][]interface{} `yaml:"describe"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we use interface{}
here instead of a concrete type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interface used here to make the extractPodInfo() more readable and easier to process the pod description data from the test input file (specific to secondary network test config).
Please check extractPodsInfo() for the details.
} else { | ||
t.Logf("Error in Ping: Target interface %v of %v Pod not created", podData[targetPod].nameOfInterfacePerPod[targetInterface], podData[targetPod].nameOfPods) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
for _, s := range service.CreatePod.Describe { | ||
output := describePodInfo{nameOfPods: s[podName].(string), countOfVirtualNetworksPerPod: s[podVNsCount].(int), nameOfVirtualNetworkPerPod: strings.Split(s[podVirtualNetwork].(string), ","), nameOfInterfacePerPod: strings.Split(s[podInterfaceName].(string), ",")} | ||
podData = append(podData, output) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
afec677
to
fcf62ed
Compare
@arunvelayutham : please fix the test errors (could you check the failed tests after an update?):
|
sorry @jianjuns, today's push was not ready for review. I did push while moving to a different workspace. I will ensure all works before request for review |
0cbbe9b
to
1c20970
Compare
/test-all |
dd1b668
to
6af366c
Compare
test/e2e/framework.go
Outdated
// PodInfo combines OS info with a Pod name. It is useful when choosing commands and options on Pods of different OS (Windows, Linux). | ||
type PodInfo struct { | ||
Name string | ||
Os string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Os -> OS
Use upper case for abbreviations.
package e2e | ||
|
||
import ( | ||
antreae2e "antrea.io/antrea/test/e2e" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We divide imports to sub-groups, like:
import (
"log"
"os"
"path/filepath"
"time"
corev1 "k8s.io/api/core/v1"
antreae2e "antrea.io/antrea/test/e2e"
)
|
||
var clusterInfo ClusterInfo | ||
|
||
// podWaitForRunning polls the k8s apiserver until the specified Pod is in the "running" state (or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: k8s -> K8s
|
||
// podWaitForRunning polls the k8s apiserver until the specified Pod is in the "running" state (or | ||
// until the provided timeout expires). | ||
func (data *TestData) podWaitForRunning(timeout time.Duration, name, namespace string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question - do we really need to wrap these funcs, compared to directly calling data.e2eTestData.Func()
in code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we create few wrappers with references to how a similar scenario was handled at the multicluster test implementation. removed this now and addressed as suggested. Please note: updated code was not tested (just compiled) due to K8s cluster setup issues. I will test the e2e flows once its recovered.
return err | ||
} | ||
|
||
func (data *TestData) setupLogDirectoryForTest(testName string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to re-implement this func, instead of reusing e2eTestData.setupLogDirectoryForTest()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setupLog**() was not exported at test/e2e. More over, we create few wrappers with references to how a similar scenario was handled at the multicluster test implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @antoninbas @luolanzone know the background of the MC design.
I am fine to start with the current code anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a notable difference with the existing e2e tests is that the e2e-secondary-network tests assume that antrea has already been deployed, with the correct configuration (that's by design). So we cannot reuse setupTest
for example.
That being said, I tend to agree with Jianjun about reusing TestOptions
and TestData
as is.
One thing that is also not clear to me is whether the e2e-secondary-network tests actually export any logs? In Antrea e2e tests, this is handled by
Line 277 in f7d70c3
func exportLogs(tb testing.TB, data *TestData, logsSubDir string, writeNodeLogs bool) { |
But I don't see any code here that would export logs? I could be missing something.
var interfaceType string | ||
|
||
const ( | ||
secondaryNetworkConfigYaml = "../e2e/infra/secondary_network/secondary_network_configuration.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: secondaryNetworkConfigYaml -> secondaryNetworkConfigYAML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secondary_network_configuration.yml -> secondary-network-configuration.yml
return data, nil | ||
} | ||
|
||
// extractPodsInfo extracts the Pod and secondary network interface information for the creation of Podsfrom secondary_network_configuration.yaml file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secondary_network_configuration.yml -> secondary-network-configuration.yml
return nil | ||
} | ||
|
||
// formAnnotationStringOfPod forms the annotation string, used in the generation of each Pod yaml file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: yaml -> YAML
} | ||
|
||
func TestNativeSecondaryNetwork(t *testing.T) { | ||
//skipIfHasWindowsNodes(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took care.
if err != nil { | ||
t.Logf("Error when up setupTestWithSecondaryNetworkConfig: %v", err) | ||
} | ||
//defer teardownSecondaryNetworkTest(t, data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncomment or remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took care.
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days |
6af366c
to
fd3d8f7
Compare
fd3d8f7
to
4369655
Compare
a04d1ec
to
3e9eb6c
Compare
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days |
0eb3a9f
to
3e9eb6c
Compare
3e9eb6c
to
4a3462e
Compare
/test-all |
@arunvelayutham : there are still unresolved conflicts. For example: https://github.com/antrea-io/antrea/pull/4252/files#diff-324aca6894566ddefcf73b4ec0cd7a70a4555749e4cce64fa56969c5181ad892R623 |
4a3462e
to
0d2d6fa
Compare
Signed-off-by: Arunkumar Velayutham <arunkumar.velayutham@intel.com>
0d2d6fa
to
3797ee8
Compare
/test-all |
/test-ipv6-e2e |
/test-ipv6-e2e |
/test-ipv6-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge the PR. Let us continue improving the code.
Signed-off-by: Arunkumar Velayutham arunkumar.velayutham@intel.com