Skip to content

pybind/mgr: disable sqlite3/python autocommit#56997

Merged
batrick merged 12 commits intoceph:mainfrom
batrick:i65494
Apr 30, 2024
Merged

pybind/mgr: disable sqlite3/python autocommit#56997
batrick merged 12 commits intoceph:mainfrom
batrick:i65494

Conversation

@batrick
Copy link
Member

@batrick batrick commented Apr 19, 2024 •
edited
Loading

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@batrick batrick requested a review from a team as a code owner April 19, 2024 03:37
phlogistonjohn
phlogistonjohn reviewed Apr 19, 2024
View reviewed changes
@batrick batrick force-pushed the i65494 branch 2 times, most recently from 238ef88 to 0694828 Compare April 19, 2024 18:32
@batrick
Copy link
Member Author

batrick commented Apr 19, 2024 


2024-04-19 14:45:17,824.824 INFO:__main__:----------------------------------------------------------------------
2024-04-19 14:45:17,824.824 INFO:__main__:Traceback (most recent call last):
2024-04-19 14:45:17,824.824 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/dashboard/helper.py", line 483, in assertSchema
2024-04-19 14:45:17,824.824 INFO:__main__:    return _validate_json(data, schema)
2024-04-19 14:45:17,824.824 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/dashboard/helper.py", line 710, in _validate_json
2024-04-19 14:45:17,825.825 INFO:__main__:    raise _ValError('unknown keys: {}'.format(unknown_keys), path)
2024-04-19 14:45:17,825.825 INFO:__main__:tasks.mgr.dashboard.helper._ValError: In `input`: unknown keys: {'sqlite3_killpoint'}
2024-04-19 14:45:17,825.825 INFO:__main__:
2024-04-19 14:45:17,825.825 INFO:__main__:During handling of the above exception, another exception occurred:
2024-04-19 14:45:17,825.825 INFO:__main__:
2024-04-19 14:45:17,825.825 INFO:__main__:Traceback (most recent call last):
2024-04-19 14:45:17,825.825 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/dashboard/test_mgr_module.py", line 123, in test_module_options
2024-04-19 14:45:17,825.825 INFO:__main__:    self.assertSchema(data, schema)
2024-04-19 14:45:17,825.825 INFO:__main__:  File "/home/jenkins-build/build/workspace/ceph-api/qa/tasks/mgr/dashboard/helper.py", line 485, in assertSchema
2024-04-19 14:45:17,825.825 INFO:__main__:    self.assertEqual(data, str(e))
2024-04-19 14:45:17,826.826 INFO:__main__:AssertionError: {'channel_basic': {'name': 'channel_basic[4897 chars] []}} != "In `input`: unknown keys: {'sqlite3_killpoint'}"
2024-04-19 14:45:17,826.826 INFO:__main__:
2024-04-19 14:45:17,826.826 INFO:__main__:> ip netns list
2024-04-19 14:45:17,830.830 INFO:__main__:> sudo ip link delete ceph-brx
Cannot find device "ceph-brx"
2024-04-19 14:45:17,845.845 INFO:__main__:
2024-04-19 14:45:17,845.845 INFO:__main__:----------------------------------------------------------------------
2024-04-19 14:45:17,845.845 INFO:__main__:

@epuertat rather than adding this module option which is for development, I'm wondering if it makes sense to have the dashboard filter out module options which are level "dev"? I've added a commit to allow setting the Option's level but I think adjustments need made to the dashboard. Any pointers?

@batrick
Copy link
Member Author

batrick commented Apr 20, 2024

This PR is under test in https://tracker.ceph.com/issues/65596.

epuertat
epuertat requested changes Apr 22, 2024
View reviewed changes
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@batrick the failure, I guess, is related to this schema validation. You need to udpate that to reflect the new Option interface.

@batrick batrick force-pushed the i65494 branch 8 times, most recently from 0782bca to 1f44705 Compare April 24, 2024 17:37
@batrick batrick requested a review from a team as a code owner April 24, 2024 17:37
@batrick batrick requested review from aaSharma14 and removed request for a team April 24, 2024 17:37
@batrick batrick requested a review from epuertat April 24, 2024 21:06
@batrick
Copy link
Member Author

batrick commented Apr 25, 2024

This PR is under test in https://tracker.ceph.com/issues/65661.

epuertat
epuertat approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

src/common/options.h
Comment on lines +119 to +127
static level_t str_to_level(std::string_view s) {
if (s == "basic") {
return LEVEL_BASIC;
} else if (s == "advanced") {
return LEVEL_ADVANCED;
} else if (s == "dev") {
return LEVEL_DEV;
} else {
return LEVEL_UNKNOWN;
Copy link
Member

@epuertat epuertat Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed, right?

Copy link
Member Author

@batrick batrick Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I left it in anyway.

batrick added 10 commits April 29, 2024 12:22
@batrick
mon/MgrMonitor: only drop active if exists
3fca118 
This avoids an assertion in ::drop_active.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
mon/MgrMonitor: promote standby when unsetting down flag
e044140 
In the same epoch, to simplify testing.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
mgr: allow specifying module option level
0d94eeb 
Some are for development purposes and should be filtered out by the dashboard.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
pybind/mgr: add killpoint for sqlite3 database setup
fdd0dde 
To catch unintentional autocommit by sqlite3.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
qa: add missing terminating newline
8ac4bbc 
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
qa: use position-independent link
1749edd 
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
qa: reorganize mgr unit tests
2f48dc9 
Refactor common tasks and allow loading mgrmodules before unittests start.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
qa/suites/rados/mgr: add cli/devicehealth tasks
440f25e 
These should have been part of the commit adding the tests.

Fixes: 9ebcbdb
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
qa/tasks/vstart_runner: add missing poll method
9748d0c 
Otherwise you cannot use LocalDaemon.check_status.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
qa/tasks/vstart_runner: run daemons in foreground
e2e2144 
This mirrors teuthology and makes it possible to check the exit status of a
daemon.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
batrick added a commit to batrick/ceph that referenced this pull request Apr 29, 2024
@batrick
Merge PR ceph#56997 into wip-pdonnell-testing-20240425.015853-debug
92be601 
* refs/pull/56997/head:
	pybind/mgr: disable sqlite3/python autocommit
	qa/tasks/mgr: add tests for sqlite autocommit
	qa/tasks/vstart_runner: run daemons in foreground
	qa/tasks/vstart_runner: add missing poll method
	qa/suites/rados/mgr: add cli/devicehealth tasks
	qa: reorganize mgr unit tests
	qa: use position-independent link
	qa: add missing terminating newline
	pybind/mgr: add killpoint for sqlite3 database setup
	mgr: allow specifying module option level
	mon/MgrMonitor: promote standby when unsetting down flag
	mon/MgrMonitor: only drop active if exists
batrick added 2 commits April 29, 2024 16:33
@batrick
qa/tasks/mgr: add tests for sqlite autocommit
fb82b6d 
That autocommit is properly turned off and that commits via context managers
work as expected.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
pybind/mgr: disable sqlite3/python autocommit
6eacfe9 
SQLite3 and python's binding were both committing transactions at unintended
points. Turn it off and stop using executescript.

Fixes: https://tracker.ceph.com/issues/65494
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@batrick
Copy link
Member Author

batrick commented Apr 29, 2024

This PR is under test in https://tracker.ceph.com/issues/65694.

@batrick
Copy link
Member Author

batrick commented Apr 30, 2024

jenkins test make check

@batrick
Copy link
Member Author

batrick commented Apr 30, 2024

jenkins test api

@batrick
Copy link
Member Author

batrick commented Apr 30, 2024

jenkins test dashboard

@batrick
Copy link
Member Author

batrick commented Apr 30, 2024

jenkins test make check arm64

1 similar comment
@batrick
Copy link
Member Author

batrick commented Apr 30, 2024

jenkins test make check arm64

@batrick batrick merged commit 07afb4a into ceph:main Apr 30, 2024
@batrick batrick deleted the i65494 branch April 30, 2024 20:46
This was referenced Apr 30, 2024
Merged
Merged
Merged
@hswong3i
Copy link
Contributor

hswong3i commented Nov 19, 2024

Since this PR merged into ceph-ceph-17.2.8 but I face error as reported with #57199 (comment), but NOT affecting ceph-ceph-18.2.4 NOR ceph-ceph-19.2.0 (which not yet backport PR merged), doesn't that means something goes wrong for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phlogistonjohn phlogistonjohn phlogistonjohn left review comments

@epuertat epuertat epuertat approved these changes

@aaSharma14 aaSharma14 Awaiting requested review from aaSharma14 aaSharma14 is a code owner automatically assigned from ceph/dashboard

@nizamial09 nizamial09 Awaiting requested review from nizamial09 nizamial09 is a code owner automatically assigned from ceph/dashboard

Assignees

No one assigned

Labels

common core dashboard mgr mon needs-review pybind tests wip-pdonnell-testing2

Projects

Ceph-Dashboard
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@batrick @hswong3i @phlogistonjohn @epuertat