Skip to content
This repository has been archived by the owner on May 6, 2021. It is now read-only.

Fix #223 Honour cluster-capacity-exhausted flag when unidling Jenkins #227

Merged
merged 2 commits into from
May 3, 2018
Merged

Fix #223 Honour cluster-capacity-exhausted flag when unidling Jenkins #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64381ff
Fix #223 Honour capacity flag when unidling Jenkins
sthaha May 2, 2018
c771b23
Fix #223 Honour capacity flag when unidling Jenkins
sthaha May 2, 2018
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
2 changes: 1 addition & 1 deletion cmd/fabric8-jenkins-idler/idler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (idler *Idler) startWorkers(ctx context.Context, wg *sync.WaitGroup, cancel
// Start API router
go func() {
// Create and start a Router instance to serve the REST API
idlerAPI := api.NewIdlerAPI(userIdlers, idler.clusterView)
idlerAPI := api.NewIdlerAPI(userIdlers, idler.clusterView, idler.tenantService)
apirouter := router.CreateAPIRouter(idlerAPI)
router := router.NewRouter(apirouter)
router.AddMetrics(apirouter)
Expand Down
11 changes: 1 addition & 10 deletions cmd/fabric8-jenkins-idler/idler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/fabric8-services/fabric8-jenkins-idler/internal/cluster"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/configuration"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/tenant"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/testutils/mock"
log "github.com/sirupsen/logrus"
"github.com/sirupsen/logrus/hooks/test"
Expand All @@ -22,14 +21,6 @@ func (m *mockFeatureToggle) IsIdlerEnabled(uid string) (bool, error) {
return true, nil
}

type mockTenantService struct {
}

func (m *mockTenantService) GetTenantInfoByNamespace(apiURL string, ns string) (tenant.InfoList, error) {
return tenant.InfoList{}, nil

}

type mockClusterView struct {
*mock.ClusterView
}
Expand All @@ -53,7 +44,7 @@ func Test_graceful_shutdown(t *testing.T) {
hook := test.NewGlobal()

config, _ := configuration.NewConfiguration()
idler := NewIdler(&mockFeatureToggle{}, &mockTenantService{}, &mockClusterView{}, config)
idler := NewIdler(&mockFeatureToggle{}, &mock.TenantService{}, &mockClusterView{}, config)

go func() {
// Send SIGTERM after two seconds
Expand Down
68 changes: 59 additions & 9 deletions internal/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@ package api

import (
"encoding/json"
"errors"
"fmt"
"net/http"
"strings"
"time"

"github.com/fabric8-services/fabric8-jenkins-idler/internal/cluster"
pidler "github.com/fabric8-services/fabric8-jenkins-idler/internal/idler"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/model"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/openshift"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/openshift/client"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/tenant"
"github.com/fabric8-services/fabric8-jenkins-idler/metric"
"github.com/julienschmidt/httprouter"
log "github.com/sirupsen/logrus"
Expand Down Expand Up @@ -54,20 +57,22 @@ type idler struct {
clusterView cluster.View
openShiftClient client.OpenShiftClient
controller openshift.Controller
tenantService tenant.Service
}

type status struct {
IsIdle bool `json:"is_idle"`
}

// NewIdlerAPI creates a new instance of IdlerAPI.
func NewIdlerAPI(userIdlers *openshift.UserIdlerMap, clusterView cluster.View) IdlerAPI {
func NewIdlerAPI(userIdlers *openshift.UserIdlerMap, clusterView cluster.View, ts tenant.Service) IdlerAPI {
// Initialize metrics
Recorder.Initialize()
return &idler{
userIdlers: userIdlers,
clusterView: clusterView,
openShiftClient: client.NewOpenShift(),
tenantService: ts,
}
}

Expand Down Expand Up @@ -100,23 +105,52 @@ func (api *idler) Idle(w http.ResponseWriter, r *http.Request, ps httprouter.Par
}

func (api *idler) UnIdle(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
openShiftAPI, openShiftBearerToken, err := api.getURLAndToken(r)

openshiftURL, openshiftToken, err := api.getURLAndToken(r)
if err != nil {
log.Error(err)
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(fmt.Sprintf("{\"error\": \"%s\"}", err)))
respondWithError(w, http.StatusBadRequest, err)
Copy link
Member

Choose a reason for hiding this comment

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

Why BadRequestError status and not InternalServerError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be outside the scope of the this commit. The commit does not try to modify the existing behaviour, thus BadRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

Whynot log the error as well? If the consumer does not log the exception, nor show to the user, we will not be able to know what's happening, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm my previous comment

return
}

ns := strings.TrimSpace(ps.ByName("namespace"))
if ns == "" {
err = errors.New("Missing mandatory param namespace")
respondWithError(w, http.StatusBadRequest, err)
return
}

// may be jenkins is already running and in that case we don't have to do unidle it
running, err := api.isJenkinsUnIdled(openshiftURL, openshiftToken, ns)
if err != nil {
respondWithError(w, http.StatusInternalServerError, err)
return
} else if running {
log.Infof("Jenkins is already starting/running on %s", ns)
w.WriteHeader(http.StatusOK)
return
}

// now that jenkins isn't running we need to check if the cluster has reached
// its maximum capacity
clusterFull, err := api.tenantService.HasReachedMaxCapacity(openshiftURL, ns)
if err != nil {
respondWithError(w, http.StatusInternalServerError, err)
return
} else if clusterFull {
err := fmt.Errorf("Maximum Resource limit reached on %s for %s", openshiftURL, ns)
respondWithError(w, http.StatusServiceUnavailable, err)
return
}

// unidle now
for _, service := range pidler.JenkinsServices {
startTime := time.Now()
err = api.openShiftClient.UnIdle(openShiftAPI, openShiftBearerToken, ps.ByName("namespace"), service)

err = api.openShiftClient.UnIdle(openshiftURL, openshiftToken, ns, service)
elapsedTime := time.Since(startTime).Seconds()
if err != nil {
log.Error(err)
Recorder.RecordReqDuration(service, "UnIdle", http.StatusInternalServerError, elapsedTime)
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(fmt.Sprintf("{\"error\": \"%s\"}", err)))
respondWithError(w, http.StatusInternalServerError, err)
return
}

Expand Down Expand Up @@ -190,3 +224,19 @@ func (api *idler) getURLAndToken(r *http.Request) (string, string, error) {
}
return "", "", fmt.Errorf("Unknown or invalid OpenShift API URL")
}

func (api idler) isJenkinsUnIdled(openshiftURL, openshiftToken, namespace string) (bool, error) {
state, err := api.openShiftClient.IsIdle(openshiftURL, openshiftToken, namespace, "jenkins")
if err != nil {
return false, err
}

status := state == model.JenkinsStarting || state == model.JenkinsRunning
Copy link
Member

Choose a reason for hiding this comment

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

@sthaha Eventhough it looks good to me, let's be sure about the precedence here https://golang.org/ref/spec#Operator_precedence

Is go-format getting rid of brackets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kishansagathiya I didn't put brackets because the precedence is clear to me and tbh we don't need to make any simpler ... If there is guideline I am happy to follow it but that is beyond the scope of this change.

Copy link
Member

Choose a reason for hiding this comment

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

cool then

return status, nil
}

func respondWithError(w http.ResponseWriter, status int, err error) {
log.Error(err)
w.WriteHeader(status)
w.Write([]byte(fmt.Sprintf("{\"error\": \"%s\"}", err)))
}
16 changes: 13 additions & 3 deletions internal/api/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,22 @@ func Test_success(t *testing.T) {
mockidle := idler{
openShiftClient: mosc,
clusterView: &mock.ClusterView{},
tenantService: &mock.TenantService{},
}
functions := []ReqFuncType{mockidle.Idle, mockidle.UnIdle, mockidle.IsIdle}

params := httprouter.Params{
httprouter.Param{Key: "namespace", Value: "foobar"},
}

for _, function := range functions {
reader, _ := http.NewRequest("GET", "/", nil)
q := reader.URL.Query()
q.Add(OpenShiftAPIParam, "http://localhost")
reader.URL.RawQuery = q.Encode()

writer := &mock.ResponseWriter{}
function(writer, reader, nil)
function(writer, reader, params)

require.Equal(t, http.StatusOK, writer.WriterStatus, fmt.Sprintf("Bad Error Code: %d", writer.WriterStatus))
require.Equal(t, mosc.IdleCallCount, 2, fmt.Sprintf("Idle was not called for 2 times but %d", mosc.IdleCallCount))
Expand Down Expand Up @@ -62,17 +67,22 @@ func Test_fail(t *testing.T) {
openShiftClient: &mock.OpenShiftClient{
IdleError: idleError,
},
clusterView: &mock.ClusterView{},
clusterView: &mock.ClusterView{},
tenantService: &mock.TenantService{},
}
functions = []ReqFuncType{mockidle.Idle, mockidle.UnIdle, mockidle.IsIdle}
params := httprouter.Params{
httprouter.Param{Key: "namespace", Value: "foobar"},
}

for _, function := range functions {
req, _ := http.NewRequest("GET", "/", nil)
query := req.URL.Query()
query.Add(OpenShiftAPIParam, "http://localhost")
req.URL.RawQuery = query.Encode()

writer := &mock.ResponseWriter{}
function(writer, req, nil)
function(writer, req, params)

jserror := &JSError{}
_ = json.Unmarshal(writer.Buffer.Bytes(), &jserror)
Expand Down
48 changes: 34 additions & 14 deletions internal/idler/user_idler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ 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/tenant"
"github.com/fabric8-services/fabric8-jenkins-idler/internal/toggles"
log "github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -45,11 +46,18 @@ type UserIdler struct {
user model.User
config configuration.Configuration
features toggles.Features
tenantService tenant.Service
}

// NewUserIdler creates an instance of UserIdler.
// It returns a pointer to UserIdler,
func NewUserIdler(user model.User, openShiftAPI string, openShiftBearerToken string, config configuration.Configuration, features toggles.Features) *UserIdler {
func NewUserIdler(
user model.User,
openShiftAPI, openShiftBearerToken string,
config configuration.Configuration,
features toggles.Features,
tenantService tenant.Service) *UserIdler {

logEntry := log.WithFields(log.Fields{
"component": "user-idler",
"username": user.Name,
Expand All @@ -74,6 +82,7 @@ func NewUserIdler(user model.User, openShiftAPI string, openShiftBearerToken str
user: user,
config: config,
features: features,
tenantService: tenantService,
}
return &userIdler
}
Expand Down Expand Up @@ -184,6 +193,7 @@ func (idler *UserIdler) doIdle() error {
}

func (idler *UserIdler) doUnIdle() error {

if idler.unIdleAttempts >= idler.maxRetries {
idler.logger.Warn("Skipping un-idle request since max retry count has been reached.")
return nil
Expand All @@ -193,20 +203,30 @@ func (idler *UserIdler) doUnIdle() error {
if err != nil {
return err
}
if state != model.JenkinsIdled {
return nil
}

if state == model.JenkinsIdled {
idler.incrementUnIdleAttempts()
for _, service := range JenkinsServices {
// Let's add some more reasons, we probably want to
reasonString := fmt.Sprintf("DoneBuild BuildName:%s Last:%s", idler.user.DoneBuild.Metadata.Name, idler.user.DoneBuild.Status.StartTimestamp.Time)
if idler.user.ActiveBuild.Metadata.Name != "" {
reasonString = fmt.Sprintf("ActiveBuild BuildName:%s Last:%s", idler.user.ActiveBuild.Metadata.Name, idler.user.ActiveBuild.Status.StartTimestamp.Time)
}
idler.logger.WithField("attempt", fmt.Sprintf("(%d/%d)", idler.unIdleAttempts, idler.maxRetries)).Info("About to un-idle "+service+", Reason: ", reasonString)
err := idler.openShiftClient.UnIdle(idler.openShiftAPI, idler.openShiftBearerToken, idler.user.Name+jenkinsNamespaceSuffix, service)
if err != nil {
return err
}
ns := idler.user.Name + jenkinsNamespaceSuffix
clusterFull, err := idler.tenantService.HasReachedMaxCapacity(idler.openShiftAPI, ns)
if err != nil {
return err
} else if clusterFull {
err := fmt.Errorf("Maximum Resource limit reached on %s for %s", idler.openShiftAPI, ns)
return err
}

idler.incrementUnIdleAttempts()
for _, service := range JenkinsServices {
// Let's add some more reasons, we probably want to
reasonString := fmt.Sprintf("DoneBuild BuildName:%s Last:%s", idler.user.DoneBuild.Metadata.Name, idler.user.DoneBuild.Status.StartTimestamp.Time)
if idler.user.ActiveBuild.Metadata.Name != "" {
reasonString = fmt.Sprintf("ActiveBuild BuildName:%s Last:%s", idler.user.ActiveBuild.Metadata.Name, idler.user.ActiveBuild.Status.StartTimestamp.Time)
}
idler.logger.WithField("attempt", fmt.Sprintf("(%d/%d)", idler.unIdleAttempts, idler.maxRetries)).Info("About to un-idle "+service+", Reason: ", reasonString)
err := idler.openShiftClient.UnIdle(idler.openShiftAPI, idler.openShiftBearerToken, ns, service)
if err != nil {
return err
}
}
return nil
Expand Down
21 changes: 16 additions & 5 deletions internal/idler/user_idler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ func Test_idle_check_skipped_if_feature_not_enabled(t *testing.T) {
hook := test.NewGlobal()

user := model.User{ID: "100"}
userIdler := NewUserIdler(user, "", "", &mock.Config{}, mock.NewMockFeatureToggle([]string{"42"}))
userIdler := NewUserIdler(
user, "", "", &mock.Config{},
mock.NewMockFeatureToggle([]string{"42"}),
&mock.TenantService{},
)

err := userIdler.checkIdle()
assert.NoError(t, err, "No error expected.")
Expand All @@ -51,7 +55,11 @@ func Test_idle_check_returns_error_on_evaluation_failure(t *testing.T) {
log.SetOutput(ioutil.Discard)

user := model.User{ID: "42"}
userIdler := NewUserIdler(user, "", "", &mock.Config{}, mock.NewMockFeatureToggle([]string{"42"}))
userIdler := NewUserIdler(
user, "", "", &mock.Config{},
mock.NewMockFeatureToggle([]string{"42"}),
&mock.TenantService{},
)
userIdler.Conditions.Add("error", &ErrorCondition{})

err := userIdler.checkIdle()
Expand All @@ -67,7 +75,8 @@ func Test_idle_check_occurs_even_without_openshift_events(t *testing.T) {
user := model.User{ID: "100", Name: "John Doe"}
config := &mock.Config{}
features := mock.NewMockFeatureToggle([]string{"42"})
userIdler := NewUserIdler(user, "", "", config, features)
tenantService := &mock.TenantService{}
userIdler := NewUserIdler(user, "", "", config, features, tenantService)

var wg sync.WaitGroup
ctx, cancel := context.WithCancel(context.Background())
Expand Down Expand Up @@ -120,7 +129,8 @@ func Test_number_of_idle_calls_are_capped(t *testing.T) {
maxRetry := 10
config.MaxRetries = maxRetry
features := mock.NewMockFeatureToggle([]string{"42"})
userIdler := NewUserIdler(user, "", "", config, features)
tenantService := &mock.TenantService{}
userIdler := NewUserIdler(user, "", "", config, features, tenantService)
userIdler.openShiftClient = openShiftClient

var wg sync.WaitGroup
Expand Down Expand Up @@ -167,8 +177,9 @@ func Test_number_of_unidle_calls_are_capped(t *testing.T) {
config.MaxRetries = maxRetry

features := mock.NewMockFeatureToggle([]string{"42"})
tenantService := &mock.TenantService{}

userIdler := NewUserIdler(user, "", "", config, features)
userIdler := NewUserIdler(user, "", "", config, features, tenantService)
userIdler.openShiftClient = openShiftClient
conditions := condition.NewConditions()
conditions.Add("unidle", &UnIdleCondition{})
Expand Down
2 changes: 1 addition & 1 deletion internal/openshift/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ func (c *controllerImpl) createIfNotExist(ns string) (bool, error) {
}

newUser := model.NewUser(ti.Data[0].ID, ns)
userIdler := idler.NewUserIdler(newUser, c.openShiftAPIURL, c.openShiftBearerToken, c.config, c.features)
userIdler := idler.NewUserIdler(newUser, c.openShiftAPIURL, c.openShiftBearerToken, c.config, c.features, c.tenantService)
c.userIdlers.Store(ns, userIdler)
userIdler.Run(c.ctx, c.wg, c.cancel, time.Duration(c.config.GetCheckInterval())*time.Minute, time.Duration(c.config.GetMaxRetriesQuietInterval())*time.Minute)
return true, nil
Expand Down
Loading