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

[BUG] status.master broken on Windows #66716

Open
amalaguti opened this issue Jul 15, 2024 · 1 comment · May be fixed by #66740
Open

[BUG] status.master broken on Windows #66716

amalaguti opened this issue Jul 15, 2024 · 1 comment · May be fixed by #66740
Assignees
Labels
Bug broken, incorrect, or confusing behavior Execution-Module needs-triage Windows

Comments

@amalaguti
Copy link

Description
The funcion status.master does not show connections to master ports.
The minion is configured in multimaster and is currently connected to both masters.

While netstat shows the following information

PS C:\Users\adrian> netstat -na | Select-String "172.21.0." | Select-String ":4505" | Select-String ESTABLISHED

  TCP    172.21.0.8:61266       172.21.0.10:4505       ESTABLISHED
  TCP    172.21.0.8:61288       172.21.0.11:4505       ESTABLISHED

The module status.master returns False

PS C:\Users\adrian> salt-call status.master master=saltmaster-pip -l warning
local:
    False

Reviewing the code in modules/win_status.py the status.master() function calls _get_connected_ips(port) which usues psutils.net_connections() and this function fails to check a established connection on port (default 4505)
I think it fails due it's checking on laddr.port (local) and I think it should check on raddr.port (and raddr.ip)

def _get_connected_ips(port):
    """
    List all connections on the system that have an established connection on
    the passed port. This uses psutil.net_connections instead of netstat to be
    locale agnostic.
    """
    connected_ips = set()
    # Let's use psutil to be non-locale specific
    conns = psutil.net_connections()

    for conn in conns:
        if conn.status == psutil.CONN_ESTABLISHED:
            # Checking on my masters ip addresses port 4505 
            if conn.raddr.ip in ['172.21.0.10', '172.21.0.11'] and conn.raddr.port == 4505:
                log.warning(f">>>> ADRIAN psutil.net_connections() {conn.status} {conn.laddr.ip}:{conn.laddr.port} --> {conn.raddr.ip}:{conn.raddr.port}")
            if conn.laddr.port == port:
                connected_ips.add(conn.laddr.ip)

    return connected_ips

The warning logging messages seems to be fine

PS C:\Users\adrian> salt-call status.master master=saltmaster-pip -l warning
[WARNING ] >>>> ADRIAN psutil.net_connections() ESTABLISHED 172.21.0.8:61410 --> 172.21.0.10:4505
[WARNING ] >>>> ADRIAN psutil.net_connections() ESTABLISHED 172.21.0.8:61432 --> 172.21.0.11:4505
local:
    False

So I guess changing the code to check on raddr.ip and raddr.port should make this module work fine

Setup
3006.8 minion in multimaster setup (active/active)

Steps to Reproduce the behavior
Configure a Windows minion in multimaster setup (active/active) and run salt-call state.master master=<master>

Expected behavior
Should return True if the master is reachable

NOTE: Actually there are many issues about minion disconnecting from masters, I think this should be reviewed asap. May have impact in related minion communication issues

@amalaguti amalaguti added Bug broken, incorrect, or confusing behavior needs-triage labels Jul 15, 2024
@amalaguti
Copy link
Author

@twangboy fyi

@twangboy twangboy self-assigned this Jul 16, 2024
@twangboy twangboy added this to the Argon v3008.0 milestone Jul 16, 2024
@twangboy twangboy linked a pull request Jul 23, 2024 that will close this issue
3 tasks
@dwoz dwoz modified the milestones: Argon v3008.0, Sulfur v3006.10 Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Execution-Module needs-triage Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants