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

sthaha · 2018-05-02T02:45:59Z

No description provided.

centos-ci · 2018-05-02T02:45:59Z

Can one of the admins verify this patch?

alien-ike · 2018-05-02T02:46:00Z

Ike Plugins (test-keeper)

Thank you @sthaha for this contribution!

It appears that no tests have been added or updated in this PR.

Automated tests give us confidence in shipping reliable software. Please add some as part of this change.

If you are an admin or the reviewer of this PR and you are sure that no test is needed then you can use the command /ok-without-tests as a comment to make the status green.

For more information please head over to official documentation. You can find there how to configure the plugin - for example exclude certain file types so if PR contains only them it won't be checked.

sthaha · 2018-05-02T02:51:43Z

internal/api/api.go

@@ -99,17 +102,36 @@ func (api *idler) Idle(w http.ResponseWriter, r *http.Request, ps httprouter.Par
 	w.WriteHeader(http.StatusOK)
 }

+func respondWithError(w http.ResponseWriter, status int, err error) {


May wnat to move this to a new package.

sthaha · 2018-05-02T02:56:10Z

internal/tenant/tenant.go

+		return true, err
+	}
+
+	if len(ti.Errors) != 0 {


I guess this must be handled in the GetTenantInfoNamepace itself. Thoughts @chmouel @kishansagathiya ?

should all the errors not be concatenated instead of returning just the first one?

I think that is probably happening. err is the concatenated error from all errors in ti.Errors. @sthaha Can you make sure of this?

sthaha · 2018-05-02T04:12:59Z

internal/api/api.go

+		return
+	}
+
+	if clusterFull {


Since this does not check if the service is already running, it would return ServiceUnavailable even if the services are running.

Probably we should start checking that

kishansagathiya · 2018-05-02T04:12:58Z

internal/api/api.go

-		log.Error(err)
-		w.WriteHeader(http.StatusBadRequest)
-		w.Write([]byte(fmt.Sprintf("{\"error\": \"%s\"}", err)))
+		respondWithError(w, http.StatusBadRequest, err)


Why BadRequestError status and not InternalServerError?

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

kishansagathiya · 2018-05-02T04:13:18Z

internal/api/api.go

+	ns := ps.ByName("namespace")
+	clusterFull, err := api.tenantService.HasReachedMaxCapacity(openShiftAPI, ns)
+	if err != nil {
+		respondWithError(w, http.StatusBadRequest, err)


Same,
Why BadRequestError status and not InternalServerError?

kishansagathiya · 2018-05-02T04:13:46Z

internal/api/api.go

+
+	if clusterFull {
+		err := fmt.Errorf("Maximum Resource limit reached on %s", openShiftAPI)
+		respondWithError(w, http.StatusServiceUnavailable, err)


kishansagathiya · 2018-05-02T05:00:52Z

internal/tenant/tenant.go

+		if ns.Name == name {
+			return i, true
+		}
+	}


How about

for i, ns := range namespaces { if ns.Name == name { return i, true } }

?

👍 thanks, will fix.

concaf · 2018-05-02T05:16:39Z

internal/api/api.go

+		return
+	}
+
+	ns := ps.ByName("namespace")


this returns an empty string if no matches are found. Does it make sense to check for that here?

good catch. lets return BadRequest in that case! .. btw I did not change the old implemenation ;-)

concaf · 2018-05-02T05:21:03Z

internal/tenant/tenant.go

+
+	ti, err := t.GetTenantInfoByNamespace(apiURL, ns)
+	if err != nil {
+		return true, err


returns true here means that the cluster has reached maximum capacity, along with an error, sort of does not make sense. Should this not be false, err?

No, true, to err on the side of caution. I.E. if we can't find out about the tenant information, treat it as full so that we don't allocate unless sure. Moreover the caller is expected to handle errors :)

Yes I think that returning true is safer in this case.

concaf · 2018-05-02T05:21:24Z

internal/tenant/tenant.go

+	if len(ti.Errors) != 0 {
+		firstError := ti.Errors[0]
+		err = fmt.Errorf("%s - %s", firstError.Code, firstError.Detail)
+		return true, err


false, err?

same as above.

concaf · 2018-05-02T05:23:49Z

internal/tenant/tenant.go

+
+	if len(ti.Errors) != 0 {
+		firstError := ti.Errors[0]
+		err = fmt.Errorf("%s - %s", firstError.Code, firstError.Detail)


should all the errors not be concatenated instead of returning just the first one?

I am not sure if we should but this can be addressed in a separate patch.

It is really simple logic, why not just do it in this PR?

this gets the point across to the caller

appending all errors isn't the right thing to do rather, all errors must be output as array of error
e.g. json would now be

{ "errors": [ {"error": "description .."}, {"error": "description .."}, {"error": "description .."}, {"error": "description .."}, ] }

instead of the current

{"error": "description .."},

Ok then if there is a lot of work to be done, let's do it in another PR but I encourage to do it ASAP or at least open an issue about it, because it is the typical thing that is never done and when there are problems we wish to have all info

ack - raised #231

concaf · 2018-05-02T05:26:06Z

internal/tenant/tenant.go

+		return true, err
+	}
+
+	namespaces := ti.Data[0].Attributes.Namespaces


should we check if ti.Data != nil or/and if len(ti.Data) > 0 before accessing the first element to avoid the nil pointer exception

hmm, good catch but my assumption was that the json is expected to return data if there aren't errors
However, good idea to check that assumption :) and sufficient to check the len

var x []int fmt.Printf("%d", len(x)) // print 0

concaf · 2018-05-02T05:28:41Z

internal/tenant/tenant.go

