-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
WIP: Implement deployment lifecycle hooks #1456
Conversation
26999d8
to
fcdfb28
Compare
fcdfb28
to
8e89b5e
Compare
DecodeConfig func(deployment *kapi.ReplicationController) (*deployapi.DeploymentConfig, error) | ||
} | ||
|
||
var _ = Interface(&LifecycleManager{}) |
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.
var _ Interface = &LifeycleManager{}
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.
Ah, fixed
8e89b5e
to
60f0572
Compare
@smarterclayton In general, is the approach I'm taking here making sense? At first I was basically implementing the exec pod plugin inline in the existing controllers, and decided to try and abstract it behind a generic plugin system when it started getting out of hand. I hope it's not too much for the task at hand. |
} | ||
|
||
// Handle syncs the action pod's status with its associated deployment. | ||
func (c *ExecNewPodController) Handle(pod *kapi.Pod) 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.
Why isn't this run by the deployment process itself? Why does it need a controller?
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.
Will talk more tomorrow, but I found a problem with my approach when trying to deal with the higher level status persistence that has me thinking about other ways to approach the feature in general. Any ideas welcome right now- this POC may be the one I end up tossing now that I have more of the problems in my head.
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.
While having the deployment pod itself trigger the execution of the container may increase the permissions that pod needs, it means that the retry context and completion progress is tied to an execution of a pod. On the other hand, it may make reentrant behavior more difficult.
The argument in favor of that is that I would expect a complex custom deployment to invoke its own hooks, rather than also invoke separate hooks.
On Apr 6, 2015, at 8:00 PM, Dan Mace notifications@github.com wrote:
In pkg/deploy/controller/lifecycle/execnewpod/controller.go:
- kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
- deployapi "github.com/openshift/origin/pkg/deploy/api"
+)
+// ExecNewPodController keeps an ExecNewPodAction pod's status in sync with
+// its associated deployment.
+//
+// Use ExecNewPodControllerControllerFactory to create this controller.
+type ExecNewPodController struct {
- // deploymentClient provides access to deployments.
- deploymentClient deploymentClient
+}
+// Handle syncs the action pod's status with its associated deployment.
+func (c *ExecNewPodController) Handle(pod *kapi.Pod) error {
Will talk more tomorrow, but I found a problem with my approach when trying to deal with the higher level status persistence that has me thinking about other ways to approach the feature in general. Any ideas welcome right now- this POC may be the one I end up tossing now that I have more of the problems in my head.—
Reply to this email directly or view it on GitHub.
We found a better way to go about this; I'll be revising the proposal and opening a new PR from a different branch tomorrow. |
…service-catalog/' changes from aa27078754..dabde2eb85 dabde2eb85 origin build: add origin tooling b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: dabde2eb859b5e31e97c01a704561fc27e1848b2
…service-catalog/' changes from aa27078754..510060232e 510060232e origin build: add origin tooling de45e94 v0.1.0 chart changes (openshift#1468) 0bb9982 Modify Makefile to only specify ldflags once (openshift#1471) 5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453) 630f13f fix lingering unversioned client API (openshift#1466) 6f49128 Fix several logging errors (openshift#1464) 2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450) 179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457) b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac
See: kubernetes-retired/service-catalog#1354 (review) Signed-off-by: Doug Davis <dug@us.ibm.com>
This PR tracks the effort to implement deployment hooks as specified in #1224.