Skip to content

Refactor deploy package for error handling support #1050

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.

Sign up for GitHub

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

Merged
openshift-bot merged 1 commit into from
Feb 18, 2015
Merged

Refactor deploy package for error handling support #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 67 additions & 43 deletions pkg/deploy/controller/config_change_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,41 @@
package controller

import (
"fmt"

"github.com/golang/glog"

kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
cache "github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache"
kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
runtime "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
util "github.com/GoogleCloudPlatform/kubernetes/pkg/util"

deployapi "github.com/openshift/origin/pkg/deploy/api"
deployutil "github.com/openshift/origin/pkg/deploy/util"

"github.com/golang/glog"
)

// DeploymentConfigChangeController watches for changes to DeploymentConfigs and regenerates them only
// when detecting a change to the PodTemplate of a DeploymentConfig containing a ConfigChange trigger.
type DeploymentConfigChangeController struct {
ChangeStrategy ChangeStrategy
NextDeploymentConfig func() *deployapi.DeploymentConfig
DeploymentStore cache.Store
Codec runtime.Codec
// Stop is an optional channel that controls when the controller exits
Stop <-chan struct{}
}

// ChangeStrategy knows how to generate and update DeploymentConfigs.
type ChangeStrategy interface {
GenerateDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error)
UpdateDeploymentConfig(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error)
}

// Run watches for config change events.
func (dc *DeploymentConfigChangeController) Run() {
go util.Until(func() { dc.HandleDeploymentConfig() }, 0, dc.Stop)
go util.Until(func() {
err := dc.HandleDeploymentConfig(dc.NextDeploymentConfig())
if err != nil {
glog.Errorf("%v", err)
}
}, 0, dc.Stop)
}