+}
+
+// returns the index of the namespace that equals 'name'
+func indexOfNamespaceWithName(namespaces []Namespace, name string) (int, bool) {


nit (feel free to ignore): do we need to return both, an int and a bool, here? How about returning just a -1 in the end which would convey that no match was found.

could be ... but aligns with how go does this with maps [ ] operator

v, ok := lookup[key]

but in that case, there is a good reason to return bool. I will change the implementation.

concaf · 2018-05-02T05:50:35Z

internal/tenant/tenant.go

+	namespaces := ti.Data[0].Attributes.Namespaces
+	index, found := indexOfNamespaceWithName(namespaces, ns)
+	if !found {
+		return true, fmt.Errorf("Failed to find %s in tenant info", ns)


false, err?

kishansagathiya · 2018-05-02T09:13:03Z

internal/api/api.go

@@ -100,23 +103,47 @@ 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, authToken, err := api.getURLAndToken(r)


authToken is misleading. Often times by authToken we mean OSIOToken. Use OSOToken or OpenShiftToken?

ack. going tor rename it to openshiftToken

lordofthejars · 2018-05-02T14:02:50Z

internal/api/api.go

-		log.Error(err)
-		w.WriteHeader(http.StatusBadRequest)
-		w.Write([]byte(fmt.Sprintf("{\"error\": \"%s\"}", err)))
+		respondWithError(w, http.StatusBadRequest, err)


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?

nvm my previous comment

lordofthejars · 2018-05-02T14:30:01Z

internal/tenant/tenant.go

+}
+
+// returns the index of the namespace that equals 'name'
+func indexOfNamespaceWithName(namespaces []Namespace, name string) int {


Why are you returning the index instead of the namespace. So you can do something like if ns == nil {}

IndexOf is quiet common and slice[x] is a O(1) operation

to return nil the find function would now need to return a pointer to namespace so that the caller can do a nil check which I am not a big fan of and doesn't really save us a condition check or anything.

I think it is not necessary to return a pointer you can do something like

var ns Namespace for ...... { if found { ns = namespace } return ns }

So no pointer. But it was just a comment not from the point of view of performance but as a reader.

Have you tried it? my quick trial resulted in a compilation error

package main import ( "fmt" ) type Person struct { name string age int } func nilObj() Person { var p Person return p } func main() { fmt.Println("%v", nilObj()) if nilObj() == nil { fmt.Println(".... works") } }

./nil_obj.go:19:14: cannot convert nil to type Person

Am I missing something?

Ok true you need a pointer at the end. Nevermind. Humm this golang language :P

Actually makes sense to me ... If you go by the spec, a variable declaration will always 0 initialise the variable so, var x int initialises x to 0 and var s string to "" thus avoiding the "million dollar mistake" of having to or forgetting to do nil checks all the time.

And in my case struct Person will be 0 initialised and both name and age will be "", 0 respectively. So the func objNil() Person promises to return a Person instance all the time else it fails to compile.

This commit partially addresses the [idler issue][idler-issue] which is a sub-task of [OSIO Issue][osio-issue] to honour `cluster-capacity-exhausted` flag. API requests to wake up (unidle) Jenkins and content-service is now rate-limited by checking if the flag is set. On reaching the cluster capacity, http requests to `unidle` will now receive a `503` response. The patch further modifies the behaviour of `unidle` api call to respond early with 200-OK if the service is already getting started/running. osio-issue: openshiftio/openshift.io#3320 idler-issue: #223

This commit addresses the [idler issue][idler-issue] which is a sub-task of [OSIO Issue][osio-issue] to honour `cluster-capacity-exhausted` flag. The patch fixes `user-idler` part of idler that listens to openshift events and chooses to idler/unidle Jenkins. osio-issue: openshiftio/openshift.io#3320 idler-issue: #223

kishansagathiya

Seems like quite a change. The code looks good to me, but haven't spent too much time looking and thinking of the code.

Let's observe prod-preview through kibana, for a while after we merge this.

kishansagathiya · 2018-05-03T05:33:02Z

internal/api/api.go

+		return false, err
+	}
+
+	status := state == model.JenkinsStarting || state == model.JenkinsRunning


@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?

@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.

kishansagathiya · 2018-05-03T05:36:58Z

internal/tenant/tenant.go

+		return true, err
+	}
+
+	if len(ti.Errors) != 0 {


should all the errors not be concatenated instead of returning just the first one?

I think that is probably happening. err is the concatenated error from all errors in ti.Errors. @sthaha Can you make sure of this?

concaf

Great work, @sthaha 🎉

pradeepto requested review from kishansagathiya, concaf and lordofthejars May 2, 2018 02:49

sthaha commented May 2, 2018

View reviewed changes

kishansagathiya reviewed May 2, 2018

View reviewed changes

concaf reviewed May 2, 2018

View reviewed changes

kishansagathiya reviewed May 2, 2018

View reviewed changes

sthaha changed the title ~~DNM: Fix #224 Respect capacity flag when unidling Jenkins~~ Fix #224 Respect capacity flag when unidling Jenkins May 2, 2018

lordofthejars reviewed May 2, 2018

View reviewed changes

sthaha added 2 commits May 3, 2018 01:06

sthaha changed the title ~~Fix #224 Respect capacity flag when unidling Jenkins~~ Fix #223 Respect capacity flag when unidling Jenkins May 2, 2018

sthaha changed the title ~~Fix #223 Respect capacity flag when unidling Jenkins~~ Fix #223 Honour cluster-capacity-exhausted flag when unidling Jenkins May 2, 2018

lordofthejars approved these changes May 2, 2018

View reviewed changes

kishansagathiya reviewed May 3, 2018

View reviewed changes

kishansagathiya approved these changes May 3, 2018

View reviewed changes

concaf approved these changes May 3, 2018

View reviewed changes

sthaha merged commit 26e3db4 into fabric8-services:master May 3, 2018

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

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

Conversation

sthaha commented May 2, 2018

centos-ci commented May 2, 2018

alien-ike commented May 2, 2018

Ike Plugins (test-keeper)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha May 2, 2018 • edited Loading

Choose a reason for hiding this comment

kishansagathiya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf left a comment

Choose a reason for hiding this comment

sthaha May 2, 2018 •

edited

Loading

sthaha May 2, 2018 •

edited

Loading

sthaha May 2, 2018 •

edited

Loading

kishansagathiya left a comment •

edited

Loading