Skip to content

Commit

Permalink
hotunplug: combine hotunplug flow with the plug
Browse files Browse the repository at this point in the history 
This commit fixes several review comments.
1. combine hotunplug flow with the plug
2. remove form ConfigState the podIfaceName cache. On each hotunplug,
configState will enter the virt-launcher net namespace.
The expesive case is when unplugging an ordinal interface, then on each
reconcile of the virt-handler the virt-launcher's net ns will be entered.
But it is an edge case that doesn't worth such a complex solution.
In a follow-up we should block/remove Absent state from such interfaces.


Signed-off-by: Alona Paz <alkaplan@redhat.com>
  • Loading branch information
@AlonaKaplan
AlonaKaplan committed Jun 14, 2023
1 parent 06d17d2 commit e444352
Show file tree
Hide file tree
Showing 10 changed files with 274 additions and 231 deletions.
1 change: 0 additions & 1 deletion pkg/network/setup/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ go_test(
"//pkg/network/sriov:go_default_library",
"//pkg/network/vmispec:go_default_library",
"//pkg/os/fs:go_default_library",
"//pkg/pointer:go_default_library",
"//pkg/virt-launcher/virtwrap/api:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/client-go/api:go_default_library",
Expand Down
78 changes: 26 additions & 52 deletions pkg/network/setup/configstate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,10 @@ package network
import (
"fmt"

"kubevirt.io/kubevirt/pkg/network/namescheme"
v1 "kubevirt.io/api/core/v1"

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

v1 "kubevirt.io/api/core/v1"

"kubevirt.io/client-go/log"

"kubevirt.io/kubevirt/pkg/network/cache"
Expand All @@ -41,18 +39,12 @@ type configStateCacheRUD interface {
}

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

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

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

// Run passes through the state machine flow, executing the following steps:
Expand Down Expand Up @@ -91,14 +83,6 @@ func (c *ConfigState) Run(nics []podNIC, preRunFunc func([]podNIC) ([]podNIC, er
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
Expand Down Expand Up @@ -136,30 +120,25 @@ func (c *ConfigState) plug(nics []podNIC, discoverFunc func(*podNIC) error, conf
return nil
}

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 {
func (c *ConfigState) Unplug(networks []v1.Network, filterFunc func([]v1.Network) ([]string, error), cleanupFunc func(string) error) error {
var nonPendingNetworks []v1.Network
var err error
if nonPendingNetworks, err = c.nonPendingNetworks(networks); err != nil {
return err
}

if len(networksToUnplug) == 0 {
if len(nonPendingNetworks) == 0 {
return nil
}
err = c.ns.Do(func() error {
networksToUnplug, doErr := filterFunc(nonPendingNetworks)
if doErr != nil {
return doErr
}

var cleanupErrors []error
for _, net := range networksToUnplug {
if cleanupErr := cleanupFunc(net, c.launcherPid); cleanupErr != nil {
if cleanupErr := cleanupFunc(net); cleanupErr != nil {
cleanupErrors = append(cleanupErrors, cleanupErr)
} else if cleanupErr := c.cache.Delete(net); cleanupErr != nil {
cleanupErrors = append(cleanupErrors, cleanupErr)
Expand All @@ -170,23 +149,18 @@ func (c *ConfigState) UnplugNetworks(specInterfaces []v1.Interface, dicoverFunc
return err
}

func (c *ConfigState) networksToUnplug(specInterfaces []v1.Interface) ([]string, error) {
var networksToUnplug []string
func (c *ConfigState) nonPendingNetworks(networks []v1.Network) ([]v1.Network, error) {
var nonPendingNetworks []v1.Network

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

state, err := c.cache.Read(specIface.Name)
if err != nil {
return nil, err
}
if state != cache.PodIfaceNetworkPreparationPending {
networksToUnplug = append(networksToUnplug, specIface.Name)
}
state, err := c.cache.Read(net.Name)
if err != nil {
return nil, err
}
if state != cache.PodIfaceNetworkPreparationPending {
nonPendingNetworks = append(nonPendingNetworks, net)
}
}
return networksToUnplug, nil
return nonPendingNetworks, nil
}
93 changes: 54 additions & 39 deletions pkg/network/setup/configstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ var _ = Describe("config state", func() {
BeforeEach(func() {
configStateCache = newConfigStateCacheStub()
ns = nsExecutorStub{}
configState = NewConfigState(&configStateCache, ns, launcherPid)
configState = NewConfigState(&configStateCache, ns)
nics = []podNIC{{
vmiSpecNetwork: &v1.Network{Name: testNet0},
}}
Expand Down Expand Up @@ -149,7 +149,7 @@ var _ = Describe("config state", func() {
It("runs and fails reading the cache", func() {
injectedErr := fmt.Errorf("fail read cache")
configStateCache.readErr = injectedErr
configState = NewConfigState(&configStateCache, ns, launcherPid)
configState = NewConfigState(&configStateCache, ns)

discover, config := &plugFuncStub{}, &plugFuncStub{}

Expand All @@ -163,7 +163,7 @@ var _ = Describe("config state", func() {
It("runs and fails writing the cache", func() {
injectedErr := fmt.Errorf("fail write cache")
configStateCache.writeErr = injectedErr
configState = NewConfigState(&configStateCache, ns, launcherPid)
configState = NewConfigState(&configStateCache, ns)

discover, config := &plugFuncStub{}, &plugFuncStub{}

Expand Down Expand Up @@ -225,69 +225,78 @@ var _ = Describe("config state", func() {
})
})

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

BeforeEach(func() {
configStateCache = newConfigStateCacheStub()
ns = nsExecutorStub{}
configState = NewConfigState(&configStateCache, ns, launcherPid)
specInterfaces = []v1.Interface{{Name: testNet0}, {Name: testNet1}, {Name: testNet2}}
configState = NewConfigState(&configStateCache, nsExecutorStub{})

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
specNetworks := []v1.Network{}
filterFunc = &filterFuncStub{[]string{}}
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
err := configState.Unplug(specNetworks, filterFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(BeEmpty(), "the unplug step shouldn't execute")
})
It("There are no networks to unplug since they are filtered out", func() {
specNetworks := []v1.Network{{Name: testNet0}, {Name: testNet1}, {Name: testNet2}}
filterFunc = &filterFuncStub{[]string{}}
unplugFunc := &unplugFuncStub{}
err := configState.Unplug(specNetworks, filterFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(BeEmpty(), "the unplug step shouldn't execute")
})
It("There is one network to unplug", func() {
specInterfaces[0].State = v1.InterfaceStateAbsent
specNetworks := []v1.Network{{Name: testNet0}}
filterFunc = &filterFuncStub{nil}

ns.shouldNotBeExecuted = false
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
err := configState.Unplug(specNetworks, filterFunc.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"

It("There is one network to unplug but it is Pending", func() {
specNetworks := []v1.Network{{Name: testNet0}}
ns.shouldNotBeExecuted = true
filterFunc = &filterFuncStub{nil}
Expect(configStateCache.Write(testNet0, cache.PodIfaceNetworkPreparationPending)).To(Succeed())
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
err := configState.Unplug(specNetworks, filterFunc.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
It("There are multiple networks to unplug but one is Pending", func() {
specNetworks := []v1.Network{{Name: testNet0}, {Name: testNet1}, {Name: testNet2}}
filterFunc = &filterFuncStub{nil}
Expect(configStateCache.Write(testNet2, cache.PodIfaceNetworkPreparationPending)).To(Succeed())
unplugFunc := &unplugFuncStub{}
err := configState.Unplug(specNetworks, filterFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(ConsistOf([]string{testNet0, testNet1}))
})
It("There are multiple networks to unplug but one is filtered out", func() {
specNetworks := []v1.Network{{Name: testNet0}, {Name: testNet1}, {Name: testNet2}}
filterFunc = &filterFuncStub{[]string{testNet0, testNet1}}
unplugFunc := &unplugFuncStub{}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
err := configState.Unplug(specNetworks, filterFunc.f, unplugFunc.f)
Expect(err).NotTo(HaveOccurred())
Expect(unplugFunc.executedNetworks).To(ConsistOf([]string{testNet0, testNet1}))
})
It("There are multiple networks to unplug and some have errors on cleanup", func() {
specInterfaces[0].State = v1.InterfaceStateAbsent
specInterfaces[1].State = v1.InterfaceStateAbsent
specInterfaces[2].State = v1.InterfaceStateAbsent

ns.shouldNotBeExecuted = false
specNetworks := []v1.Network{{Name: testNet0}, {Name: testNet1}, {Name: testNet2}}
filterFunc = &filterFuncStub{nil}
injectedErr := fmt.Errorf("fails unplug")
injectedErr2 := fmt.Errorf("fails unplug2")
unplugFunc := &unplugFuncStub{errRunForPodIfaces: map[string]error{testNet0: injectedErr, testNet2: injectedErr2}}
err := configState.UnplugNetworks(specInterfaces, discoverFunc.f, unplugFunc.f)
err := configState.Unplug(specNetworks, filterFunc.f, unplugFunc.f)
Expect(err.Error()).To(ContainSubstring(injectedErr.Error()))
Expect(err.Error()).To(ContainSubstring(injectedErr2.Error()))
Expect(unplugFunc.executedNetworks).To(ConsistOf([]string{testNet0, testNet1, testNet2}))
Expand Down Expand Up @@ -318,8 +327,7 @@ type unplugFuncStub struct {
errRunForPodIfaces map[string]error
}

func (f *unplugFuncStub) f(name string, pid int) error {
Expect(pid).To(Equal(launcherPid))
func (f *unplugFuncStub) f(name string) error {
f.executedNetworks = append(f.executedNetworks, name)
return f.errRunForPodIfaces[name]
}
Expand All @@ -328,10 +336,17 @@ func noopCSPreRun(nics []podNIC) ([]podNIC, error) {
return nics, nil
}

type discoverFuncStub struct {
podIfaceByNet map[string]string
type filterFuncStub struct {
networks []string
}

func (f *discoverFuncStub) f() (map[string]string, error) {
return f.podIfaceByNet, nil
func (f *filterFuncStub) f(networks []v1.Network) ([]string, error) {
if f.networks == nil {
netNames := []string{}
for _, network := range networks {
netNames = append(netNames, network.Name)
}
return netNames, nil
}
return f.networks, nil
}

0 comments on commit e444352

Please sign in to comment.