// HandleDeploymentConfig handles the next DeploymentConfig change that happens.
func (dc *DeploymentConfigChangeController) HandleDeploymentConfig() {
config := dc.NextDeploymentConfig()

func (dc *DeploymentConfigChangeController) HandleDeploymentConfig(config *deployapi.DeploymentConfig) error {
Copy link
Contributor

@smarterclayton smarterclayton Feb 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to make this more obviously the focus of this controller - like Next(...)

Copy link
Contributor

@smarterclayton smarterclayton Feb 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or "Do" or "Work" or "Handle"

Copy link
Contributor Author

@ironcladlou ironcladlou Feb 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already "HandleSomething", and one of the controller has two Handles to handle more than one thing. Doesn't seem worth changing to me. Handle handle handle. That word now sounds weird to me.

hasChangeTrigger := false
for _, trigger := range config.Triggers {
if trigger.Type == deployapi.DeploymentTriggerOnConfigChange {
Expand All @@ -47,59 +45,57 @@ func (dc *DeploymentConfigChangeController) HandleDeploymentConfig() {
}

if !hasChangeTrigger {
glog.V(4).Infof("Config has no change trigger; skipping")
return
glog.V(4).Infof("Ignoring config %s; no change triggers detected", labelFor(config))
return nil
}

if config.LatestVersion == 0 {
glog.V(4).Infof("Creating new deployment for config %v", config.Name)
dc.generateDeployment(config, nil)
return
_, _, err := dc.generateDeployment(config)
if err != nil {
return fmt.Errorf("couldn't create initial deployment for config %s: %v", labelFor(config), err)
}
glog.V(4).Infof("Created initial deployment for config %s", labelFor(config))
return nil
}

latestDeploymentID := deployutil.LatestDeploymentNameForConfig(config)
obj, exists, err := dc.DeploymentStore.Get(&kapi.ReplicationController{ObjectMeta: kapi.ObjectMeta{Name: latestDeploymentID, Namespace: config.Namespace}})
latestDeploymentName := deployutil.LatestDeploymentNameForConfig(config)
deployment, err := dc.ChangeStrategy.GetDeployment(config.Namespace, latestDeploymentName)
if err != nil {
glog.Errorf("Unable to retrieve deployment from store: %v", err)
}

if !exists {
glog.V(4).Info("Ignoring config change due to lack of existing deployment")
return
if kerrors.IsNotFound(err) {
glog.V(4).Info("Ignoring config change for %s; no existing deployment found", labelFor(config))
return nil
}
return fmt.Errorf("couldn't retrieve deployment for %s: %v", labelFor(config), err)
}

deployment := obj.(*kapi.ReplicationController)

deployedConfig, err := deployutil.DecodeDeploymentConfig(deployment, dc.Codec)
if err != nil {
glog.V(0).Infof("Error decoding deploymentConfig from deployment %s: %v", deployment.Name, err)
return
return fmt.Errorf("error decoding deploymentConfig from deployment %s for config %s: %v", labelForDeployment(deployment), labelFor(config), err)
}

if deployutil.PodSpecsEqual(config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec) {
glog.V(4).Infof("Ignoring updated config %s with LatestVersion=%d because it matches deployed config %s", config.Name, config.LatestVersion, deployment.Name)
return
glog.V(4).Infof("Ignoring config change for %s (latestVersion=%d); same as deployment %s", labelFor(config), config.LatestVersion, labelForDeployment(deployment))
return nil
}
glog.V(4).Infof("Diff:\n%s", util.ObjectDiff(config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec))

dc.generateDeployment(config, deployment)
fromVersion, toVersion, err := dc.generateDeployment(config)
if err != nil {
return fmt.Errorf("couldn't generate deployment for config %s: %v", labelFor(config), err)
}
glog.V(4).Infof("Updated config %s from version %d to %d for existing deployment %s", labelFor(config), fromVersion, toVersion, labelForDeployment(deployment))
return nil
}

func (dc *DeploymentConfigChangeController) generateDeployment(config *deployapi.DeploymentConfig, deployment *kapi.ReplicationController) {
func (dc *DeploymentConfigChangeController) generateDeployment(config *deployapi.DeploymentConfig) (int, int, error) {
newConfig, err := dc.ChangeStrategy.GenerateDeploymentConfig(config.Namespace, config.Name)
if err != nil {
glog.V(2).Infof("Error generating new version of deploymentConfig %v: %#v", config.Name, err)
return
return config.LatestVersion, 0, fmt.Errorf("Error generating new version of deploymentConfig %s: %v", labelFor(config), err)
}

if newConfig.LatestVersion == config.LatestVersion {
newConfig.LatestVersion++
}

if deployment != nil {
glog.V(4).Infof("Updating config %s (LatestVersion: %d -> %d) to advance existing deployment %s", config.Name, config.LatestVersion, newConfig.LatestVersion, deployment.Name)
}

// set the trigger details for the new deployment config
causes := []*deployapi.DeploymentCause{}
causes = append(causes,
Expand All @@ -114,6 +110,34 @@ func (dc *DeploymentConfigChangeController) generateDeployment(config *deployapi
// okay - we can just ignore the update for the old resource and any changes to the more
// current config will be captured in future events.
if _, err = dc.ChangeStrategy.UpdateDeploymentConfig(config.Namespace, newConfig); err != nil {
glog.V(2).Infof("Error updating deploymentConfig %v: %#v", config.Name, err)
return config.LatestVersion, newConfig.LatestVersion, fmt.Errorf("couldn't update deploymentConfig %s: %v", labelFor(config), err)
}

return config.LatestVersion, newConfig.LatestVersion, nil
}

// ChangeStrategy knows how to generate and update DeploymentConfigs.
type ChangeStrategy interface {
GetDeployment(namespace, name string) (*kapi.ReplicationController, error)
GenerateDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error)
UpdateDeploymentConfig(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error)
}

// ChangeStrategyImpl is a pluggable ChangeStrategy.
type ChangeStrategyImpl struct {
GetDeploymentFunc func(namespace, name string) (*kapi.ReplicationController, error)
GenerateDeploymentConfigFunc func(namespace, name string) (*deployapi.DeploymentConfig, error)
UpdateDeploymentConfigFunc func(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error)
}

func (i *ChangeStrategyImpl) GetDeployment(namespace, name string) (*kapi.ReplicationController, error) {
return i.GetDeploymentFunc(namespace, name)
}

func (i *ChangeStrategyImpl) GenerateDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error) {
return i.GenerateDeploymentConfigFunc(namespace, name)
}

func (i *ChangeStrategyImpl) UpdateDeploymentConfig(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
return i.UpdateDeploymentConfigFunc(namespace, config)
}
98 changes: 39 additions & 59 deletions pkg/deploy/controller/config_change_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,43 +8,31 @@ import (
api "github.com/openshift/origin/pkg/api/latest"
deployapi "github.com/openshift/origin/pkg/deploy/api"
deployapitest "github.com/openshift/origin/pkg/deploy/api/test"
deploytest "github.com/openshift/origin/pkg/deploy/controller/test"
deployutil "github.com/openshift/origin/pkg/deploy/util"
)

// Test the controller's response to a new DeploymentConfig with a config change trigger.
func TestNewConfigWithoutTrigger(t *testing.T) {
generated := false
updated := false

controller := &DeploymentConfigChangeController{
Codec: api.Codec,
ChangeStrategy: &testChangeStrategy{
ChangeStrategy: &ChangeStrategyImpl{
GenerateDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) {
generated = true
t.Fatalf("unexpected generation of deploymentConfig")
return nil, nil
},
UpdateDeploymentConfigFunc: func(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
updated = true
t.Fatalf("unexpected update of deploymentConfig")
return config, nil
},
},
NextDeploymentConfig: func() *deployapi.DeploymentConfig {
config := deployapitest.OkDeploymentConfig(1)
config.Triggers = []deployapi.DeploymentTriggerPolicy{}
return config
},
DeploymentStore: deploytest.NewFakeDeploymentStore(nil),
}

controller.HandleDeploymentConfig()

if generated {
t.Error("Unexpected generation of deploymentConfig")
}
config := deployapitest.OkDeploymentConfig(1)
config.Triggers = []deployapi.DeploymentTriggerPolicy{}
err := controller.HandleDeploymentConfig(config)

if updated {
t.Error("Unexpected update of deploymentConfig")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
}

Expand All @@ -53,7 +41,7 @@ func TestNewConfigWithTrigger(t *testing.T) {

controller := &DeploymentConfigChangeController{
Codec: api.Codec,
ChangeStrategy: &testChangeStrategy{
ChangeStrategy: &ChangeStrategyImpl{
GenerateDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) {
return deployapitest.OkDeploymentConfig(1), nil
},
Expand All @@ -62,15 +50,15 @@ func TestNewConfigWithTrigger(t *testing.T) {
return config, nil
},
},
NextDeploymentConfig: func() *deployapi.DeploymentConfig {
config := deployapitest.OkDeploymentConfig(0)
config.Triggers = []deployapi.DeploymentTriggerPolicy{deployapitest.OkConfigChangeTrigger()}
return config
},
DeploymentStore: deploytest.NewFakeDeploymentStore(nil),
}

controller.HandleDeploymentConfig()
config := deployapitest.OkDeploymentConfig(0)
config.Triggers = []deployapi.DeploymentTriggerPolicy{deployapitest.OkConfigChangeTrigger()}
err := controller.HandleDeploymentConfig(config)

if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if updated == nil {
t.Fatalf("expected config to be updated")
Expand All @@ -92,29 +80,32 @@ func TestNewConfigWithTrigger(t *testing.T) {
// Test the controller's response when the pod template is changed
func TestChangeWithTemplateDiff(t *testing.T) {
var updated *deployapi.DeploymentConfig
deployment, _ := deployutil.MakeDeployment(deployapitest.OkDeploymentConfig(1), kapi.Codec)

controller := &DeploymentConfigChangeController{
Codec: api.Codec,
ChangeStrategy: &testChangeStrategy{
ChangeStrategy: &ChangeStrategyImpl{
GenerateDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) {
return deployapitest.OkDeploymentConfig(2), nil
},
UpdateDeploymentConfigFunc: func(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
updated = config
return config, nil
},
GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) {
deployment, _ := deployutil.MakeDeployment(deployapitest.OkDeploymentConfig(1), kapi.Codec)
return deployment, nil
},
},
NextDeploymentConfig: func() *deployapi.DeploymentConfig {
config := deployapitest.OkDeploymentConfig(1)
config.Triggers = []deployapi.DeploymentTriggerPolicy{deployapitest.OkConfigChangeTrigger()}
config.Template.ControllerTemplate.Template.Spec.Containers[1].Name = "modified"
return config
},
DeploymentStore: deploytest.NewFakeDeploymentStore(deployment),
}

controller.HandleDeploymentConfig()
config := deployapitest.OkDeploymentConfig(1)
config.Triggers = []deployapi.DeploymentTriggerPolicy{deployapitest.OkConfigChangeTrigger()}
config.Template.ControllerTemplate.Template.Spec.Containers[1].Name = "modified"
err := controller.HandleDeploymentConfig(config)

if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if updated == nil {
t.Fatalf("expected config to be updated")
Expand All @@ -137,14 +128,12 @@ func TestChangeWithoutTemplateDiff(t *testing.T) {
config := deployapitest.OkDeploymentConfig(1)
config.Triggers = []deployapi.DeploymentTriggerPolicy{deployapitest.OkConfigChangeTrigger()}

deployment, _ := deployutil.MakeDeployment(deployapitest.OkDeploymentConfig(1), kapi.Codec)

generated := false
updated := false

controller := &DeploymentConfigChangeController{
Codec: api.Codec,
ChangeStrategy: &testChangeStrategy{
ChangeStrategy: &ChangeStrategyImpl{
GenerateDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) {
generated = true
return config, nil
Expand All @@ -153,14 +142,18 @@ func TestChangeWithoutTemplateDiff(t *testing.T) {
updated = true
return config, nil
},
GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) {
deployment, _ := deployutil.MakeDeployment(deployapitest.OkDeploymentConfig(1), kapi.Codec)
return deployment, nil
},
},
NextDeploymentConfig: func() *deployapi.DeploymentConfig {
return config
},
DeploymentStore: deploytest.NewFakeDeploymentStore(deployment),
}

controller.HandleDeploymentConfig()
err := controller.HandleDeploymentConfig(config)

if err != nil {
t.Fatalf("unexpected error: %v", err)
}

if generated {
t.Error("Unexpected generation of deploymentConfig")
Expand All @@ -170,16 +163,3 @@ func TestChangeWithoutTemplateDiff(t *testing.T) {
t.Error("Unexpected update of deploymentConfig")
}
}

type testChangeStrategy struct {
GenerateDeploymentConfigFunc func(namespace, name string) (*deployapi.DeploymentConfig, error)
UpdateDeploymentConfigFunc func(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error)
}

func (i *testChangeStrategy) GenerateDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error) {
return i.GenerateDeploymentConfigFunc(namespace, name)
}

func (i *testChangeStrategy) UpdateDeploymentConfig(namespace string, config *deployapi.DeploymentConfig) (*deployapi.DeploymentConfig, error) {
return i.UpdateDeploymentConfigFunc(namespace, config)
}
Loading