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.

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

Misc ip-masq-agent improvements #11317

Merged
merged 3 commits into from May 6, 2020
Merged

Misc ip-masq-agent improvements #11317

merged 3 commits into from May 6, 2020

Conversation

brb
Copy link
Member

@brb brb commented May 4, 2020

This PR adds some improvements to the BPF ip-masq-agent:

  • Allows to configure ip-masq-agent in JSON.
  • Fixes the race condition in the unit tests.
  • Use fsnotify instead of polling the config file.

Reviewable per commit.

Fix #11289.

In addition to YAML, from now on users are able to configure
the ip-masq-agent in JSON (to replicate the vanilla ip-masq-agent
behaviour).

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb added kind/bug This is a bug in the Cilium logic. kind/feature This introduces new functionality. area/daemon Impacts operation of the Cilium daemon. labels May 4, 2020
@brb brb requested a review from a team as a code owner May 4, 2020 16:28
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

3 similar comments
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 4, 2020
@brb brb added the release-note/misc This PR makes changes that have no direct user impact. label May 4, 2020
@brb
Copy link
Member Author

brb commented May 4, 2020

test-me-please

@brb brb force-pushed the pr/brb/ipmasq-improvements branch from 2e6e0f4 to 6c26f53 Compare May 4, 2020 18:14
@brb
Copy link
Member Author

brb commented May 4, 2020

test-me-please

@coveralls
Copy link

coveralls commented May 4, 2020

Coverage Status

Coverage increased (+0.004%) to 44.456% when pulling ac23b20 on pr/brb/ipmasq-improvements into 9f32546 on master.

@brb brb requested a review from a team May 5, 2020 08:43
@brb brb requested a review from a team as a code owner May 5, 2020 08:43
@brb brb requested a review from aanm May 5, 2020 08:44
@brb
Copy link
Member Author

brb commented May 5, 2020

test-me-please

pkg/ipmasq/ipmasq.go Show resolved Hide resolved
@brb brb force-pushed the pr/brb/ipmasq-improvements branch 2 times, most recently from fc94169 to a83b26c Compare May 5, 2020 09:09
@brb brb requested a review from a team as a code owner May 5, 2020 09:09
@brb
Copy link
Member Author

brb commented May 5, 2020

test-me-please

@brb brb requested a review from tklauser May 5, 2020 09:09
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

pkg/ipmasq/ipmasq_test.go Outdated Show resolved Hide resolved
pkg/ipmasq/ipmasq.go Outdated Show resolved Hide resolved
@brb brb force-pushed the pr/brb/ipmasq-improvements branch from a83b26c to 8bed017 Compare May 5, 2020 10:05
@brb
Copy link
Member Author

brb commented May 5, 2020

test-me-please

Protect the ipMasqMap by sync.RWMutex to avoid the race conditions:

=== RUN   Test
fatal error: concurrent map read and map write
goroutine 33 [running]:
runtime.throw(0xc67924, 0x21)
	/home/travis/.gimme/versions/go1.14.2.linux.amd64/src/runtime/panic.go:1116 +0x72 fp=0xc000574bb0 sp=0xc000574b80 pc=0x4370d2
runtime.mapaccess2_faststr(0xb78a00, 0xc00009e810, 0xc00003a320, 0xa, 0xc000574fb8, 0x0)
	/home/travis/.gimme/versions/go1.14.2.linux.amd64/src/runtime/map_faststr.go:116 +0x47c fp=0xc000574c20 sp=0xc000574bb0 pc=0x4155bc
github.com/cilium/cilium/pkg/ipmasq.(*ipMasqMapMock).Update(0xc0001241c0, 0xc00003a2bc, 0x4, 0x4, 0xc00003a2b8, 0x4, 0x4, 0x0, 0x0)
	/home/travis/gopath/src/github.com/cilium/cilium/pkg/ipmasq/ipmasq_test.go:42 +0x6a fp=0xc000574c88 sp=0xc000574c20 pc=0xabc63a
github.com/cilium/cilium/pkg/ipmasq.(*IPMasqAgent).Update(0xc00009e870, 0x22cf4b36, 0xc630b7c75d)

Reported-by: Andre Martins <andre@cilium.io>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
pkg/ipmasq/ipmasq.go Outdated Show resolved Hide resolved
@brb brb requested a review from aanm May 5, 2020 13:28
pkg/ipmasq/ipmasq.go Outdated Show resolved Hide resolved
@brb brb requested a review from aanm May 5, 2020 13:39
@brb brb force-pushed the pr/brb/ipmasq-improvements branch from 8bed017 to ac23b20 Compare May 5, 2020 13:45
@brb
Copy link
Member Author

brb commented May 5, 2020

test-me-please

Instead of periodically reading the ip-masq-agent config file, subscribe
to inotify events which are fired when the config is updated.

This allows us to save a couple of CPU cycles, and also to get rid of
the sync period flag.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb requested a review from joestringer May 5, 2020 18:11
@rolinh rolinh merged commit acaa1d1 into master May 6, 2020
1.8.0 automation moved this from In progress to Merged May 6, 2020
@rolinh rolinh deleted the pr/brb/ipmasq-improvements branch May 6, 2020 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. kind/bug This is a bug in the Cilium logic. kind/feature This introduces new functionality. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

CI: concurrent map read and map write in ipmasq tests
6 participants