From e1554c0f4debc9738bc5ea00b07ca52486c043de Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Fri, 8 Jan 2016 14:46:08 -0800 Subject: [PATCH 1/2] Fix flaky e2e: Use expectation model for deployment's new rc creation --- .../deployment/deployment_controller.go | 53 ++++++++++++++----- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 848af478350..59deb4c4af3 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -78,7 +78,11 @@ type DeploymentController struct { podStoreSynced func() bool // A TTLCache of pod creates/deletes each deployment expects to see - expectations controller.ControllerExpectationsInterface + podExpectations controller.ControllerExpectationsInterface + + // A TTLCache of rc creates/deletes each deployment expects to see + // TODO: make expectation model understand (rc) updates (besides adds and deletes) + rcExpectations controller.ControllerExpectationsInterface // Deployments that need to be synced queue *workqueue.Type @@ -91,11 +95,12 @@ func NewDeploymentController(client client.Interface, resyncPeriod controller.Re eventBroadcaster.StartRecordingToSink(client.Events("")) dc := &DeploymentController{ - client: client, - expClient: client.Extensions(), - eventRecorder: eventBroadcaster.NewRecorder(api.EventSource{Component: "deployment-controller"}), - queue: workqueue.New(), - expectations: controller.NewControllerExpectations(), + client: client, + expClient: client.Extensions(), + eventRecorder: eventBroadcaster.NewRecorder(api.EventSource{Component: "deployment-controller"}), + queue: workqueue.New(), + podExpectations: controller.NewControllerExpectations(), + rcExpectations: controller.NewControllerExpectations(), } dc.dStore.Store, dc.dController = framework.NewInformer( @@ -192,6 +197,12 @@ func (dc *DeploymentController) addRC(obj interface{}) { rc := obj.(*api.ReplicationController) glog.V(4).Infof("Replication controller %s added.", rc.Name) if d := dc.getDeploymentForRC(rc); rc != nil { + dKey, err := controller.KeyFunc(d) + if err != nil { + glog.Errorf("Couldn't get key for deployment controller %#v: %v", d, err) + return + } + dc.rcExpectations.CreationObserved(dKey) dc.enqueueDeployment(d) } } @@ -329,7 +340,7 @@ func (dc *DeploymentController) deletePod(obj interface{}) { glog.Errorf("Couldn't get key for deployment controller %#v: %v", d, err) return } - dc.expectations.DeletionObserved(dKey) + dc.podExpectations.DeletionObserved(dKey) } } @@ -384,7 +395,8 @@ func (dc *DeploymentController) syncDeployment(key string) error { } if !exists { glog.Infof("Deployment has been deleted %v", key) - dc.expectations.DeleteExpectations(key) + dc.podExpectations.DeleteExpectations(key) + dc.rcExpectations.DeleteExpectations(key) return nil } d := *obj.(*extensions.Deployment) @@ -392,7 +404,7 @@ func (dc *DeploymentController) syncDeployment(key string) error { // Sleep so we give the rc reflector goroutine a chance to run. time.Sleep(RcStoreSyncedPollPeriod) glog.Infof("Waiting for rc controller to sync, requeuing deployment %s", d.Name) - dc.enqueueDeployment(d) + dc.enqueueDeployment(&d) return nil } @@ -475,6 +487,15 @@ func (dc *DeploymentController) getNewRC(deployment extensions.Deployment) (*api if err != nil || existingNewRC != nil { return existingNewRC, err } + // Check the rc expectations of deployment before creating a new rc + dKey, err := controller.KeyFunc(&deployment) + if err != nil { + return nil, fmt.Errorf("couldn't get key for deployment %#v: %v", deployment, err) + } + if !dc.rcExpectations.SatisfiedExpectations(dKey) { + dc.enqueueDeployment(&deployment) + return nil, fmt.Errorf("RC expectations not met yet before getting new RC\n") + } // new RC does not exist, create one. namespace := deployment.ObjectMeta.Namespace podTemplateSpecHash := deploymentutil.GetPodTemplateSpecHash(deployment.Spec.Template) @@ -482,6 +503,13 @@ func (dc *DeploymentController) getNewRC(deployment extensions.Deployment) (*api // Add podTemplateHash label to selector. newRCSelector := deploymentutil.CloneAndAddLabel(deployment.Spec.Selector, deployment.Spec.UniqueLabelKey, podTemplateSpecHash) + // Set RC expectations (1 rc should be created) + dKey, err = controller.KeyFunc(&deployment) + if err != nil { + return nil, fmt.Errorf("couldn't get key for deployment controller %#v: %v", deployment, err) + } + dc.rcExpectations.ExpectCreations(dKey, 1) + // Create new RC newRC := api.ReplicationController{ ObjectMeta: api.ObjectMeta{ GenerateName: deployment.Name + "-", @@ -495,6 +523,7 @@ func (dc *DeploymentController) getNewRC(deployment extensions.Deployment) (*api } createdRC, err := dc.client.ReplicationControllers(namespace).Create(&newRC) if err != nil { + dc.rcExpectations.DeleteExpectations(dKey) return nil, fmt.Errorf("error creating replication controller: %v", err) } return createdRC, nil @@ -556,8 +585,8 @@ func (dc *DeploymentController) reconcileOldRCs(allRCs []*api.ReplicationControl if err != nil { return false, fmt.Errorf("Couldn't get key for deployment %#v: %v", deployment, err) } - if expectationsCheck && !dc.expectations.SatisfiedExpectations(dKey) { - fmt.Printf("Expectations not met yet before reconciling old RCs\n") + if expectationsCheck && !dc.podExpectations.SatisfiedExpectations(dKey) { + glog.V(4).Infof("Pod expectations not met yet before reconciling old RCs\n") return false, nil } // Find the number of ready pods. @@ -593,7 +622,7 @@ func (dc *DeploymentController) reconcileOldRCs(allRCs []*api.ReplicationControl return false, fmt.Errorf("Couldn't get key for deployment %#v: %v", deployment, err) } if expectationsCheck { - dc.expectations.ExpectDeletions(dKey, scaleDownCount) + dc.podExpectations.ExpectDeletions(dKey, scaleDownCount) } } return true, err From c2463a5aefb3c1de671227a32334ca77509a2f0a Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Mon, 11 Jan 2016 16:01:39 -0800 Subject: [PATCH 2/2] Generalize name of expectations since they aren't only used for pods --- pkg/controller/controller_utils.go | 64 +++++++++++++++--------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index 2fe65a56900..9575b5151ee 100644 --- a/pkg/controller/controller_utils.go +++ b/pkg/controller/controller_utils.go @@ -76,16 +76,16 @@ func StaticResyncPeriodFunc(resyncPeriod time.Duration) ResyncPeriodFunc { // } // // Implementation: -// PodExpectation = pair of atomic counters to track pod creation/deletion -// ControllerExpectationsStore = TTLStore + a PodExpectation per controller +// ControlleeExpectation = pair of atomic counters to track controllee's creation/deletion +// ControllerExpectationsStore = TTLStore + a ControlleeExpectation per controller // // * Once set expectations can only be lowered // * A controller isn't synced till its expectations are either fulfilled, or expire -// * Controllers that don't set expectations will get woken up for every matching pod +// * Controllers that don't set expectations will get woken up for every matching controllee -// ExpKeyFunc to parse out the key from a PodExpectation +// ExpKeyFunc to parse out the key from a ControlleeExpectation var ExpKeyFunc = func(obj interface{}) (string, error) { - if e, ok := obj.(*PodExpectations); ok { + if e, ok := obj.(*ControlleeExpectations); ok { return e.key, nil } return "", fmt.Errorf("Could not find key for obj %#v", obj) @@ -96,7 +96,7 @@ var ExpKeyFunc = func(obj interface{}) (string, error) { // Warning: if using KeyFunc it is not safe to use a single ControllerExpectationsInterface with different // types of controllers, because the keys might conflict across types. type ControllerExpectationsInterface interface { - GetExpectations(controllerKey string) (*PodExpectations, bool, error) + GetExpectations(controllerKey string) (*ControlleeExpectations, bool, error) SatisfiedExpectations(controllerKey string) bool DeleteExpectations(controllerKey string) SetExpectations(controllerKey string, add, del int) error @@ -111,10 +111,10 @@ type ControllerExpectations struct { cache.Store } -// GetExpectations returns the PodExpectations of the given controller. -func (r *ControllerExpectations) GetExpectations(controllerKey string) (*PodExpectations, bool, error) { - if podExp, exists, err := r.GetByKey(controllerKey); err == nil && exists { - return podExp.(*PodExpectations), true, nil +// GetExpectations returns the ControlleeExpectations of the given controller. +func (r *ControllerExpectations) GetExpectations(controllerKey string) (*ControlleeExpectations, bool, error) { + if exp, exists, err := r.GetByKey(controllerKey); err == nil && exists { + return exp.(*ControlleeExpectations), true, nil } else { return nil, false, err } @@ -122,22 +122,22 @@ func (r *ControllerExpectations) GetExpectations(controllerKey string) (*PodExpe // DeleteExpectations deletes the expectations of the given controller from the TTLStore. func (r *ControllerExpectations) DeleteExpectations(controllerKey string) { - if podExp, exists, err := r.GetByKey(controllerKey); err == nil && exists { - if err := r.Delete(podExp); err != nil { + if exp, exists, err := r.GetByKey(controllerKey); err == nil && exists { + if err := r.Delete(exp); err != nil { glog.V(2).Infof("Error deleting expectations for controller %v: %v", controllerKey, err) } } } // SatisfiedExpectations returns true if the required adds/dels for the given controller have been observed. -// Add/del counts are established by the controller at sync time, and updated as pods are observed by the controller +// Add/del counts are established by the controller at sync time, and updated as controllees are observed by the controller // manager. func (r *ControllerExpectations) SatisfiedExpectations(controllerKey string) bool { - if podExp, exists, err := r.GetExpectations(controllerKey); exists { - if podExp.Fulfilled() { + if exp, exists, err := r.GetExpectations(controllerKey); exists { + if exp.Fulfilled() { return true } else { - glog.V(4).Infof("Controller still waiting on expectations %#v", podExp) + glog.V(4).Infof("Controller still waiting on expectations %#v", exp) return false } } else if err != nil { @@ -145,9 +145,9 @@ func (r *ControllerExpectations) SatisfiedExpectations(controllerKey string) boo } else { // When a new controller is created, it doesn't have expectations. // When it doesn't see expected watch events for > TTL, the expectations expire. - // - In this case it wakes up, creates/deletes pods, and sets expectations again. - // When it has satisfied expectations and no pods need to be created/destroyed > TTL, the expectations expire. - // - In this case it continues without setting expectations till it needs to create/delete pods. + // - In this case it wakes up, creates/deletes controllees, and sets expectations again. + // When it has satisfied expectations and no controllees need to be created/destroyed > TTL, the expectations expire. + // - In this case it continues without setting expectations till it needs to create/delete controllees. glog.V(4).Infof("Controller %v either never recorded expectations, or the ttl expired.", controllerKey) } // Trigger a sync if we either encountered and error (which shouldn't happen since we're @@ -157,9 +157,9 @@ func (r *ControllerExpectations) SatisfiedExpectations(controllerKey string) boo // SetExpectations registers new expectations for the given controller. Forgets existing expectations. func (r *ControllerExpectations) SetExpectations(controllerKey string, add, del int) error { - podExp := &PodExpectations{add: int64(add), del: int64(del), key: controllerKey} - glog.V(4).Infof("Setting expectations %+v", podExp) - return r.Add(podExp) + exp := &ControlleeExpectations{add: int64(add), del: int64(del), key: controllerKey} + glog.V(4).Infof("Setting expectations %+v", exp) + return r.Add(exp) } func (r *ControllerExpectations) ExpectCreations(controllerKey string, adds int) error { @@ -172,10 +172,10 @@ func (r *ControllerExpectations) ExpectDeletions(controllerKey string, dels int) // Decrements the expectation counts of the given controller. func (r *ControllerExpectations) lowerExpectations(controllerKey string, add, del int) { - if podExp, exists, err := r.GetExpectations(controllerKey); err == nil && exists { - podExp.Seen(int64(add), int64(del)) + if exp, exists, err := r.GetExpectations(controllerKey); err == nil && exists { + exp.Seen(int64(add), int64(del)) // The expectations might've been modified since the update on the previous line. - glog.V(4).Infof("Lowering expectations %+v", podExp) + glog.V(4).Infof("Lowering expectations %+v", exp) } } @@ -194,31 +194,31 @@ type Expectations interface { Fulfilled() bool } -// PodExpectations track pod creates/deletes. -type PodExpectations struct { +// ControlleeExpectations track controllee creates/deletes. +type ControlleeExpectations struct { add int64 del int64 key string } // Seen decrements the add and del counters. -func (e *PodExpectations) Seen(add, del int64) { +func (e *ControlleeExpectations) Seen(add, del int64) { atomic.AddInt64(&e.add, -add) atomic.AddInt64(&e.del, -del) } // Fulfilled returns true if this expectation has been fulfilled. -func (e *PodExpectations) Fulfilled() bool { +func (e *ControlleeExpectations) Fulfilled() bool { // TODO: think about why this line being atomic doesn't matter return atomic.LoadInt64(&e.add) <= 0 && atomic.LoadInt64(&e.del) <= 0 } -// GetExpectations returns the add and del expectations of the pod. -func (e *PodExpectations) GetExpectations() (int64, int64) { +// GetExpectations returns the add and del expectations of the controllee. +func (e *ControlleeExpectations) GetExpectations() (int64, int64) { return atomic.LoadInt64(&e.add), atomic.LoadInt64(&e.del) } -// NewControllerExpectations returns a store for PodExpectations. +// NewControllerExpectations returns a store for ControlleeExpectations. func NewControllerExpectations() *ControllerExpectations { return &ControllerExpectations{cache.NewTTLStore(ExpKeyFunc, ExpectationsTimeout)} }