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

Corosync cmapctl gatherer #67

Merged
merged 5 commits into from
Aug 4, 2022
Merged

Corosync cmapctl gatherer #67

merged 5 commits into from
Aug 4, 2022

Conversation

rtorrero
Copy link
Contributor

@rtorrero rtorrero commented Aug 4, 2022

This PR adds a gatherer to collect facts from the corosync-cmapctl binary and makes it available through our factsengine

Note that I had to move the utils to a separate package as I was incurring in cyclic dependencies when attempting it to use it in the internal package

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Nice @rtorrero ,
You will need to rebase from main to get the latest changes, which will break some tests I'm afraid hehe

Besides that, once this or the package_version gatherer PRs are merge, we will need to rebase back again, as they have duplicated code for the command executor.

Finally, this new gatherer must be enabled in the factsengine code, as corosyncconf is right now. Otherwise we cannot really use it.

I would try to compile the binary and run to see that it actually works:

./trento-agent facts gather --gatherer corosync-cmapctl --argument runtime.config.totem.consensus

)

const (
CorosyncCmapCtlFactKey = "corosync-cmapctl"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the package version gatherer I have used _ as "splitter". We should agree on this hehe
- or _?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not really splitting anything, but reflecting the name of the binary as-is in the same way corosync.conf is the fact key for the corosync configuration gatherer. I still understand the confusion and don't have a strong opinion on this, wdyt?

internal/factsengine/gatherers/corosynccmapctl.go Outdated Show resolved Hide resolved
internal/factsengine/gatherers/corosynccmapctl.go Outdated Show resolved Hide resolved
@arbulu89 arbulu89 added the enhancement New feature or request label Aug 4, 2022
@rtorrero
Copy link
Contributor Author

rtorrero commented Aug 4, 2022

Thanks for the review, indeed I was missing enabling the gatherer in the facts engine. Seems to work correctly now:

INFO[2022-08-04 11:24:01] Using config file: /etc/trento/agent.yaml    
INFO[2022-08-04 11:24:01] Starting corosync-cmapctl facts gathering process 
INFO[2022-08-04 11:24:01] Requested corosync-cmapctl facts gathered    
INFO[2022-08-04 11:24:01] Gathered fact for "corosync-cmapctl" with argument "runtime.config.totem.consensus": 
INFO[2022-08-04 11:24:01] {
  "name": "runtime.config.totem.consensus",
  "value": "36000",
  "check_id": ""
} 

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Perfect!
Green light from my side

fact := NewFactWithRequest(factReq, fmt.Sprint(value))
facts = append(facts, fact)
} else {
log.Warnf("%s gatherer: requested fact %s not found", CorosyncCmapCtlFactKey, factReq.Argument)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have used log.Errorf in other gatherers, but I guess it doesn't really matter that much.
Once we know how to populate the failed fact gatherings we will need to come back and finish the job hehe

@rtorrero rtorrero merged commit 4da9b5a into main Aug 4, 2022
@stefanotorresi stefanotorresi deleted the corosync-cmapctl-gatherer branch August 17, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants