From 925187c690a441a570ebb29f0db7b3b262da98dd Mon Sep 17 00:00:00 2001 From: Brandon Palm Date: Thu, 14 Nov 2024 14:32:44 -0600 Subject: [PATCH] remove NAD config processing; only look at SriovNetwork objects --- .github/workflows/pre-main.yaml | 6 ++ pkg/provider/pods.go | 52 ++---------- pkg/provider/pods_test.go | 141 -------------------------------- 3 files changed, 13 insertions(+), 186 deletions(-) diff --git a/.github/workflows/pre-main.yaml b/.github/workflows/pre-main.yaml index 889b226ab..9fabe93ef 100644 --- a/.github/workflows/pre-main.yaml +++ b/.github/workflows/pre-main.yaml @@ -250,6 +250,9 @@ jobs: - name: Check the smoke test results against the expected results template run: ./certsuite check results --log-file="certsuite-out/certsuite.log" + - name: Print the certsuite.log + run: cat certsuite-out/certsuite.log + - name: 'Test: Run preflight specific test suite' run: ./certsuite run --label-filter=preflight --log-level="${SMOKE_TESTS_LOG_LEVEL}" @@ -376,6 +379,9 @@ jobs: - name: Check the smoke test results against the expected results template run: ./certsuite check results --log-file="${CERTSUITE_OUTPUT_DIR}"/certsuite.log + - name: Print the certsuite.log + run: cat "${CERTSUITE_OUTPUT_DIR}"/certsuite.log + - name: 'Test: Run Preflight Specific Smoke Tests in a Certsuite container with the certsuite command' run: | docker run --rm --network host \ diff --git a/pkg/provider/pods.go b/pkg/provider/pods.go index 6f244f32b..867b9aa55 100644 --- a/pkg/provider/pods.go +++ b/pkg/provider/pods.go @@ -450,8 +450,6 @@ func (p *Pod) IsUsingSRIOV() (bool, error) { } // IsUsingSRIOVWithMTU returns true if any of the pod's interfaces is a sriov one with MTU set. -// -//nolint:funlen func (p *Pod) IsUsingSRIOVWithMTU() (bool, error) { const ( cncfNetworksAnnotation = "k8s.v1.cni.cncf.io/networks" @@ -469,13 +467,6 @@ func (p *Pod) IsUsingSRIOVWithMTU() (bool, error) { // whether its config's type is "sriov" oc := clientsholder.GetClientsHolder() - - // Steps: - // 1. Compare the network name with the NAD name and check if the MTU is set. - // 2. If the MTU is not set in the NAD config, we should double-check the network-status annotation. - // The network status (if the NAD name matches) could possibly have the MTU set. - // 3. If neither of the above steps is true, then check the SriovNetwork/SriovNetworkNodePolicy CRs - for _, networkName := range cncfNetworkNames { log.Debug("%s: Reviewing network-attachment definition %q", p, networkName) nad, err := oc.CNCFNetworkingClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions( @@ -484,33 +475,6 @@ func (p *Pod) IsUsingSRIOVWithMTU() (bool, error) { return false, fmt.Errorf("failed to get NetworkAttachment %s: %v", networkName, err) } - // Check the NAD config to see if the MTU is set - isSRIOVwithMTU, err := isNetworkAttachmentDefinitionSRIOVConfigMTUSet(nad.Spec.Config) - if err != nil { - log.Warn("Failed to know if network-attachment %s is using SRIOV with MTU: %v", networkName, err) - } - - log.Debug("%s: NAD config: %s", p, nad.Spec.Config) - if isSRIOVwithMTU { - return true, nil - } - // If the NAD is defined and the MTU value is not found, let's check - // the network-status annotation to see if the MTU is set there and matches - // the NAD name. - - // Get the network-status annotation (if any) - if networkStatuses, exist := p.Annotations[CniNetworksStatusKey]; exist { - log.Info("Network-status annotation found: %s", networkStatuses) - networkStatusResult, err := networkStatusUsesMTU(networkStatuses, nad.Name) - if err != nil { - log.Warn("Failed to know if network-status %s is sriov with MTU: %v", networkName, err) - } - - if networkStatusResult { - return true, nil - } - } - // If the network-status annotation is not set, let's check the SriovNetwork/SriovNetworkNodePolicy CRs // to see if the MTU is set there. log.Debug("Number of SriovNetworks: %d", len(env.AllSriovNetworks)) @@ -524,16 +488,14 @@ func (p *Pod) IsUsingSRIOVWithMTU() (bool, error) { } func sriovNetworkUsesMTU(sriovNetworks []sriovNetworkOp.SriovNetwork, sriovNetworkNodePolicies []sriovNetworkOp.SriovNetworkNodePolicy, nadName string) bool { - //nolint:gocritic - for _, sriovNetwork := range sriovNetworks { - log.Debug("Checking SriovNetwork %s", sriovNetwork.Name) - if sriovNetwork.Name == nadName { - log.Debug("SriovNetwork %s found to match the NAD name %s", sriovNetwork.Name, nadName) - //nolint:gocritic - for _, nodePolicy := range sriovNetworkNodePolicies { + for sn := range sriovNetworks { + log.Debug("Checking SriovNetwork %s", sriovNetworks[sn].Name) + if sriovNetworks[sn].Name == nadName { + log.Debug("SriovNetwork %s found to match the NAD name %s", sriovNetworks[sn].Name, nadName) + for nodePolicy := range sriovNetworkNodePolicies { log.Debug("Checking SriovNetworkNodePolicy %v", nodePolicy) - if nodePolicy.Namespace == sriovNetwork.Namespace && nodePolicy.Spec.ResourceName == sriovNetwork.Spec.ResourceName { - if nodePolicy.Spec.Mtu > 0 { + if sriovNetworkNodePolicies[nodePolicy].Namespace == sriovNetworks[sn].Namespace && sriovNetworkNodePolicies[nodePolicy].Spec.ResourceName == sriovNetworks[sn].Spec.ResourceName { + if sriovNetworkNodePolicies[nodePolicy].Spec.Mtu > 0 { return true } } diff --git a/pkg/provider/pods_test.go b/pkg/provider/pods_test.go index d754e7129..482610931 100644 --- a/pkg/provider/pods_test.go +++ b/pkg/provider/pods_test.go @@ -17,13 +17,11 @@ package provider import ( - "context" "errors" "reflect" "strings" "testing" - nadClient "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" sriovNetworkOp "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" "github.com/redhat-best-practices-for-k8s/certsuite/internal/clientsholder" "github.com/redhat-best-practices-for-k8s/certsuite/internal/log" @@ -312,145 +310,6 @@ func TestGetSRIOVNetworksNamesFromCNCFNetworks(t *testing.T) { } } -func TestIsUsingSRIOVWithMTU(t *testing.T) { - // Generate a pod with corresponding SriovNetwork and SriovNetworkNodePolicy resources - testCases := []struct { - testPod Pod - testNetworkAttachmentDefinitions []nadClient.NetworkAttachmentDefinition - expectedError error - expectedOutput bool - }{ - { // Test Case #1 - Pod annotation contains MTU field, return true - testPod: Pod{ - Pod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "k8s.v1.cni.cncf.io/networks": "net1", - }, - Name: "test-pod", - Namespace: "test-namespace", - }, - }, - }, - testNetworkAttachmentDefinitions: []nadClient.NetworkAttachmentDefinition{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "net1", - Namespace: "test-namespace", - }, - Spec: nadClient.NetworkAttachmentDefinitionSpec{ - Config: `{"cniVersion":"0.4.0","name":"vlan-100","plugins":[{"type":"sriov","master":"ext0","mtu":1500,"vlanId":100,"linkInContainer":true,"ipam":{"type":"whereabouts","ipRanges":[{"range":"1.1.1.0/24"}]}}]}`, - }, - }, - }, - expectedError: nil, - expectedOutput: true, - }, - { // Test Case #2 - Pod has network-status annotation with MTU field, return true - testPod: Pod{ - Pod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "k8s.v1.cni.cncf.io/network-status": `[{"name": "net1", "interface": "net2", "mac": "40:04:0f:f1:89:02", "mtu": 9000, "dns": {}, "device-info": {"type": "pci", "version": "1.1.0", "pci": {"pci-address": "0000:37:0b.6"}}}]`, - "k8s.v1.cni.cncf.io/networks": "net1", - }, - Name: "test-pod", - Namespace: "test-namespace", - }, - }, - }, - testNetworkAttachmentDefinitions: []nadClient.NetworkAttachmentDefinition{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "net1", - Namespace: "test-namespace", - }, - Spec: nadClient.NetworkAttachmentDefinitionSpec{ - Config: `{"cniVersion":"0.4.0","name":"sample-namespace/net1","plugins":[{"type":"sriov","master":"ext0","vlanId":100,"linkInContainer":true,"ipam":{"type":"sriov","ipRanges":[{"range":"1.1.1.0/24"}]}}]}`, - }, - }, - }, - expectedError: nil, - expectedOutput: true, - }, - { // Test Case #3 - No MTU field set in either the pod annotation or the network-status annotation, return false - testPod: Pod{ - Pod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "k8s.v1.cni.cncf.io/networks": "net1", - }, - Name: "test-pod", - Namespace: "test-namespace", - }, - }, - }, - testNetworkAttachmentDefinitions: []nadClient.NetworkAttachmentDefinition{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "net1", - Namespace: "test-namespace", - }, - - Spec: nadClient.NetworkAttachmentDefinitionSpec{ - Config: `{"cniVersion":"0.4.0","name":"sample-namespace/net1","plugins":[{"type":"notapplicable","master":"ext0","vlanId":100,"linkInContainer":true,"ipam":{"type":"notapplicable","ipRanges":[{"range":"1.1.1.0/24"}]}}]}`, - }, - }, - }, - expectedError: nil, - expectedOutput: false, - }, - { // Test Case #4 - Pod annotation "networks" name and NAD name do not match, return false and error about missing NAD - testPod: Pod{ - Pod: &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - "k8s.v1.cni.cncf.io/networks": "net2", - }, - Name: "test-pod", - Namespace: "test-namespace", - }, - }, - }, - testNetworkAttachmentDefinitions: []nadClient.NetworkAttachmentDefinition{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "net1", - Namespace: "test-namespace", - }, - Spec: nadClient.NetworkAttachmentDefinitionSpec{ - Config: `{"cniVersion":"0.4.0","name":"vlan-100","plugins":[{"type":"sriov","master":"ext0","mtu":1500,"vlanId":100,"linkInContainer":true,"ipam":{"type":"whereabouts","ipRanges":[{"range":"1.1.1.0/24"}]}}]}`, - }, - }, - }, - expectedError: errors.New("failed to get NetworkAttachment net2: network-attachment-definitions.k8s.cni.cncf.io \"net2\" not found"), - expectedOutput: false, - }, - } - - defer clientsholder.ClearTestClientsHolder() - - for _, testCase := range testCases { - // Temporarily set the clientsHolder to the test clientsHolder - oc := clientsholder.GetTestClientsHolder(nil) - - // Create the NetworkAttachmentDefinition resources - _, createErr := oc.CNCFNetworkingClient.K8sCniCncfIoV1(). - NetworkAttachmentDefinitions(testCase.testNetworkAttachmentDefinitions[0].Namespace). - Create(context.TODO(), &testCase.testNetworkAttachmentDefinitions[0], metav1.CreateOptions{}) - assert.Nil(t, createErr) - - // NOTE: - // This test case is not able to test the SriovNetworkNodePolicy check for MTU. - // This is because there is no 'fake' client for the SriovNetworkNodePolicy resource(s). - // In the future, we might be able to add testing for this but there is no way to spoof it as of right now. - - isUsingSRIOVWithMTU, err := testCase.testPod.IsUsingSRIOVWithMTU() - assert.Equal(t, testCase.expectedError, err) - assert.Equal(t, testCase.expectedOutput, isUsingSRIOVWithMTU) - } -} - func TestSriovNetworkUsesMTU(t *testing.T) { testCases := []struct { testSriovNetwork []sriovNetworkOp.SriovNetwork