Bug fixes: 1. NPE when model is not found 2. Port is considered 0 when LLMServerPool is not initialized #79
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package backend | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "math/rand" | ||
| "sync" | ||
|
|
||
|
|
@@ -9,24 +10,65 @@ import ( | |
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| func NewK8sDataStore(options ...K8sDatastoreOption) *K8sDatastore { | ||
| store := &K8sDatastore{ | ||
| poolMu: sync.RWMutex{}, | ||
| llmServices: &sync.Map{}, | ||
| pods: &sync.Map{}, | ||
| } | ||
| for _, opt := range options { | ||
| opt(store) | ||
| } | ||
| return store | ||
| } | ||
|
|
||
| // The datastore is a local cache of relevant data for the given LLMServerPool (currently all pulled from k8s-api) | ||
| type K8sDatastore struct { | ||
| // poolMu is used to synchronize access to the llmServerPool. | ||
| poolMu sync.RWMutex | ||
| llmServerPool *v1alpha1.LLMServerPool | ||
| llmServices *sync.Map | ||
| pods *sync.Map | ||
| } | ||
|
|
||
| type K8sDatastoreOption func(*K8sDatastore) | ||
|
|
||
| // WithPods can be used in tests to override the pods. | ||
| func WithPods(pods []*PodMetrics) K8sDatastoreOption { | ||
| return func(store *K8sDatastore) { | ||
| store.pods = &sync.Map{} | ||
| for _, pod := range pods { | ||
| store.pods.Store(pod.Pod, true) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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 |
||
| defer ds.poolMu.Unlock() | ||
| ds.llmServerPool = pool | ||
| } | ||
|
|
||
| func (ds *K8sDatastore) getLLMServerPool() (*v1alpha1.LLMServerPool, error) { | ||
| ds.poolMu.RLock() | ||
| defer ds.poolMu.RUnlock() | ||
| if ds.llmServerPool == nil { | ||
| return nil, fmt.Errorf("LLMServerPool hasn't been initialized yet") | ||
| } | ||
| return ds.llmServerPool, nil | ||
| } | ||
|
|
||
| func (ds *K8sDatastore) GetPodIPs() []string { | ||
| var ips []string | ||
| ds.pods.Range(func(name, pod any) bool { | ||
| ips = append(ips, pod.(*corev1.Pod).Status.PodIP) | ||
| return true | ||
| }) | ||
| return ips | ||
| } | ||
|
|
||
| func (s *K8sDatastore) FetchModelData(modelName string) (returnModel *v1alpha1.Model) { | ||
| s.llmServices.Range(func(k, v any) bool { | ||
| service := v.(*v1alpha1.LLMService) | ||
| klog.V(3).Infof("Service name: %v", service.Name) | ||
| for _, model := range service.Spec.Models { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I agree with keeping these properties private. This assumes that access will only ever be from the
backendpackage 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.