all: add AIX provider #77

Helflym · 2020-01-27T15:23:58Z

Add a implementation without CGO for AIX.
Host information are retrieved using AIX commands (uname, oslevel)
Process information are retrieved by /proc folder.

A CGO implementation could be added to more accurate syscalls and libraries
(getprocs, libperfstat, etc). But github.com/elastic/gosigar provides such
things.

A few features are still missing:

Host CPU usage since boot time: there is no easy way to retrieve it.
Process' environment

providers/aix/machineid_aix.go

providers/aix/kernel_aix.go

providers/aix/host_aix.go

providers/aix/defs_aix.go

providers/aix/boottime_aix.go

providers/aix/process_aix.go

providers/aix/os_aix.go

fearful-symmetry

Due to the security restrictions on Metricbeat, which this is used by, We can't exec to other binaries, so this won't work:

out, err := exec.Command("/usr/bin/svmon", "-G").Output()

Helflym · 2020-03-02T15:10:00Z

Due to the security restrictions on Metricbeat, which this is used by, We can't exec to other binaries, so this won't work:
out, err := exec.Command("/usr/bin/svmon", "-G").Output()

Isn't seccomp only available on Linux ? I don't think there is any way to achieve a similar behavior on AIX and I'm sure we aren't the only one. Does Windows, Macos and BSD have something similar ?

Anyway, I can reworked the whole patch, in order to avoid exec syscalls. But it's using cgo, which I was trying to avoid here, and external C libraries. I've done something similar for elastic/gosigar (I haven't created the PR yet). Do you prefer to have something like that ?

andrewkroh · 2020-03-02T21:49:02Z

Despite not having a seccomp on AIX, I still think a native approach would be better. Usually the native approach is less CPU intensive and less prone to breakage over time (or at least we are quicker to detect the breakage when an API changes vs when the output of a command changes).

The biggest concern I have about adding an AIX provider is that we have no way to test it. So we have very limited ability to maintain it.

For my own education, what version of AIX does Go support? Is it AIX 7.2 on power8? And is this done with GOOS=aix GOARCH=ppc64?

Helflym · 2020-03-03T07:36:45Z

Despite not having a seccomp on AIX, I still think a native approach would be better. Usually the native approach is less CPU intensive and less prone to breakage over time (or at least we are quicker to detect the breakage when an API changes vs when the output of a command changes).

Actually, AIX has a real strict compatibility behavior and therefore commands' outputs won't change. However, I understand your point of view and I'll move to a CGO version as soon as I can.

The biggest concern I have about adding an AIX provider is that we have no way to test it. So we have very limited ability to maintain it.

Sadly, that's the case for every Go packages. My opinion is that if anything ever go wrong, the AIX community will be able to send PRs to fix it.
For this package, as it's included inside metricbeat and that we are planning to deliver it for almost every version, we will be able to find any bugs quite quickly.

For my own education, what version of AIX does Go support? Is it AIX 7.2 on power8? And is this done with GOOS=aix GOARCH=ppc64?

Yes, the Go toolchain is available only for AIX 7.2 on Power8 and Power9 (but I haven't try on P9). As you said, we are using GOOS=aix and GOARCH=ppc64.
It might work on previous AIX versions with gccgo, but I haven't tested that yet.

providers/aix/kernel_aix.go

providers/aix/doc.go

providers/aix/defs_aix.go

providers/aix/kernel_aix.go

providers/aix/doc.go

providers/aix/defs_aix.go

fearful-symmetry

We're making progress! We still need some standardization of the names to make the linters happy, and tests.

providers/aix/boottime_aix.go

providers/aix/defs_aix.go

providers/aix/host_aix.go

providers/aix/boottime_aix.go

fearful-symmetry

We still need to make the linter happy and deal with some standardization issues.

providers/aix/boottime_aix.go

providers/aix/boottime_aix.go.rej

providers/aix/defs_aix.go.rej

providers/aix/doc.go

providers/aix/host_aix.go

providers/aix/os_aix.go

providers/aix/process_aix.go.rej

providers/aix/os_aix.go

fearful-symmetry

Almost done from my perspective. I'd like @andrewkroh to sign off on the PR too. We can save testing for later, but we're gonna eventually need to set up environments in AIX.

Not sure what @andrewkroh thinks, but I'd like to avoid cutting a release until we have an AIX environment setup so we can at least do manual testing.

providers/aix/process_aix.go

andrewkroh

I made a suggestion for one simple test that can be executed outside of AIX. If there are any other unit tests you can add that would be great. Even if we can't execute them today on CI they will still be helpful to anyone that's maintaining the AIX code.

Overall it LGTM. Can you please

update the README.md with a column for AIX/ppc64.
run the tests and paste the output into a comment

providers/aix/boottime_aix.go

Helflym · 2020-04-17T08:50:26Z

The two other unit tests which can be made are (*process) User() and (*process) CPUTime() as they are the only ones dealing with /proc files. Tell me if you wish me to add them but I don't think it will be worth it to just check these two functions while all other dealing with processes aren't.

For the test output, you wish to have the result of "go test -v" that's it ? Even for all processes running on my local machines, or just a sample will be enough ?

fearful-symmetry · 2020-07-09T20:17:46Z

Hey @Helflym sorry for the long silence. We've been waiting until Elastic has access to a proper AIX environment before we go any further. That took...longer than anticipated.

A few things:

The tests are failing because in system_test.go you marked Environment as false, but it appears to be implemented anyways.
I don't think it's worth it right now to add additional unit tests, since Elastic people can now test manually, and because it's unlikely we'll have AIX CI set up in the short-medium term.

Helflym · 2020-07-10T08:39:54Z

Yeah don't worry. I was warned by the IBM team for that.

The tests are failing because in system_test.go you marked Environment as false, but it appears to be implemented anyways.
Because it's set as True in my environment. I don't know how I missed that one...

fearful-symmetry · 2020-07-14T02:18:46Z

@Helflym thanks. Hopefully close to getting this merged.

Sorry to change my mind, would it be possible to get these tests after all?

The two other unit tests which can be made are (*process) User() and (*process) CPUTime() as they are the only ones dealing with /proc files.

Assuming everything is green, we should be good to merge after that.

Helflym · 2020-07-15T13:35:36Z

Sorry to change my mind, would it be possible to get these tests after all?

The two other unit tests which can be made are (*process) User() and (*process) CPUTime() as they are the only ones dealing with /proc files.

Actually, it's a bit more complicated than I thought. I either need to change the way I'm retrieving "/proc" information or I need to check the test process but it seems to unstable for me.
Do you really wish these tests or can they be added to a TODO ? I'm a bit overwhelmed by other stuff right now, so I would be glad if I can add them later.

fearful-symmetry · 2020-07-15T19:29:24Z

@Helflym We can add them them later if it takes too much effort. I'd like to get this in a state where it can be merged.

Add a implementation with CGO for AIX. Process information are retrieved by /proc folder or with libperfstat, getprocs syscalls.

houndci-bot reviewed Jan 27, 2020

View reviewed changes

providers/aix/process_aix.go Outdated Show resolved Hide resolved

providers/aix/os_aix.go Outdated Show resolved Hide resolved

providers/aix/os_aix.go Show resolved Hide resolved

Helflym force-pushed the master branch 3 times, most recently from 4c0255b to c5c07ec Compare January 27, 2020 15:45

Helflym mentioned this pull request Feb 14, 2020

Add AIX support elastic/beats#15785

Open

urso requested review from fearful-symmetry, andrewkroh and kvch February 26, 2020 16:20

fearful-symmetry requested changes Mar 2, 2020

View reviewed changes

Helflym force-pushed the master branch from c5c07ec to fcc5cac Compare March 16, 2020 08:36

houndci-bot reviewed Mar 16, 2020

View reviewed changes

providers/aix/kernel_aix.go Show resolved Hide resolved

providers/aix/doc.go Outdated Show resolved Hide resolved

providers/aix/defs_aix.go Outdated Show resolved Hide resolved

houndci-bot reviewed Mar 16, 2020

View reviewed changes

providers/aix/kernel_aix.go Show resolved Hide resolved

providers/aix/doc.go Outdated Show resolved Hide resolved

providers/aix/defs_aix.go Outdated Show resolved Hide resolved

fearful-symmetry requested changes Apr 1, 2020

View reviewed changes

Helflym force-pushed the master branch from fcc5cac to b578db4 Compare April 2, 2020 08:13

houndci-bot reviewed Apr 2, 2020

View reviewed changes

providers/aix/boottime_aix.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 2, 2020

View reviewed changes

providers/aix/boottime_aix.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 2, 2020

View reviewed changes

providers/aix/boottime_aix.go Outdated Show resolved Hide resolved

fearful-symmetry requested changes Apr 2, 2020

View reviewed changes

Helflym force-pushed the master branch from b578db4 to d2f5261 Compare April 3, 2020 08:40

fearful-symmetry requested changes Apr 3, 2020

View reviewed changes

providers/aix/process_aix.go Show resolved Hide resolved

Helflym force-pushed the master branch from d2f5261 to 540a705 Compare April 6, 2020 07:58

andrewkroh reviewed Apr 16, 2020

View reviewed changes

providers/aix/boottime_aix.go Outdated Show resolved Hide resolved

Helflym force-pushed the master branch from 540a705 to 59b1510 Compare April 17, 2020 08:41

Helflym force-pushed the master branch from 59b1510 to 1378366 Compare July 10, 2020 08:41

Helflym force-pushed the master branch 3 times, most recently from a8b20c1 to e729806 Compare July 17, 2020 08:47

Clément Chigot and others added 2 commits July 17, 2020 10:48

all: add AIX provider

e729806

Add a implementation with CGO for AIX. Process information are retrieved by /proc folder or with libperfstat, getprocs syscalls.

change constraints on test

6724726

fearful-symmetry requested review from fearful-symmetry and andrewkroh July 17, 2020 21:38

andrewkroh approved these changes Jul 20, 2020

View reviewed changes

fearful-symmetry approved these changes Jul 20, 2020

View reviewed changes

fearful-symmetry merged commit 77fcdd7 into elastic:master Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: add AIX provider #77

all: add AIX provider #77

Helflym commented Jan 27, 2020

fearful-symmetry left a comment

Helflym commented Mar 2, 2020

andrewkroh commented Mar 2, 2020

Helflym commented Mar 3, 2020

fearful-symmetry left a comment

fearful-symmetry left a comment

fearful-symmetry left a comment

andrewkroh left a comment •

edited

Loading

Helflym commented Apr 17, 2020

fearful-symmetry commented Jul 9, 2020

Helflym commented Jul 10, 2020

fearful-symmetry commented Jul 14, 2020

Helflym commented Jul 15, 2020

fearful-symmetry commented Jul 15, 2020

all: add AIX provider #77

all: add AIX provider #77

Conversation

Helflym commented Jan 27, 2020

fearful-symmetry left a comment

Choose a reason for hiding this comment

Helflym commented Mar 2, 2020

andrewkroh commented Mar 2, 2020

Helflym commented Mar 3, 2020

fearful-symmetry left a comment

Choose a reason for hiding this comment

fearful-symmetry left a comment

Choose a reason for hiding this comment

fearful-symmetry left a comment

Choose a reason for hiding this comment

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

Helflym commented Apr 17, 2020

fearful-symmetry commented Jul 9, 2020

Helflym commented Jul 10, 2020

fearful-symmetry commented Jul 14, 2020

Helflym commented Jul 15, 2020

fearful-symmetry commented Jul 15, 2020

andrewkroh left a comment •

edited

Loading