Skip to content

Add Redis password support to KV cache scorer#131

Closed
relyt0925 wants to merge 1 commit intollm-d:mainfrom
relyt0925:redis-password
Closed

Add Redis password support to KV cache scorer#131
relyt0925 wants to merge 1 commit intollm-d:mainfrom
relyt0925:redis-password

Conversation

@relyt0925
Copy link
Contributor

Introduced handling for a new environment variable KVCACHE_INDEXER_REDIS_PWD to enable optional Redis password configuration. This improves flexibility for connecting to secured Redis instances.

Introduced handling for a new environment variable `KVCACHE_INDEXER_REDIS_PWD` to enable optional Redis password configuration. This improves flexibility for connecting to secured Redis instances.

Signed-off-by: Tyler Lisowski <lisowski@us.ibm.com>
Copy link
Collaborator

@elevran elevran left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @relyt0925 !
I think there is an unintended cut-n-paste error - please see suggestion.

Please also let us know what you think regarding the use of the Redis connection to encode the needed parameters. If you agree with the comment - please consider opening a PR to fix the documentation and extend it to show how one might go about adding the username, password, etc. to the connection string.


redisPassword := os.Getenv(kvCacheRedisPasswordEnvVar)
if redisPassword != "" {
config.KVBlockIndexerConfig.RedisPassword = redisAddr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
config.KVBlockIndexerConfig.RedisPassword = redisAddr
config.KVBlockIndexerConfig.RedisPassword = redisPassword

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect the PR to also contain a change where password is used in the Redis connection. Otherwise, the environment variable has no effect on the connection.

Redis connection string format is redis[s]://[[username][:password]@][host][:port][/db-number].
So we either encode all information in the kvCacheRedisEnvVar variable (which I think is the current intent), or we should expose different variables for each URL element to make it completely configurable.

At this time, the use of a single connection string is simpler and allows setting all needed components, including the password.
Perhaps documentation should make this clearer with some examples.

CC: @vMaroon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my gosh sorry for that basic error will fix!

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem. That's what PR reviews are for.
Would appreciate your thoughts on the use of connection string to capture all needed aspects, not just the password

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 like using the connection string for all of it! can edit

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per @vMaroon 👍on #131 (comment) - I think this was the intent: the Redis Address would capture all needed parameters and there's no need for a per element environment variable.

I think this means that no code changes are required, and we should emphasize this in the documentation, possibly with examples.

I like using the connection string for all of it! can edit

Do you mean that you will create a documentation PR update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elevran let me double check with a test that the connection string is fully passing today (we use a secure redis deployment using rediss protocol with user pass. If so: yep I can just add a reference in the doc on how to specify that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now seeing

{"level":"error","ts":"2025-05-27T15:45:15Z","caller":"pd/scheduler.go:167","msg":"KVCache scorer creation failed","error":"failed to create KVCacheIndexer: failed to create RedisKVBlockIndexer: could not connect to Redis: dial tcp: address rediss://XXXXXXXXXX:XXXXX@bd73f810-0eea-46a9-b330-84c9bc36d00d.blrrvkdw0thh68l98t20.private.databases.appdomain.cloud:31394/0: too many colons in address","stacktrace":"github.com/llm-d/llm-d-inference-scheduler/pkg/scheduling/pd.(*Scheduler).pluginsFromConfig\n\t/workspace/pkg/scheduling/pd/scheduler.go:167\ngithub.com/llm-d/llm-d-inference-scheduler/pkg/scheduling/pd.(*Scheduler).generateSchedulerConfig\n\t/workspace/pkg/scheduling/pd/scheduler.go:216\ngithub.com/llm-d/llm-d-inference-scheduler/pkg/scheduling/pd.NewScheduler\n\t/workspace/pkg/scheduling/pd/scheduler.go:79\nmain.run\n\t/workspace/cmd/epp/main.go:185\nmain.main\n\t/workspace/cmd/epp/main.go:122\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:283"}

Copy link
Member

@vMaroon vMaroon May 27, 2025

Choose a reason for hiding this comment

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

Apologies for missing this earlier, @relyt0925 the option Etai referred to would go as follows (from redis-go docs):

opt, err := redis.ParseURL("redis://<user>:<pass>@localhost:6379/<db>")
if err != nil {
	panic(err)
}

client := redis.NewClient(opt)

While that was the intention originally, the code uses:

	redisClient := redis.NewClient(&redis.Options{
		Addr:     config.RedisAddr,
		Password: config.RedisPassword,
		DB:       config.RedisDB,
	})

The configuration that feeds that can be seen here: https://github.com/llm-d/llm-d-kv-cache-manager/blob/aecde47b630d5f85d5fe27716b2bbed558d00e80/pkg/kv-cache/kvblock-indexer.go#L61

I propose that you:

  1. Implement the parsing here. The returned object contains the address, password and rest of fields that can feed into the config linked above. Alternatively, can revert back to a password env-var temporarily.
    • The tradeoff is a go-redis module import
  2. Follow-up on a "fix" in the llm-d-kv-cache-manager repo
  3. Come back with a clean-up and import version bump in a follow-up PR here

If you and @elevran agree, this should be tracked in an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes agreed let me move this to issue and copy discussion and will implement!

@relyt0925 relyt0925 closed this May 27, 2025
@relyt0925
Copy link
Contributor Author

closing for now: likely will reopen to a document only pr if address var passes all variables through

@relyt0925 relyt0925 reopened this May 27, 2025
@relyt0925
Copy link
Contributor Author

Moving to issue and will adjust implementation

@relyt0925 relyt0925 closed this May 27, 2025
Jooho referenced this pull request in Jooho/llm-d-inference-scheduler Sep 17, 2025
…pendatahub-io#131)

Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
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.

3 participants