Repository navigation
refactor(worker): declare the elastic width bounds in one place - #724
Merged
Merged
Conversation
Workload rendering, admission and native observation each carried their own copy of the elastic collective's supported width range. Three copies are three answers, and they drift in the direction nobody reads: widening admission alone leaves native observation refusing a width the deployment is already allowed to ask for. pkg/worker/elasticprofile now declares the range once and all three consumers read it from there. The profile is not widened -- these are the values the three consumers already enforced. Each consumer gains a contract test that asks the shared constants for the boundary widths and checks its own behaviour against them, so a bound restated inside a consumer fails the check rather than passing unnoticed. The native observer keeps its own floor of one, which is a probe budget and not a profile claim, and a test holds that floor below the profile's so a profile-legal width can never become unmeasurable. Fixes #716 Signed-off-by: thxCode <thxcode0824@gmail.com>
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
📄
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind cleanup
/area worker
What this PR does / why we need it:
Workload rendering, admission and native observation each carried their own copy of the elastic collective's supported width range. Three copies are three answers, and they drift in the direction nobody reads: widening admission alone leaves native observation refusing a width the deployment is already allowed to ask for.
pkg/worker/elasticprofilenow declares the range once and all three consumers read it from there:model_deployment_elastic_workload.go— the inlinewidth < 2 || width > 64literals are gone.webhooks/worker/model_deployment_elastic.go— the localmodelDeploymentElasticWidthMin/Maxpair is gone.elasticengine/effective_observation.go—maxObservedWorldWidthis nowelasticprofile.WidthMax.The profile is not widened. These are the values the three consumers already enforced.
Each consumer gains a contract test that asks the shared constants for the boundary widths and checks its own behaviour, so a bound restated inside a consumer fails the check:
TestProfileWidthBoundsTestValidateModelDeploymentElasticEPWidthBoundsTestModelDeploymentElasticBootstrapFromMasterWidthBoundsTestObserveNativeWorldElasticProfileWidthsWidthMax+1and0before sending a requestTestObserveNativeWorldProbeFloorIsNotTheProfileFloorOne disagreement with the issue, reported rather than resolved silently. #716 says the three
consumers "currently agree on widths 2 through 64". They did not quite:
ObserveNativeWorldenforced a floor of 1, not 2. That floor is a probe budget rather than a profile claim — a
single-rank world is a real world, and refusing to measure it would say nothing true about it.
Narrowing it would be a behaviour change on a public path, so it was left alone and made explicit
instead. The ceiling — the bound that actually couples the two — is the shared one.
RED proof. Giving one consumer its own bound copy (the webhook's pair restored as local
constants with a widened maximum of 128) fails the check, naming the exact width that diverged:
Reverting restores the shared implementation and the check goes green (
GREEN RUN EXIT: 0).Known residual.
api/worker/v1alpha1/model_deployment.gocarries the same pair as kubebuildermarkers on
ModelDeploymentRoleElasticEP.Width(+k8s:validation:minimum=2,+k8s:validation:maximum=64). That is a fourth declaration, outside this change's scope. It isnot a live hazard — it constrains the API server's schema, and the webhook below it would still
refuse an out-of-range width, so the failure direction stays fail-closed.
Which issue(s) this PR links to:
Fixes #716
Special notes for your reviewer:
The two changes do not overlap: #715 touches the allocation and router-observation paths, this one
touches rendering, admission and the engine client, so they are two PRs rather than one.
Verification:
go build ./...andgo test ./pkg/worker/... -count=1both exit 0 on this branchalone.
make lintcould not complete on this host — the Go stage is clean (golangci-lintreports0 issues) but the unrelated
.agentsshell self-test cannot clean up its temp directory under thisruntime's deletion interception, which reproduces on a pristine tree too.
Does this PR introduce a user-facing change?