src/mon/ConnectionTracker.cc: Fix dump function #57146

kamoltat · 2024-04-30T04:41:25Z

Problem:

Currently, the ConnectionTracker::dump()
will dump a duplicate key which is not
ideal when you want to write a test that
converts the dump into a JSON since
each key must be unique. Basically without resolving this,
you cannot do something like:
json.load(connection_score) because the dump is not a valid JSON format

e.g.,

ceph daemon mon.a connection scores dump

peer is a duplicate key.

"peer_scores": {
    "peer": {
        "peer_rank": 0,
        "peer_score": 0.99966364610282044,
        "peer_alive": true
    },
    "peer": {
        "peer_rank": 2,
        "peer_score": 0.9941396935402752,
        "peer_alive": false
    }
}

report is a duplicate key

      "report": {
            "rank": 0,
            "epoch": 6,
            "version": 3940,
            "peer_scores": {...}
        },
        "report": {
            "rank": 1,
            "epoch": 6,
            "version": 345,
            "peer_scores": {...}
        },
        "report": {
            "rank": 2,
            "epoch": 6,
            "version": 84,
            "peer_scores": {...}
        }

Solution:
Use open_array_section and convert
peer_scores and reports into an
array instead.

peer is no longer a duplicate.

"peer_scores": [
   {
        "peer_rank": 0,
        "peer_score": 0.99966364610282044,
        "peer_alive": true
    },
    {
        "peer_rank": 2,
        "peer_score": 0.9941396935402752,
        "peer_alive": false
    }
]

report is no longer a duplicate

"reports": [
       {
            "rank": 0,
            "epoch": 6,
            "version": 3940,
            "peer_scores": {...}
        },
       {
            "rank": 1,
            "epoch": 6,
            "version": 345,
            "peer_scores": {...}
        },
        {
            "rank": 2,
            "epoch": 6,
            "version": 84,
            "peer_scores": {...}
        }
]

Test:
Added some tests to verify that our connection score is clean and to also verify that we can actually load the dump into a JSON format.

Fixes: https://tracker.ceph.com/issues/65695

Signed-off-by: Kamoltat ksirivad@redhat.com

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)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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

Problem: Currently, the ConnectionTracker::dump() will dump a duplicate key which is not ideal when you want to write a test that converts the dump into a JSON since JSON objects are key-value pairs where each key must be unique. Solution: Use open_array_section and convert `peer_scores` and `reports` into an array instead. Fixes: https://tracker.ceph.com/issues/65695 Signed-off-by: Kamoltat <ksirivad@redhat.com>

kamoltat · 2024-04-30T14:24:55Z

teuthology run on the newly added integration test:
/ksirivad-2024-04-30_06:05:19-rados:singleton-wip-ksirivad-fix-connection-score-json-distro-default-smithi/

1/1 Passed

kamoltat · 2024-04-30T14:35:46Z

jenkins test api

kamoltat · 2024-04-30T14:56:57Z

teuthology test:
https://pulpito.ceph.com/ksirivad-2024-04-30_14:27:04-rados:singleton-wip-ksirivad-fix-connection-score-json-distro-default-smithi/

15/15 Passed

ronen-fr

LGTM apart from a questionable case (see comments)

src/mon/ConnectionTracker.cc

qa/tasks/ceph_test_case.py

ronen-fr · 2024-05-07T15:27:44Z

qa/tasks/ceph_test_case.py

+                    return
+            else:
+                if elapsed >= timeout:
+                    if check_fn and check_fn() and retry_count < 5:


check_fn is allowed to be null, right? I am not sure that's handled correctly

yep, it's allowed to be null, basically, the check_fn part allows the function to keep looping if we determine that the actual thing that we are checking is making progress.

What I was trying to ask:
suppose there's no check_fn. I wanted to make sure you really meant to ignore the ' retry_count'
(I guess that makes sense, anyway).

qa/tasks/mon_connection_score.py

Basically when we deploy a 3 MONS Check if the connection scores are clean with a 60 seconds grace period Fixes: https://tracker.ceph.com/issues/65695 Signed-off-by: Kamoltat <ksirivad@redhat.com>

kamoltat · 2024-05-09T21:13:18Z

@ronen-fr all of your comments should be addressed in the new commit, let me know what you think! Thank you so much for your review!

ronen-fr

(note my followup question)

kamoltat requested a review from a team as a code owner April 30, 2024 04:41

github-actions bot added core mon tests labels Apr 30, 2024

kamoltat self-assigned this Apr 30, 2024

kamoltat added the needs-review label Apr 30, 2024

kamoltat force-pushed the wip-ksirivad-fix-connection-score-json branch 2 times, most recently from d0f55ef to 143fe58 Compare May 2, 2024 20:18

kamoltat requested a review from gregsfortytwo May 6, 2024 14:45

ronen-fr reviewed May 7, 2024

View reviewed changes

qa: Added mon connection score tests

842f8b3

Basically when we deploy a 3 MONS Check if the connection scores are clean with a 60 seconds grace period Fixes: https://tracker.ceph.com/issues/65695 Signed-off-by: Kamoltat <ksirivad@redhat.com>

kamoltat force-pushed the wip-ksirivad-fix-connection-score-json branch from 143fe58 to 842f8b3 Compare May 9, 2024 21:12

ronen-fr approved these changes May 10, 2024

View reviewed changes

kamoltat added needs-qa and removed needs-review labels May 10, 2024

ljflores added the wip-yuri7-testing label May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src/mon/ConnectionTracker.cc: Fix dump function #57146

src/mon/ConnectionTracker.cc: Fix dump function #57146

kamoltat commented Apr 30, 2024 •

edited

kamoltat commented Apr 30, 2024 •

edited

kamoltat commented Apr 30, 2024

kamoltat commented Apr 30, 2024

ronen-fr left a comment

ronen-fr May 7, 2024

kamoltat May 9, 2024 •

edited

ronen-fr May 10, 2024

kamoltat commented May 9, 2024

ronen-fr left a comment

src/mon/ConnectionTracker.cc: Fix dump function #57146

Are you sure you want to change the base?

src/mon/ConnectionTracker.cc: Fix dump function #57146

Conversation

kamoltat commented Apr 30, 2024 • edited

Contribution Guidelines

Checklist

kamoltat commented Apr 30, 2024 • edited

kamoltat commented Apr 30, 2024

kamoltat commented Apr 30, 2024

ronen-fr left a comment

Choose a reason for hiding this comment

ronen-fr May 7, 2024

Choose a reason for hiding this comment

kamoltat May 9, 2024 • edited

Choose a reason for hiding this comment

ronen-fr May 10, 2024

Choose a reason for hiding this comment

kamoltat commented May 9, 2024

ronen-fr left a comment

Choose a reason for hiding this comment

kamoltat commented Apr 30, 2024 •

edited

kamoltat commented Apr 30, 2024 •

edited

kamoltat May 9, 2024 •

edited