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

ddclient: T5791: Keep ddclient.service in foreground #4276

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

indrajitr
Copy link
Contributor

Change summary

Since the distributed ddclient.service is of type 'exec' now, avoid using process forking and let systemd manage the process directly.

This might help spurious issues with smoketest as well.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T5791

Related PR(s)

dns dynamic

How to test / Smoketest result

vyos@vyos-0104:~$ /usr/libexec/vyos/tests/smoke/cli/test_service_dns_dynamic.py
test_01_dyndns_service_standard (__main__.TestServiceDDNS.test_01_dyndns_service_standard) ... ok
test_02_dyndns_service_ipv6 (__main__.TestServiceDDNS.test_02_dyndns_service_ipv6) ... ok
test_03_dyndns_service_dual_stack (__main__.TestServiceDDNS.test_03_dyndns_service_dual_stack) ... ok
test_04_dyndns_rfc2136 (__main__.TestServiceDDNS.test_04_dyndns_rfc2136) ... ok
test_05_dyndns_hostname (__main__.TestServiceDDNS.test_05_dyndns_hostname) ... ok
test_06_dyndns_web_options (__main__.TestServiceDDNS.test_06_dyndns_web_options) ... ok
test_07_dyndns_dynamic_interface (__main__.TestServiceDDNS.test_07_dyndns_dynamic_interface) ... ok
test_08_dyndns_vrf (__main__.TestServiceDDNS.test_08_dyndns_vrf) ... ok

----------------------------------------------------------------------
Ran 8 tests in 325.930s

OK

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

Copy link

github-actions bot commented Jan 4, 2025

👍
No issues in PR Title / Commit Title

@c-po
Copy link
Member

c-po commented Jan 5, 2025

Did you validate smoketests? The immadiately fail for me:

DDCLIENT test - run #2 previous good/bad (0/1)
test_01_dyndns_service_standard (__main__.TestServiceDDNS.test_01_dyndns_service_standard) ... FAIL

======================================================================
FAIL: test_01_dyndns_service_standard (__main__.TestServiceDDNS.test_01_dyndns_service_standard)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/libexec/vyos/tests/smoke/cli/test_service_dns_dynamic.py", line 54, in tearDown
    self.assertFalse(process_named_running(DDCLIENT_PNAME))
AssertionError: 5232 is not false

----------------------------------------------------------------------
Ran 1 test in 23.499s

FAILED (failures=1)

Using:

#!/bin/sh

good=0
bad=0
for ii in $(seq 1 1000)
do
    echo "DDCLIENT test - run #$ii previous good/bad ($good/$bad)"
    /usr/libexec/vyos/tests/smoke/cli/test_service_dns_dynamic.py
    if [ "x$?" != "x0" ]; then
        bad=$((bad+1))
        continue
    fi
    good=$((good+1))
done

@c-po
Copy link
Member

c-po commented Jan 6, 2025

@indrajitr my bad - upon debugging the issue further I noticed the following process running: <bound method Process.name of psutil.Process(pid=15383, name='ddclient.sh', status='sleeping', started='11:46:41')>

So my testcase test broke the test, as my script container ddclient in it's name 🤦‍♂️

indrajitr and others added 2 commits January 6, 2025 11:52
Since the distributed ddclient.service is of type 'exec' now, avoid using
process forking and let systemd manage the process directly.
@c-po c-po force-pushed the ddclient-process-2025-01-04 branch from fd2d452 to dbf42ed Compare January 6, 2025 10:53
Copy link

github-actions bot commented Jan 6, 2025

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

@c-po
Copy link
Member

c-po commented Jan 6, 2025

Fix looks promising

image

@dmbaturin dmbaturin merged commit 53cb3e5 into vyos:current Jan 6, 2025
13 of 14 checks passed
@indrajitr indrajitr deleted the ddclient-process-2025-01-04 branch January 6, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants