Replace manual process handling by local_cmd() #359

guzu · 2025-10-17T08:03:13Z

No description provided.

guzu · 2025-10-20T09:19:21Z

I didn't manage to test everything because I don't know yet how to setup some tests.

stormi · 2025-10-20T12:59:54Z

I didn't manage to test everything because I don't know yet how to setup some tests.

Can you list the ones you could test and the ones you couldn't? We could likely either help you run the remaining ones, or have them run on the CI hosts from your branch.

guzu · 2025-10-20T13:19:59Z

Can you list the ones you could test and the ones you couldn't? We could likely either help you run the remaining ones, or have them run on the CI hosts from your branch.

Sure, I did:

tests/network/test_vif_allowed_ip.py
tests/storage/ext/test_ext_sr.py : mostly to test reboot()
tests/xapi_plugins/plugin_netdata/test_netdata.py

I think we need to test the following:

scripts/install_xcpng.py: which is not a test but is using lots of lib functions used also by the tests
tests/xo/test_xo_connection.py : I didn't manage to register xo-cli register
tests/fs_diff/test_fs_diff.py

stormi · 2025-10-20T14:50:57Z

Ok, so the first one and the third one are not covered by the main CI runs, they're in installation/upgrade tests that @ydirson is much knowledgeable about.

The test_xo_connection.py one, it would be good to make it work. It can be useful to have a local xo-cli and to connect to a XO instance.

guzu · 2025-10-21T12:19:27Z

Update:

test_xo_connection.py is working fine with @stormi VM, not mine for unknown reason yet, at least the test is not broken
no news for other tests

ydirson · 2025-10-28T14:23:05Z

I think we need to test the following:

* `scripts/install_xcpng.py`: which is not a test but is using lots of lib functions used also by the tests

You should be able to use it to launch an automated install of a nested host

* `tests/fs_diff/test_fs_diff.py`

