Skip to content

Prepare for GCP IPAM#10691

Merged
borkmann merged 9 commits intomasterfrom
pr/tgraf/gcp-ipam-prep
Mar 26, 2020
Merged

Prepare for GCP IPAM#10691
borkmann merged 9 commits intomasterfrom
pr/tgraf/gcp-ipam-prep

Conversation

@tgraf
Copy link
Copy Markdown
Contributor

@tgraf tgraf commented Mar 24, 2020

No description provided.

@tgraf tgraf added wip release-note/misc This PR makes changes that have no direct user impact. labels Mar 24, 2020
@tgraf tgraf requested review from a team as code owners March 24, 2020 23:28
@tgraf tgraf force-pushed the pr/tgraf/gcp-ipam-prep branch 2 times, most recently from 90a5883 to 4b760a4 Compare March 25, 2020 08:34
errordeveloper
errordeveloper reviewed Mar 25, 2020
View reviewed changes
Comment thread operator/main.go Outdated
errordeveloper
errordeveloper reviewed Mar 25, 2020
View reviewed changes
Comment thread operator/azure.go Outdated
tgraf added 8 commits March 25, 2020 14:44
@tgraf
ipam: Move iterator logic into generic InstanceMap
5ed4607 
Move code to iterate over interfaces and addresses into the InstanceMap.
This allows to defer deep copying until it is really necessary.

This also allows to move the logic to reserve IPs of all interfaces of
all instances into the generic GroupPoolAllocator for reuse in future
IPAM code.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam: Fix data race in node manager test
99a0a79 
Fixes:
```
WARNING: DATA RACE
Read at 0x00c0001b48c0 by goroutine 37:
  github.com/cilium/cilium/pkg/ipam.(*nodeOperationsMock).PrepareIPAllocation()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager_test.go:103 +0x1c5
  github.com/cilium/cilium/pkg/ipam.(*Node).determineMaintenanceAction()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:492 +0x184
  github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:528 +0x53
  github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:613 +0x69
  github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:205 +0x8b
  github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9

Previous write at 0x00c0001b48c0 by goroutine 35:
  github.com/cilium/cilium/pkg/ipam.(*nodeOperationsMock).AllocateIPs()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager_test.go:111 +0x117
  github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:564 +0xa0d
  github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:613 +0x69
  github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:205 +0x8b
  github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9
```

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam: Simplify locking in IPAM node interactions
71dd2c3 
Don't hold any locks when calling into IPAM specific operations. Use
node.mutex exclusively to protect data access and keep locking periods
as short as possible.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam: Move instance ID retrieval into generic code
9899eb6 
This removes the need to track the CiliumNode resource in the Azure
specific code entirely.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam: Don't recalculate if resource is not attached yet
74fd199 
There is no point in attempting recalculation of required addresses
unless the resource has been attached.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam/allocator: Allow allocating IPs without knowing pool ID
9b34dd9 
Ideally, the pool ID is known. If not known, the list of pools can be
searched to identify the correct pool.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
operator: Remove global nodeManager variable
9defaaa 
Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam: Add InstanceMap.GetInterface
71f1000 
Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/gcp-ipam-prep branch from 4b760a4 to 71f1000 Compare March 25, 2020 13:44
@tgraf
operator: Move initial sync and background sync in generic code
a1cdb3f 
Signed-off-by: Thomas Graf <thomas@cilium.io>
@coveralls
Copy link
Copy Markdown

coveralls commented Mar 25, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.08%) to 45.511% when pulling a1cdb3f on pr/tgraf/gcp-ipam-prep into 8fd7415 on master.

@tgraf
Copy link
Copy Markdown
Contributor Author

tgraf commented Mar 25, 2020 •
edited
Loading

test-me-please

00:49:18.049    Checks CIDR L3 Policy [It]
00:49:18.049    /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:430
00:49:18.049  
00:49:18.049    Unable to import policy: cannot render the policy:  write policy_fe935aaf.json: no space left on device
[2020-03-25T15:17:25.119Z]   Expected
[2020-03-25T15:17:25.119Z]       <*errors.errorString | 0xc0004323f0>: {
[2020-03-25T15:17:25.119Z]           s: "cannot render the policy:  write policy_fe935aaf.json: no space left on device",
[2020-03-25T15:17:25.119Z]       }
[2020-03-25T15:17:25.119Z]   to be nil

@tgraf
Copy link
Copy Markdown
Contributor Author

tgraf commented Mar 25, 2020

test-me-please

@tgraf tgraf added pending-review and removed wip labels Mar 26, 2020
@borkmann borkmann merged commit 63055b7 into master Mar 26, 2020
@borkmann borkmann deleted the pr/tgraf/gcp-ipam-prep branch March 26, 2020 07:35
@christarazi christarazi mentioned this pull request Jun 5, 2020
Merged
HadrienPatte added a commit that referenced this pull request Apr 14, 2026
@HadrienPatte
ipam: Remove unused InterfaceRevision wrapper struct
160de1f 
While going through the `pkg/ipam` package, I noticed a typo in a
comment that drew my attention to a specific struct definition,
`InterfaceRevision`. While reading about its `Fingerprint` field, I got
curious about how and where that field was used. After a deeper
investigation, I discovered that not only is this `Fingerprint` field
currently never set or read, but it has never been ever since its
introduction 6 years ago (#10691).

At that point I figured that there is no short or long term plans to start
using that field so it can be removed, but since `InterfaceRevision`
only had two fields, removing one of them makes it an unnecessary
wrapper struct.

So this PR removes the `InterfaceRevision` struct that used to have an
`Resource` (`Interface`) and a `Fingerprint` (`string`) fields and replace
it with that `Interface`. This eliminates the `.Resource` indirection at
every call site.

Also since the whole struct definition is now gone, the typo that started this
whole thing is gone as well.

Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
@HadrienPatte HadrienPatte mentioned this pull request Apr 14, 2026
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Apr 16, 2026
@HadrienPatte
ipam: Remove unused InterfaceRevision wrapper struct
424512b 
While going through the `pkg/ipam` package, I noticed a typo in a
comment that drew my attention to a specific struct definition,
`InterfaceRevision`. While reading about its `Fingerprint` field, I got
curious about how and where that field was used. After a deeper
investigation, I discovered that not only is this `Fingerprint` field
currently never set or read, but it has never been ever since its
introduction 6 years ago (#10691).

At that point I figured that there is no short or long term plans to start
using that field so it can be removed, but since `InterfaceRevision`
only had two fields, removing one of them makes it an unnecessary
wrapper struct.

So this PR removes the `InterfaceRevision` struct that used to have an
`Resource` (`Interface`) and a `Fingerprint` (`string`) fields and replace
it with that `Interface`. This eliminates the `.Resource` indirection at
every call site.

Also since the whole struct definition is now gone, the typo that started this
whole thing is gone as well.

Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@errordeveloper errordeveloper errordeveloper approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tgraf @coveralls @errordeveloper @borkmann