Skip to content

revive the liveness/readiness probe config#3335

Merged
jmuk merged 3 commits intoistio:masterfrom
jmuk:fix-probe
Feb 22, 2018
Merged

revive the liveness/readiness probe config#3335
jmuk merged 3 commits intoistio:masterfrom
jmuk:fix-probe

Conversation

@jmuk
Copy link
Copy Markdown
Contributor

@jmuk jmuk commented Feb 8, 2018

The real problem of 'crash loop' of mixer is failure of initialization;
the mixer CRD store will wait for up to 30 seconds for init.
(see
https://github.com/istio/istio/blob/master/mixer/pkg/config/crd/store.go#L52)

Considering this, this PR sets the initial delay for liveness
to be 60 seconds for safety.

@jmuk
revive the liveness/readiness probe config
32ced2d 
The real problem of 'crash loop' of mixer is failure of initialization;
the mixer CRD store will wait for up to 30 seconds for init.
(see
https://github.com/istio/istio/blob/master/mixer/pkg/config/crd/store.go#L52)

Considering this, this PR sets the initial delay for liveness
to be 60 seconds for safety.
@jmuk jmuk requested a review from a team February 8, 2018 23:17
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Feb 8, 2018

/assign @myidpt

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Feb 9, 2018

/test istio-pilot-e2e
/test e2e-smoke

@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented Feb 11, 2018

this sounds risky for 0.6 given the recent history of those checks ?

@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented Feb 11, 2018

/test istio-pilot-e2e

@jmuk jmuk mentioned this pull request Feb 13, 2018
Closed
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Feb 20, 2018

/retest

1 similar comment
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Feb 20, 2018

/retest

@myidpt
Copy link
Copy Markdown

myidpt commented Feb 20, 2018 •
edited
Loading

Thanks a lot for the work!
I'm also a little worried that this is risky. Can we put this into a new template: istio-mixer-with-health-check.yaml.tmpl
Then we have two different config files that integrates the Mixer, CA, etc. with health checks:
istio-with-control-plane-health-check.yaml
istio-auth-with-control-plane-health-check.yaml
And have our users try out these config first, before we set them to default? How does this sound like?
@ldemailly @jmuk
It's an alpha feature at this stage :)

@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented Feb 20, 2018

probably istio-auth-with-control-plane-health-check.yaml is enough but yes, pending helm settings adding this to the default has proven risky in the past

@myidpt myidpt mentioned this pull request Feb 20, 2018
Merged
@myidpt
Copy link
Copy Markdown

myidpt commented Feb 20, 2018

I think we can have both non-auth and auth config files. This fits the needs for different users. For example, Oath don't have auth enabled :)

@myidpt
Copy link
Copy Markdown

myidpt commented Feb 21, 2018

@jmuk Please see the PR for CA:
#3620

@jmuk jmuk requested a review from a team February 22, 2018 19:13
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Feb 22, 2018

Okay, separated the liveness / readiness for mixer into a different file. PTAL.

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Feb 22, 2018

/test e2e-bookInfo
/test istio-unit-tests

myidpt
myidpt approved these changes Feb 22, 2018
View reviewed changes
Copy link
Copy Markdown

@myidpt myidpt left a comment

Choose a reason for hiding this comment

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

Thanks!

@myidpt
Copy link
Copy Markdown

myidpt commented Feb 22, 2018

/lgtm
/approve

@istio-merge-robot
Copy link
Copy Markdown

istio-merge-robot commented Feb 22, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: myidpt
We suggest the following additional approver: frankbu

Assign the PR to them by writing /assign @frankbu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@jmuk jmuk merged commit c87983a into istio:master Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@myidpt myidpt myidpt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@myidpt myidpt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jmuk @ldemailly @myidpt @istio-merge-robot @googlebot @istio-testing