Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Charts - Supports to customize livenessProbe and readinessProbe #4049

Merged
merged 1 commit into from Dec 22, 2021
Merged

Charts - Supports to customize livenessProbe and readinessProbe #4049

merged 1 commit into from Dec 22, 2021

Conversation

haoxins
Copy link
Contributor

@haoxins haoxins commented Dec 22, 2021

Description

When I use Argo CD to deploy the Dapr, some components (Operator and Placement) need more time to start.
Sometimes the Argo CD may kill/restart the pods because of the health check.

So I think it's a easy and safe way to increase the initial delay and period seconds

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing

@haoxins haoxins requested review from a team as code owners December 22, 2021 05:37
@yaron2
Copy link
Member

yaron2 commented Dec 22, 2021

Instead of changing the defaults globally in Dapr, isn't it better to use the already configurable settings to increase the delay for your Argo deployment?

@haoxins
Copy link
Contributor Author

haoxins commented Dec 22, 2021

{{- if eq .Values.debug.enabled false }}
          initialDelaySeconds: 5
{{- else }}
          initialDelaySeconds: {{ .Values.debug.initialDelaySeconds }}
{{- end }}

Maybe we should accept the custom params when Values.debug.enabled = false?

@yaron2
Copy link
Member

yaron2 commented Dec 22, 2021

{{- if eq .Values.debug.enabled false }}
          initialDelaySeconds: 5
{{- else }}
          initialDelaySeconds: {{ .Values.debug.initialDelaySeconds }}
{{- end }}

Maybe we should accept the custom params when Values.debug.enabled = false?

That's a much better idea.

Increase the dapr-operator and dapr-placement delay seconds

Signed-off-by: Hao Xin <haoxinst@gmail.com>

WIP
@haoxins haoxins changed the title Charts - Increase the dapr-operator and dapr-placement delay seconds Charts - Supports to customize livenessProbe and readinessProbe Dec 22, 2021
@haoxins
Copy link
Contributor Author

haoxins commented Dec 22, 2021

{{- if eq .Values.debug.enabled false }}
          initialDelaySeconds: 5
{{- else }}
          initialDelaySeconds: {{ .Values.debug.initialDelaySeconds }}
{{- end }}

Maybe we should accept the custom params when Values.debug.enabled = false?

That's a much better idea.

PR updated~

Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #4049 (4a1f46f) into master (63f664f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4049   +/-   ##
=======================================
  Coverage   63.41%   63.41%           
=======================================
  Files         101      101           
  Lines        9567     9567           
=======================================
  Hits         6067     6067           
  Misses       3026     3026           
  Partials      474      474           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63f664f...4a1f46f. Read the comment docs.

@yaron2 yaron2 merged commit 220e1b9 into dapr:master Dec 22, 2021
@yaron2 yaron2 added this to the v1.6 milestone Dec 22, 2021
@haoxins haoxins deleted the increase-the-prob-seconds branch December 23, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants