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

Conditionally include Redis connection information #1484

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Conditionally include Redis connection information #1484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES/9070.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Set Redis connection information in status to null unless it's used. Redis is
needed for RQ tasking or content caching.
5 changes: 4 additions & 1 deletion pulpcore/app/serializers/status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ class StatusSerializer(serializers.Serializer):
help_text=_("Database connection information")
)

redis_connection = RedisConnectionSerializer(help_text=_("Redis connection information"))
redis_connection = RedisConnectionSerializer(
required=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming you can't return null unless required=False?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought required was only used for creating resources. It usually allows the non-presence of the entire key rather than affecting nullability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must admit I didn't even check it and assumed it. Note that storage also already is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For completeness, from a current Pulp 3.14 without this patch. When I go to /pulp/api/v3 using a browser (text/html):

    StatusResponse:
      type: object
      description: Serializer for the status information of the app
      properties:
        versions:
          type: array
          items:
            $ref: '#/components/schemas/VersionResponse'
          description: Version information of Pulp components
        online_workers:
          type: array
          items:
            $ref: '#/components/schemas/WorkerResponse'
          description: List of online workers known to the application. An online
            worker is actively heartbeating and can respond to new work
        online_content_apps:
          type: array
          items:
            $ref: '#/components/schemas/ContentAppStatusResponse'
          description: List of online content apps known to the application. An online
            content app is actively heartbeating and can serve data to clients
        database_connection:
          allOf:
          - $ref: '#/components/schemas/DatabaseConnectionResponse'
          description: Database connection information
        redis_connection:
          allOf:
          - $ref: '#/components/schemas/RedisConnectionResponse'
          description: Redis connection information
        storage:
          allOf:
          - $ref: '#/components/schemas/StorageResponse'
          description: Storage information
      required:
      - database_connection
      - online_content_apps
      - online_workers
      - redis_connection
      - versions

Note storage is not in required. While it may describe inputs, it is the closest thing there is to also describe the output. That's why I think it still makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds fine to me

Copy link
Member

Choose a reason for hiding this comment

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

The thing to check, IMHO is whether the bindings can parse the result without error.

help_text=_("Redis connection information"),
)

storage = StorageSerializer(required=False, help_text=_("Storage information"))
6 changes: 5 additions & 1 deletion pulpcore/app/views/status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,11 @@ def get(self, request):
for app in pulp_plugin_configs():
versions.append({"component": app.label, "version": app.version})

redis_status = {"connected": self._get_redis_conn_status()}
if settings.CACHE_ENABLED or not settings.USE_NEW_WORKER_TYPE:
redis_status = {"connected": self._get_redis_conn_status()}
else:
redis_status = None

db_status = {"connected": self._get_db_conn_status()}

try:
Expand Down
11 changes: 11 additions & 0 deletions pulpcore/tests/functional/api/test_status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Test the status page."""
import unittest

from django.test import override_settings
from jsonschema import validate
from pulp_smash import api, cli, config, utils
from pulp_smash.pulp3.constants import STATUS_PATH
Expand Down Expand Up @@ -91,10 +92,20 @@ def verify_get_response(self, status):
"""
validate(status, self.status_response)
self.assertTrue(status["database_connection"]["connected"])
self.assertIsNotNone(status["redis_connection"])
self.assertTrue(status["redis_connection"]["connected"])
self.assertNotEqual(status["online_workers"], [])
self.assertNotEqual(status["versions"], [])
if self.storage == "pulpcore.app.models.storage.FileSystem":
self.assertIsNotNone(status["storage"])
else:
self.assertIsNone(status["storage"])

@override_settings(CACHE_ENABLED=False, USE_NEW_WORKER_TYPE=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I had no idea this was a thing, neat!

def verify_get_response_without_redis(self, status):
"""Verify the response to an HTTP GET call when Redis is not used.

Verify that redis_connection is null
"""
validate(status, self.status_response)
self.assertIsNone(status["redis_connection"])