Skip to content

Add Redis password support to KV cache scorer #131

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

Closed
relyt0925 wants to merge 1 commit into from
Closed

Add Redis password support to KV cache scorer #131

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
10 changes: 8 additions & 2 deletions pkg/scheduling/plugins/scorer/kvcache-aware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ import (
const (
kvCacheAwareScorerName = "kvcache-aware-scorer"

kvCacheRedisEnvVar = "KVCACHE_INDEXER_REDIS_ADDR"
huggingFaceTokenEnvVar = "HF_TOKEN"
kvCacheRedisEnvVar = "KVCACHE_INDEXER_REDIS_ADDR"
kvCacheRedisPasswordEnvVar = "KVCACHE_INDEXER_REDIS_PWD"
huggingFaceTokenEnvVar = "HF_TOKEN"
)

// KVCacheAwareScorer uses the KVCacheIndexer to score pods based on KVCache
Expand All @@ -42,6 +43,11 @@ func NewKVCacheAwareScorer(ctx context.Context) (plugins.Scorer, error) {
return nil, fmt.Errorf("environment variable %s is not set", kvCacheRedisEnvVar)
}

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

@elevran elevran May 21, 2025

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

@elevran elevran May 21, 2025

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

@relyt0925 relyt0925 May 21, 2025

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

@elevran elevran May 21, 2025

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

@relyt0925 relyt0925 May 22, 2025

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

@elevran elevran May 22, 2025

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

@relyt0925 relyt0925 May 27, 2025

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

@relyt0925 relyt0925 May 27, 2025

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 •
edited
Loading

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

@relyt0925 relyt0925 May 27, 2025

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!

}

hfToken := os.Getenv(huggingFaceTokenEnvVar)
if hfToken != "" {
config.TokenizersPoolConfig.HuggingFaceToken = hfToken
Expand Down
Loading