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.

Sign up for GitHub

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

Jump to bottom

Resolver gatherer #274

Merged

rtorrero merged 22 commits into main from resolver-gatherer

Oct 19, 2023

Contributor

rtorrero commented Oct 10, 2023 •

edited

Loading

This PR adds a resolver gatherer, it uses the filesystem to search for running sap systems and the profile data to get the virtual hostnames associated to each instance of the sap system. It will then attempt to resolve those hostnames to confirm that the OS knows about them.

rtorrero requested review from CDimonaco, arbulu89 and EMaksy

October 10, 2023 09:04

arbulu89 reviewed

View reviewed changes

Contributor

arbulu89 left a comment

@rtorrero Some initial comments about the generic logic of the code

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

arbulu89 reviewed

View reviewed changes

Contributor

arbulu89 left a comment

Looking good @rtorrero
Good progress

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/gatherer.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver_test.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver_test.go Outdated Show resolved Hide resolved

arbulu89 reviewed

View reviewed changes

Contributor

arbulu89 left a comment

More comments

pkg/utils/hostnameresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

rtorrero force-pushed the resolver-gatherer branch 2 times, most recently from ed99aea to 55d8719 Compare

October 17, 2023 07:19

rtorrero marked this pull request as ready for review

October 17, 2023 07:21

rtorrero force-pushed the resolver-gatherer branch from 5b37fcd to dee2be1 Compare

October 17, 2023 08:02

CDimonaco requested changes

View reviewed changes

Member

CDimonaco left a comment

I think the code looks good, I'm requesting changes not on the logic itself, but on style and readability

Anyway GJ

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

pkg/utils/hostnameresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

rtorrero requested a review from CDimonaco

October 17, 2023 16:15

rtorrero force-pushed the resolver-gatherer branch from e23cd56 to 360197b Compare

October 17, 2023 16:27

rtorrero added 14 commits

October 17, 2023 17:28


          Add hostname resolver mock

250f0e9


          Add saplocalhost resolver gatherer

0b03fd0


          Use NewFactValue instead of literal FactValue object

7cbc3ac


          Improve code

46b5f5f


          No need to create an actual profile file

73da6b2


          Use a list of maps as there can be multiple hostnames per sid

819a32f


          Add more tests

260026b


          Remove unnecessary test

fde2e4c


          Address linter issues

d523215


          Introduce HostPinger util

37e2b01


          Export SapDefaultProfile constant

46f36fb


          Cleanup code and introduce HostPinger usage

8df9f79


          Move mock interfaces from utils to the gatherer

64b8a59


          Add additional profile for the success test with failing ping

360197b

Contributor Author

rtorrero commented Oct 17, 2023

@CDimonaco @arbulu89 thanks for the reviews, I think I've addressed everything, the only thing that I'm not 100% I did accordingly was the mocks, I didn't fully understand @arbulu89 proposal but I think I came up with a solution that addresses the main concern of not adding stuff to the utils when it's not being used elsewhere. In addition to the comments, I also:

Added the 'reachability' details (using ping) in the resulting fact value.
Improved a bit the main success test to include additional profiles and a failing ping.


          Address missed linter issues

02aea64

arbulu89 requested changes

View reviewed changes

Contributor

arbulu89 left a comment

Suggestions from my side

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver_test.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/saplocalhostresolver_test.go Outdated Show resolved Hide resolved

internal/factsengine/gatherers/mocks/HostPinger.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

rtorrero added 2 commits

October 18, 2023 12:41


          Revert unneeded module version updates

4af9a03


          Address comments

84a21fe

rtorrero requested a review from arbulu89

October 18, 2023 11:44


          Re-generate HostPinger mock with mockery 2.32.3

f693b15

arbulu89 approved these changes

View reviewed changes

Contributor

arbulu89 left a comment

@rtorrero Great! It looks good now.
Now....
the most difficult thing. Naming XD
I would vote for SapInstanceNameResolver or SapInstanceResolver or SapHostnameResolver, with preference for the first.
And the gatherer name would be sap_instancename_resolver maybe.
@abravosuse any idea?

go.mod

               	google.golang.org/protobuf v1.31.0
               )
               require (
               	github.com/d-tux/go-fstab v0.0.0-20141204152952-eb4090f26517
               	github.com/iancoleman/strcase v0.2.0
+              	github.com/spf13/afero v1.9.5

Contributor

arbulu89 Oct 18, 2023

I don't know why golang moves this to other require. Mysteries XD

abravosuse commented Oct 18, 2023 •

edited

Loading

@rtorrero @arbulu89 It's not the instance name that we are trying to resolve but the instance hostname. Therefore my preference would be SapInstanceHostnameResolver.


          Adjust gatherer name

arbulu89 reviewed

View reviewed changes

internal/factsengine/gatherers/sapinstancehostnameresolver.go Outdated

    
              	hostnameRegexCompiled                   = regexp.MustCompile(`(.+)_(.+)_(.+)`) // <SID>_<InstanceNumber>_<Hostname>

              	regexSubgroupsCount                     = 4

              	SapInstanceHostnameResolverDetailsError = entities.FactGatheringError{

              		Type:    "sapinstance_hostname_resolver-details-error",

Contributor

arbulu89 Oct 18, 2023

In other gatherers package_version, even though gatherer uses _, we use - here.
I don't want to create a big fuzz about that, but just wanted to comment hehe

Contributor Author

rtorrero Oct 18, 2023

no worries, I can change it

rtorrero added 2 commits

October 18, 2023 15:22


          Change _ for - in error codes

7b819ae


          Use statistics to check for ping result

556cf88

rtorrero mentioned this pull request

Add sapinstance-hostname-resolver gatherer doc trento-project/wanda#320

Merged


          Reduce ping timeout

8bc4bb4

rtorrero force-pushed the resolver-gatherer branch from 2aab482 to 8bc4bb4 Compare

October 18, 2023 15:27

EMaksy reviewed

View reviewed changes

Member

EMaksy left a comment

Hey nice work, lgtm :D

CDimonaco approved these changes

View reviewed changes

rtorrero merged commit c1e5123 into main

9 checks passed

rtorrero deleted the resolver-gatherer branch

October 19, 2023 07:51

CDimonaco added the enhancement label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels