Repository: SeldonIO/seldon-core (language: python)

Issue:
Scheduler panic on server restart with model loaded
Reproduced by killing mlserver after model sucessfully loaded on k8s.

```
│ panic: runtime error: invalid memory address or nil pointer dereference                                                                                                 │
│ [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1a0244d]                                                                                                 │
│                                                                                                                                                                         │
│ goroutine 190 [running]:                                                                                                                                                │
│ github.com/seldonio/seldon-core/scheduler/v2/pkg/envoy/processor.(*IncrementalProcessor).updateEnvoyForModelVersion(0xc0005c1ad0, {0xc0020360a0, 0x4}, 0xc0018d5d00, 0x │
│     /build/scheduler/pkg/envoy/processor/incremental.go:264 +0xcd                                                                                                       │
│ github.com/seldonio/seldon-core/scheduler/v2/pkg/envoy/processor.(*IncrementalProcessor).addModelTraffic(0xc0005c1ad0, {0xc0020360a0, 0x4}, 0xc001849fb0, 0x1a44f18?, 0 │
│     /build/scheduler/pkg/envoy/processor/incremental.go:337 +0x652                                                                                                      │
│ github.com/seldonio/seldon-core/scheduler/v2/pkg/envoy/processor.(*IncrementalProcessor).addModel(0xc0005c1ad0, 0xc001849fb0)                                           │
│     /build/scheduler/pkg/envoy/processor/incremental.go:393 +0x305                                                                                                      │
│ github.com/seldonio/seldon-core/scheduler/v2/pkg/envoy/processor.(*IncrementalProcessor).modelUpdate(0xc0005c1ad0, {0xc0020360a0, 0x4})                                 │
│     /build/scheduler/pkg/envoy/processor/incremental.go:592 +0xa8b                                                                                                      │
│ github.com/seldonio/seldon-core/scheduler/v2/pkg/envoy/processor.(*IncrementalProcessor).handleModelEvents.func1()                                                      │
│     /build/scheduler/pkg/envoy/processor/incremental.go:183 +0x54                                                                                                       │
│ created by github.com/seldonio/seldon-core/scheduler/v2/pkg/envoy/processor.(*IncrementalProcessor).handleModelEvents                                                   │
│     /build/scheduler/pkg/envoy/processor/incremental.go:182 +0x1b8  
```


Discussion / hints:

