[Essentials] Instance Health Checking JEP: ready for first review

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[Essentials] Instance Health Checking JEP: ready for first review

Baptiste MATHUS
Hello everyone,

Just published the draft of the JEP for the "Essentials Instance Client Health Checking", it is ready for, and warmly welcoming, review:


To sum up, this will be the component used to decide if we trigger a rollback or not after we ran an upgrade. (in which case, we will roll back using jep-302).

Please answer with your feedback in this thread as much as possible.

Thank you!

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS55c6LXzLOx1wyW0h%3D2%2Br0DyQejLesbNUKP3u-VNF6VRQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Baptiste MATHUS

2018-04-10 14:09 GMT+02:00 Baptiste Mathus <[hidden email]>:
Hello everyone,

Just published the draft of the JEP for the "Essentials Instance Client Health Checking", it is ready for, and warmly welcoming, review:


To sum up, this will be the component used to decide if we trigger a rollback or not after we ran an upgrade. (in which case, we will roll back using jep-302).

Please answer with your feedback in this thread as much as possible.

Thank you!


--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS5_CWd5nfzXoivoy%3D%3DoUrDMwc-6KQ%2B9Ji_%3DJ7zmPZmMEw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Jesse Glick-4
I for one would find it easier to comment on a PR, but IIRC JEP
editors have discouraged this for some reason…?


“Healthness” is not a word—it is just “health”.


I would suggest `/metrics/evergreen/healthcheck` just return an empty
JSON document, or a simple “OK” marker by default—only listing things
that are _not_ healthy.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2U160HAC7VTSe%2B4UXz2TMokTmBnxFVNhbwD-CtZG_%2BZA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

R. Tyler Croy
(replies inline)

On Tue, 10 Apr 2018, Jesse Glick wrote:

> I for one would find it easier to comment on a PR, but IIRC JEP
> editors have discouraged this for some reason????
>
>
> ???Healthness??? is not a word???it is just ???health???.
>
>
> I would suggest `/metrics/evergreen/healthcheck` just return an empty
> JSON document, or a simple ???OK??? marker by default???only listing things
> that are _not_ healthy.
It was my understanding that this format was simply the Dropwizard healthcheck
format.

Do you have some reasoning for this you could share? Otherwise this seems like
a bit of personal preference to me, but I'm sure there's some logic I could be
missing here.




--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/20180410143346.discdbodxgiudmfz%40grape.lasagna.io.
For more options, visit https://groups.google.com/d/optout.

