palantir / k8s-spark-scheduler

A Kubernetes Scheduler Extender to provide gang scheduling support for Spark on Kubernetes
Apache License 2.0
175 stars 43 forks source link

Set CreationTimestamp for newly created resource reservations #262

Closed Alexis-D closed 1 year ago

Alexis-D commented 1 year ago

Fixes #257

CreationTimestamp is meant to be (optionally) set by the server, however due to how k8s-spark-scheduler works currently we never get to see the value that k8s assigns to it.

The underlying reason lies in the fact that https://github.com/palantir/k8s-spark-scheduler/blob/a7122ad7c79f607960bcf44e910bee8ad04a2810/internal/cache/cache.go#L29-L31 purposely does not reflect 'outside' updates, which for most intent and purposes isn't an issue, except when we try to rely on the k8s api server setting CreationTimestamp.

So instead, we now always set CreationTimestamp when creating a new resource reservation, which is turn is going to be ignored by k8s, but it allows us to properly track the metric we introduced in https://github.com/palantir/k8s-spark-scheduler/pull/256, w/o this, we observe that the metric is useless (CreationTimestamp defaults to the zero value for meta1.Time, which in turns means the duration we're measuring overflows).

After this PR

==COMMIT_MSG== Set CreationTimestamp for newly created resource reservations ==COMMIT_MSG==

Possible downsides?

This isn't the cleanest option, but it's a bit unclear whether changing https://github.com/palantir/k8s-spark-scheduler/blob/a7122ad7c79f607960bcf44e910bee8ad04a2810/internal/cache/store/store.go#L29 in favour of a new OverrideResourceVersionIfNewerOrEqual would have unforeseen consequences. In addition, this would that solution would yield more occurrences of CreationTimestamp being zero-valued.

Additionally this PR only solves the issue for resource reservations and not other CRDs (e.g. demands) since, at least for now, we do not rely on their CreationTimestamp.

svc-autorelease commented 1 year ago

Released 0.65.0