Bug fixes: 1. NPE when model is not found 2. Port is considered 0 when LLMServerPool is not initialized #79
Conversation
liu-cong
commented
Dec 9, 2024
liu-cong
commented
- Also make some fields unexported
- Also improves logging.
k8s-ci-robot
added
cncf-cla: yes
| } | ||
|
|
||
| func (c *EndpointSliceReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| llmServerPoolAvailable := func(object client.Object) bool { |
There was a problem hiding this comment.
Important: Do not reconcile EndpointSlice until LLMServerPool is initialized
| Pods *sync.Map | ||
| // poolMu is used to synchronize access to the llmServerPool. | ||
| poolMu sync.RWMutex | ||
| llmServerPool *v1alpha1.LLMServerPool |
There was a problem hiding this comment.
I don't think I agree with keeping these properties private. This assumes that access will only ever be from the backend package and I'm not sure that will be true. This package is already rather a catchall and could use some refactoring.
There was a problem hiding this comment.
This assumes that access will only ever be from the backend package and I'm not sure that will be true
Then you can make it public or expose a public method. That's what I am doing here. I have synchronized access to the fields so other packages won't mess up with it.
The bug is that in main.go, it sets a empty LLMServerPool which later in EndpointSlicerReconcilers uses the default port value 0. Making fields private prevents such bugs.
| } | ||
|
|
||
| func (ds *K8sDatastore) SetLLMServerPool(pool *v1alpha1.LLMServerPool) { | ||
| ds.poolMu.Lock() |
There was a problem hiding this comment.
have we experienced/do we expect a race condition with the serverPool?
There was a problem hiding this comment.
yes, happens pretty often actually. If you try restarting EPP many times, you can probably hit it
There was a problem hiding this comment.
Yes, I agree that the startup race condition is annoying, and this PR resolves that in your predicate func. But I think this is unrelated. I suppose its not a big deal, and can keep us safer. Getters/Setters can just add code bloat and can complicate simple logic. Tying this into the export comment thread as well based on the same idea.
There was a problem hiding this comment.
Yeah it's common practice to hide internal fields until it's necessary to be exported, it reduces chances of bugs. The llmServerPool field is particular important here because I need to synchronize the access to it.
There was a problem hiding this comment.
Yeah, I think thats more Java/C# style, but I won't dig too hard here.
There was a problem hiding this comment.
So go doesn't encourage the "GetXX" naming, so in this case I should name it "LLMServerPool" instead of "GetLLMServerPool" But Getter/Setter pattern is common in Go as well, see the "Getter" section https://go.dev/doc/effective_go
|
/hold fixing a test |
k8s-ci-robot
added
the
do-not-merge/hold
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm looks ok to me, I will leave the approve to Kellen |
k8s-ci-robot
added
the
lgtm
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain, liu-cong The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
