-
Notifications
You must be signed in to change notification settings - Fork 825
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
Move cluster node addition/removal out of "experimental" #271
Move cluster node addition/removal out of "experimental" #271
Conversation
Implemented workerqueue for node modifications in the PortAllocator, so that if the master happens to go down, then the operations the PortAllocator has to do to keep things in check will retry and result in a correct state. Closes googleforgames#60
Build Succeeded 👏 Build Id: 821d23b9-e497-429d-8413-84eca82f3906 The following development artifacts have been built, and will exist for the next 30 days:
|
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.
One minor question. Otherwise, this looks good.
|
||
pa.gameServerInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
DeleteFunc: pa.syncDeleteGameServer, | ||
}) | ||
|
||
// Experimental support for node adding/removal | ||
pa.nodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
AddFunc: pa.syncAddNode, | ||
AddFunc: func(obj interface{}) { | ||
node := obj.(*corev1.Node) |
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 know the semantics of the event handler, but is there a chance that obj
isn't of *corev1.Node
type? In other words, is this a safe type assertion?
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.
A good question - basically, there isn't really. But a good question!
If something other than a Node comes through a Node event handler, then something very weird has happened -- at which point, it really should panic.
Here another example in the Kubernetes sample-controller doing similar things:
https://github.com/kubernetes/sample-controller/blob/master/controller.go#L129
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 panicking is exactly the right thing to do.
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.
LGTM
@markmandel If you rebase onto master, I'll be happy to merge. 😄 |
Build Succeeded 👏 Build Id: ce29bafc-7668-4e57-bd1c-fb9795a8c526 The following development artifacts have been built, and will exist for the next 30 days:
|
Implemented workerqueue for node modifications in the PortAllocator, so that if the master happens to go
down, then the operations the PortAllocator has to do to keep things in check will retry and result in a correct state.
Closes #60