Conditionally include Redis connection information #1484

ekohl · 2021-07-13T09:52:23Z

Redis is only used when cache is enabled or RQ tasking is used. In other cases, the connection is not needed. We've changed the Foreman installer to only install Redis if needed, just to stop wasting resources. However, this leads to warnings.

This PR changes the status view to only include Redis if it's used.

Right now there are no tests since I first want to discuss if this is even the right direction. That's also why this PR is a draft.

https://pulp.plan.io/issues/9070

pulpbot · 2021-07-13T10:00:10Z

Attached issue: https://pulp.plan.io/issues/9070

ekohl · 2021-07-13T10:00:34Z

@dralley I saw you're assigned to https://pulp.plan.io/issues/8997 and I think this can be considered a blocker to that effort. We're now running into this. Please have a look.

pulpcore/app/views/status.py

dralley · 2021-07-13T13:58:08Z

A few concerns:

The status page will now change depending on what their settings declare, which is a little strange. What if a downstream user is parsing the status - it's not clear what data they should be expecting back. I think most status pages would simply list all components, optional or not.
re: theforeman/puppet-pulpcore@7a5543b
- I thought (re: ehelms email) that Foreman / Katello already did require Redis for other reasons (Dynflow / Sidekiq)? Is that not the case?
- if the goal is to only to avoid unnecessary resource usage, would it be possible to install Redis but leave it disabled, and make it socket-activated, so that it only comes online if needed?

ekohl · 2021-07-13T14:10:48Z

The status page will now change depending on what their settings declare, which is a little strange. What if a downstream user is parsing the status - it's not clear what data they should be expecting back. I think most status pages would simply list all components, optional or not.

This is indeed something I also thought about. However, I think the output format should clarify that it's optional then. Right now the output is (simplified):

{
  "redis_connection": {
    "connected": false
  }
}

So what should it then become if it's optional?

{
  "redis_connection": false
}

Or:

{
  "redis_connection": {
    "connected": false,
    "used": false
  }
}

* I thought (re: ehelms email) that Foreman / Katello already did require Redis for other reasons (Dynflow / Sidekiq)?  Is that not the case?

Foreman does indeed have Redis (but we support an external Redis as well) but on content proxies there is no Foreman so no Redis. That means we'll reduce resources if we don't install it. Fewer services is generally also appreciated by security folks.

* if the goal is to only to avoid unnecessary resource usage, wouldn't the best solution be to install Redis but leave it disabled, and make it socket-activated, so that it only comes online if needed?  Otherwise it seems more difficult  than necessary to enable/disable caching, because the components aren't there.

Our install can handle conditional inclusion, but doesn't handle clean up. I like to socket activation suggestion but I'm not sure if Redis supports that today. I'm looking into unix sockets anyway (theforeman/puppet-pulpcore#220) but it appears that now results in internal server errors. I'll need to dive into that.

daviddavis · 2021-07-13T19:23:43Z

I agree that we want to avoid having conditional keys in our response.

I have no objection to the two proposals you mention (just setting connection to false or adding a used key).

Another option is to return null or {} for redis_connection if Pulp is not using redis. This would be consistent with the storage key which returns "storage": null when Pulp is not using a filesystem.

dralley · 2021-07-13T19:36:20Z

Another option is to return null or {} for redis_connection if Pulp is not using redis. This would be consistent with the storage key which returns "storage": null when Pulp is not using a filesystem.

I'm in favor of this approach

ekohl · 2021-07-14T08:40:50Z

I like null: it's very straight forward and feels natural to me. I've updated the PR to implement this and added a test to verify it. I'm relying on CI to run tests but I think this is ready.

daviddavis · 2021-07-14T12:18:01Z

pulpcore/app/serializers/status.py

@@ -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,


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

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

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

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.

Sounds fine to me

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

daviddavis · 2021-07-14T12:19:01Z

CHANGES/9070.bugfix

@@ -0,0 +1,2 @@
+Remove Redis connection information from status unless it's used, either by RQ


I guess this is perhaps still valid? Maybe better to say "Return null for redis connection key..."

I debated it but this morning I couldn't think of a better text. Technically it sort of still removes it but I agree explicitly saying that it's null is clearer.

Updated. Let me know what you think of it now.

Redis is only used when cache is enabled or RQ tasking is used. In other cases, the connection is not needed. We've changed the Foreman installer to only install Redis if needed, just to stop wasting resources[1]. However, this leads to warnings. This PR changes the status view to only include Redis if it's used. [1]: theforeman/puppet-pulpcore@7a5543b closes #9070 https://pulp.plan.io/issues/9070

dralley · 2021-07-14T15:12:48Z

pulpcore/tests/functional/api/test_status.py

        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)


I had no idea this was a thing, neat!

ekohl · 2021-07-15T13:57:10Z

I should probably create an issue for this, but before I do: this changes the API. Does that disqualify it for a cherry pick to 3.14?

The reasoning is that with that we can avoid ever installing Redis in some scenarios.

daviddavis · 2021-07-15T16:50:38Z

This is a good question and I'm a bit torn. I'll add it to Tuesday's pulpcore meeting agenda for us to discuss and report back.

dralley · 2021-07-15T17:07:07Z

I'm also slightly conflicted, it's probably not something we would normally do - but on the other hand, it's not a particularly meaningful change but it is something that I assume Katello would probably want to add to their backport list.

And a lot of our users, most of them even, are using the Katello RPMs, so we don't necessarily want a divergence between the Katello 3.14 and the upstream 3.14 either. So that would be a reason in favor of merging it anyways.

ekohl · 2021-07-15T18:57:20Z

Thanks! I completely understand being torn because I'd be the same. Perhaps a good question is what @mdellweg said: will the bindings change? If it needs updated bindings, I can see the barrier being much higher. Let me know if I need to do anything.

dralley reviewed Jul 13, 2021

View reviewed changes

pulpcore/app/views/status.py Outdated Show resolved Hide resolved

ekohl force-pushed the 9070-remove-redis-from-status-if-unused branch 2 times, most recently from aa92697 to e4bc10a Compare July 14, 2021 08:37

ekohl marked this pull request as ready for review July 14, 2021 08:39

daviddavis reviewed Jul 14, 2021

View reviewed changes

ekohl force-pushed the 9070-remove-redis-from-status-if-unused branch from e4bc10a to c3db975 Compare July 14, 2021 14:28

daviddavis approved these changes Jul 14, 2021

View reviewed changes

dralley reviewed Jul 14, 2021

View reviewed changes

dralley approved these changes Jul 14, 2021

View reviewed changes

dralley merged commit 8ab8afb into pulp:master Jul 14, 2021

ekohl deleted the 9070-remove-redis-from-status-if-unused branch July 14, 2021 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally include Redis connection information #1484

Conditionally include Redis connection information #1484

ekohl commented Jul 13, 2021

pulpbot commented Jul 13, 2021

ekohl commented Jul 13, 2021

dralley commented Jul 13, 2021 •

edited

Loading

ekohl commented Jul 13, 2021

daviddavis commented Jul 13, 2021 •

edited

Loading

dralley commented Jul 13, 2021

ekohl commented Jul 14, 2021

daviddavis Jul 14, 2021

dralley Jul 14, 2021

ekohl Jul 14, 2021

ekohl Jul 14, 2021

dralley Jul 14, 2021

mdellweg Jul 15, 2021

daviddavis Jul 14, 2021

ekohl Jul 14, 2021

ekohl Jul 14, 2021

dralley Jul 14, 2021

ekohl commented Jul 15, 2021

daviddavis commented Jul 15, 2021

dralley commented Jul 15, 2021 •

edited

Loading

ekohl commented Jul 15, 2021

		@@ -0,0 +1,2 @@
		Remove Redis connection information from status unless it's used, either by RQ

Conditionally include Redis connection information #1484

Conditionally include Redis connection information #1484

Conversation

ekohl commented Jul 13, 2021

pulpbot commented Jul 13, 2021

ekohl commented Jul 13, 2021

dralley commented Jul 13, 2021 • edited Loading

ekohl commented Jul 13, 2021

daviddavis commented Jul 13, 2021 • edited Loading

dralley commented Jul 13, 2021

ekohl commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Jul 15, 2021

daviddavis commented Jul 15, 2021

dralley commented Jul 15, 2021 • edited Loading

ekohl commented Jul 15, 2021

dralley commented Jul 13, 2021 •

edited

Loading

daviddavis commented Jul 13, 2021 •

edited

Loading

dralley commented Jul 15, 2021 •

edited

Loading