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

cnf network: Add Rdma Metrics Test Cases #202

Merged
merged 1 commit into from
Sep 26, 2024
Merged

cnf network: Add Rdma Metrics Test Cases #202

merged 1 commit into from
Sep 26, 2024

Conversation

ajaggapa
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@evgenLevin evgenLevin left a comment

Choose a reason for hiding this comment

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

overall looks good. Just several comments

@@ -15,4 +15,6 @@ const (
LabelExposeMTUTestCases = "exposemtu"
// LabelSriovMetricsTestCases represents Sriov Metrics Exporter label that can be used for test cases selection.
LabelSriovMetricsTestCases = "sriovmetrics"
// LabelRdmaMetricsTestCases represents Sriov Metrics Exporter label that can be used for test cases selection.
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong comment

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

var (
workerNodeList []*nodes.Builder
sriovInterfacesUnderTest []string
sriovDeviceID string
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove pls

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

)

BeforeAll(func() {
By("Verifying if Sriov Metrics Exporter tests can be executed on given cluster")
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong message

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

By("Verifying if Sriov Metrics Exporter tests can be executed on given cluster")
err := netenv.DoesClusterHasEnoughNodes(APIClient, NetConfig, 1, 1)
Expect(err).ToNot(HaveOccurred(),
"Cluster doesn't support Sriov Metrics Exporter test cases as it doesn't have enough nodes")
Copy link
Collaborator

Choose a reason for hiding this comment

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

here as well

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

By("Fetch PCI Address and Rdma device from Pod Annotations")
pciAddress, rdmaDevice, err := getInterfacePci(testPod, "net1")
Expect(err).ToNot(HaveOccurred(),
fmt.Sprintf("Could not get PCI Address and/or Rdma device from Pod Annotations with Error: %s", err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need err in the descriotion, It'll be shown in any way

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

Expect(err).ToNot(HaveOccurred(), "Failed to serialize ignition config")

createdMC, err := mco.NewMCBuilder(APIClient, "02-rdma-netns-exclusive-mode").
WithLabel("machineconfiguration.openshift.io/role", "workercnf").
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

CreateAndWaitUntilRunning(2 * time.Minute)
Expect(err).ToNot(HaveOccurred(), "Failed to create Pod %s")

tPod.Exists()
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CreateAndWaitUntilRunning() returns the pod builder which is still in Pending status. So it does not have network-status annotations set. Exists() updates the builder with all fields, so we can see the annotations.

Ideally this should be part of CreateAndWaitUntilRunning method.


pciAddress, rdmaDevice, err := getInterfacePci(inputPod, iName)
Expect(err).ToNot(HaveOccurred(),
fmt.Sprintf("Could not get PCI Address and/or Rdma device from Pod Annotations with Error: %s", err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need err in the description

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

@ajaggapa ajaggapa requested a review from evgenLevin September 17, 2024 12:16
evgenLevin
evgenLevin previously approved these changes Sep 18, 2024
Copy link
Collaborator

@evgenLevin evgenLevin left a comment

Choose a reason for hiding this comment

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

lgtm


tPol := defineAndCreateNodePolicy("rdmapolicy1", "sriovpf1", sriovInterfacesUnderTest[0], 1, 0)
tNet := defineAndCreateSriovNetworkWithRdma("sriovnet1", tPol.Object.Spec.ResourceName, false)
testPod := defineAndCreatePod(tNet.Object.Name, "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
testPod := defineAndCreatePod(tNet.Object.Name, "")
testPod := defineAndCreatePod(tNet.Object.Name)

By("Define and Create Test Resources")
tPol := defineAndCreateNodePolicy("rdmapolicy1", "sriovpf1", sriovInterfacesUnderTest[0], 1, 0)
tNet := defineAndCreateSriovNetworkWithRdma("sriovnet1", tPol.Object.Spec.ResourceName, true)
testPod := defineAndCreatePod(tNet.Object.Name, "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
testPod := defineAndCreatePod(tNet.Object.Name, "")
testPod := defineAndCreatePod(tNet.Object.Name)

Copy link
Member

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

lgtm just two small comments

Comment on lines +8 to +25
ignition "github.com/coreos/ignition/v2/config/v3_1/types"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/openshift-kni/eco-goinfra/pkg/mco"
"github.com/openshift-kni/eco-goinfra/pkg/namespace"
"github.com/openshift-kni/eco-goinfra/pkg/nodes"
"github.com/openshift-kni/eco-goinfra/pkg/pod"
"github.com/openshift-kni/eco-goinfra/pkg/reportxml"
"github.com/openshift-kni/eco-goinfra/pkg/sriov"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netenv"
. "github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netinittools"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netparam"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/sriov/internal/sriovenv"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/sriov/internal/tsparams"
"github.com/openshift-kni/eco-gotests/tests/internal/cluster"
"gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/types"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ignition "github.com/coreos/ignition/v2/config/v3_1/types"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/openshift-kni/eco-goinfra/pkg/mco"
"github.com/openshift-kni/eco-goinfra/pkg/namespace"
"github.com/openshift-kni/eco-goinfra/pkg/nodes"
"github.com/openshift-kni/eco-goinfra/pkg/pod"
"github.com/openshift-kni/eco-goinfra/pkg/reportxml"
"github.com/openshift-kni/eco-goinfra/pkg/sriov"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netenv"
. "github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netinittools"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netparam"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/sriov/internal/sriovenv"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/sriov/internal/tsparams"
"github.com/openshift-kni/eco-gotests/tests/internal/cluster"
"gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/types"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
ignition "github.com/coreos/ignition/v2/config/v3_1/types"
"gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/types"
"github.com/openshift-kni/eco-goinfra/pkg/mco"
"github.com/openshift-kni/eco-goinfra/pkg/namespace"
"github.com/openshift-kni/eco-goinfra/pkg/nodes"
"github.com/openshift-kni/eco-goinfra/pkg/pod"
"github.com/openshift-kni/eco-goinfra/pkg/reportxml"
"github.com/openshift-kni/eco-goinfra/pkg/sriov"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netenv"
. "github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netinittools"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/internal/netparam"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/sriov/internal/sriovenv"
"github.com/openshift-kni/eco-gotests/tests/cnf/core/network/sriov/internal/tsparams"
"github.com/openshift-kni/eco-gotests/tests/internal/cluster"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated as mentioned.

Expect(sriovDeviceID).ToNot(BeEmpty(), "Expected sriovDeviceID not to be empty")

By("Skipping test cases if the Sriov device is not of Mellanox")
if !(sriovDeviceID == netparam.MlxDeviceID || sriovDeviceID == netparam.MlxBFDeviceID) {
Copy link
Member

Choose a reason for hiding this comment

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

can can check the vendorID as any of the mlx devices support RDMA

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I updated it to use Vendor ID now.

@gkopels
Copy link
Collaborator

gkopels commented Sep 24, 2024

lgtm

@evgenLevin evgenLevin merged commit 777029e into openshift-kni:main Sep 26, 2024
1 check passed
josclark42 pushed a commit to josclark42/eco-gotests that referenced this pull request Oct 3, 2024
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.

4 participants