Skip to content

Commit

Permalink
remove NAD config processing; only look at SriovNetwork objects
Browse files Browse the repository at this point in the history
  • Loading branch information
@sebrandon1
sebrandon1 committed Nov 14, 2024
1 parent 1e0b834 commit 925187c
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 186 deletions.
6 changes: 6 additions & 0 deletions .github/workflows/pre-main.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}"

Expand Down Expand Up @@ -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 \
Expand Down
52 changes: 7 additions & 45 deletions pkg/provider/pods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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(
Expand All @@ -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))
Expand All @@ -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
}
}
Expand Down
141 changes: 0 additions & 141 deletions pkg/provider/pods_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 925187c

Please sign in to comment.