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

Mount info gatherer #284

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Mount info gatherer #284

merged 3 commits into from
Oct 30, 2023

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Oct 26, 2023

Add mount_info gatherer. This gatherer returns if a given path is mounted, and its information. Besides that, in case of mounted, it returns the blkid UUID value of that block.

The path to check is the required argument.

If it is not mounted, it returns all the fields as empty string. I needed to do so, because checking if a path (/usr/sap) is not mounted (or it is directly in the root folder` is the main reason this gatherer exists.

If the path is mounted in nfs-kind of sharing system, the block_uuid comes empty

Some outputs:

// trento-agent facts gather --gatherer mount_info --argument /usr/sap
#{
  "block_uuid": "f45cf408-efgh-abcd-88ec-2f9269a12f07",
  "fs_type": "xfs",
  "mount_point": "/usr/sap",
  "options": "rw,relatime",
  "source": "/dev/mapper/vg_hana_usrsap-lv_hana_usrsap_0"
} 

// trento-agent facts gather --gatherer mount_info --argument /sapmnt
#{
  "block_uuid": "",
  "fs_type": "cifs",
  "mount_point": "/sapmnt",
  "options": "rw,relatime",
  "source": "10.1.1.10://sapmnt"
} 

@arbulu89 arbulu89 added the enhancement New feature or request label Oct 26, 2023
@arbulu89 arbulu89 marked this pull request as ready for review October 26, 2023 07:42
Copy link

@abravosuse abravosuse left a comment

Choose a reason for hiding this comment

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

The output looks good to me @arbulu89

Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

Perfect @arbulu89, great work! left some thoughts in the comments but no need to do anything about them

{
Name: "shared",
Gatherer: "mount_info",
CheckID: "check1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that sometimes we use the same checkid for factrequests in the tests, while other times we put incrementally increased ones and sometimes we don't put it at all. Should we apply some logic when doing so? if on the contrary, it doesn't matter at all for the tests, should we better just remove it?

No need to change anything, just wondering about this.

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 is true that it doesn't add a lot of real value. This is already tested in other places. At most, if you don't use the NewFactGatheredWithRequest, it could fail, as you forgot to pass the check id.
For me it is a good practice, as the CheckID is a mandatory field for our contract data

Value: map[string]entities.FactValue{
"block_uuid": &entities.FactValueString{Value: ""},
"fs_type": &entities.FactValueString{Value: ""},
"mount_point": &entities.FactValueString{Value: ""},
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, this is perfect IMHO, in the future if we want to also detect more complex scenarios we could decompose the mountpoint when it's not found and test backwards if the parent exists, e.g. /usr/sap passed as argument would first need to check /usr/sap and if doesn't find it, it would check /usr and if not found, it could then fallback to / and just return the details stating in mount_point the parent mountpoint of that path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see if we have such a requirement hehe

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey man codewise lgtm :)

@arbulu89 arbulu89 merged commit c4ab8e2 into main Oct 30, 2023
10 checks passed
@arbulu89 arbulu89 deleted the mount-info-gatherer branch October 30, 2023 07:58
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.

4 participants