Skip to content

Commit 49ae513

Browse files
kyrtapzkyrtapz
committed
Refactor ipallocator.AddOrUpdateSubnet to take a struct
Refactor the AddOrUpdateSubnet function to accept a SubnetConfig struct instead of multiple arguments. Signed-off-by: Patryk Diak <[email protected]>
1 parent ad5bb22 commit 49ae513

File tree

6 files changed

+89
-39
lines changed

6 files changed

+89
-39
lines changed

‎go-controller/pkg/allocator/ip/subnet/allocator.go‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,18 @@ import (
1616
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
1717
)
1818

19+
// SubnetConfig contains configuration parameters for adding or updating a subnet
20+
type SubnetConfig struct {
21+
Name string
22+
Subnets []*net.IPNet
23+
ReservedSubnets []*net.IPNet
24+
ExcludeSubnets []*net.IPNet
25+
}
26+
1927
// Allocator manages the allocation of IP within specific set of subnets
2028
// identified by a name. Allocator should be threadsafe.
2129
type Allocator interface {
22-
AddOrUpdateSubnet(name string, subnets []*net.IPNet, reservedSubnets []*net.IPNet, excludeSubnets ...*net.IPNet) error
30+
AddOrUpdateSubnet(config SubnetConfig) error
2331
DeleteSubnet(name string)
2432
GetSubnets(name string) ([]*net.IPNet, error)
2533
AllocateUntilFull(name string) error
@@ -94,48 +102,48 @@ func NewAllocator() *allocator {
94102
}
95103

96104
// AddOrUpdateSubnet set to the allocator for IPAM management, or update it.
97-
func (allocator *allocator) AddOrUpdateSubnet(name string, subnets []*net.IPNet, reservedSubnets []*net.IPNet, excludeSubnets ...*net.IPNet) error {
105+
func (allocator *allocator) AddOrUpdateSubnet(config SubnetConfig) error {
98106
allocator.Lock()
99107
defer allocator.Unlock()
100-
if subnetInfo, ok := allocator.cache[name]; ok && !reflect.DeepEqual(subnetInfo.subnets, subnets) {
101-
klog.Warningf("Replacing subnets %v with %v for %s", util.StringSlice(subnetInfo.subnets), util.StringSlice(subnets), name)
108+
if subnetInfo, ok := allocator.cache[config.Name]; ok && !reflect.DeepEqual(subnetInfo.subnets, config.Subnets) {
109+
klog.Warningf("Replacing subnets %v with %v for %s", util.StringSlice(subnetInfo.subnets), util.StringSlice(config.Subnets), config.Name)
102110
}
103111
var ipams []ipallocator.ContinuousAllocator
104-
for _, subnet := range subnets {
112+
for _, subnet := range config.Subnets {
105113
ipam, err := allocator.ipamFunc(subnet)
106114
if err != nil {
107-
return fmt.Errorf("failed to initialize IPAM of subnet %s for %s: %w", subnet, name, err)
115+
return fmt.Errorf("failed to initialize IPAM of subnet %s for %s: %w", subnet, config.Name, err)
108116
}
109117
ipams = append(ipams, ipam)
110118
}
111119

112120
// reservedSubnets is a subset of subnets, and it should not be used by automatic IPAM
113-
for _, excludeFromIPAM := range append(reservedSubnets, excludeSubnets...) {
121+
for _, excludeFromIPAM := range append(config.ReservedSubnets, config.ExcludeSubnets...) {
114122
var excluded bool
115-
for i, subnet := range subnets {
123+
for i, subnet := range config.Subnets {
116124
if util.ContainsCIDR(subnet, excludeFromIPAM) {
117125
err := reserveSubnets(excludeFromIPAM, ipams[i])
118126
if err != nil {
119-
return fmt.Errorf("failed to exclude subnet %s for %s: %w", excludeFromIPAM, name, err)
127+
return fmt.Errorf("failed to exclude subnet %s for %s: %w", excludeFromIPAM, config.Name, err)
120128
}
121129
}
122130
excluded = true
123131
}
124132
if !excluded {
125-
return fmt.Errorf("failed to exclude subnet %s for %s: not contained in any of the subnets", excludeFromIPAM, name)
133+
return fmt.Errorf("failed to exclude subnet %s for %s: not contained in any of the subnets", excludeFromIPAM, config.Name)
126134
}
127135
}
128136

129137
var staticIPAMs []ipallocator.StaticAllocator
130-
for _, reservedSubnet := range reservedSubnets {
138+
for _, reservedSubnet := range config.ReservedSubnets {
131139
ipam, err := allocator.reservedIPAMFunc(reservedSubnet)
132140
if err != nil {
133-
return fmt.Errorf("failed to initialize IPAM of reserved subnet %s for %s: %w", reservedSubnet, name, err)
141+
return fmt.Errorf("failed to initialize IPAM of reserved subnet %s for %s: %w", reservedSubnet, config.Name, err)
134142
}
135143
staticIPAMs = append(staticIPAMs, ipam)
136144
}
137-
allocator.cache[name] = subnetInfo{
138-
subnets: subnets,
145+
allocator.cache[config.Name] = subnetInfo{
146+
subnets: config.Subnets,
139147
ipams: ipams,
140148
staticIPAMs: staticIPAMs,
141149
}

‎go-controller/pkg/allocator/ip/subnet/allocator_test.go‎

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
3030

3131
expectedIPs := []string{"10.1.1.1", "2000::1"}
3232

33-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
33+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
34+
Name: subnetName,
35+
Subnets: ovntest.MustParseIPNets(subnets...),
36+
})
3437
gomega.Expect(err).NotTo(gomega.HaveOccurred())
3538

3639
ips, err := allocator.AllocateNextIPs(subnetName)
@@ -48,7 +51,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
4851

4952
expectedIPs := []string{"10.1.1.1", "2000::1"}
5053

51-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
54+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
55+
Name: subnetName,
56+
Subnets: ovntest.MustParseIPNets(subnets...),
57+
})
5258
gomega.Expect(err).NotTo(gomega.HaveOccurred())
5359

5460
ips, err := allocator.AllocateNextIPs(subnetName)
@@ -58,7 +64,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
5864
}
5965
subnets = []string{"10.1.2.0/24"}
6066
expectedIPs = []string{"10.1.2.1"}
61-
err = allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
67+
err = allocator.AddOrUpdateSubnet(SubnetConfig{
68+
Name: subnetName,
69+
Subnets: ovntest.MustParseIPNets(subnets...),
70+
})
6271
gomega.Expect(err).NotTo(gomega.HaveOccurred())
6372

6473
ips, err = allocator.AllocateNextIPs(subnetName)
@@ -78,7 +87,11 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
7887

7988
expectedIPs := []string{"10.1.1.8"}
8089

81-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil, ovntest.MustParseIPNets(excludes...)...)
90+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
91+
Name: subnetName,
92+
Subnets: ovntest.MustParseIPNets(subnets...),
93+
ExcludeSubnets: ovntest.MustParseIPNets(excludes...),
94+
})
8295
gomega.Expect(err).NotTo(gomega.HaveOccurred())
8396

8497
ips, err := allocator.AllocateNextIPs(subnetName)
@@ -102,7 +115,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
102115
{"10.1.1.2", "2000::2"},
103116
}
104117

105-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
118+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
119+
Name: subnetName,
120+
Subnets: ovntest.MustParseIPNets(subnets...),
121+
})
106122
gomega.Expect(err).NotTo(gomega.HaveOccurred())
107123
for _, expectedIPs := range expectedIPAllocations {
108124
ips, err := allocator.AllocateNextIPs(subnetName)
@@ -122,8 +138,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
122138
{"10.1.1.1"},
123139
{"10.1.1.2"},
124140
}
125-
126-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
141+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
142+
Name: subnetName,
143+
Subnets: ovntest.MustParseIPNets(subnets...),
144+
})
127145
gomega.Expect(err).NotTo(gomega.HaveOccurred())
128146
for _, expectedIPs := range expectedIPAllocations {
129147
ips, err := allocator.AllocateNextIPs(subnetName)
@@ -141,7 +159,11 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
141159
ginkgo.It("fails to allocate multiple IPs from the same subnet", func() {
142160
subnets := []string{"10.1.1.0/24", "2000::/64"}
143161

144-
gomega.Expect(allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)).To(gomega.Succeed())
162+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
163+
Name: subnetName,
164+
Subnets: ovntest.MustParseIPNets(subnets...),
165+
})
166+
gomega.Expect(err).NotTo(gomega.HaveOccurred())
145167

146168
ips, err := util.ParseIPNets([]string{"10.1.1.1/24", "10.1.1.2/24"})
147169
gomega.Expect(err).NotTo(gomega.HaveOccurred())
@@ -164,7 +186,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
164186
{"10.1.1.5", "10.1.2.5"},
165187
}
166188

167-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
189+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
190+
Name: subnetName,
191+
Subnets: ovntest.MustParseIPNets(subnets...),
192+
})
168193
gomega.Expect(err).NotTo(gomega.HaveOccurred())
169194
// exhaust valid ips in second subnet
170195
for _, expectedIPs := range expectedIPAllocations {
@@ -202,7 +227,10 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
202227
"2000::1/64",
203228
}
204229

205-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), nil)
230+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
231+
Name: subnetName,
232+
Subnets: ovntest.MustParseIPNets(subnets...),
233+
})
206234
gomega.Expect(err).NotTo(gomega.HaveOccurred())
207235
ips, err := allocator.AllocateNextIPs(subnetName)
208236
gomega.Expect(err).NotTo(gomega.HaveOccurred())
@@ -226,7 +254,11 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
226254
}
227255
expectedIP := "10.1.1.16/24"
228256

229-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), ovntest.MustParseIPNets(reservedSubnets...))
257+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
258+
Name: subnetName,
259+
Subnets: ovntest.MustParseIPNets(subnets...),
260+
ReservedSubnets: ovntest.MustParseIPNets(reservedSubnets...),
261+
})
230262
gomega.Expect(err).NotTo(gomega.HaveOccurred())
231263

232264
ips, err := allocator.AllocateNextIPs(subnetName)
@@ -257,7 +289,12 @@ var _ = ginkgo.Describe("Subnet IP allocator operations", func() {
257289
}
258290
expectedIP := "10.1.1.16/24"
259291

260-
err := allocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets(subnets...), ovntest.MustParseIPNets(reservedSubnets...), ovntest.MustParseIPNets(excludeSubnets...)...)
292+
err := allocator.AddOrUpdateSubnet(SubnetConfig{
293+
Name: subnetName,
294+
Subnets: ovntest.MustParseIPNets(subnets...),
295+
ExcludeSubnets: ovntest.MustParseIPNets(excludeSubnets...),
296+
ReservedSubnets: ovntest.MustParseIPNets(reservedSubnets...),
297+
})
261298
gomega.Expect(err).NotTo(gomega.HaveOccurred())
262299

263300
ips, err := allocator.AllocateNextIPs(subnetName)

‎go-controller/pkg/clustermanager/network_cluster_controller.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -791,12 +791,12 @@ func newIPAllocatorForNetwork(netInfo util.NetInfo) (subnet.Allocator, error) {
791791
excludeSubnets = append(excludeSubnets, infrastructureExcludeCIDRs(netInfo)...)
792792
}
793793

794-
if err := ipAllocator.AddOrUpdateSubnet(
795-
netInfo.GetNetworkName(),
796-
ipNets,
797-
netInfo.ReservedSubnets(),
798-
excludeSubnets...,
799-
); err != nil {
794+
if err := ipAllocator.AddOrUpdateSubnet(subnet.SubnetConfig{
795+
Name: netInfo.GetNetworkName(),
796+
Subnets: ipNets,
797+
ReservedSubnets: netInfo.ReservedSubnets(),
798+
ExcludeSubnets: excludeSubnets,
799+
}); err != nil {
800800
return nil, err
801801
}
802802

‎go-controller/pkg/clustermanager/pod/allocator_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ type ipAllocatorStub struct {
8282
fullIPPool bool
8383
}
8484

85-
func (a *ipAllocatorStub) AddOrUpdateSubnet(string, []*net.IPNet, []*net.IPNet, ...*net.IPNet) error {
85+
func (a *ipAllocatorStub) AddOrUpdateSubnet(_ subnet.SubnetConfig) error {
8686
panic("not implemented") // TODO: Implement
8787
}
8888

‎go-controller/pkg/ovn/logical_switch_manager/logical_switch_manager.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,12 @@ func (manager *LogicalSwitchManager) AddOrUpdateSwitch(switchName string, hostSu
7878
}
7979
}
8080
}
81-
return manager.allocator.AddOrUpdateSubnet(switchName, hostSubnets, reservedSubnets, excludeSubnets...)
81+
return manager.allocator.AddOrUpdateSubnet(subnet.SubnetConfig{
82+
Name: switchName,
83+
Subnets: hostSubnets,
84+
ReservedSubnets: reservedSubnets,
85+
ExcludeSubnets: excludeSubnets,
86+
})
8287
}
8388

8489
// AddNoHostSubnetSwitch adds/updates a switch without any host subnets
@@ -87,7 +92,7 @@ func (manager *LogicalSwitchManager) AddNoHostSubnetSwitch(switchName string) er
8792
// setting the hostSubnets slice argument to nil in the cache means an object
8893
// exists for the switch but it was not assigned a hostSubnet by ovn-kubernetes
8994
// this will be true for switches created on nodes that are marked as host-subnet only.
90-
return manager.allocator.AddOrUpdateSubnet(switchName, nil, nil)
95+
return manager.allocator.AddOrUpdateSubnet(subnet.SubnetConfig{Name: switchName})
9196
}
9297

9398
// Remove a switch from the the logical switch manager

‎go-controller/pkg/persistentips/allocator_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,10 @@ var _ = Describe("Persistent IP allocator operations", func() {
6767
}
6868

6969
ipAllocator := subnet.NewAllocator()
70-
Expect(ipAllocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64"), nil)).To(Succeed())
70+
Expect(ipAllocator.AddOrUpdateSubnet(subnet.SubnetConfig{Name: subnetName, Subnets: ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64")})).To(Succeed())
7171
namedAllocator = ipAllocator.ForSubnet(subnetName)
7272
ipamClaimsReconciler = NewIPAMClaimReconciler(ovnkapiclient, netInfo, nil)
73-
Expect(ipAllocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64"), nil)).To(Succeed())
73+
Expect(ipAllocator.AddOrUpdateSubnet(subnet.SubnetConfig{Name: subnetName, Subnets: ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64")})).To(Succeed())
7474
})
7575

7676
It("nothing to do when reconciling nil IPAMClaims", func() {
@@ -140,7 +140,7 @@ var _ = Describe("Persistent IP allocator operations", func() {
140140
toRuntimeObj(originalClaims)...,
141141
),
142142
}
143-
Expect(ipAllocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64"), nil)).To(Succeed())
143+
Expect(ipAllocator.AddOrUpdateSubnet(subnet.SubnetConfig{Name: subnetName, Subnets: ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64")})).To(Succeed())
144144
namedAllocator = ipAllocator.ForSubnet(subnetName)
145145
ipamClaimsReconciler = NewIPAMClaimReconciler(ovnkapiclient, netInfo, nil)
146146
})
@@ -166,7 +166,7 @@ var _ = Describe("Persistent IP allocator operations", func() {
166166
BeforeEach(func() {
167167
initialIPs = []string{"192.168.200.2/24", "fd10::1/64"}
168168
ipAllocator := subnet.NewAllocator()
169-
Expect(ipAllocator.AddOrUpdateSubnet(subnetName, ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64"), nil)).To(Succeed())
169+
Expect(ipAllocator.AddOrUpdateSubnet(subnet.SubnetConfig{Name: subnetName, Subnets: ovntest.MustParseIPNets("192.168.200.0/24", "fd10::/64")})).To(Succeed())
170170
Expect(ipAllocator.AllocateIPPerSubnet(subnetName, ovntest.MustParseIPNets(initialIPs...))).To(Succeed())
171171
namedAllocator = ipAllocator.ForSubnet(subnetName)
172172

0 commit comments

Comments
 (0)