Repository navigation
Conversation
ce0c66a to
be9a640
Compare
negz
changed the title
be9a640 to
2c37243
Compare
| name = base.engine_name(replica, engine) | ||
| # The last group's worker StatefulSet has the longest name. | ||
| name = base.dns_1035_label( | ||
| base.engine_name(replica, engine), "lws", _STATEFULSET_NAME_MAX - len(f"-{int(engine.copies or 1) - 1}") |
There was a problem hiding this comment.
This makes the LWS name depend on copies, which is the LWS spec.replicas and something folks scale.
A 50-char engine name keeps its name at 1–10 copies but becomes lws-…-ad679 at 11 (test_name pins this). provider-kubernetes would then leaves the old LWS behind, still holding GPUs, and the scale-up becomes a recreate?
Could we budget for a fixed max group index instead (e.g. -999)? That renames todays 49–50-char names once, but after that the name never moves on scale.
| # 50 characters, and {lws}-0 is 52. | ||
| ("a" * 39, 1, "a" * 39 + "-main-f8695"), | ||
| # The same 50 characters, but {lws}-10 would be 53. | ||
| ("a" * 39, 11, "lws-" + "a" * 39 + "-ad679"), |
There was a problem hiding this comment.
Related to the comment on llmd.py:77
this case and the one above it are the same replica getting two different LWS names depending only on copies. If that stays intentional, it might be worth a comment here saying that it renames on scale?
| a name LWS accepts; a Grove gang uses grove_pcs_name instead, which budgets | ||
| for Grove's own name-length validation. | ||
| """ | ||
| return resource.child_name(_name(replica.metadata), engine.name) |
There was a problem hiding this comment.
child_name only does rstrip("-"), so the trailing-. problem fixed in opaque_name can still happen here. child_name("a"*56 + ".b-12345", "main") gives aaaa….-685b0, which is not a valid subdomain ?
llm-d is now covered because dns_1035_label replaces the dot, but the native Deployment still uses this name as is?
| return name | ||
| full = f"{prefix}-{name}" | ||
| h = hashlib.sha256(full.encode()).hexdigest()[:5] | ||
| return f"{full[: max_length - len(h) - 1].rstrip('-')}-{h}".replace(".", "-") |
There was a problem hiding this comment.
Nit: if the truncation lands on a ., it survives the rstrip('-') and the replace turns it into foo--abcde. That is still valid, just untidy; rstrip("-.") avoids it.
2c37243 to
289ac58
Compare
A ModelReplica's name comes from its ModelDeployment's, so it may contain
dots or start with a digit, and runs to 63 characters. It and several names
built from it could break a rule for their kind:
- The endpoint picker's Service, {name}-epp, must be a DNS-1035 label, and
its pods' app label, the same value, at most 63 characters.
- LWS rejects a LeaderWorkerSet name that isn't a DNS-1035 label, since it
names the set's headless Service. Kubernetes can't create pods for a
StatefulSet named after it once that name passes 52 characters.
- Grove names a gang's headless Service after its PodCliqueSet, which kept
the replica's dots.
- The ModelReplica's own name could end its truncated prefix in a '.' next
to the '-' before its hash.
This derives a valid name wherever the one it would get isn't, and leaves
every name that worked alone, since provider-kubernetes finds an object only
by its name and would leave a renamed one's predecessor behind. There are
two exceptions. A picker Service starting with a digit worked from
Kubernetes 1.36, so there a harmless duplicate Service is left behind. A
LeaderWorkerSet's name budgets for the 64 copies an engine can have, so
scaling an engine never renames it. That renames one whose name is exactly
50 characters, which worked with up to ten copies, and leaves its
predecessor behind holding GPUs.
A Grove gang keeps a leading digit, since it may be running on 1.36, so on
older clusters that name still fails.
Signed-off-by: Nic Cope <nicc@rk0n.org>
289ac58 to
e1c78e6
Compare
Description of your changes
Broken by #99, which named the LeaderWorkerSet from the replica, and #142, which named the endpoint picker's Service
{name}-epp(#342 then gave every replica one).A ModelReplica's name comes from its ModelDeployment's, so it may contain dots or start with a digit, and runs to 63 characters. Several names built from it broke a rule for their kind:
applabel, also{name}-epp, can have at most 63 characters. Aqwen2.5-0.5bdeployment couldn't create its picker Service:.next to the-before its hash.An invalid name is now replaced by one hashed behind a prefix, as compose-inference-cluster derives its gateway's Service name. A LeaderWorkerSet's name stays short enough that its last group's worker StatefulSet fits in 52 characters.
Every name that already worked stays the same, since provider-kubernetes finds an object only by its name and would leave a renamed one's predecessor behind. A rename does leave behind the old object under a name that never worked. That object can be a LeaderWorkerSet whose worker StatefulSets couldn't create pods, so its leaders may still hold GPUs. Delete that one by hand. It can also be a dotted PodCliqueSet whose headless Service failed, or the old picker Service of a replica whose name starts with a digit on Kubernetes 1.36 or later, which accepts that name. A Grove gang keeps a leading digit, since it may be running on 1.36, so on older clusters that case still fails.
On EKS, a
qwen2.5-0.5bdeployment and one with a 56-character name both served through the gateway with an earlier version of this PR, whose picker naming matches this one. Only unit tests cover the LeaderWorkerSet, Grove and ModelReplica name changes.I have:
nix flake check(or./nix.sh flake check) and made sure it passes.git commit -s.