search

project-zot / helm-charts

7 stars 19 forks source link

feat: Make startup probe configurable #35

Closed Kortekaasy closed 9 months ago

Kortekaasy commented 10 months ago

What type of PR is this? feature

Which issue does this PR fix:

34

What does this PR do / Why do we need it: This PR adds a configurable startupProbe to the deployment of the Zot pods.

Testing done on this change:

N/A Automation added to e2e:

N/A

Will this break upgrades or downgrades? No

Does this PR introduce any user-facing change?:

Yes: 

 - Added startupProbe to zot deployment with configurable `initialDelaySeconds`, `periodSeconds`, and `failureThreshold`

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rchincha commented 10 months ago

@Kortekaasy yes, this is a good PR to have. A good search ux needed an index and that definitely takes a little time at least initially. Is there a test we can add?

@Andreea-Lupu can you review pls.

rchincha commented 10 months ago

@Kortekaasy just curious, has this been tried with latest zot release (v2.0.0)?

Kortekaasy commented 10 months ago

@rchincha Yes, main motivator for adding this was the upgrade of our Zot instance to 2.0.0 from 1.4.3, and enabling the web UI.

Kortekaasy commented 9 months ago

@rchincha @Andreea-Lupu

Any idea when you can review this PR?

Andreea-Lupu commented 9 months ago

Hi @Kortekaasy. Sorry for the delayed response. This PR looks good to me. Regarding the test that we could add for it, maybe it makes sense to add a new subchart under tests/ci containing a configuration with the ui extension enabled and having custom startupProbe values. What do you think about this suggestion?

Kortekaasy commented 9 months ago

@Andreea-Lupu that sounds like a good suggestion, but in that case we probably need a way to artificially delay the startup of the Zot registry software to make sure the startup probe actually tested. Do you have any suggestions there?

Kortekaasy commented 9 months ago

@Andreea-Lupu I have added a test for the startup probe. Test results show that the chart is still valid, and it looks like the values specified in the test are used in the chart. Further testing than this seem illogical to me, since then we would be testing whether Kubernetes has properly implemented their startup probe functionality, which seems out of scope to me.