This one requires 2 hosts to be compared, it should be possible to run using any 2 nested hosts (eg. the one you installed by hand, and one installed with scripts/install_xcpng.py

conftest.py

lib/xo.py

scripts/install_xcpng.py

tests/fs_diff/test_fs_diff.py

tests/packages/netdata/test_netdata.py

stormi · 2025-11-21T15:55:34Z

lib/commands.py

    assert False, "unexpected type"

 def scp(hostname_or_ip, src, dest, check=True, suppress_fingerprint_warnings=True, local_dest=False):
+    from lib.netutil import wrap_ip


I suppose there's a reason for not importing at the top level, but such exception should be documented with a comment.

This is to avoid cycling import. Or I should have put the new function in something different than netutils.
Or maybe you prefer that the local import be done in netutils ? It should not make any diffrence.
I will add a comment anyway.

Comment added

The cyclic import could be solved in several ways:

considering that after all we only need to check port 22 and make this a part of commands.py (yeah, that's a bit meh)

acknowledging that after all local_cmd and ssh are not on the same level (but here it is unfortunate the one that would be the best candidate to move out into a new file was the first occupant and most-imported over the repo, ie. ssh and friends)

Maybe we could have a variant of the 2nd solution, by moving everyone out, into lib/ssh.py and lib.command.py (note the lack of s - or maybe local.py would be better? I'm not sure), and let commands.py import the relevant symbols from those as a compatibility lib until we cleanup everything (avoiding to break all those in-flight PRs)

What do you of moving it to lib.netutil ?
Anyway we could make that in second time, not in that PR, it would change too much files.

PR #366 created for that purpose.

stormi · 2025-11-21T15:58:58Z

lib/efi.py

        Returns path to signed image.
        """
        assert self._owner_cert is not None
+        assert self._owner_cert.key is not None


I'm not sure how this change relates to the rest of the commit

This is here to please type checking since I added type checking information for local_cmd()

Thanks. Probably something to mention in the commit message, as it's not obvious.

ydirson

There is also an "introdice" typo in the first commit message

lib/host.py

tests/fs_diff/test_fs_diff.py

ydirson · 2025-11-21T15:50:16Z

lib/commands.py

+    if simple_output:
+        return output.strip()
    return LocalCommandResult(res.returncode, output)


Making the default simple_output=True actually changes the default behavior of the function. A separate commit for this change would make it easier for review.

Could be good to mention in commit message that the API indeed changes.

OTOH, now that I see how the change looks like, I wonder if it's so much of a good idea: if seems to make the non-default calls harder to read:

- result = commands.local_cmd(COMMAND, check=False) - output = commands.local_cmd(COMMAND, check=False).stdout + result = commands.local_cmd(COMMAND, check=False, simple_output=False) + output = commands.local_cmd(COMMAND, check=False)

Wouldn't we rather want to change the ssh APIs to match this simpler model (later, and possibly in the PR moving them in another module)?

lib/netutil.py

lib/commands.py

conftest.py

rzr · 2025-11-25T09:02:35Z

lib/netutil.py

+        wait_for(lambda: local_cmd(['ping', '-c1', host], check=False, simple_output=False).returncode == 0,
+                 "Wait for host up (ICMP ping)", timeout_secs=10 * 60, retry_delay_secs=10)
+    wait_for(lambda: local_cmd(['nc', '-zw5', host, str(port)], check=False, simple_output=False).returncode == 0,
+             f"Wait for {port_desc} up on host {host_desc}", timeout_secs=10 * 60, retry_delay_secs=5)


i would add timers, retry as defauts params

We can do that when/if we need to use a different value IMO

sure i was a just a suggestion at this stage, btw the motivation of values will not hurt if there is any

rzr · 2025-11-25T09:03:11Z

lib/netutil.py

+             f"Wait for {port_desc} up on host {host_desc}", timeout_secs=10 * 60, retry_delay_secs=5)
+
+def wait_for_ssh(host, host_desc=None, ping=True):
+    wait_for_tcp_port(host, 22, "SSH", ping, host_desc)


same for port, should't it be a default param ?

I understand, but in this why not just wait_for_tcp_port() in this case. The function doesn't bother if it's a real SSH server.

well my comment was mostly from API perspective but not a big deal, you could add extra params for ssh if needed, like minimal version requiered etc

rzr · 2025-11-25T09:06:42Z

scripts/install_xcpng.py

-    cmd = ['openssl', 'passwd', '-6', password]
-    res = subprocess.run(cmd, stdout=subprocess.PIPE)
-    encrypted_password = res.stdout.decode().strip()
+    encrypted_password = local_cmd(['openssl', 'passwd', '-6', password])


Suggested change

encrypted_password = local_cmd(['openssl', 'passwd', '-6', password])

algorithm_option = '-6' # SHA

encrypted_password = local_cmd(['openssl', 'passwd', algorithm_option, password])

rzr · 2025-11-25T09:12:14Z

tests/misc/test_file_server.py

    regex = fr"{name}:\s?{re.escape(value)}"
    return re.match(regex, header) is not None

 def test_fileserver_redirect_https(host):


I should add port 80 as default param, along path

That's not the purpose of this PR. Maybe we could plan a task to review the lib/ part and make futur proof. Maybe refactor some parts.

rzr

LGTM but i would use more defaults params to make the lib parts future proof

guzu · 2025-11-25T09:32:14Z

@glehmann I just added one more commit to add root conftest.py to ruff check.
Is that ok for you ?

glehmann · 2025-11-25T09:51:39Z

@glehmann I just added one more commit to add root conftest.py to ruff check. Is that ok for you ?

Yes 👍
We should check the whole repository with ruff now. We couldn't do it before using uv because some scripts were using newer python features than our python version, but that's not true anymore (This is not a request for this PR).

ydirson

Essentially minor polishing stuff, except for this big questioning about adding simple_output. What do y'all think?

lib/xo.py

tests/fs_diff/test_fs_diff.py

ydirson · 2025-11-26T16:34:44Z

tests/misc/test_access_links.py

-        f"Failed to fetch URL locally: {local_result.stderr}"
+        f"Failed to fetch URL locally: {local_result.stdout}"


Why change this and not show stderr?

Because it there only stdout. Standard error output is redirected to stdout.
Maybe we could rename stdout in output which would be less misleading.

Wow, I did not notice that. It looks like an awful idea to have them merged, actually :(

I split stdout and add stderr into LocalCommandFailed and LocalCommandResult.
Can you check if you're ok with that ?

That seems to look good from the range-diff output (but would have been easier t read in a separate commit 😉 - if we keep it inside this commit it would be worth describing in the commit desc).
I wonder if it would not make sense to replace in LocalCommandFailed the output by the full LocalCommandResult, so we can easily still get access to stdout if needed.

tests/misc/test_file_server.py

ydirson · 2025-11-26T16:45:59Z

lib/commands.py

+    if simple_output:
+        return output.strip()
    return LocalCommandResult(res.returncode, output)


Could be good to mention in commit message that the API indeed changes.

OTOH, now that I see how the change looks like, I wonder if it's so much of a good idea: if seems to make the non-default calls harder to read:

- result = commands.local_cmd(COMMAND, check=False) - output = commands.local_cmd(COMMAND, check=False).stdout + result = commands.local_cmd(COMMAND, check=False, simple_output=False) + output = commands.local_cmd(COMMAND, check=False)

Wouldn't we rather want to change the ssh APIs to match this simpler model (later, and possibly in the PR moving them in another module)?

lib/netutil.py

And change a bit local_cmd() interface to behave like ssh(); introduce simple_output argument. Assertion added in lib/efi.py is here to please linter, because self._owner_cert.key is used in argument to local_cmd() below. Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

To match the ssh() API. Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

Specific error messages are no longer available with this refactoring. Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

guzu requested a review from ydirson October 17, 2025 08:03

guzu force-pushed the eva/local-cmd-generalize branch 4 times, most recently from 31da723 to 33bd187 Compare October 17, 2025 08:40

ydirson reviewed Oct 28, 2025

View reviewed changes

conftest.py Outdated Show resolved Hide resolved

ydirson reviewed Oct 28, 2025

View reviewed changes

lib/xo.py Outdated Show resolved Hide resolved

stormi reviewed Nov 19, 2025

View reviewed changes

lib/xo.py Outdated Show resolved Hide resolved

stormi reviewed Nov 19, 2025

View reviewed changes

scripts/install_xcpng.py Outdated Show resolved Hide resolved

stormi reviewed Nov 19, 2025

View reviewed changes

tests/fs_diff/test_fs_diff.py Outdated Show resolved Hide resolved

stormi reviewed Nov 19, 2025

View reviewed changes

tests/packages/netdata/test_netdata.py Show resolved Hide resolved

guzu force-pushed the eva/local-cmd-generalize branch from 9d42bbb to 9bfd3cc Compare November 19, 2025 17:07

guzu requested a review from stormi November 19, 2025 17:08

guzu force-pushed the eva/local-cmd-generalize branch from 9bfd3cc to 49fbbca Compare November 19, 2025 17:19

guzu mentioned this pull request Nov 20, 2025

Add support for VM serial/UART console logging #364

Merged

guzu force-pushed the eva/local-cmd-generalize branch 2 times, most recently from 27b9248 to 233a249 Compare November 21, 2025 09:16

guzu requested a review from ydirson November 21, 2025 09:20

stormi reviewed Nov 21, 2025

View reviewed changes

ydirson requested changes Nov 21, 2025

View reviewed changes

guzu force-pushed the eva/local-cmd-generalize branch from 233a249 to 97b16ea Compare November 24, 2025 17:21

guzu requested a review from a team as a code owner November 24, 2025 17:21

guzu force-pushed the eva/local-cmd-generalize branch from 97b16ea to 0a44e00 Compare November 24, 2025 17:22

stormi requested a review from ydirson November 24, 2025 17:23

guzu force-pushed the eva/local-cmd-generalize branch from 0a44e00 to 13a9446 Compare November 24, 2025 17:50

guzu requested a review from stormi November 24, 2025 17:50

rzr reviewed Nov 25, 2025

View reviewed changes

glehmann approved these changes Nov 25, 2025

View reviewed changes

rzr reviewed Nov 25, 2025

View reviewed changes

guzu force-pushed the eva/local-cmd-generalize branch from 13a9446 to 7317117 Compare November 25, 2025 09:30

guzu mentioned this pull request Nov 26, 2025

Move SSH and related to netutil #366

Draft

guzu force-pushed the eva/local-cmd-generalize branch from 7317117 to 7f29d69 Compare November 26, 2025 16:17

ydirson requested changes Nov 26, 2025

View reviewed changes

guzu force-pushed the eva/local-cmd-generalize branch from 7f29d69 to 8a9650d Compare November 27, 2025 12:49

guzu force-pushed the eva/local-cmd-generalize branch from 8a9650d to 096553e Compare November 27, 2025 13:01

guzu added 5 commits November 27, 2025 14:11

fixup! Replace manual process handling by local_cmd()

20a4d40

lib: add simple_output argument to local_cmd()

5ff8705

To match the ssh() API. Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

Refactor TCP port test with netcat into netutil

e3b5c78

Specific error messages are no longer available with this refactoring. Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

conftest: fix or hide some linter issues

45c3b35

Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

Add conftest.py to ruff check

1e20bf2

Signed-off-by: Emmanuel Varagnat <emmanuel.varagnat@vates.tech>

guzu force-pushed the eva/local-cmd-generalize branch from 096553e to 1e20bf2 Compare November 27, 2025 13:13

guzu requested a review from ydirson November 27, 2025 13:13

guzu mentioned this pull request Dec 1, 2025

Add support for VM serial/UART console logging #370

Draft

	encrypted_password = local_cmd(['openssl', 'passwd', '-6', password])
	algorithm_option = '-6' # SHA
	encrypted_password = local_cmd(['openssl', 'passwd', algorithm_option, password])

		f"Failed to fetch URL locally: {local_result.stderr}"
		f"Failed to fetch URL locally: {local_result.stdout}"

Replace manual process handling by local_cmd() #359

Are you sure you want to change the base?

Replace manual process handling by local_cmd() #359

Uh oh!

Conversation

guzu commented Oct 17, 2025

Uh oh!

guzu commented Oct 20, 2025

Uh oh!

stormi commented Oct 20, 2025

Uh oh!

guzu commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stormi commented Oct 20, 2025

Uh oh!

guzu commented Oct 21, 2025

Uh oh!

ydirson commented Oct 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydirson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzr Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

guzu commented Oct 20, 2025 •

edited

Loading

rzr Nov 25, 2025 •

edited

Loading