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

Enable support for unicast messages #417

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

JenySadadia
Copy link
Collaborator

Use Redis list for enabling support for unicast messages for load-balancing use-case.
Introduced endpoints /push and /pop for pushing messages to a Redis list and popping a message from the
list respectively.

Drop `aioredis` package as the project has been archived and
it is now a part of `redis` package. Add `redis` to
`requirementes.txt` file. To maintain compatibilities between
packages, update `fakeredis` version in `requirements-tests.txt`.

Signed-off-by: Jeny Sadadia <[email protected]>
@JenySadadia
Copy link
Collaborator Author

I have been investigating unit test failure in test_pubsub.
Somehow after changing fakeredis version to 2.20.0, redis mock stays the same for all the tests even if mock_pubsub fixture has the default scope function. That's why value of _redis.ID_KEY key becomes global among all the tests and pubsub.subscribe increments it based on the value from the previous test run.

@pytest.fixture
def mock_pubsub(mocker):
    """Mocks `_redis` member of PubSub class instance"""
    pubsub = PubSub()
    redis_mock = fakeredis.aioredis.FakeRedis()
    mocker.patch.object(pubsub, '_redis', redis_mock)
    return pubsub

First, test_subscribe_single_channel test will be run and _redis.ID_KEY will become 1 after the execution.

async def test_subscribe_single_channel(mock_pubsub):
    """
    Test Case: Subscribe for one channel with a PubSub.subscribe() method.

    Expected Result:
        Subscription-like object is returned from PubSub.subscribe() method
        with channel set to 'CHANNEL' and id set to 1.
        PubSub._subscriptions dict should have one entry. This entry's
        key should be equal 1.
    """
    result = await mock_pubsub.subscribe('CHANNEL')
    assert result.channel == 'CHANNEL'
    assert result.id == 1
    assert len(mock_pubsub._subscriptions) == 1
    assert 1 in mock_pubsub._subscriptions

When test_subscribe_multiple_channels executes after it, mock_pubsub._redis doesn't get re-instantiated and uses the same instance from the previous test. This results in test failure as _redis.ID_KEY has been set to 1 in the previous run and now it becomes 2.

async def test_subscribe_multiple_channels(mock_pubsub):
    """
    Test Case: Subscribe for three channels with subsequent calls of
    PubSub.subscribe() method.

    Expected Result:
        Subscription-like objects are returned for each call of
        PubSub.subscribe() method
        Subsequent calls should have channel names: 'CHANNEL1', 'CHANNEL2',
        'CHANNEL3' and should have ids 1, 2, 3 respectively.
        PubSub._subscriptions dict should have 2 entries. This entries'
        keys should be 1, 2, and 3.
    """
    channels = ((1, 'CHANNEL1'), (2, 'CHANNEL2'), (3, 'CHANNEL3'))
    for expected_id, expected_channel in channels:
        result = await mock_pubsub.subscribe(expected_channel)
        assert result.channel == expected_channel
        assert result.id == expected_id
    assert len(mock_pubsub._subscriptions) == 3
    assert (1, 2, 3) == tuple(mock_pubsub._subscriptions.keys())

This seems some kind of mock leak.

@JenySadadia
Copy link
Collaborator Author

It fixed the issue when I reset ID_KEY in test_subscribe_multiple_channels but need to investigate the root cause.

await mock_pubsub._redis.set(mock_pubsub.ID_KEY, 0)`

@JenySadadia JenySadadia marked this pull request as ready for review November 24, 2023 12:28
@JenySadadia JenySadadia requested a review from gctucker November 24, 2023 12:29
@JenySadadia
Copy link
Collaborator Author

The issue I am facing is somewhat similar to what explained here: https://stackoverflow.com/questions/75112530/pytest-monkeypatch-mock-leak

Copy link
Contributor

@gctucker gctucker left a comment

Choose a reason for hiding this comment

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

LGTM - The only issue is what we discussed in a meeting last week, there needs to be a way to close the queue or we'll have resource leaks in Redis. That's why I suggested to have the open / close endpoints (and kci commands). I'll create a GitHub issue for it.

api/pubsub.py Outdated Show resolved Hide resolved
Jeny Sadadia added 3 commits November 28, 2023 17:12
Add methods to `PubSub` class to push messages to Redis List,
pop message from the list, and push `CloudEvent` to the list.

Signed-off-by: Jeny Sadadia <[email protected]>
In order to enable support for unicast messages,
introduce endpoints `/push` and `/pop` for
pushing a message to a Redis list and popping a message from the
list respectively. This is to serve load-balancing purpose
by sending message to a single consumer.

Signed-off-by: Jeny Sadadia <[email protected]>
Somehow after changing version of `fakeredis`, redis mock stays
the same for all the unit tests even if `mock_pubsub` fixture has the
default scope `function`. That's why value of `PubSub._redis.ID_KEY`
becomes global among all the tests and `pubsub.subscribe` increments
it based on the value from the previous test run.
Fix the issue by resetting `ID_KEY` value to get subscription ID
starting from 1 in `test_subscribe_multiple_channels`.

Signed-off-by: Jeny Sadadia <[email protected]>
@JenySadadia JenySadadia force-pushed the pubsub-single-consumer branch from 1a73daf to 5c17c88 Compare November 28, 2023 11:42
@JenySadadia JenySadadia requested a review from gctucker November 28, 2023 11:43
Copy link
Contributor

@gctucker gctucker left a comment

Choose a reason for hiding this comment

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

OK following the merge-and-fix approach to untangle the various pending PRs and conflicts with kernelci-core related changes.

@gctucker gctucker added this pull request to the merge queue Dec 5, 2023
Merged via the queue into kernelci:main with commit 870415b Dec 5, 2023
4 checks passed
@JenySadadia JenySadadia deleted the pubsub-single-consumer branch December 5, 2023 09:58
@JenySadadia JenySadadia linked an issue Dec 7, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for single consumers of pub/sub events
2 participants