Poll ncat tunnel pid

[dosas]

Problem Statement

when checking the pid right after ncat startup
it could happen that no pid was found despite ncat being started

Solution

polling the ncat pid

Some tests using the feature

• tests/foreman/cli/test_organization.py::test_positive_add_and_remove_capsules
• tests/foreman/api/test_location.py::TestLocation::test_positive_create_update_and_remove_capsule

[omkarkhatavkar]

Can one of the admins verify this patch?

[dosas]

trigger: test-robottelo
pytest: tests/foreman/cli/test_organization.py::test_positive_add_and_remove_capsules

[rplevka]

Mind leveraging wait_for in the retry logic?

btw, Am a bit surprised that we got this far with the pid listing diff logic. Looks quite race-condition-prone to me now. If we had multiple (xdist) tests running against the same machine (that is configurable), such check could be easily fooled by listing other xdist worker's PIDs.
However, since we hadn't run into any issues for so many years, I'd not fiddle with it ;)

[dosas]

Mind leveraging wait_for in the retry logic?

Not at all.

btw, Am a bit surprised that we got this far with the pid listing diff logic. Looks quite race-condition-prone to me now. If we had multiple (xdist) tests running against the same machine (that is configurable), such check could be easily fooled by listing other xdist worker's PIDs. However, since we hadn't run into any issues for so many years, I'd not fiddle with it ;)

Yes, now that you mentioned it. Probably the session (broker) should take care of keeping track of started processes.

[rplevka]

Mind leveraging wait_for in the retry logic?

Not at all.

btw, Am a bit surprised that we got this far with the pid listing diff logic. Looks quite race-condition-prone to me now. If we had multiple (xdist) tests running against the same machine (that is configurable), such check could be easily fooled by listing other xdist worker's PIDs. However, since we hadn't run into any issues for so many years, I'd not fiddle with it ;)

Yes, now that you mentioned it. Probably the session (broker) should take care of keeping track of started processes.

I'd rather make the check to look at the target port directly, as that's what we ultimately wait for. There still would be some room for race conditions, but the chances of hitting one might be much, much slimmer.
(example scenario of a possible rc would be: worker1 tries to spawn ncat to listen to randomly selected port 123456 but ncat fails to start the process for immediately, while worker2 accidentally chooses the same port and succeeds - this would lead to both workers to 'think', their ncat command was successful). One way around this could be to implement some sort of lockfile (e.g. function could create a file .ncat_<port_number> in a tempdir, after it chooses an available port and the same function would also check for the eventual lockfiles inside the port-picking logic).

Anyway, as I mentioned before - as long as nobody has hit the RC yet, it's probably a corner-case scenario (perhaps it might be worth adding a comment about it, so it's not lost) and i would not over-engineer it.

[omkarkhatavkar]

trigger: test-robottelo
pytest: tests/foreman/cli/test_organization.py::test_positive_add_and_remove_capsules

[rplevka]

trigger: test-robottelo
pytest: tests/foreman/cli/test_organization.py::test_positive_add_and_remove_capsules

[Satellite-QE]

PRT Result

Build Number: 6947
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_organization.py::test_positive_add_and_remove_capsules --external-logging
Test Result : ================== 1 passed, 9 warnings in 848.81s (0:14:08) ===================

[rplevka]

@lpramuk mind giving this a review?
I am fine with an extra wait_for.