From f47714b15282b5aabc1764264ca39cada47c2805 Mon Sep 17 00:00:00 2001 From: Hardy Ferentschik Date: Fri, 16 Feb 2018 22:13:30 +0100 Subject: [PATCH] Issue #110 Making sure idler feature is also checked on timeout --- internal/idler/user_idler.go | 19 +++++++- internal/openshift/client/openshift_client.go | 28 +++-------- internal/openshift/controller.go | 48 +++++-------------- internal/openshift/controller_test.go | 7 +-- 4 files changed, 37 insertions(+), 65 deletions(-) diff --git a/internal/idler/user_idler.go b/internal/idler/user_idler.go index 857d192..2b0d0d9 100644 --- a/internal/idler/user_idler.go +++ b/internal/idler/user_idler.go @@ -8,11 +8,14 @@ import ( "github.com/fabric8-services/fabric8-jenkins-idler/internal/configuration" "github.com/fabric8-services/fabric8-jenkins-idler/internal/model" "github.com/fabric8-services/fabric8-jenkins-idler/internal/openshift/client" + "github.com/fabric8-services/fabric8-jenkins-idler/internal/toggles" log "github.com/sirupsen/logrus" "sync" "time" ) +var logger = log.WithFields(log.Fields{"component": "user-idler"}) + const ( bufferSize = 10 ) @@ -25,9 +28,10 @@ type UserIdler struct { userChan chan model.User user model.User config configuration.Configuration + features toggles.Features } -func NewUserIdler(user model.User, openShiftClient client.OpenShiftClient, config configuration.Configuration) *UserIdler { +func NewUserIdler(user model.User, openShiftClient client.OpenShiftClient, config configuration.Configuration, features toggles.Features) *UserIdler { logEntry := log.WithFields(log.Fields{ "component": "user-idler", "username": user.Name, @@ -46,6 +50,7 @@ func NewUserIdler(user model.User, openShiftClient client.OpenShiftClient, confi userChan: userChan, user: user, config: config, + features: features, } return &userIdler } @@ -57,6 +62,18 @@ func (idler *UserIdler) GetChannel() chan model.User { // checkIdle verifies the state of conditions and decides if we should idle/unidle // and performs the required action if needed func (idler *UserIdler) checkIdle() error { + enabled, err := idler.features.IsIdlerEnabled(idler.user.ID) + if err != nil { + return err + } + + if enabled { + logger.WithFields(log.Fields{"user": idler.user.Name, "uuid": idler.user.ID}).Debug("Idler enabled. Evaluating conditions.") + } else { + logger.WithFields(log.Fields{"user": idler.user.Name, "uuid": idler.user.ID}).Debug("Idler not enabled. Skipping idle check.") + return nil + } + eval := idler.Conditions.Eval(idler.user) if eval { diff --git a/internal/openshift/client/openshift_client.go b/internal/openshift/client/openshift_client.go index 0c3fcaa..e3abd9b 100644 --- a/internal/openshift/client/openshift_client.go +++ b/internal/openshift/client/openshift_client.go @@ -25,8 +25,8 @@ type OpenShiftClient interface { IsIdle(namespace string, service string) (int, error) GetRoute(n string, s string) (r string, tls bool, err error) GetApiURL() string - WatchBuilds(namespace string, buildType string, callback func(model.Object) (bool, error)) error - WatchDeploymentConfigs(namespace string, nsSuffix string, callback func(model.DCObject) (bool, error)) error + WatchBuilds(namespace string, buildType string, callback func(model.Object) error) error + WatchDeploymentConfigs(namespace string, nsSuffix string, callback func(model.DCObject) error) error } // OpenShift is a client for OpenShift API @@ -289,7 +289,7 @@ func (o OpenShift) getProjects() (projects []string, err error) { } //WatchBuilds consumes stream of build events from OpenShift and calls callback to process them -func (o OpenShift) WatchBuilds(namespace string, buildType string, callback func(model.Object) (bool, error)) error { +func (o OpenShift) WatchBuilds(namespace string, buildType string, callback func(model.Object) error) error { //Use a http client with disabled timeout c := &http.Client{ Transport: &http.Transport{ @@ -339,22 +339,18 @@ func (o OpenShift) WatchBuilds(namespace string, buildType string, callback func continue } logger.Infof("Handling Build change for user %s", o.Object.Metadata.Namespace) - ok, err := callback(o) + err = callback(o) if err != nil { logger.Errorf("Error from callback: %s", err) continue } - - if ok { - logger.Debugf("Event summary: Build %s -> %s, %s/%s", o.Object.Metadata.Name, o.Object.Status.Phase, o.Object.Status.StartTimestamp, o.Object.Status.CompletionTimestamp) - } } logger.Debug("Fell out of loop for Build") } } // WatchDeploymentConfigs consumes stream of DC events from OpenShift and calls callback to process them -func (o OpenShift) WatchDeploymentConfigs(namespace string, nsSuffix string, callback func(model.DCObject) (bool, error)) error { +func (o OpenShift) WatchDeploymentConfigs(namespace string, nsSuffix string, callback func(model.DCObject) error) error { //Use a http client with disabled timeout c := &http.Client{ Transport: &http.Transport{ @@ -407,23 +403,11 @@ func (o OpenShift) WatchDeploymentConfigs(namespace string, nsSuffix string, cal } logger.Infof("Handling DC change for user %s", o.Object.Metadata.Namespace) - ok, err := callback(o) + err = callback(o) if err != nil { logger.Errorf("Error from DC callback: %s", err) continue } - - if ok { - // TODO - should this go away or at least be conditional? - // Get piece of status for debug info - c, err := o.Object.Status.GetByType("Available") - if err != nil { - logger.Error(err) - continue - } - - logger.Debugf("Event summary: DeploymentConfig %s, %s/%s", o.Object.Metadata.Name, c.Status, c.LastUpdateTime) - } } logger.Debugf("Fall out od loop for watching DC") } diff --git a/internal/openshift/controller.go b/internal/openshift/controller.go index 2461595..7c117a0 100644 --- a/internal/openshift/controller.go +++ b/internal/openshift/controller.go @@ -26,8 +26,8 @@ var logger = log.WithFields(log.Fields{"component": "controller"}) // Controller defines the interface for watching the OpenShift cluster for changes. type Controller interface { - HandleBuild(o model.Object) (bool, error) - HandleDeploymentConfig(dc model.DCObject) (bool, error) + HandleBuild(o model.Object) error + HandleDeploymentConfig(dc model.DCObject) error GetUser(ns string) model.User } @@ -67,29 +67,16 @@ func (oc *OpenShiftController) GetUser(ns string) model.User { // user structure with latest build info. NOTE: In most cases the only change in // build object is stage timestamp, which we don't care about, so this function // just does couple comparisons and returns -func (oc *OpenShiftController) HandleBuild(o model.Object) (bool, error) { +func (oc *OpenShiftController) HandleBuild(o model.Object) error { ns := o.Object.Metadata.Namespace logger.WithField("ns", ns).Infof("Processing build event %s", o.Object.Metadata.Name) err := oc.createIfNotExist(o.Object.Metadata.Namespace) if err != nil { - return false, err + return err } user := oc.userForNamespace(ns) - logger.WithFields(log.Fields{"ns": ns, "uuid": user.ID}).Debug("Checking if Idler is enabled") - - enabled, err := oc.features.IsIdlerEnabled(user.ID) - if err != nil { - return false, err - } - - if enabled { - logger.WithFields(log.Fields{"ns": ns, "uuid": user.ID}).Debug("Idler enabled") - } else { - logger.WithFields(log.Fields{"ns": ns, "uuid": user.ID}).Debug("Idler not enabled") - return false, nil - } if oc.isActive(&o.Object) { lastActive := user.ActiveBuild @@ -116,40 +103,27 @@ func (oc *OpenShiftController) HandleBuild(o model.Object) (bool, error) { oc.sendUserToIdler(ns, user) } - return true, nil + return nil } // HandleDeploymentConfig processes new DC event collected from OpenShift and updates // user structure with info about the changes in DC. NOTE: This is important for cases // like reset tenant and update tenant when DC is updated and Jenkins starts because // of ConfigChange or manual intervention. -func (oc *OpenShiftController) HandleDeploymentConfig(dc model.DCObject) (bool, error) { +func (oc *OpenShiftController) HandleDeploymentConfig(dc model.DCObject) error { ns := dc.Object.Metadata.Namespace[:len(dc.Object.Metadata.Namespace)-len(jenkinsNamespaceSuffix)] logger.WithField("ns", ns).Infof("Processing deployment config change event %s", dc.Object.Metadata.Name) err := oc.createIfNotExist(ns) if err != nil { - return false, err + return err } user := oc.userForNamespace(ns) - logger.WithFields(log.Fields{"ns": ns, "uuid": user.ID}).Debug("Checking if Idler is enabled") - - enabled, err := oc.features.IsIdlerEnabled(user.ID) - if err != nil { - return false, err - } - - if enabled { - logger.WithFields(log.Fields{"ns": ns, "uuid": user.ID}).Debug("Idler enabled") - } else { - logger.WithFields(log.Fields{"ns": ns, "uuid": user.ID}).Debug("Idler not enabled") - return false, nil - } c, err := dc.Object.Status.GetByType(availableCond) if err != nil { - return false, err + return err } // TODO Verify if we need Generation vs. ObservedGeneration @@ -163,7 +137,7 @@ func (oc *OpenShiftController) HandleDeploymentConfig(dc model.DCObject) (bool, // Also check if the event means that Jenkins just started (OS AvailableCondition.Status == true) and update time status, err := strconv.ParseBool(c.Status) if err != nil { - return false, err + return err } if status == true { @@ -172,7 +146,7 @@ func (oc *OpenShiftController) HandleDeploymentConfig(dc model.DCObject) (bool, oc.sendUserToIdler(ns, user) } - return true, nil + return nil } // createIfNotExist check existence of a user in the map, initialise if it does not exist @@ -200,7 +174,7 @@ func (oc *OpenShiftController) createIfNotExist(ns string) error { newUser := model.NewUser(ti.Data[0].Id, ns, state == model.JenkinsRunning) oc.users.Store(ns, newUser) - userIdler := idler.NewUserIdler(newUser, oc.openShiftClient, oc.config) + userIdler := idler.NewUserIdler(newUser, oc.openShiftClient, oc.config, oc.features) oc.userChannels.Store(ns, userIdler.GetChannel()) userIdler.Run(oc.wg, oc.ctx, oc.cancel) diff --git a/internal/openshift/controller_test.go b/internal/openshift/controller_test.go index 0e344f1..59a671d 100644 --- a/internal/openshift/controller_test.go +++ b/internal/openshift/controller_test.go @@ -5,7 +5,6 @@ import ( "testing" "context" - "fmt" "github.com/fabric8-services/fabric8-jenkins-idler/internal/model" "github.com/fabric8-services/fabric8-jenkins-idler/internal/openshift/client" "github.com/fabric8-services/fabric8-jenkins-idler/internal/tenant" @@ -50,9 +49,8 @@ func Test_handle_build(t *testing.T) { Type: "MODIFIED", } - ok, err := controller.HandleBuild(obj) + err := controller.HandleBuild(obj) assert.NoError(t, err) - assert.True(t, ok, fmt.Sprintf("Namespace '%s' should be watched", obj.Object.Metadata.Namespace)) } func Test_handle_deployment_config(t *testing.T) { @@ -76,9 +74,8 @@ func Test_handle_deployment_config(t *testing.T) { Type: "MODIFIED", } - ok, err := controller.HandleDeploymentConfig(obj) + err := controller.HandleDeploymentConfig(obj) assert.NoError(t, err) - assert.True(t, ok, fmt.Sprintf("Namespace '%s' should be watched", obj.Object.Metadata.Namespace)) } func setUp(t *testing.T) {