[WX-1670] Don't allocate job tokens for hog groups experiencing quota exhaustion #7520

salonishah11 · 2024-08-30T16:00:28Z

Jira: https://broadworkbench.atlassian.net/browse/WX-1670

Description

Cromwell will use information provided in the new GROUP_METRICS_ENTRY table to allocate new tokens for job requests whose hog group is not experiencing any cloud quota exhaustion. Note that this will be applied to jobs seeking "execution" tokens. Jobs seeking "restart" tokens are not affected by this change.

TODO:

test changes in BEE
fix unit tests
add new unit tests
update Changelog?

Release Notes Confirmation

`CHANGELOG.md`

I updated CHANGELOG.md in this PR
I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

I added a suggested release notes entry in this Jira ticket
I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

salonishah11 · 2024-08-30T16:01:10Z

engine/src/main/scala/cromwell/engine/workflow/tokens/JobTokenDispenserActor.scala

@@ -145,6 +181,7 @@ class JobTokenDispenserActor(override val serviceRegistryActor: ActorRef,
      val hogGroupCounts =
        nextTokens.groupBy(t => t.queuePlaceholder.hogGroup).map { case (hogGroup, list) => s"$hogGroup: ${list.size}" }
      log.info(s"Assigned new job $dispenserType tokens to the following groups: ${hogGroupCounts.mkString(", ")}")
+//      System.out.println(s"##### FIND ME $dispenserType tokens for actors:  ${nextTokens.map(t => t.queuePlaceholder.actor.path).mkString(",")}")


TODO: remove this after testing

This is now removed

salonishah11 · 2024-08-30T17:06:55Z

backend/src/main/scala/cromwell/backend/standard/GroupMetricsActor.scala


 class GroupMetricsActor(engineDbInterface: EngineSqlDatabase) extends Actor with ActorLogging {

  implicit val ec: MessageDispatcher = context.system.dispatchers.lookup(Dispatcher.EngineDispatcher)

+  final private val QUOTA_EXHAUSTION_THRESHOLD_IN_SECS = 15 * 60 // 15 minutes


Question for reviewers - what should be this threshold? Is 15 mins good?

How hard would it be to make this configurable? I think 15m is a good place to start, if anything a little long.

I don't think it would be that hard. I can try that.

Configurable sounds good, yeah.

How long is our polling interval for cloud jobs? It seems like the quota backoff time should be more than the poll interval.

How long is our polling interval for cloud jobs?

It seems it initially starts with 30 seconds and the max interval is 10 mins. So maybe 15 mins is a good starting point?

Agreed, sounds great to me.

jgainerdewar

This looks like a great approach!

aednichols · 2024-08-30T20:51:31Z

engine/src/main/scala/cromwell/engine/workflow/tokens/JobTokenDispenserActor.scala

+          case Success(GetQuotaExhaustedGroupsFailure(errorMsg)) =>
+            log.error(s"Failed to fetch quota exhausted groups. Error: $errorMsg")
+            dispense(n, List.empty)
+          case Failure(exception) =>
+            log.error(s"Unexpected failure while fetching quota exhausted groups. Error: ${exception.getMessage}")
+            dispense(n, List.empty)


I appreciate the fail-safe approach here.

aednichols · 2024-08-30T20:53:40Z

engine/src/main/scala/cromwell/engine/workflow/tokens/RoundRobinQueueIterator.scala

-    extends Iterator[LeasedActor] {
+final class RoundRobinQueueIterator(initialTokenQueue: List[TokenQueue],
+                                    initialPointer: Int,
+                                    quotaExhaustedGroups: List[String]


Internally to this class, there's nothing specific about quota. We could suggest future extension with a generic name.

Suggested change

quotaExhaustedGroups: List[String]

excludedGroups: List[String]

jgainerdewar

This looks great! I do think we want both a changelog entry and a Terra release note for this.

Since this is a big change to the way we start jobs, I'm wondering if we want to include an "off switch" in the initial release. If we discovered a problem with this behavior and want to quickly revert to the old behavior, can we do that by setting the config to a 0 minute threshold? Should we build in an enabled flag for this behavior in config?

jgainerdewar · 2024-09-05T15:29:58Z

engine/src/test/scala/cromwell/engine/workflow/tokens/RoundRobinQueueIteratorSpec.scala

@@ -17,15 +17,33 @@ class RoundRobinQueueIteratorSpec extends TestKitSuite with AnyFlatSpecLike with
  val tokenEventLogger = NullTokenEventLogger

  it should "be empty if there's no queue" in {
-    new RoundRobinQueueIterator(List.empty, 0).hasNext shouldBe false
+    new RoundRobinQueueIterator(List.empty, 0, List.empty).hasNext shouldBe false
  }



Nice tests!

salonishah11 · 2024-09-05T16:35:43Z

Since this is a big change to the way we start jobs, I'm wondering if we want to include an "off switch" in the initial release. If we discovered a problem with this behavior and want to quickly revert to the old behavior, can we do that by setting the config to a 0 minute threshold? Should we build in an enabled flag for this behavior in config?

@jgainerdewar yes I had thought about that and setting the config to 0 should work. But I like your suggestion about having an actual config value like enabled instead which should make it more clear. Use the enabled flag would also be better so that if it is set to false the JobTokenDsispenserActor won't ask GroupMetricsActor about exhausted groups at all.

aednichols

LGTM 👍

jgainerdewar · 2024-09-06T20:04:30Z

CHANGELOG.md

@@ -2,6 +2,14 @@

 ## 88 Release Notes

+### New feature: Prevent Job start during Cloud Quota exhaustion
+
+This optional feature prevents Cromwell from starting new jobs in a billing project that is currently experiencing 


Should we say "in a hog group" here?

I can replace billing project with hog group 👍

I'm of the belief that we should not use "hog group" in any new, external-facing places.

What do you think would be clearer? I don't think "billing project" is either accurate in Terra or useful to other users.

Maybe just group might work?

Yeah I usually just use group.

jgainerdewar · 2024-09-06T20:09:17Z

CHANGELOG.md

+cloud quota exhaustion. Jobs will be started once the project's quota becomes available. To enable this feature, 
+set `quota-exhaustion-job-start-control.enabled` to true.
+
+Note: Jobs that are being restarted will not be affected by this feature, even if it is enabled.


I suspect that this note will confuse people, since there are so many kinds of job restarts. I think we can probably just take it out, users probably need to understand a lot about Cromwell internals to even have this question. 😂

I see. I added this so that users know that restarts won't affect their jobs not "being restarted" but I didn't know there could be different kind of restarts. I am happy to remove this.

jgainerdewar · 2024-09-06T20:28:52Z

core/src/main/resources/reference.conf

@@ -270,6 +270,15 @@ system {
    # token-log-interval-seconds = 300
  }

+  # If enabled, Cromwell will not allocate new execution tokens to jobs whose hog groups is actively


grammar nit - "hog groups are actively"

jgainerdewar · 2024-09-06T20:37:33Z

engine/src/main/scala/cromwell/engine/workflow/tokens/JobTokenDispenserActor.scala

+    if (tokenQueues.nonEmpty) {
+      // don't fetch cloud quota exhausted groups for token dispenser allocating 'restart' tokens
+      if (dispenserType == "execution" && groupMetricsActor.nonEmpty) {
+        groupMetricsActor.get


We try to avoid unprotected gets - could use a case statement instead here, like

(dispenserType, groupMetricsActor) match { case ("execution", Some(a)) => a.ask... case _ => dispense(...) }

I did add groupMetricsActor.nonEmpty check on L44 check before doing the .get. Would you still recommend to use pattern matching here instead of what I have?

Yeah, it's true that the current code state is guaranteed to not cause a problem, but the compiler isn't checking that, and it's easy for the association between the nonEmpty check and the .get to degrade over time as the code is changed.

I see your point. I will change it not use .get and refactor the way you suggested 👍

salonishah11 added 5 commits August 29, 2024 14:19

wip

7f5773f

formatter

ce63b6a

pass exhausted groups info to dequeue

95bb235

refactor to not use Await.result()

01b5282

cleanup

44252c4

salonishah11 commented Aug 30, 2024

View reviewed changes

change if condition position

5e4f331

salonishah11 commented Aug 30, 2024

View reviewed changes

salonishah11 changed the title ~~[WX-1670] Don't allocate tokens for hog groups experiencing quota exhaustion~~ [WX-1670] Don't allocate job tokens for hog groups experiencing quota exhaustion Aug 30, 2024

jgainerdewar reviewed Aug 30, 2024

View reviewed changes

aednichols reviewed Aug 30, 2024

View reviewed changes

salonishah11 added 4 commits September 3, 2024 13:34

PR feedback

59ab583

fix issue; formatter

63ab888

Merge branch 'develop' into sps_use_new_quota_table

f21b11f

fix tests; add tests

ad935b9

salonishah11 marked this pull request as ready for review September 4, 2024 23:05

salonishah11 requested a review from a team as a code owner September 4, 2024 23:05

salonishah11 requested review from aednichols and jgainerdewar September 5, 2024 14:57

fix tests

94fcaac

jgainerdewar reviewed Sep 5, 2024

View reviewed changes

salonishah11 added 4 commits September 6, 2024 15:36

PR feedback; Changelog; disable feature by default

7da14d7

Merge branch 'develop' into sps_use_new_quota_table

471a798

formatter

be981b6

actually disable it this time :face-palm:

83a6841

salonishah11 requested a review from jgainerdewar September 6, 2024 20:01

aednichols approved these changes Sep 6, 2024

View reviewed changes

jgainerdewar reviewed Sep 6, 2024

View reviewed changes

salonishah11 added 2 commits September 6, 2024 18:06

PR feedback

256ad35

Merge branch 'develop' into sps_use_new_quota_table

5df2a18

salonishah11 requested a review from jgainerdewar September 9, 2024 15:13

jgainerdewar approved these changes Sep 9, 2024

View reviewed changes

Merge branch 'develop' into sps_use_new_quota_table

faaad44

salonishah11 merged commit c1d1302 into develop Sep 9, 2024
37 checks passed

salonishah11 deleted the sps_use_new_quota_table branch September 9, 2024 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WX-1670] Don't allocate job tokens for hog groups experiencing quota exhaustion #7520

[WX-1670] Don't allocate job tokens for hog groups experiencing quota exhaustion #7520

salonishah11 commented Aug 30, 2024 •

edited

Loading

salonishah11 Aug 30, 2024 •

edited

Loading

salonishah11 Sep 5, 2024

salonishah11 Aug 30, 2024

jgainerdewar Aug 30, 2024

salonishah11 Aug 30, 2024

aednichols Aug 30, 2024

salonishah11 Aug 30, 2024 •

edited

Loading

aednichols Sep 3, 2024

jgainerdewar left a comment

aednichols Aug 30, 2024

aednichols Aug 30, 2024

jgainerdewar left a comment

jgainerdewar Sep 5, 2024

salonishah11 commented Sep 5, 2024

aednichols left a comment

jgainerdewar Sep 6, 2024

salonishah11 Sep 6, 2024

aednichols Sep 6, 2024

jgainerdewar Sep 6, 2024

salonishah11 Sep 6, 2024

aednichols Sep 6, 2024

jgainerdewar Sep 6, 2024

salonishah11 Sep 6, 2024

jgainerdewar Sep 6, 2024

jgainerdewar Sep 6, 2024

salonishah11 Sep 6, 2024 •

edited

Loading

jgainerdewar Sep 6, 2024

salonishah11 Sep 6, 2024

	quotaExhaustedGroups: List[String]
	excludedGroups: List[String]

[WX-1670] Don't allocate job tokens for hog groups experiencing quota exhaustion #7520

[WX-1670] Don't allocate job tokens for hog groups experiencing quota exhaustion #7520

Conversation

salonishah11 commented Aug 30, 2024 • edited Loading

Description

Release Notes Confirmation

CHANGELOG.md

Terra Release Notes

salonishah11 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonishah11 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgainerdewar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgainerdewar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonishah11 commented Sep 5, 2024

aednichols left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonishah11 Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonishah11 commented Aug 30, 2024 •

edited

Loading

`CHANGELOG.md`

salonishah11 Aug 30, 2024 •

edited

Loading

salonishah11 Aug 30, 2024 •

edited

Loading

salonishah11 Sep 6, 2024 •

edited

Loading