opened
KnVerey
username_0
@Shopify/cloudplatform @Shopify/redis-memcached @username_3 @kirs 

## What this does

This PR makes it possible to add a `kubernetes-deploy.shopify.io/timeout-override-seconds` annotation to any individual resource. If that annotation is present and valid, its value will be used in place of the global hard timeout for that resource type.

I think this is a reasonable feature even though it may let people shoot themselves/Shipit in the foot with reeeeeaaaaaly long timeouts because:
- We've tinkered with the global timeout on Deployment in the past just for core.
- #225 requested a timeout override for StatefulSets (and the same thing was raised internally for kafka)
- #231 is asking to bump the timout for Redis because of a single instance.
- This gem is open-source, and different organizations may be using it in very different contexts and for deploying very different things.


## Alternatives

* The various pod controllers are the main use case for this override (even Redis is actually watching a deployment), and for those using something like ProgressDeadlineSeconds (which only exists on Deployment) would be ideal. I've seen issues in k8s core about bringing all the workload controllers in line with each other before v1, so presumably this will be an option eventually. But last I checked StatefulSets (for example) still don't have a Progressing condition on k8s core master.
* Make Redis use ProgressDeadlineSeconds (since as mentioned above it is watching a Deployment), and set that value really high on the offending deployment. This isn't crazy... in fact the Redis controller should probably use that strategy when we switch to controller-reported statuses for CRs.
* Implement ProgressDeadlineSeconds-like behaviour for StatefulSets (and possibly other pod controller classes) client-side. I'm hesitant to do this since it would not be straightforward, and the feature is eventually coming upstream (where it belongs).
Enable per-resource timeout customization via an annotation
issue
created
devonboyer
username_1
@kwestein Once this PR ships we will be able to fix the timeout problems you are having.
comment
created
katdrobnjakovic
username_2
Is this something that is happening soon? I'm okay with shipping this (as I understand it it will be useful for others as well) and adding the annotations to quickly fix the few apps we have seen have problems with their deployments timing out and then refactor to the ProgressDeadlineSeconds strategy. 

On our side, we are not in a rush to ship this today as everyone is on holiday and we have a workaround in place if those apps do need to deploy over the break.
comment
created
KnVerey
username_0
Thanks for letting me know 👍
comment
created
psycotica0-shopify
username_3
It should be possible to write a test like
- Construct resource, set `deploy_started_at` to 6 minutes ago, validate that `dummy.deploy_timed_out?` is true.
- Construct a resource with a custom timeout of 10m, set `deploy_started_at` to 6 minutes ago, validate that `dummy.deploy_timed_out?` is false.

Unless that's too Integrationy for the unit tests.
comment
created
KnVerey
username_0
Updated to use ISO8601 durations* and add more tests.

\* These are neat and powerful and standard, and it's great to get the parser for free, but I felt like the intuitive "1s/m/h" should also work without the "PT" prefix, and that being case-sensitive would be annoying. So I wrapped the parser in a custom one with those features. LMK what you think.
comment
created
psycotica0-shopify
username_3
(These tests seem great, by the way!)
comment