signature.asc (201 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Jesse Glick-4
On Tue, Apr 10, 2018 at 10:33 AM, R. Tyler Croy <[hidden email]> wrote:
> Do you have some reasoning for this you could share?

Merely that “nothing in particular is broken” is not news, so there is
not much a client needs to know.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr0df4JRZiyr0hEgHxzvTyw0XL3JDk1OMRgZdHnyuGzQcw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Baptiste MATHUS
In reply to this post by R. Tyler Croy


2018-04-10 16:33 GMT+02:00 R. Tyler Croy <[hidden email]>:
(replies inline)

On Tue, 10 Apr 2018, Jesse Glick wrote:

> I for one would find it easier to comment on a PR, but IIRC JEP
> editors have discouraged this for some reason????
>
>
> ???Healthness??? is not a word???it is just ???health???.
>
>
> I would suggest `/metrics/evergreen/healthcheck` just return an empty
> JSON document, or a simple ???OK??? marker by default???only listing things
> that are _not_ healthy.

It was my understanding that this format was simply the Dropwizard healthcheck
format.

Yes. Exactly. Also, though TBH I didn't test it myself yet, I expect one can contribute healthchecks, and they'd appear there.

That "OK" idea is already somehow available if one enables the `canPing` configuration field. Then `curl`ing the URL will reply with just PONG and http-200 IIRC. But I felt this was unnecessary to enable that too.
 

Do you have some reasoning for this you could share? Otherwise this seems like
a bit of personal preference to me, but I'm sure there's some logic I could be
missing here.




--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/20180410143346.discdbodxgiudmfz%40grape.lasagna.io.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS6FcSp_faJL5C26-xESdUrSQvw%3D40%2BGVxf-SkWM5TOvQQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Liam Newman
Jesse, 
Thanks for the reminder about that point around commenting.

As an editor, I have no objections to having pre-draft discussions in whatever way contributors see fit.   

As a sponsor of JEP-1, it is important to me that when a sponsor submits a JEP for "Approval as Draft", that feedback they get on _that_ PR focuses on issues that would prevent the JEP being approved as draft rather than on design or other questions.  I'll add a note about this to  https://github.com/jenkinsci/jep/pull/76 .  

-Liam




On Tue, Apr 10, 2018 at 12:41 PM Baptiste Mathus <[hidden email]> wrote:
2018-04-10 16:33 GMT+02:00 R. Tyler Croy <[hidden email]>:
(replies inline)

On Tue, 10 Apr 2018, Jesse Glick wrote:

> I for one would find it easier to comment on a PR, but IIRC JEP
> editors have discouraged this for some reason????
>
>
> ???Healthness??? is not a word???it is just ???health???.
>
>
> I would suggest `/metrics/evergreen/healthcheck` just return an empty
> JSON document, or a simple ???OK??? marker by default???only listing things
> that are _not_ healthy.

It was my understanding that this format was simply the Dropwizard healthcheck
format.

Yes. Exactly. Also, though TBH I didn't test it myself yet, I expect one can contribute healthchecks, and they'd appear there.

That "OK" idea is already somehow available if one enables the `canPing` configuration field. Then `curl`ing the URL will reply with just PONG and http-200 IIRC. But I felt this was unnecessary to enable that too.
 

Do you have some reasoning for this you could share? Otherwise this seems like
a bit of personal preference to me, but I'm sure there's some logic I could be
missing here.




--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/20180410143346.discdbodxgiudmfz%40grape.lasagna.io.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS6FcSp_faJL5C26-xESdUrSQvw%3D40%2BGVxf-SkWM5TOvQQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAA0qCNxaUqBC6ckV-BE9qn%3DabQpoZqKMTE3oQ%3Di%3DGpqHkWG6Tw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

James Nord-2
In reply to this post by Baptiste MATHUS

Login URL

We check that:

  • it is reachable,

  • and returns a 200 HTTP status code.


I think that is potentially a bad idea.  Whilst currently the /login page returns a 200 it should only do this if you are not logged in and are using an AbstractPasswordBasedSecurityRealm based SecurityReakm.  For a non username password provider (google/SAML etc) IMO it should redirect to the actual login url AuthRealm  (link).

I consider the current behaviour of showing a username/password form a bug if you are either logged in or are not using something that can actually authenticate with a username and password. 

Regards

/James


--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/38b68e1c-c962-4cd0-947d-a0228672598f%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Jesse Glick-4
On Wed, Apr 11, 2018 at 9:05 AM, James Nord <[hidden email]> wrote:
> Whilst currently the /login page
> returns a 200 it should only do this if you are not logged in and are using
> an AbstractPasswordBasedSecurityRealm based SecurityRealm.

Good point, this is not a good choice of page for a general-purpose
anonymous HTML rendering check. I would suggest `/instance-identity/`
(from `IdentityRootAction/index.jelly`). You could also use `/whoAmI/`
(from `WhoAmI/index.jelly`).

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3iufgV6J5eSf72tYk87ZXE%3DfBWk-d3WTfuME_0jmrBTA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Baptiste MATHUS
Good point. I just addressed it in the proposal and the Essentials tests.

FTR, the associated PR on Essentials side: https://github.com/jenkins-infra/evergreen/pull/52

I chose to go with /instance-identity, because I thought it might become useful in the near future to extract/check the instance-identity, so it looked like a better guess than /whoAmI where we always get the page for anonymous.

2018-04-11 15:48 GMT+02:00 Jesse Glick <[hidden email]>:
On Wed, Apr 11, 2018 at 9:05 AM, James Nord <[hidden email]> wrote:
> Whilst currently the /login page
> returns a 200 it should only do this if you are not logged in and are using
> an AbstractPasswordBasedSecurityRealm based SecurityRealm.

Good point, this is not a good choice of page for a general-purpose
anonymous HTML rendering check. I would suggest `/instance-identity/`
(from `IdentityRootAction/index.jelly`). You could also use `/whoAmI/`
(from `WhoAmI/index.jelly`).

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3iufgV6J5eSf72tYk87ZXE%3DfBWk-d3WTfuME_0jmrBTA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS4_KqMHRYRz4kEUuRRSYPeDUan%3DKyy01JwJpLHeiTcJ_g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Essentials] Instance Health Checking JEP: ready for first review

Baptiste MATHUS
Just submitted the PR as draft as https://github.com/jenkinsci/jep/pull/91

Feedback still more than welcome.

Cheers

2018-04-11 18:55 GMT+02:00 Baptiste Mathus <[hidden email]>:
Good point. I just addressed it in the proposal and the Essentials tests.

FTR, the associated PR on Essentials side: https://github.com/jenkins-infra/evergreen/pull/52

I chose to go with /instance-identity, because I thought it might become useful in the near future to extract/check the instance-identity, so it looked like a better guess than /whoAmI where we always get the page for anonymous.

2018-04-11 15:48 GMT+02:00 Jesse Glick <[hidden email]>:
On Wed, Apr 11, 2018 at 9:05 AM, James Nord <[hidden email]> wrote:
> Whilst currently the /login page
> returns a 200 it should only do this if you are not logged in and are using
> an AbstractPasswordBasedSecurityRealm based SecurityRealm.

Good point, this is not a good choice of page for a general-purpose
anonymous HTML rendering check. I would suggest `/instance-identity/`
(from `IdentityRootAction/index.jelly`). You could also use `/whoAmI/`
(from `WhoAmI/index.jelly`).

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3iufgV6J5eSf72tYk87ZXE%3DfBWk-d3WTfuME_0jmrBTA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.


--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS5Y5b%2B2u7cVvwtpfYot6UpmOpQCDG3NbR%3D3UKz%2BhDo8pg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.