-
Notifications
You must be signed in to change notification settings - Fork 135
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
cnf-tests: add tap device tests #725
cnf-tests: add tap device tests #725
Conversation
/retest |
/retest |
1 similar comment
/retest |
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.
Thanks for the PR I leave some comments but the main issue in the PR is that I was not able to find the place where you configure the sriovNetworkNodePolicy to mount the vhost-net device.
you can please also add a check before you start the dpdk application that /dev/vhost-net exist inside the container?
if err != nil { | ||
return "", err | ||
} | ||
macLine := strings.Trim(strings.Split(getMacOutput.String(), "\n")[1], " ") |
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.
please check the len here before you do the [1]
to be sure we are not going to fly
Waiting for: k8snetworkplumbingwg/sriov-network-device-plugin#394 |
@mmirecki everything is merged in the sriov operator, can you please rebase and check this PR. Also please remove the mknod capability and the creation of the tun device. |
sleep INF | ||
`, dpdkResourceName, testpmdCommand, dpdkResourceName, runningTime)}, | ||
command, | ||
}, | ||
SecurityContext: &corev1.SecurityContext{ | ||
RunAsUser: pointer.Int64Ptr(0), | ||
Capabilities: &corev1.Capabilities{ | ||
// Enable NET_RAW is required by mellanox nics as they are using the netdevice driver |
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.
can you move the comment above this line (capabilities := []corev1.Capability{"IPC_LOCK", "SYS_RESOURCE", "NET_RAW"}
)
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.
Done
@@ -1397,3 +1506,42 @@ func CleanSriov() { | |||
} | |||
sriov.WaitStable(sriovclient) | |||
} | |||
|
|||
func getDeviceMac(pod *corev1.Pod, device string) (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.
no need for this function lets just static allocate the mac addresses
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.
Done
Hi @mmirecki please take a look on the CI lane
|
@@ -74,6 +77,8 @@ var ( | |||
intelVendorID = "8086" | |||
|
|||
sriovNicsTable []TableEntry | |||
|
|||
dpdkWorkloadMac = "60:00:00:00:00:01" |
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: I think this can be a const
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.
done
}, 2*time.Minute, 1*time.Second).Should(ContainSubstring(LOG_ENTRY), | ||
"Cannot find accumulated statistics") | ||
By("Checking the tx output from the client DPDK application") | ||
checkTxOnly(out) |
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.
can you please also check the tap side here
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.
done
Expect(len(lines)).To(BeNumerically(">=", 3)) | ||
for i, line := range lines { | ||
if strings.Contains(line, "NIC statistics for port") { | ||
if getNumberOfPackets(lines[i+1], "RX") > 0 { |
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.
should you check here if i+1 exist?
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.
done
return out | ||
}, 8*time.Minute, 1*time.Second).Should(ContainSubstring("NIC statistics for port"), | ||
"Cannot find accumulated statistics") | ||
checkRx(out) |
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.
should the function be checkRxOnly
like checkTxOnly
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.
done
/retest |
/retest |
1 similar comment
/retest |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mmirecki, SchSeba The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.