-
Notifications
You must be signed in to change notification settings - Fork 456
Windows: Build tetragon on Windows (Part -2) #3488
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
Windows: Build tetragon on Windows (Part -2) #3488
Conversation
cafb9c1
to
5cf27cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please find some comments bellow.
Generally, let's try to avoid code duplication as much as possible. If it's not possible, then let's at least document why.
f905fc6
to
0cc7f7c
Compare
0cc7f7c
to
dcf79cc
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
dcf79cc
to
b18d1e0
Compare
) | ||
|
||
// isList checks if a value specifies a list, and if so it returns it (or nil if list does not exist) | ||
func isList(val string, lists []v1alpha1.ListSpec) (bool, *v1alpha1.ListSpec) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? I feel that it can be either removed or use a common version for both windows/linux
|
||
func createLoaderEvents() error { | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we return an error (e.g., not supported) here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I misclicked and some text was not submitted in the review.
Adding it here.
It seems to me that there is a lot of code duplication between the linux and the windows version. For example, looking at load_windows.go
and load_linux.go
, there are 15 and 17 functions, respectively. Out of those only 3 seem to have differences. Can we just add all the common code in a load.go
file?
$ git grep ^func pkg/sensors/load_windows.go | wc -l
15
$ git grep ^func pkg/sensors/load_linux.go | wc -l
17
$ diff -u pkg/sensors/load_linux.go pkg/sensors/load_windows.go | grep func
-func (s *Sensor) setMapPinPath(m *program.Map) {
-func (s *Sensor) loadMap(bpfDir string, m *program.Map) error {
func mergeSensors(sensors []*Sensor) *Sensor {
func observerMinReqs() (bool, error) {
b18d1e0
to
556b8cf
Compare
'load.go' is added in latest commit. |
556b8cf
to
d650737
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, left one minor comment, thanks
return nil | ||
} | ||
|
||
func observerLoadInstance(bpfDir string, load *program.Program, maps []*program.Map) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note, linux version has some additional code (with unload for tracepoint),
but I think it can be removed (need to check) and we could move observerLoadInstance to load.go,
but I'll put this on my todo and submit that as follow up
@@ -0,0 +1,24 @@ | |||
// SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, could you plz rename rename 'sensor' to 'sensors' in the path, so we'd have:
pkg/sensors/sensors_types_linux.go
pkg/sensors/sensors_types_windows.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits.
LGTM, thanks!
Use Win32 API QueryPerformanceCounter to implement the ktime package on Windows. monotonic version uses time package Signed-off-by: Anadi Anadi <[email protected]>
Add constants_windows.go to allow access to constants to windows specific code. Some windows constants are same as those defined in sys/unix package, their values are hardcoded. Some windows constants are specific to Windows and are imported from sys/windows package Signed-off-by: Anadi Anadi <[email protected]>
Add platform specific and platform common code in sensors/tracing package Signed-off-by: Anadi Anadi <[email protected]>
In order to port sensor package on Windows, adding exec parser and loader / unloader for Windows Tried to keep common functions between two OS in pkg/sensors/load.go. The windows implementation of some functions in load_windows.go looks like Linux implementation but is different because of unavailability of some features on windows like bpffs and btf. Also, the sensors.load() function in Windows is a work in progress and we might deprioritize code-reuse in favour of an ability to make future changes independently. Signed-off-by: Anadi Anadi <[email protected]>
d650737
to
8f3eaa8
Compare
Description
This change attempts to port
ktime
,constants
andpkg/sensors
package to Windows following changes made via #3445. This is the strategyCaveats
This is a change to prepare for Windows build and port. This does not compile on Windows yet, as more changes to dependent packages will be added in subsequent PR.
Changelog