Skip to content

Commit

Permalink
Use strongly-typed Durations for time
Browse files Browse the repository at this point in the history
This PR improves time & duration handling in Riff Raff by switching representation from `Int` or `Long` types (types which do not incorporate a time-unit, meaning we have to work out from _context_ whether we are using seconds, milliseconds, epoch-ms, etc) to using the great `java.time.*` classes `Instant` & `Duration`.

This fixes a lot of things like:

* slightly obscure integer constants like `5 * 60 * 1000` (denoting 5 minutes)
* the inability of the compiler to check we are performing the right kind of operation to change time-units - the compiler can't know if what we're doing to an `Int` makes sense!
* variable names like `durationMillis`, where 'millis' had to be included because the declared type of `Long` isn't _telling_ us what the type is
* frequent multiplication by 1000 to transform a value that we are quite sure is measured in seconds, into milliseconds!
* the risk that integer arithmetic might overflow when making time calculations, silently corrupting the answer.

If you can use a strongly-typed time class instead - like `Duration` or `Instant` - the compiler and library author can do all of that stuff for us, it's much better! You also stand a better chance of avoiding the hallowed [_falsehoods programmers believe about time_](https://gist.github.com/timvisee/fcda9bbdff88d45cc9061606b4b923ca).

### From `Param[Int]` to `Param[Duration]` - durations are still specified in seconds ⏱️

Riff Raff has several parameters which specify durations (eg `healthcheckGrace`, `warmupGrace`, etc). Historically these have all expected integer values and been documented to be duration in **seconds**:

```yaml
    parameters:
      healthcheckGrace: 420
      warmupGrace: 300
```

We don't want to make everyone change their Riff Raff configuration, and so those parameters need to continue to accept the same numeric values and still interpret them as seconds.

The `Param.waitingSecondsFor()` helper method lets us create parameters that are a `Param[Duration]` (which could have been problematic because `java.time.Duration` has no _specified_ time unit) that still _explicitly_ parse their input as seconds, and are tested to do so.

### Migrating from Joda-Time to java.time
We should be using `java.time.*` classes in our codebases, in preference to the wonderful but old Joda-Time library:

https://blog.joda.org/2014/11/converting-from-joda-time-to-javatime.html

See also:

* guardian/ophan#3988 - another PR achieving a similar thing for Ophan
  • Loading branch information
rtyley committed Jun 4, 2024
1 parent 2504809 commit 3560ae2
Show file tree
Hide file tree
Showing 15 changed files with 296 additions and 277 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import software.amazon.awssdk.services.autoscaling.model.AutoScalingGroup
import magenta.tasks.{S3 => S3Tasks}
import software.amazon.awssdk.services.ssm.SsmClient

import java.time.Duration
import java.time.Duration.{ofMinutes, ofSeconds}

sealed trait MigrationTagRequirements
case object NoMigration extends MigrationTagRequirements
case object MustBePresent extends MigrationTagRequirements
Expand Down Expand Up @@ -88,22 +91,24 @@ object AutoScaling extends DeploymentType with BucketParameters {
| - scale up, wait for the new instances to become healthy and then scale back down
""".stripMargin

val secondsToWait = Param(
"secondsToWait",
"Number of seconds to wait for instances to enter service"
).default(15 * 60)
val healthcheckGrace = Param(
"healthcheckGrace",
"Number of seconds to wait for the AWS api to stabilise"
).default(20)
val warmupGrace = Param(
"warmupGrace",
"Number of seconds to wait for the instances in the load balancer to warm up"
).default(1)
val terminationGrace = Param(
"terminationGrace",
"Number of seconds to wait for the AWS api to stabilise after instance termination"
).default(10)
val secondsToWait: Param[Duration] = Param
.waitingSecondsFor("secondsToWait", "instances to enter service")
.default(ofMinutes(15))
val healthcheckGrace: Param[Duration] = Param
.waitingSecondsFor("healthcheckGrace", "the AWS api to stabilise")
.default(ofSeconds(20))
val warmupGrace: Param[Duration] = Param
.waitingSecondsFor(
"warmupGrace",
"the instances in the load balancer to warm up"
)
.default(ofSeconds(1))
val terminationGrace: Param[Duration] = Param
.waitingSecondsFor(
"terminationGrace",
"the AWS api to stabilise after instance termination"
)
.default(ofSeconds(10))

val prefixStage = Param[Boolean](
"prefixStage",
Expand Down Expand Up @@ -165,7 +170,7 @@ object AutoScaling extends DeploymentType with BucketParameters {
autoScalingGroup: AutoScalingGroupInfo
): List[ASGTask] = {
List(
WaitForStabilization(autoScalingGroup, 5 * 60 * 1000, target.region),
WaitForStabilization(autoScalingGroup, ofMinutes(5), target.region),
CheckGroupSize(autoScalingGroup, target.region),
SuspendAlarmNotifications(autoScalingGroup, target.region),
TagCurrentInstancesWithTerminationTag(autoScalingGroup, target.region),
Expand All @@ -174,32 +179,32 @@ object AutoScaling extends DeploymentType with BucketParameters {
HealthcheckGrace(
autoScalingGroup,
target.region,
healthcheckGrace(pkg, target, reporter) * 1000
healthcheckGrace(pkg, target, reporter)
),
WaitForStabilization(
autoScalingGroup,
secondsToWait(pkg, target, reporter) * 1000,
secondsToWait(pkg, target, reporter),
target.region
),
WarmupGrace(
autoScalingGroup,
target.region,
warmupGrace(pkg, target, reporter) * 1000
warmupGrace(pkg, target, reporter)
),
WaitForStabilization(
autoScalingGroup,
secondsToWait(pkg, target, reporter) * 1000,
secondsToWait(pkg, target, reporter),
target.region
),
CullInstancesWithTerminationTag(autoScalingGroup, target.region),
TerminationGrace(
autoScalingGroup,
target.region,
terminationGrace(pkg, target, reporter) * 1000
terminationGrace(pkg, target, reporter)
),
WaitForStabilization(
autoScalingGroup,
secondsToWait(pkg, target, reporter) * 1000,
secondsToWait(pkg, target, reporter),
target.region
),
ResumeAlarmNotifications(autoScalingGroup, target.region)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import magenta.tasks.UpdateCloudFormationTask.LookupByTags
import magenta.tasks._
import org.joda.time.DateTime

import java.time.Duration
import java.time.Duration.ofMinutes

trait BuildTags {
// Returns tags for a build, which should be added to the Cloudformation
// stack. Tags are named with a `gu:` prefix.
Expand Down Expand Up @@ -91,10 +94,12 @@ class CloudFormation(tagger: BuildTags)
"If set to true then the cloudformation stack will be created if it doesn't already exist"
).default(true)

val secondsToWaitForChangeSetCreation = Param(
"secondsToWaitForChangeSetCreation",
"Number of seconds to wait for the change set to be created"
).default(15 * 60)
val secondsToWaitForChangeSetCreation: Param[Duration] = Param
.waitingSecondsFor(
"secondsToWaitForChangeSetCreation",
"the change set to be created"
)
.default(ofMinutes(15))

val manageStackPolicyDefault = true
val manageStackPolicyLookupKey = "cloudformation:manage-stack-policy"
Expand Down Expand Up @@ -231,7 +236,7 @@ class CloudFormation(tagger: BuildTags)
new CheckChangeSetCreatedTask(
target.region,
stackLookup,
secondsToWaitForChangeSetCreation(pkg, target, reporter) * 1000
secondsToWaitForChangeSetCreation(pkg, target, reporter)
),
new ExecuteChangeSetTask(
target.region,
Expand Down
15 changes: 12 additions & 3 deletions magenta-lib/src/main/scala/magenta/deployment_type/GCS.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,22 @@ object GCS extends DeploymentType {
val documentation = "For uploading files into a GCS bucket."

val prefixStage =
Param("prefixStage", "Prefix the GCS bucket key with the target stage")
Param[Boolean](
"prefixStage",
"Prefix the GCS bucket key with the target stage"
)
.default(true)
val prefixPackage =
Param("prefixPackage", "Prefix the GCS bucket key with the package name")
Param[Boolean](
"prefixPackage",
"Prefix the GCS bucket key with the package name"
)
.default(true)
val prefixStack =
Param("prefixStack", "Prefix the GCS bucket key with the target stack")
Param[Boolean](
"prefixStack",
"Prefix the GCS bucket key with the target stack"
)
.default(true)
val pathPrefixResource = Param[String](
"pathPrefixResource",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import magenta.tasks.gcp.GCP.DeploymentManagerApi.DeploymentBundle
import magenta.{DeployReporter, KeyRing}
import software.amazon.awssdk.services.s3.S3Client

import scala.concurrent.duration._
import java.time.Duration
import java.time.Duration.ofMinutes

object GcpDeploymentManager extends DeploymentType {
val GCP_PROJECT_NAME_PRISM_KEY: String = "gcp:project-name"
Expand All @@ -23,12 +24,10 @@ object GcpDeploymentManager extends DeploymentType {
|
|""".stripMargin

val maxWaitParam: Param[Int] = Param[Int](
name = "maxWait",
documentation = """
|Number of seconds to wait for the deployment operations to complete
|""".stripMargin
).default(1800) // half an hour
val maxWaitParam: Param[Duration] =
Param
.waitingSecondsFor("maxWait", "the deployment operations to complete")
.default(ofMinutes(30))

val deploymentNameParam: Param[String] = Param(
name = "deploymentName",
Expand Down Expand Up @@ -93,7 +92,7 @@ object GcpDeploymentManager extends DeploymentType {
}
implicit val artifactClient: S3Client = resources.artifactClient

val maxWaitDuration = maxWaitParam(pkg, target, reporter).seconds
val maxWaitDuration = maxWaitParam(pkg, target, reporter)
val deploymentName = deploymentNameParam(pkg, target, reporter)
val upsert = upsertParam(pkg, target, reporter)
val preview = previewParam(pkg, target, reporter)
Expand Down
35 changes: 30 additions & 5 deletions magenta-lib/src/main/scala/magenta/deployment_type/Param.scala
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package magenta.deployment_type

import magenta.{DeployReporter, DeployTarget, DeploymentPackage}
import play.api.libs.json.{Json, Reads}
import play.api.libs.json.{JsValue, Json, Reads}

import java.time.Duration

trait ParamRegister {
def add(param: Param[_]): Unit
Expand Down Expand Up @@ -38,21 +40,24 @@ case class Param[T](
(DeploymentPackage, DeployTarget) => Either[String, T]
] = None,
deprecatedDefault: Boolean = false
)(implicit register: ParamRegister) {
)(implicit register: ParamRegister, reads: Reads[T], manifest: Manifest[T]) {
register.add(this)

val required =
!optional && defaultValue.isEmpty && defaultValueFromContext.isEmpty

def get(pkg: DeploymentPackage)(implicit reads: Reads[T]): Option[T] =
def get(pkg: DeploymentPackage): Option[T] =
pkg.pkgSpecificData
.get(name)
.flatMap(jsValue => Json.fromJson[T](jsValue).asOpt)
.flatMap(jsValue => parse(jsValue))

def parse(jsValue: JsValue): Option[T] = Json.fromJson[T](jsValue).asOpt

def apply(
pkg: DeploymentPackage,
target: DeployTarget,
reporter: DeployReporter
)(implicit reads: Reads[T], manifest: Manifest[T]): T = {
): T = {
val maybeValue = get(pkg)
val defaultFromContext = defaultValueFromContext.map(_(pkg, target))

Expand Down Expand Up @@ -96,3 +101,23 @@ case class Param[T](
this.copy(defaultValueFromContext = Some(defaultFromContext))
}
}

object Param {
private val ReadIntAsSeconds: Reads[Duration] =
Reads.of[Int].map(Duration.ofSeconds(_))

/** Create a parameter that represents a number of seconds to wait for
* something. Riff Raff has many existing parameters for setting durations
* where the values are *expected* to be in seconds, so this helper method
* makes that clear and ensures that the value is explicitly parsed as a
* duration in *seconds*.
*/
def waitingSecondsFor(name: String, waitingOn: String)(implicit
register: ParamRegister
): Param[Duration] =
Param[Duration](name, s"Number of seconds to wait for $waitingOn")(
register,
ReadIntAsSeconds,
implicitly[Manifest[Duration]]
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,24 @@ trait S3ObjectPrefixParameters {
this: DeploymentType =>

val prefixStage: Param[Boolean] =
Param("prefixStage", "Prefix the S3 bucket key with the target stage")
Param[Boolean](
"prefixStage",
"Prefix the S3 bucket key with the target stage"
)
.default(true)

val prefixPackage: Param[Boolean] =
Param("prefixPackage", "Prefix the S3 bucket key with the package name")
Param[Boolean](
"prefixPackage",
"Prefix the S3 bucket key with the package name"
)
.default(true)

val prefixStack: Param[Boolean] =
Param("prefixStack", "Prefix the S3 bucket key with the target stack")
Param[Boolean](
"prefixStack",
"Prefix the S3 bucket key with the target stack"
)
.default(true)

val prefixApp: Param[Boolean] = Param[Boolean](
Expand Down
33 changes: 17 additions & 16 deletions magenta-lib/src/main/scala/magenta/tasks/ASGTasks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import software.amazon.awssdk.services.autoscaling.model.{
LifecycleState,
SetInstanceProtectionRequest
}
import software.amazon.awssdk.services.ec2.Ec2Client

import java.time.Duration
import scala.jdk.CollectionConverters._

case class CheckGroupSize(info: AutoScalingGroupInfo, region: Region)(implicit
Expand Down Expand Up @@ -118,8 +118,12 @@ case class DoubleSize(info: AutoScalingGroupInfo, region: Region)(implicit
s"Double the size of the auto-scaling group called $asgName"
}

sealed abstract class Pause(durationMillis: Long)(implicit val keyRing: KeyRing)
sealed abstract class Pause(duration: Duration)(implicit val keyRing: KeyRing)
extends ASGTask {
val purpose: String

def description: String = s"Wait extra ${duration.toMillis}ms to $purpose"

def execute(
asg: AutoScalingGroup,
resources: DeploymentResources,
Expand All @@ -131,43 +135,40 @@ sealed abstract class Pause(durationMillis: Long)(implicit val keyRing: KeyRing)
"Skipping pause as there are no instances and desired capacity is zero"
)
else
Thread.sleep(durationMillis)
Thread.sleep(duration.toMillis)
}
}

case class HealthcheckGrace(
info: AutoScalingGroupInfo,
region: Region,
durationMillis: Long
duration: Duration
)(implicit keyRing: KeyRing)
extends Pause(durationMillis) {
def description: String =
s"Wait extra ${durationMillis}ms to let Load Balancer report correctly"
extends Pause(duration) {
val purpose: String = "let Load Balancer report correctly"
}

case class WarmupGrace(
info: AutoScalingGroupInfo,
region: Region,
durationMillis: Long
duration: Duration
)(implicit keyRing: KeyRing)
extends Pause(durationMillis) {
def description: String =
s"Wait extra ${durationMillis}ms to let instances in Load Balancer warm up"
extends Pause(duration) {
val purpose: String = "let instances in Load Balancer warm up"
}

case class TerminationGrace(
info: AutoScalingGroupInfo,
region: Region,
durationMillis: Long
duration: Duration
)(implicit keyRing: KeyRing)
extends Pause(durationMillis) {
def description: String =
s"Wait extra ${durationMillis}ms to let Load Balancer report correctly"
extends Pause(duration) {
val purpose: String = "let Load Balancer report correctly"
}

case class WaitForStabilization(
info: AutoScalingGroupInfo,
duration: Long,
duration: Duration,
region: Region
)(implicit val keyRing: KeyRing)
extends ASGTask
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import software.amazon.awssdk.services.cloudformation.model.ChangeSetStatus._
import software.amazon.awssdk.services.cloudformation.model._
import software.amazon.awssdk.services.s3.S3Client

import java.time.Duration
import scala.jdk.CollectionConverters._
import scala.util.{Success, Try}

Expand Down Expand Up @@ -150,7 +151,7 @@ class CreateChangeSetTask(
class CheckChangeSetCreatedTask(
region: Region,
stackLookup: CloudFormationStackMetadata,
override val duration: Long
override val duration: Duration
)(implicit val keyRing: KeyRing, artifactClient: S3Client)
extends Task
with RepeatedPollingCheck {
Expand Down
Loading

0 comments on commit 3560ae2

Please sign in to comment.