Skip to content

Commit

Permalink
hotunplug, virt-handler: don't unplug nics with ordinal name scheme
Browse files Browse the repository at this point in the history
Signed-off-by: Alona Paz <[email protected]>
  • Loading branch information
AlonaKaplan committed Jun 15, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent f1fe544 commit c249c1e
Showing 8 changed files with 142 additions and 69 deletions.
1 change: 0 additions & 1 deletion pkg/network/setup/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -70,6 +70,5 @@ go_test(
"//vendor/github.com/onsi/gomega:go_default_library",
"//vendor/github.com/vishvananda/netlink:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/utils/pointer:go_default_library",
],
)
44 changes: 37 additions & 7 deletions pkg/network/setup/configstate.go
Original file line number Diff line number Diff line change
@@ -22,6 +22,8 @@ package network
import (
"fmt"

"kubevirt.io/kubevirt/pkg/network/namescheme"

"k8s.io/apimachinery/pkg/util/errors"

v1 "kubevirt.io/api/core/v1"
@@ -39,13 +41,18 @@ type configStateCacheRUD interface {
}

type ConfigState struct {
cache configStateCacheRUD
ns NSExecutor
launcherPid int
cache configStateCacheRUD
ns NSExecutor
launcherPid int
podIfaceNameByNetwork map[string]string
}

func NewConfigState(configStateCache configStateCacheRUD, ns NSExecutor, launcherPid int) ConfigState {
return ConfigState{cache: configStateCache, ns: ns, launcherPid: launcherPid}
return NewConfigStateWithPodIfaceMap(configStateCache, ns, launcherPid, nil)
}

func NewConfigStateWithPodIfaceMap(configStateCache configStateCacheRUD, ns NSExecutor, launcherPid int, podIfaceNameByNetwork map[string]string) ConfigState {
return ConfigState{cache: configStateCache, ns: ns, launcherPid: launcherPid, podIfaceNameByNetwork: podIfaceNameByNetwork}
}

// Run passes through the state machine flow, executing the following steps:
@@ -55,7 +62,7 @@ func NewConfigState(configStateCache configStateCacheRUD, ns NSExecutor, launche
//
// The discovery step can be executed repeatedly with no limitation.
// The configuration step is allowed to run only once. Any attempt to run it again will cause a critical error.
func (c ConfigState) Run(nics []podNIC, preRunFunc func([]podNIC) ([]podNIC, error), discoverFunc func(*podNIC) error, configFunc func(*podNIC) error) error {
func (c *ConfigState) Run(nics []podNIC, preRunFunc func([]podNIC) ([]podNIC, error), discoverFunc func(*podNIC) error, configFunc func(*podNIC) error) error {
var pendingNICs []podNIC
for _, nic := range nics {
state, err := c.cache.Read(nic.vmiSpecNetwork.Name)
@@ -84,12 +91,20 @@ func (c ConfigState) Run(nics []podNIC, preRunFunc func([]podNIC) ([]podNIC, err
if preErr != nil {
return preErr
}
if c.podIfaceNameByNetwork == nil {
c.podIfaceNameByNetwork = map[string]string{}
}
for _, nic := range nics {
if _, exist := c.podIfaceNameByNetwork[nic.vmiSpecNetwork.Name]; !exist {
c.podIfaceNameByNetwork[nic.vmiSpecNetwork.Name] = nic.podInterfaceName
}
}
return c.plug(nics, discoverFunc, configFunc)
})
return err
}

func (c ConfigState) plug(nics []podNIC, discoverFunc func(*podNIC) error, configFunc func(*podNIC) error) error {
func (c *ConfigState) plug(nics []podNIC, discoverFunc func(*podNIC) error, configFunc func(*podNIC) error) error {
for i := range nics {
if ferr := discoverFunc(&nics[i]); ferr != nil {
return ferr
@@ -121,7 +136,18 @@ func (c ConfigState) plug(nics []podNIC, discoverFunc func(*podNIC) error, confi
return nil
}

func (c *ConfigState) UnplugNetworks(specInterfaces []v1.Interface, cleanupFunc func(string, int) error) error {
func (c *ConfigState) UnplugNetworks(specInterfaces []v1.Interface, dicoverFunc func() (map[string]string, error), cleanupFunc func(string, int) error) error {
if c.podIfaceNameByNetwork == nil {
err := c.ns.Do(func() error {
var dErr error
c.podIfaceNameByNetwork, dErr = dicoverFunc()
return dErr
})
if err != nil {
return err
}
}

networksToUnplug, err := c.networksToUnplug(specInterfaces)
if err != nil {
return err
@@ -149,6 +175,10 @@ func (c *ConfigState) networksToUnplug(specInterfaces []v1.Interface) ([]string,

for _, specIface := range specInterfaces {
if specIface.State == v1.InterfaceStateAbsent {
if podIfaceName, exist := c.podIfaceNameByNetwork[specIface.Name]; exist && namescheme.OrdinalSecondaryInterfaceName(podIfaceName) {
continue
}

state, err := c.cache.Read(specIface.Name)
if err != nil {
return nil, err
39 changes: 31 additions & 8 deletions pkg/network/setup/configstate_test.go
Original file line number Diff line number Diff line change
@@ -226,7 +226,10 @@ var _ = Describe("config state", func() {
})

Context("UnplugNetworks", func() {
var specInterfaces []v1.Interface
var (
specInterfaces []v1.Interface
discoverFunc *discoverFuncStub
)

BeforeEach(func() {
configStateCache = newConfigStateCacheStub()
@@ -236,11 +239,13 @@ var _ = Describe("config state", func() {
Expect(configStateCache.Write(testNet0, cache.PodIfaceNetworkPreparationFinished)).To(Succeed())
Expect(configStateCache.Write(testNet1, cache.PodIfaceNetworkPreparationStarted)).To(Succeed())
Expect(configStateCache.Write(testNet2, cache.PodIfaceNetworkPreparationFinished)).To(Succeed())

discoverFunc = &discoverFuncStub{map[string]string{testNet0: "pod1", testNet1: "pod2", testNet2: "pod3"}}
})
It("There are no networks to unplug", func() {
ns.shouldNotBeExecuted = true
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, unplugFunc.f)
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(BeEmpty(), "the unplug step shouldn't execute")
})
@@ -249,17 +254,27 @@ var _ = Describe("config state", func() {

ns.shouldNotBeExecuted = false
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, unplugFunc.f)
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(ConsistOf([]string{testNet0}))
})
It("There is one network to unplug but is has ordinal name", func() {
specInterfaces[0].State = v1.InterfaceStateAbsent
discoverFunc.podIfaceByNet[testNet0] = "net1"

ns.shouldNotBeExecuted = true
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(BeEmpty(), "the unplug step shouldn't execute")
})
It("There are multiple networks to unplug", func() {
specInterfaces[0].State = v1.InterfaceStateAbsent
specInterfaces[1].State = v1.InterfaceStateAbsent

ns.shouldNotBeExecuted = false
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, unplugFunc.f)
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(ConsistOf([]string{testNet0, testNet1}))
})
@@ -270,11 +285,11 @@ var _ = Describe("config state", func() {

ns.shouldNotBeExecuted = false
injectedErr := fmt.Errorf("fails unplug")
injectedErr3 := fmt.Errorf("fails unplug3")
unplugFunc := &unplugFuncStub{errRunForPodIfaces: map[string]error{testNet0: injectedErr, testNet2: injectedErr3}}
err := configState.UnplugNetworks(specInterfaces, unplugFunc.f)
injectedErr2 := fmt.Errorf("fails unplug2")
unplugFunc := &unplugFuncStub{errRunForPodIfaces: map[string]error{testNet0: injectedErr, testNet2: injectedErr2}}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
Expect(err.Error()).To(ContainSubstring(injectedErr.Error()))
Expect(err.Error()).To(ContainSubstring(injectedErr3.Error()))
Expect(err.Error()).To(ContainSubstring(injectedErr2.Error()))
Expect(unplugFunc.executedNetworks).To(ConsistOf([]string{testNet0, testNet1, testNet2}))
})
})
@@ -312,3 +327,11 @@ func (f *unplugFuncStub) f(name string, pid int) error {
func noopCSPreRun(nics []podNIC) ([]podNIC, error) {
return nics, nil
}

type discoverFuncStub struct {
podIfaceByNet map[string]string
}

func (f *discoverFuncStub) f() (map[string]string, error) {
return f.podIfaceByNet, nil
}
16 changes: 9 additions & 7 deletions pkg/network/setup/netconf.go
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ type cacheCreator interface {
type NetConf struct {
cacheCreator cacheCreator
nsFactory nsFactory
configState map[string]ConfigState
configState map[string]*ConfigState
configStateMutex *sync.RWMutex
}

@@ -52,14 +52,14 @@ type NSExecutor interface {

func NewNetConf() *NetConf {
var cacheFactory cache.CacheCreator
return NewNetConfWithCustomFactory(func(pid int) NSExecutor {
return NewNetConfWithCustomFactoryAndConfigState(func(pid int) NSExecutor {
return netns.New(pid)
}, cacheFactory)
}, cacheFactory, map[string]*ConfigState{})
}

func NewNetConfWithCustomFactory(nsFactory nsFactory, cacheCreator cacheCreator) *NetConf {
func NewNetConfWithCustomFactoryAndConfigState(nsFactory nsFactory, cacheCreator cacheCreator, configState map[string]*ConfigState) *NetConf {
return &NetConf{
configState: map[string]ConfigState{},
configState: configState,
configStateMutex: &sync.RWMutex{},
cacheCreator: cacheCreator,
nsFactory: nsFactory,
@@ -84,13 +84,15 @@ func (c *NetConf) Setup(vmi *v1.VirtualMachineInstance, networks []v1.Network, l
return err
}
ns := c.nsFactory(launcherPid)
configState = NewConfigState(configStateCache, ns, launcherPid)
newConfigState := NewConfigState(configStateCache, ns, launcherPid)
configState = &newConfigState
c.configStateMutex.Lock()
c.configState[string(vmi.UID)] = configState
c.configStateMutex.Unlock()
}

err := netConfigurator.SetupPodNetworkPhase1(launcherPid, networks, configState)

if err != nil {
return fmt.Errorf("setup failed, err: %w", err)
}
@@ -143,5 +145,5 @@ func (c *NetConf) HotUnplugInterfaces(vmi *v1.VirtualMachineInstance) error {
return nil
}
netConfigurator := NewVMNetworkConfigurator(vmi, c.cacheCreator)
return netConfigurator.UnplugPodNetworksPhase1(vmi, &configState)
return netConfigurator.UnplugPodNetworksPhase1(vmi, configState)
}
61 changes: 30 additions & 31 deletions pkg/network/setup/netconf_test.go
Original file line number Diff line number Diff line change
@@ -27,8 +27,6 @@ import (

dutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"

"k8s.io/utils/pointer"

kfs "kubevirt.io/kubevirt/pkg/os/fs"

. "github.com/onsi/ginkgo/v2"
@@ -48,8 +46,8 @@ var _ = Describe("netconf", func() {
const launcherPid = 0

BeforeEach(func() {
netConf = netsetup.NewNetConfWithCustomFactory(nsNoopFactory, &tempCacheCreator{})
vmi = &v1.VirtualMachineInstance{ObjectMeta: metav1.ObjectMeta{UID: "123"}}
netConf = netsetup.NewNetConfWithCustomFactoryAndConfigState(nsNoopFactory, &tempCacheCreator{}, map[string]*netsetup.ConfigState{})
vmi = &v1.VirtualMachineInstance{ObjectMeta: metav1.ObjectMeta{UID: "123", Name: "vmi1"}}
})

It("runs setup successfully", func() {
@@ -61,7 +59,7 @@ var _ = Describe("netconf", func() {
})

It("fails the setup run", func() {
netConf := netsetup.NewNetConfWithCustomFactory(nsFailureFactory, &tempCacheCreator{})
netConf := netsetup.NewNetConfWithCustomFactoryAndConfigState(nsFailureFactory, &tempCacheCreator{}, map[string]*netsetup.ConfigState{})
vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{{
Name: "default",
}}
@@ -73,16 +71,22 @@ var _ = Describe("netconf", func() {
})

It("fails the teardown run", func() {
netConf := netsetup.NewNetConfWithCustomFactory(nil, failingCacheCreator{})
netConf := netsetup.NewNetConfWithCustomFactoryAndConfigState(nil, failingCacheCreator{}, map[string]*netsetup.ConfigState{})
Expect(netConf.Teardown(vmi)).NotTo(Succeed())
})

Context("HotUnplugInterfaces", func() {
var shouldFail *bool
var wasExecuted *bool

const netName = "multusNet"
const nadName = "blue"
const (
netName = "multusNet"
nadName = "blue"
)

var (
nsStub netnsStub
configMap map[string]*netsetup.ConfigState
cacheCreator tempCacheCreator
configCache netsetup.ConfigStateCache
)

BeforeEach(func() {
dutils.MockDefaultOwnershipManager()
@@ -98,51 +102,46 @@ var _ = Describe("netconf", func() {
}
vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{iface}

cacheCreator := tempCacheCreator{}
shouldFail = pointer.Bool(false)
wasExecuted = pointer.Bool(false)
nsStub := netnsStub{shouldFail, wasExecuted}
factory := func(_ int) netsetup.NSExecutor { return nsStub }
netConf = netsetup.NewNetConfWithCustomFactory(factory, &cacheCreator)
cacheCreator = tempCacheCreator{}
nsStub = netnsStub{}
configMap = map[string]*netsetup.ConfigState{}

podIfaceCacheData := &cache.PodIfaceCacheData{State: cache.PodIfaceNetworkPreparationFinished}
Expect(cache.WritePodInterfaceCache(&cacheCreator, string(vmi.UID), netName, podIfaceCacheData)).To(Succeed())
_ = netConf.Setup(vmi, vmi.Spec.Networks, launcherPid, netPreSetupDummyNoop)
vmi.Spec.Domain.Devices.Interfaces[0].State = v1.InterfaceStateAbsent

configCache = netsetup.NewConfigStateCache(string(vmi.UID), &cacheCreator)
netConf = netsetup.NewNetConfWithCustomFactoryAndConfigState(nil, &cacheCreator, configMap)
})

It("runs successfully", func() {
configState := netsetup.NewConfigStateWithPodIfaceMap(&configCache, nsStub, launcherPid, map[string]string{netName: "pod1"})
configMap[string(vmi.UID)] = &configState

Expect(netConf.HotUnplugInterfaces(vmi)).To(Succeed())
Expect(*wasExecuted).To(BeTrue())
})
It("fails the unplug", func() {
*shouldFail = true
nsStub.shouldFail = true
configState := netsetup.NewConfigStateWithPodIfaceMap(&configCache, nsStub, launcherPid, map[string]string{netName: "pod1"})
configMap[string(vmi.UID)] = &configState

Expect(netConf.HotUnplugInterfaces(vmi)).NotTo(Succeed())
Expect(*wasExecuted).To(BeTrue())
})
})
})

type netnsStub struct {
shouldFail *bool
executed *bool
shouldFail bool
}

func (n netnsStub) Do(func() error) error {
if n.executed != nil {
if *n.executed == true {
panic("executed should be false before execution")
}
*n.executed = true
}
if n.shouldFail != nil && *n.shouldFail == true {
if n.shouldFail == true {
return fmt.Errorf("do-netns failure")
}
return nil
}
func nsNoopFactory(_ int) netsetup.NSExecutor { return netnsStub{} }
func nsFailureFactory(_ int) netsetup.NSExecutor { return netnsStub{shouldFail: pointer.Bool(true)} }
func nsFailureFactory(_ int) netsetup.NSExecutor { return netnsStub{shouldFail: true} }

func netPreSetupDummyNoop() error { return nil }

Loading

0 comments on commit c249c1e

Please sign in to comment.