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.

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

Jump to bottom

configd: T6608: report uncaught config script exceptions as commit error #3876

Open
wants to merge 1 commit into
base: current
Choose a base branch
from
Open

configd: T6608: report uncaught config script exceptions as commit error #3876

wants to merge 1 commit into from

Conversation

jestabro
Copy link
Contributor

@jestabro jestabro commented Jul 25, 2024 •
edited
Loading

Change Summary

Exceptions uncaught by config scripts should be caught by configd and returned as commit error. The previous behavior of configd was to catch and report as a daemon error to the shim, allowing for the possibility of a pass-through execution (as with any other daemon originating error). The introduction of config dependencies, however, requires the return of a commit error.

The error output returns the traceback, consistent with the output running without configd; simple test case below for comparison:

with configd:

vyos@vyos# commit
[ simple some-tag-node some0 ]
Traceback (most recent call last):
  File "/usr/libexec/vyos/services/vyos-configd", line 134, in run_script
    script.verify(c)
  File "/usr/libexec/vyos//conf_mode/simple.py", line 50, in verify
    raise OSError('This is an uncaught exception')
OSError: This is an uncaught exception

[[simple some-tag-node some0]] failed
Commit failed

without configd:

vyos@vyos# set simple some-tag-node some0
[edit]
vyos@vyos# commit
[ simple some-tag-node some0 ]
Traceback (most recent call last):
  File "/usr/libexec/vyos/conf_mode/simple.py", line 64, in <module>
    verify(c)
  File "/usr/libexec/vyos/conf_mode/simple.py", line 50, in verify
    raise OSError('This is an uncaught exception')
OSError: This is an uncaught exception

[[simple some-tag-node some0]] failed
Commit failed

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)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

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

@jestabro jestabro self-assigned this Jul 25, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 25, 2024 •
edited
Loading

👍
No issues in PR Title / Commit Title

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 25, 2024 •
edited
Loading

✅ No issues found in unused-imports check.. Please refer the workflow run

@jestabro jestabro force-pushed the uncaught-conf-script-err-as-commit-err branch 6 times, most recently from 7bf3538 to eec5e75 Compare July 30, 2024 15:05
@github-actions GitHub Actions
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed

@jestabro
Copy link
Contributor Author

This PR will be split into separate submissions for (1) those bugs obscured by the design choice described in T6608, and (2) the actual change in behavior (this PR) from pass-through to error on exceptions uncaught by config mode scripts. This PR will be moved from draft after the two or three instances in (1) are merged.

@jestabro jestabro force-pushed the uncaught-conf-script-err-as-commit-err branch from eec5e75 to be47832 Compare July 31, 2024 16:23
@jestabro
Copy link
Contributor Author

jestabro commented Aug 7, 2024

This will be moved out of draft following
#3955
#3937
and resolution of T6638, which manifests in configtest vrf-bgp-pppoe-underlay

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@jestabro jestabro force-pushed the uncaught-conf-script-err-as-commit-err branch from be47832 to f9fa72e Compare September 21, 2024 01:47
@github-actions GitHub Actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@jestabro jestabro force-pushed the uncaught-conf-script-err-as-commit-err branch from f9fa72e to 68d80dc Compare September 21, 2024 03:19
@jestabro
configd: T6608: report uncaught config script exceptions as commit error
5034db8 
In the case of config mode script exceptions other than ConfigError,
vyos-configd would previously trigger the shim to re-run the script in
the CLI context. The use of config dependencies require this case to
return a commit error. A traceback is returned as output, consistent
with running without vyos-configd support.
@jestabro jestabro force-pushed the uncaught-conf-script-err-as-commit-err branch from 68d80dc to 5034db8 Compare September 22, 2024 14:23
@jestabro jestabro marked this pull request as ready for review September 22, 2024 19:55
@jestabro jestabro requested a review from a team as a code owner September 22, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sever-sever sever-sever sever-sever approved these changes

@dmbaturin dmbaturin Awaiting requested review from dmbaturin dmbaturin is a code owner automatically assigned from vyos/reviewers

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev is a code owner automatically assigned from vyos/reviewers

@zdc zdc Awaiting requested review from zdc zdc is a code owner automatically assigned from vyos/reviewers

@c-po c-po Awaiting requested review from c-po c-po is a code owner automatically assigned from vyos/reviewers

@nicolas-fort nicolas-fort Awaiting requested review from nicolas-fort nicolas-fort is a code owner automatically assigned from vyos/reviewers

At least 2 approving reviews are required to merge this pull request.

Assignees

@jestabro jestabro

Labels
current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jestabro @sever-sever