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

Migrate from go-ps to gopsutil for better process detection #597

Merged
merged 2 commits into from
Sep 7, 2020
Merged

Conversation

aelsabbahy
Copy link
Member

@aelsabbahy aelsabbahy commented Jul 3, 2020

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description of change

Fixes #451

@stale
Copy link

stale bot commented Sep 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Used by https://probot.github.io/apps/stale/ label Sep 1, 2020
@petemounce
Copy link
Collaborator

LGTM!

@stale stale bot removed the stale Used by https://probot.github.io/apps/stale/ label Sep 1, 2020
@aelsabbahy aelsabbahy merged commit 85ce2c8 into v4 Sep 7, 2020
@aelsabbahy aelsabbahy deleted the gopsutil branch September 7, 2020 17:10
@ripienaar
Copy link
Collaborator

Hmm any chance this can be done to master as well? go-ps has some issues with newer go

@aelsabbahy
Copy link
Member Author

Reason I kept it v4 is because it introduces breaking changes and I'm bundling up all the breaking changes for a "major" release.

I can upgrade to new version of go as part of v4 too.

Curious, what's the go-ps issue with new go?

@ripienaar
Copy link
Collaborator

I will ask again, someone who is packaging my software on FreeBSD where they use 1.15 already complained but he didnt give me details and is no afk, will see if I can get specifics.

do you have views on a v4 timeline?

@ripienaar
Copy link
Collaborator

Ah, https://github.com/mitchellh/go-ps/blob/master/process_freebsd.go isnt limited to amd64 only, thats the issue.

@ripienaar
Copy link
Collaborator

ripienaar commented Oct 8, 2020

Basically just mitchellh/go-ps@32d7659#diff-bc4fae79b38e742342740544c00892f0, so I can do a PR against your fork to bring it up to same if possible, and a follow up here to update your master

@aelsabbahy
Copy link
Member Author

Yup sounds good. I can definitely have that in the next minor release.

Thanks!

@ns-tghosh
Copy link

Hmm any chance this can be done to master as well? go-ps has some issues with newer go

@aelsabbahy Would this change be merged to master? We have encountered the same issue of the process being misread.

# ps -p 13 -o comm,cmd
COMMAND         CMD
verdict_control /usr/bin/python3.8 /usr/local/bin/verdict_controller_server.py --port 5000

# cat process.yaml
process:
  verdict_control:
    running: true

# ../goss -g process.yaml validate
F

Failures/Skipped:

Process: verdict_control: running:
Expected
    false
to equal
    true

Total Duration: 0.027s
Count: 1, Failed: 1, Skipped: 0

Goss version and linux distribution details:

# ../goss --version
goss version 0.13.18

# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04.7 LTS
Release:        16.04
Codename:       xenial

@aelsabbahy
Copy link
Member Author

0.13.18 was this a copy/paste error or is the binary a custom local build?

I think this might be a new issue since the issue described by @ripienaar only affected freebsd.

Can you open a new issue with this, I assume goss a process verdict_control has the same issue?

@aelsabbahy
Copy link
Member Author

Out of curiosity, what does cat /proc/13/comm say? I wonder if ubuntu's ps -o comm .. command doesn't truncate the comm or does some enrichment to it.

@ns-tghosh
Copy link

0.13.18 was this a copy/paste error or is the binary a custom local build?
It's a custom local build after cloning v4 branch

Out of curiosity, what does cat /proc/13/comm say? I wonder if ubuntu's ps -o comm .. command doesn't truncate the comm or does some enrichment to it.
Process info:
root 15 1 0 19:08 ? 00:00:00 /usr/bin/python3.8 /usr/local/bin/verdict_controller_server.py --port 5000

# cat /proc/15/comm
verdict_control

@ns-tghosh
Copy link

Can you open a new issue with this, I assume goss a process verdict_control has the same issue?

#775

aelsabbahy added a commit that referenced this pull request Jun 25, 2023
@aelsabbahy aelsabbahy mentioned this pull request Jun 25, 2023
3 tasks
aelsabbahy added a commit that referenced this pull request Jul 19, 2023
* Migrate from go-ps to gopsutil for better process detection (#597)

* Migrate from go-ps to gopsutil for better process detection

* Remove debugging code

* Mount (#601)

* Add stale.yml config

* Change stale.yml label

* Add VfsOpts to mount

* Use mountinfo filter

* Fix build failures

* Normalize seliux mount options for integration testing

* Attempt to normalize tests across docker installs and fix osx test setup

* Try different osx image

* Rename binaries to strip '-alpha' (#671)

* Rename binaries to strip '-alpha'

As discussed in #663 (comment); strip the -alpha naming, keep the messaging, keep the --use-alpha flag and env-var.

This allows people to remove edge-cases from their installation, yet retains the clearly-set expectations of support being community-driven.

Fixes #651.

* set expectations for future-us re: macOS+Windows vs linux

* fix doc

* F openrc runlevels (#668)

* service: Add support for OpenRC runlevels

* Omit empty runlevel

* Update documentation

* Update integration tests

* Use consistent pattern for configu default values

- Remove struct tag `default` value reflection

* Make runLevels a testable attribute

* empty commit to try to trigger travis

* Update trusty to reflect precise changes

Co-authored-by: Berney <[email protected]>

* POC/DRAFT: Add transforms (#576)

* Add transforms

* wip add transforms work

* Add some examples to make sense of this

* Handle floats/ints better

* Convert validatecontails to gomega

* wip

* Add gjson and contain-substring

* wip

* Subclass all matchers adding String()

* I don't evne know what this is.. should have checked it in back when I wrote it

* Migrate GomegaMatcher -> GossMatcher

* lots of changes + drop json-iterator

* Move output related logic to outputs

* Add include_raw flag

* Add bench output format

* Update have_patterns to check input. Add stubs to make Not matcher act as omegaMatcher

* SetEscapeHTML(false) for semver constraint

* Ensure have-Key_with_value works

* remove have-key-with-value

* Fix error compact output. Add oMegaMatcher stubs. Ensure matcher vs m consistent

* Cleanup matchers

* Cleanup TestResult struct and remove need for dyno

* Update all deps

* Update tranformer tests and add summary line to bench output format

* Update readme

* Conditionally print missing and empty values. Sanitize found and expected values for consistency

* Check value is valid before checking IsNil()

* Initial docs change

* WIP fix for pretty print

* Swap order on video vs blog for dgoss docs

* Updated all tests except for semver

* Fix all unit tests

* Fix detection of when matcher is set

* Update documentation

* Update docs/manual.md

Co-authored-by: Peter Mounce <[email protected]>

* Update docs/manual.md

Co-authored-by: Peter Mounce <[email protected]>

* gjson: Validate json before doing gets

* Make errors more clear

* Remove unused field

* Bump go version

* Update doc

* Fix regression with gomega errors

* Fix #262 no need to add file size by default

* Fix output formats

* Use proper quoting for windows test

* Add error checking for gjson path not found

Co-authored-by: Peter Mounce <[email protected]>

* Fix mountinfo splitting when there's a quoted comma

* Improve have_patterns error message

* Add syntax checks for type casting

* Rename file.contains to file.contents

* ToString converter add suppert for []interface{}

* Convert headers to lowercase. fixes #760

* Fix contains -> contents, lowercase headers and contain-element float64

* Enhance all resources to support key override

closes #518, closes #742

* Sort output in documentation format

closes #416

* Fix typo in contain_element message

* Track start and end times per-test

This also changes the way total time is calculated
in the output summary.

total time = endTime of last test - start time of first test

* Cache test results in serve instead of output

closes #612

* Use exit code 78 if test file is unparseable

closes #317

* Full EVR for rpm and fix failing tests

* more tests

* Increase test coverage

* Fixed numeric eq bug

* Make output more consistent

* Update dependencies

* Fix some merge conflicts

* Revert "Migrate from go-ps to gopsutil for better process detection (#597)"

This reverts commit 85ce2c8.

* Revert 1fe5571 change http.headers back to io.Reader

* FIX: use filepath.Join() in order to be OS agnostic

* FIX: only run matcher_tests on linux

* FIX: replace failing www.microsoft.com test

* FIX: trusty dockerfile merge regression

* Run all tests (except failing prometheus) as part of CI. Seems they were not running

* Remove bench output, add some comments

* Changed: include_raw by default, provide exclude_raw as a format flag

* Changed: go-funk -> lo

* Changed: temp fix to deal with httpbin.org slowness.

Need to move to offline testing to avoid flakiness

* Locking down version in install.sh, to avoid RC being installed

---------

Co-authored-by: Peter Mounce <[email protected]>
Co-authored-by: Berney <[email protected]>
Co-authored-by: Peter Mounce <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants