-
Notifications
You must be signed in to change notification settings - Fork 232
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
Daemon set merge #107
Daemon set merge #107
Conversation
@dgrove-oss can you squash your two commits to one so I can credit both committers when merging? |
3a66798
to
4878cb3
Compare
squashed my two into 1. |
apiVersion: apps/v1beta1 | ||
kind: StatefulSet | ||
apiVersion: extensions/v1beta1 | ||
kind: DaemonSet |
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.
First of all, thanks for your work on the OpenWhisk+Kubernetes integration!
I know this has been merged already, but is this really what you want? Using a DaemonSet will ensure only one instance per node, but you can do this in a Deployment as well, with node anti-affinity. This opens up the opportunity to use HorizontalPodAutoscaler and node pool autoscaling, and AFAICS it also removes the need to maintain node labels by default?
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'll open up an issue to discuss further. As the upstream system is now, using autoscaling isn't a great idea because we are missing the lifecycle methods on the invoker to allow an orderly draining and suspension of the invoker-N topic before the invoker-N pod is removed during scale down. So scaling down could lose user function invocations.
The choice of Daemonset vs. Deployment is probably also related to which ContainerFactoryProvider implementation is being used and whether OpenWhisk is being deployed to a kube cluster dedicated to just running OpenWhisk or if the cluster is also running additional services.
cherry-picks @DanLavine commit from #55 and updates them to work with current master.