Use feature probes to detect kernel support for sockops #10941
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
pchaigno
added
area/sockops
release-note/misc
pchaigno
requested changes
Apr 11, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
d29d73d
to
d5f3543
Compare
pchaigno
reviewed
Apr 14, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
d5f3543
to
175f622
Compare
aanm
requested changes
Apr 15, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
175f622
to
18085c1
Compare
|
test-me-please |
qmonnet
requested changes
Apr 15, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
18085c1
to
cea62ad
Compare
aanm
requested changes
Apr 16, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
2 times, most recently
from
4d16f3b
to
165d149
Compare
aanm
requested changes
Apr 20, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
165d149
to
1c0d98c
Compare
aanm
approved these changes
Apr 21, 2020
pchaigno
reviewed
Apr 23, 2020
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
1c0d98c
to
315ab0a
Compare
|
@soumynathan it seems a real failure: |
qmonnet
reviewed
Apr 24, 2020
daemon/cmd/daemon_main.go
Outdated
| if option.Config.SockopsEnable { | ||
| if h := probes.NewProbeManager().GetHelpers("sk_msg"); h != nil { | ||
| if _, ok := h["bpf_msg_redirect_hash"]; ok { | ||
| return nil |
There was a problem hiding this comment.
The return nil here might be the cause for the build error. Simply return should work?
Note: Instead of having all the code of that function if the if option.Config.SockopsEnable block, you could also return early if the option is not set and avoid indenting all the rest:
if !option.Config.SockopsEnable {
return
}
if h := probes.NewProbeManager()...It usually makes the code a bit easier to read, I find.
There was a problem hiding this comment.
@qmonnet The probe still needs to be fixed anyway ;-)
There was a problem hiding this comment.
ok will fix it. @pchaigno is the probe fix that you mentioned above related to this patch or we need to handle it in a different PR>
There was a problem hiding this comment.
Related to this patch. Please see #10941 (comment) and #10941 (comment).
This patch probes the kernel for bpf sockops support and validates the user configuration for SockopsEnable. Fixes: cilium#10931 Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>
soumynathan
force-pushed
the
detect-kernel-support-for-sockops
branch
from
315ab0a
to
57c625b
Compare
|
test-me-please |
qmonnet
approved these changes
Apr 28, 2020
Comment on lines
+1460
to
+1461
| k := probes.NewProbeManager().GetHelpers("sock_ops") | ||
| h := probes.NewProbeManager().GetHelpers("sk_msg") |
There was a problem hiding this comment.
Suggested change
| k := probes.NewProbeManager().GetHelpers("sock_ops") | |
| h := probes.NewProbeManager().GetHelpers("sk_msg") | |
| pm := probes.NewProbeManager() | |
| k := pm.GetHelpers("sock_ops") | |
| h := pm.GetHelpers("sk_msg") |
Not an issue because probe results are cached, but suggestion for next time: don't create more probe managers than necessary :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch probes the kernel for bpf sockops support and validates
the user configuration for SockopsEnable.
Fixes: #10931
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com