Fix patch:
diff --git a/scheduler/pkg/envoy/processor/incremental.go b/scheduler/pkg/envoy/processor/incremental.go
index e6a81287df..bf10ae93ab 100644
--- a/scheduler/pkg/envoy/processor/incremental.go
+++ b/scheduler/pkg/envoy/processor/incremental.go
@@ -308,6 +308,9 @@ func (p *IncrementalProcessor) addModelTraffic(routeName string, model *store.Mo
 	modelName := model.Name
 	latestModel := model.GetLatest()
 	if latestModel == nil || !model.CanReceiveTraffic() {
+		if latestModel == nil {
+			logger.Infof("latest model is nil for model %s route %s", model.Name, routeName)
+		}
 		return fmt.Errorf("No live replica for model %s for model route %s", model.Name, routeName)
 	}
 
@@ -566,38 +569,48 @@ func (p *IncrementalProcessor) modelUpdate(modelName string) error {
 		return p.updateEnvoy() // in practice we should not be here
 	}
 
+	// Add a modelRemoved boolean so we can continue to batch update but skip steps if we have
+	// decided this model can't be added for some reason. This allows batch deletion of routes
+	// to take place for errors as well as the successful path through the methods
+	modelRemoved := false
 	if !model.CanReceiveTraffic() {
 		logger.Debugf("sync: Model can't receive traffic - removing for %s", modelName)
 		if err := p.removeRouteForServerInEnvoyCache(modelName); err != nil {
 			logger.WithError(err).Errorf("Failed to remove model route from envoy %s", modelName)
 			return err
 		}
+		modelRemoved = true
 	}
 
-	server, err := p.modelStore.GetServer(latestModel.Server(), false, false)
-	if err != nil || server == nil {
-		logger.Debugf("sync: No server - removing for %s", modelName)
-		if err := p.removeRouteForServerInEnvoyCache(modelName); err != nil {
-			logger.WithError(err).Errorf("Failed to remove model route from envoy %s", modelName)
-			return err
+	if !modelRemoved {
+		_, err = p.modelStore.GetServer(latestModel.Server(), false, false)
+		if err != nil {
+			logger.Debugf("sync: No server - removing for %s", modelName)
+			if err := p.removeRouteForServerInEnvoyCache(modelName); err != nil {
+				logger.WithError(err).Errorf("Failed to remove model route from envoy %s", modelName)
+				return err
+			}
+			modelRemoved = true
 		}
 	}
 
-	// Remove routes before we recreate
-	if err := p.removeRouteForServerInEnvoyCache(modelName); err != nil {
-		logger.Debugf("Failed to remove route before starting update for %s", modelName)
-		return err
-	}
-
-	err = p.addModel(model)
-	if err != nil {
-		// note that on error for `addModel` we specifically do not return so that we can do batched
-		// delete of envoy routes
-		logger.WithError(err).Errorf("Failed to add traffic for model %s", modelName)
+	if !modelRemoved {
+		// Remove routes before we recreate
 		if err := p.removeRouteForServerInEnvoyCache(modelName); err != nil {
-			logger.WithError(err).Errorf("Failed to remove model route from envoy %s", modelName)
+			logger.Debugf("Failed to remove route before starting update for %s", modelName)
 			return err
 		}
+
+		err = p.addModel(model)
+		if err != nil {
+			// note that on error for `addModel` we specifically do not return so that we can do batched
+			// delete of envoy routes
+			logger.WithError(err).Errorf("Failed to add traffic for model %s", modelName)
+			if err := p.removeRouteForServerInEnvoyCache(modelName); err != nil {
+				logger.WithError(err).Errorf("Failed to remove model route from envoy %s", modelName)
+				return err
+			}
+		}
 	}
 
 	// Add to batch pending Envoy sync
@@ -658,7 +671,7 @@ func (p *IncrementalProcessor) modelSync() {
 		}
 
 		s, err := p.modelStore.GetServer(v.Server(), false, false)
-		if err != nil || s == nil {
+		if err != nil {
 			logger.Debugf("Failed to get server for model %s server %s", mv.name, v.Server())
 			p.modelStore.UnlockModel(mv.name)
 			continue
diff --git a/scheduler/pkg/server/server.go b/scheduler/pkg/server/server.go
index e8bfd289d1..ed88dee82b 100644
--- a/scheduler/pkg/server/server.go
+++ b/scheduler/pkg/server/server.go
@@ -425,9 +425,6 @@ func (s *SchedulerServer) ServerStatus(
 		if err != nil {
 			return status.Errorf(codes.FailedPrecondition, err.Error())
 		}
-		if server == nil {
-			return status.Errorf(codes.FailedPrecondition, fmt.Sprintf("Failed to find server %s", req.GetName()))
-		}
 		resp := createServerStatusResponse(server)
 		err = stream.Send(resp)
 		if err != nil {
diff --git a/scheduler/pkg/server/server_status.go b/scheduler/pkg/server/server_status.go
index 0cb11464ab..087fd7dd1e 100644
--- a/scheduler/pkg/server/server_status.go
+++ b/scheduler/pkg/server/server_status.go
@@ -221,10 +221,6 @@ func (s *SchedulerServer) sendServerStatus() {
 			logger.Errorf("Failed to get server %s", serverName)
 			continue
 		}
-		if server == nil {
-			logger.Warnf("Server %s does not exist", serverName)
-			continue
-		}
 		ssr := createServerStatusResponse(server)
 
 		for stream, subscription := range s.serverEventStream.streams {
diff --git a/scheduler/pkg/store/memory.go b/scheduler/pkg/store/memory.go
index d5d7311066..6325b67f0a 100644
--- a/scheduler/pkg/store/memory.go
+++ b/scheduler/pkg/store/memory.go
@@ -231,7 +231,7 @@ func (m *MemoryStore) GetServer(serverKey string, shallow bool, modelDetails boo
 	defer m.mu.RUnlock()
 	server := m.store.servers[serverKey]
 	if server == nil {
-		return nil, nil
+		return nil, fmt.Errorf("Server [%s] not found", serverKey)
 	} else {
 		return server.CreateSnapshot(shallow, modelDetails), nil
 	}


Test patch:
diff --git a/scheduler/pkg/store/memory_test.go b/scheduler/pkg/store/memory_test.go
index a7d4b4834e..946ea974e9 100644
--- a/scheduler/pkg/store/memory_test.go
+++ b/scheduler/pkg/store/memory_test.go
@@ -1530,10 +1530,11 @@ func TestRemoveServerReplica(t *testing.T) {
 			g.Expect(err).To(BeNil())
 			g.Expect(test.modelsReturned).To(Equal(len(models)))
 			server, err := ms.GetServer(test.serverName, false, true)
-			g.Expect(err).To(BeNil())
 			if test.serverExists {
+				g.Expect(err).To(BeNil())
 				g.Expect(server).ToNot(BeNil())
 			} else {
+				g.Expect(err).ToNot(BeNil())
 				g.Expect(server).To(BeNil())
 			}
 		})