-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
switching to watches for allocation #31
Conversation
@@ -200,14 +200,7 @@ func attachSidecar(gs *mpsv1alpha1.GameServer, pod *corev1.Pod) { | |||
Name: SidecarContainerName, | |||
ImagePullPolicy: corev1.PullIfNotPresent, | |||
Image: SidecarImage, | |||
Ports: []corev1.ContainerPort{ |
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.
No need anymore for sidecar to expose its HTTP port
@@ -133,37 +128,6 @@ func (h *allocateHandler) handle(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
var pod corev1.Pod |
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.
No need to call the HTTP endpoint on the sidecar anymore.
@@ -173,37 +223,3 @@ func (h *httpHandler) transitionStateToStandingBy(ctx context.Context, hb *Heart | |||
h.previousGameState = hb.CurrentGameState | |||
return nil | |||
} | |||
|
|||
func (h *httpHandler) getGameServerStateFromK8s(ctx context.Context) (*SessionDetails, error) { |
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.
wasn't used
SessionCookie string `json:"sessionCookie,omitempty"` | ||
InitialPlayers []string `json:"initialPlayers,omitempty"` | ||
State string `json:"state,omitempty"` | ||
SessionID string |
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.
this were coming in JSON by the controller calling the sidecar's HTTP API service
- name: controller | ||
newName: thundernetes-operator | ||
newTag: 19ecf2d | ||
resources: |
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.
Is this file auto-generated every build? Should it be part of the ignore list?
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.
Not sure :( I think it's autogenerated every time we run e2e tests locally. If we .gitignore it, this will ignore any changes we do to it subsequently though (not sure if we ever gonna do anything to it but again, not sure :( ). Will revert for the time being
previousGameState GameState | ||
previousGameHealth string | ||
gameServerName string | ||
gameServerNamespace string | ||
} | ||
|
||
func NewHttpHandler(k8sClient dynamic.Interface, gameServerName, gameServerNamespace string) httpHandler { |
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.
It feels like this type is now more of a watchHandler vs httpHandler. Perhaps a rename is in order here
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.
you've got a point. At the same time it consumes the http heartbeats from the GSDK in the GameServer, so not sure :( sidecarManager? Maybe we can split it in two structs (one for heartbeat handling and one for the watch)? If you don't mind, I'll merge as is (since the PR is already bigger than I wanted it to be) and I'll file an item for refactoring this file. Thanks!
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.
Aside from the minor comments, everything else looks great!
Fixes #25
This PR switches to Kubernetes watches for the allocation service. Instead of having the sidecar host an HTTP server to get the change in state, it creates a Watch to Kubernetes API server for the GameServer CRD instance updates. Once it transitions to Active, the Watch is killed to lessen the pressure on Kubernetes API Server.
Also opportunistically fixed two other issues: