Skip to content

facts.server, operations.server: Make Mount fact and operation portable #1382

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

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Open

facts.server, operations.server: Make Mount fact and operation portable #1382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pyinfra/facts/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import json
import os
import platform
import re
import shutil
from datetime import datetime
Expand All @@ -13,6 +12,7 @@
from distro import distro
from typing_extensions import TypedDict, override

from pyinfra import host
from pyinfra.api import FactBase, ShortFactBase
from pyinfra.api.util import try_int
from pyinfra.facts import crontab
Expand Down Expand Up @@ -208,7 +208,7 @@ class Mounts(FactBase[Dict[str, MountsDict]]):

@override
def command(self):
if platform.system() == "FreeBSD":
if host.get_fact(Kernel).strip() == "FreeBSD":
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this won’t work (can’t call facts from facts). Will need to include the check condition within the command itself.

Also noting to self should make this error properly here, as shown by the failing test case.

Copy link

Choose a reason for hiding this comment

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

Somehow the check get_fact call does work though. It returns what it should on my remote FreeBSD machine:

pyinfra  inventories/physical.py fact server.Mounts
--> Fact data for: server.Mounts
{
    "xx.xx.xx.xx": {
        "/": {
            "device": "zroot/ROOT/FreeBSD",
            "type": "zfs",
...

WIthout this change it returns an error:

[x] Loaded fact server.Os
[x] cat: /proc/self/mountinfo: No such file or directory
[x] Error: could not load fact: server.Mounts

Unintentional side effect?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! Not intentional (which is why unit tests fail since they don’t mock this path), but this feels like something that would be very useful..

Need to have a proper think about the implications, but this would be a good addition I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had doubts about that change but I have the same result as @jgelens comments, it works. The other solution is to put a conditional check through sh(1) in the command(...) function but using pyinfra for this is better.

This is what happens when I run pyinfra (excerpt):

...
--> Gathering facts...
    [control-centralita] Loaded fact server.Kernel
    [control-centralita] Loaded fact server.Kernel
    [control-centralita] Loaded fact server.Mounts
...

Copy link
Member

Choose a reason for hiding this comment

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

Figured it out! So it does appear to work, and indeed it will in nearly all cases. It falls apart when you want to get a fact from a different host that isn't the current one. Something like:

from pyinfra import inventory

other_host_kernel = inventory.get_host("main").get_fact(Kernel)

Which breaks if you use the host context within the fact. The fact module is super old and still takes state and host arguments to each function. I don't see any reason this can't be tweaked to set the state/host context before gathering facts, which would then work in all cases. Working on this today to unblock this/release.

return "mount -p --libxo json"
else:
return "cat /proc/self/mountinfo"
Expand All @@ -227,7 +227,7 @@ def replace_octal(s: str) -> str:
"""
return re.sub(r"\\[0-7]{3}", unescape_octal, s)

if platform.system() == "FreeBSD":
if host.get_fact(Kernel).strip() == "FreeBSD":
full_output = "\n".join(output)
json_output = json.loads(full_output)
mount_fstab = json_output["mount"]["fstab"]
Expand Down
4 changes: 2 additions & 2 deletions pyinfra/operations/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

from __future__ import annotations

import platform
from io import StringIO
from itertools import filterfalse, tee
from os import path
Expand All @@ -21,6 +20,7 @@
Groups,
Home,
Hostname,
Kernel,
KernelModules,
Locales,
Mounts,
Expand Down Expand Up @@ -336,7 +336,7 @@ def mount(
mounted_options = mounts[path]["options"]
needed_options = set(options) - set(mounted_options)
if needed_options:
if platform.system() == "FreeBSD":
if host.get_fact(Kernel).strip() == "FreeBSD":
fs_type = mounts[path]["type"]
device = mounts[path]["device"]

Expand Down
Loading