Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IPAM related bugfixes #10587

Merged
merged 5 commits into from Mar 17, 2020
Merged

IPAM related bugfixes #10587

merged 5 commits into from Mar 17, 2020

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Mar 16, 2020 •
edited

A series of bugfixes discovered while refactoring the IPAM code. No backporting is required. While the data races in the mock code also affect 1.7 and 1.6 branches, it is relatively harmless as we have not seen any unit test flakes so far.

This change is Reviewable

@tgraf tgraf added kind/bug This is a bug in the Cilium logic. pending-review release-note/bug This PR fixes an issue in a previous release of Cilium. labels Mar 16, 2020
@tgraf tgraf requested a review from a team as a code owner March 16, 2020 11:51
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 16, 2020
@tgraf tgraf mentioned this pull request Mar 16, 2020
Merged
@coveralls
Copy link

coveralls commented Mar 16, 2020 •
edited

Coverage Status

Coverage increased (+0.1%) to 45.674% when pulling 6dbaf74 on pr/tgraf/ipam-panic-fix into beb401b on master.

@tgraf
Copy link
Member Author

tgraf commented Mar 16, 2020 •
edited

test-me-please

Hit #10565 (comment), other tests passed

@tgraf
ipam: Fix SIGSEGV when accessing n.resource
f9ca6a8 
It is possible for the IP deficit resolution to be triggered before the
initial CiliumNode resource is asigned to the node. n.resource can be
nil in this scenario.

Fixes: #10582

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
eni: Fix data race when updating limits
a478808 
Fixes:
```
WARNING: DATA RACE
Write at 0x00c0005b0750 by goroutine 308:
  runtime.mapassign_faststr()
      /usr/local/go/src/runtime/map_faststr.go:202 +0x0
  github.com/cilium/cilium/pkg/aws/eni.UpdateLimitsFromUserDefinedMappings()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/limits.go:269 +0xdf
  github.com/cilium/cilium/pkg/aws/eni.(*ENISuite).TestUpdateLimitsFromUserDefinedMappings()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/limits_test.go:47 +0x11d
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:539 +0x3a
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:321 +0xd3
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/vendor/gopkg.in/check.v1/check.go:781 +0xa0a
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/vendor/gopkg.in/check.v1/check.go:675 +0xd9
```

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ipam: Add mising DeepCopy code
e694298 
Fixes: da04e6e ("ipam: Use cidr.CIDR in types.Subnet")
Fixes: d7b84d2 ("Move PoolID and PoolQuota to pkg/ipam/types")

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
aws: Add DeepCopy() to SecurityGroup type
9e04026 
Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
ec2/mock: Fix data races
6dbaf74 
Data updates are passed in as pointers, deep copies must be made for
data passed in and for data passed out.

Fixes:
```
WARNING: DATA RACE
Write at 0x00c000276970 by goroutine 31:
  github.com/cilium/cilium/pkg/aws/ec2/mock.(*API).AssignPrivateIpAddresses()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/ec2/mock/mock.go:296 +0x4f7
  github.com/cilium/cilium/pkg/aws/eni.(*Node).AllocateIPs()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/node.go:253 +0x102
  github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:502 +0xa0d
  github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:532 +0x90
  github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:249 +0x8b
  github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9
```

```
WARNING: DATA RACE
Read at 0x0000048bffa8 by goroutine 25:
  github.com/cilium/cilium/pkg/aws/ec2/mock.(*API).CreateNetworkInterface()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/ec2/mock/mock.go:199 +0x5fd
  github.com/cilium/cilium/pkg/aws/eni.(*Node).CreateInterface()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/node.go:387 +0x927
  github.com/cilium/cilium/pkg/ipam.(*Node).createInterface()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:330 +0x2d4
  github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:516 +0x85d
  github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:532 +0x90
  github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:249 +0x8b
  github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9
```

```
WARNING: DATA RACE
Read at 0x00c0006aa9f0 by goroutine 30:
  github.com/cilium/cilium/pkg/aws/eni/types.(*ENI).DeepCopyInto()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/types/zz_generated.deepcopy.go:60 +0x65
  github.com/cilium/cilium/pkg/aws/eni/types.(*ENI).DeepCopy()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/types/zz_generated.deepcopy.go:82 +0x476
  github.com/cilium/cilium/pkg/aws/ec2/mock.(*API).GetInstances()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/ec2/mock/mock.go:382 +0x1b8
  github.com/cilium/cilium/pkg/aws/eni.(*InstancesManager).Resync()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/instances.go:171 +0x851
  github.com/cilium/cilium/pkg/aws/eni.(*ENISuite).TestNodeManagerManyNodes()
      /home/vagrant/go/src/github.com/cilium/cilium/pkg/aws/eni/node_manager_test.go:570 +0xaf4
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:539 +0x3a
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:321 +0xd3
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/vendor/gopkg.in/check.v1/check.go:781 +0xa0a
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /home/vagrant/go/src/github.com/cilium/cilium/vendor/gopkg.in/check.v1/check.go:675 +0xd9
```

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
Copy link
Member Author

tgraf commented Mar 16, 2020

test-me-please

@aanm aanm merged commit 4829aa7 into master Mar 17, 2020
1.8.0 automation moved this from In progress to Merged Mar 17, 2020
@aanm aanm deleted the pr/tgraf/ipam-panic-fix branch March 17, 2020 10:42
@christarazi christarazi mentioned this pull request May 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tgraf @coveralls @aanm