chore: ft slot capacity check for each trial [DET-9897] #8213

jgongd · 2023-10-20T15:59:48Z

Description

Test Plan

e2e tests.

Commentary (optional)

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-9897

NicholasBlaskey · 2023-10-20T16:27:13Z

master/internal/logpattern/logpattern.go

+			// The first line of trial logs is printing expconf which has the regex pattern.
+			// We skip monitoring this line.
+			regex := "(.*)(\\\"log_pattern_policies\\\":)(.*)"
+			compiledRegex, err := l.getCompiledRegex(regex)


we can move this outside the policy loop and under the range logs loop

NicholasBlaskey · 2023-10-20T16:31:44Z

master/internal/sproto/agent_resource_manager.go

@@ -112,3 +112,37 @@ func (t TerminateDecision) String() string {
 	}
 	return strings.Join(item, ",")
 }
+
+// ValidateResourcePoolAvailabilityParam contains the params for ValidateResourcePoolAvailability().
+type ValidateResourcePoolAvailabilityParam struct {


nit: call this ValidateResourcePoolAvailabilityRequest to better match other parameters like GetDefaultAuxResourcePoolRequest

NicholasBlaskey · 2023-10-20T16:39:29Z

master/internal/rm/agentrm/resource_pool.go

-				totalSlots += len(a.slotStates)
+				if !disallowedNodes.Contains(a.Handler.Address().Local()) {
+					totalSlots += len(a.slotStates)
+				}
 			}
 		case rp.config.Provider.AWS != nil:
 			totalSlots = rp.config.Provider.MaxInstances * rp.config.Provider.AWS.SlotsPerInstance()


could we somehow subtract the blocklist nodes fromtotalSlots if the agents blocked are in the provided pool and been launched by the provider?

I'm not sure how hard this will be, if it is too hard I think it might be okay to not do

NicholasBlaskey · 2023-10-20T16:40:52Z

master/internal/sproto/agent_resource_manager.go

+
+// ValidateResourcePoolAvailabilityParamOption accepts functions that may modify the
+// *ValidateResourcePoolAvailabilityParam instance.
+type ValidateResourcePoolAvailabilityParamOption func(v *ValidateResourcePoolAvailabilityParam)


non blocking: These helpers seem like a little overkill to me. I'm not against them but I think just the struct would be fine for me too.

NicholasBlaskey · 2023-10-20T16:42:30Z

master/internal/trial.go

+	// TODO: The first return value is []command.LaunchWarning{command.CurrentSlotsExceeded}.
+	// How do we want to handle this? In newExperiment() the warning is part of the response back
+	// to the client.
+	_, err = t.rm.ValidateResourcePoolAvailability(


I think the best we can do is just log any launch warnings if they have them?

NicholasBlaskey · 2023-10-20T16:45:29Z

master/internal/trial.go

+	// TODO: The first return value is []command.LaunchWarning{command.CurrentSlotsExceeded}.
+	// How do we want to handle this? In newExperiment() the warning is part of the response back
+	// to the client.
+	_, err = t.rm.ValidateResourcePoolAvailability(


can this be moved earlier in the function, so we don't

call this prom.AssociateJobExperiment(t.jobID, strconv.Itoa(t.experimentID), t.config.Labels()) or any other functions we might not want to?

NicholasBlaskey · 2023-10-20T16:45:44Z

master/internal/trial.go

+	// TODO: The first return value is []command.LaunchWarning{command.CurrentSlotsExceeded}.
+	// How do we want to handle this? In newExperiment() the warning is part of the response back
+	// to the client.
+	_, err = t.rm.ValidateResourcePoolAvailability(


I think we don't want to run this in the restore case

For understanding, what's the issue if we run this in the restore case?

We want skip the restore case because I think there is a possibility not all agents will be reconnected when the job is restoring.

In addition, really the only time we want this to fail is when a trial decides to reschedule and finds it can't.

master/internal/rm/agentrm/resource_pool.go

NicholasBlaskey

Looks good to me

I think e2e test is pretty much the only non nit comment

NicholasBlaskey · 2023-10-27T18:12:39Z

master/internal/rm/agentrm/resource_pool.go

 		switch {
 		case rp.config.Provider == nil:
-			rp.agentStatesCache = rp.fetchAgentStates(ctx)
+
 			defer func() {


move this defer to outside of the switch so we are consistent about unsetting the cache

NicholasBlaskey · 2023-10-27T18:14:01Z

master/internal/logpattern/logpattern.go

+				continue
+			}
+
+			regex = fmt.Sprintf("(.*)%s(.*)", policy.Pattern())


remove the (.*), I think this undoes a change that removed it in the original PR

Did a cherry pick on top of your last commit in ft-hw-failure-e2e-cleanup. Forgot to check diff.

NicholasBlaskey · 2023-10-27T18:15:49Z

master/internal/logpattern/logpattern.go

@@ -61,9 +61,21 @@ func (l *logPatternPolicies) monitor(ctx context.Context,
 		if log.AgentID == nil {
 			return fmt.Errorf("agentID must be non nil to monitor logs")
 		}
+		// The first line of trial logs is printing expconf which has the regex pattern.
+		// We skip monitoring this line.
+		regex := "(.*)(\\\"log_pattern_policies\\\":)(.*)"


can we make this a package level variable and use mustCompile since we will always need this regex

Like we do for latin text

determined/master/internal/api_user.go

Line 36 in a33d2b8

latinText = regexp.MustCompile("[^[:graph:]\\s]")

NicholasBlaskey · 2023-10-27T18:17:04Z

master/internal/logpattern/logpattern.go

+			if err != nil {
+				return err
+			}
+			if compiledRegex.MatchString(log.Log) {


I think this check can go outside of the policy loop

Good catch!

NicholasBlaskey · 2023-10-27T18:19:37Z

master/internal/trial.go

@@ -459,6 +476,7 @@ func (t *trial) maybeAllocateTask() error {
 		Debugf("starting new trial allocation")

 	prom.AssociateJobExperiment(t.jobID, strconv.Itoa(t.experimentID), t.config.Labels())
+


nit: don't add line here

master/internal/trial.go

NicholasBlaskey · 2023-10-27T18:23:51Z

master/internal/trial.go

+	// TODO: The first return value is []command.LaunchWarning{command.CurrentSlotsExceeded}.
+	// How do we want to handle this? In newExperiment() the warning is part of the response back
+	// to the client.
+	_, err = t.rm.ValidateResourcePoolAvailability(


We want skip the restore case because I think there is a possibility not all agents will be reconnected when the job is restoring.

In addition, really the only time we want this to fail is when a trial decides to reschedule and finds it can't.

master/internal/rm/resource_manager_iface.go

cla-bot · 2023-10-31T12:34:14Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Ubuntu.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

netlify · 2023-10-31T17:57:19Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`74e6ac5`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/654a4d616faa5b00088f23d2
😎 Deploy Preview	https://deploy-preview-8213--determined-ui.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

NicholasBlaskey · 2023-10-31T18:06:20Z

master/internal/rm/agentrm/resource_pool.go

+		if msg.TaskID != nil {
+			blockedNodes, err := logpattern.GetBlockedNodes(context.TODO(), *msg.TaskID)
+			if err != nil {
+				panic(err.Error())


don't panic here instead we can

ctx.Respond(err) return nil

NicholasBlaskey · 2023-10-31T18:12:44Z

master/internal/trial.go

@@ -381,6 +381,33 @@ func (t *trial) maybeAllocateTask() error {
 	}

 	restoredAllocation, err := t.maybeRestoreAllocation()
+
+	if restoredAllocation == nil {
+		launchWarnings, err := t.rm.ValidateResourcePoolAvailability(


can we put this inside another function?

we should also call it after we check the err from t.maybeRestoreAllocation

NicholasBlaskey · 2023-10-31T18:16:32Z

master/internal/trial.go

+
+	if restoredAllocation == nil {
+		launchWarnings, err := t.rm.ValidateResourcePoolAvailability(
+			&sproto.ValidateResourcePoolAvailabilityRequest{


can we add another check that we only check the capacity if we have at least one blocked nodes from logpattern.GetBlockedNodes(context.TODO(), *msg.TaskID)

I am somewhat worried that someone might lose a bunch of agents and all their jobs would fail because of this

So this only apply to the restarted experiment?

I would add the check right before we call ValidateResourcePoolAvailability

Yes so only don't check for the restart case of calling ValidateResourcePoolAvailability.

NicholasBlaskey

LGTM nice work

NicholasBlaskey · 2023-11-01T16:30:14Z

master/internal/trial.go

+		return t.transition(model.StateWithReason{
+			State:               model.ErrorState,
+			InformationalReason: exitReason,
+		})


I think we can just log this and move on, we don't need to error

The job stuck in queue without this. Output of test_log_policy_exclude_node_single_agent:

ERRO[2023-11-01T14:13:08-04:00] task ID 57.65b30b87-e141-41fe-8816-7e1ee1a0ff36 slots requested exceeds default resource pool capacity DEBU[2023-11-01T14:13:08-04:00] starting new trial allocation allocation-id=57.65b30b87-e141-41fe-8816-7e1ee1a0ff36.2 component=trial experiment-id=57 job-id=f54aab51-d245-46f5-a200-c8830908fe46 task-id=57.65b30b87-e141-41fe-8816-7e1ee1a0ff36 task-type=TRIAL trial-id=57 trial-run-id=2 DEBU[2023-11-01T14:13:08-04:00] requestResources add allocation experiment-id=57 job-id=f54aab51-d245-46f5-a200-c8830908fe46 task-id=57.65b30b87-e141-41fe-8816-7e1ee1a0ff36 task-type=TRIAL trial-id=57 trial-run-id=2 INFO[2023-11-01T14:13:08-04:00] resources are requested by Trial 57 (Experiment 57) (Allocation ID: 57.65b30b87-e141-41fe-8816-7e1ee1a0ff36.2) actor-local-addr=default actor-system=master allocation-id=57.65b30b87-e141-41fe-8816-7e1ee1a0ff36.2 go-type=resourcePool resource-pool=default restore=false restoring=false

oh my bad, I thought the above code handled this check

determined/master/internal/api_command.go

Line 122 in 7772fb3

a.m.config.LaunchError &&

https://docs.determined.ai/latest/reference/deploy/master-config-reference.html#launch-error

I think we should just return an error only if that launch-error config is set?

Address comments

stoksc · 2023-11-07T15:40:51Z

master/internal/mocks/rm.go

@@ -0,0 +1,728 @@
+// Code generated by mockery v2.20.0. DO NOT EDIT.


we need to .gitgnore mocks or something.

stoksc · 2023-11-07T15:44:52Z

master/internal/trial.go

+		}
+		if len(blockedNodes) > 0 {
+			if err := t.checkResourcePoolRemainingCapacity(); err != nil {
+				return err


how did we test this? i don't think just returning an error is quite right here. i think like all the code around it, we need to transition to a terminal state.

there is an invariant here we need to have commented so folks don't miss it (cc @erikwilson ). handleAllocationExit must transition to a terminal state or reallocate, otherwise the system is in an invalid state.

stoksc · 2023-11-07T15:46:15Z

master/internal/trial.go

+				InformationalReason: msg,
+			})
+		}
+		logrus.Warn(msg)


use t.syslog here, so we get more info on the structured log

jgongd requested a review from NicholasBlaskey October 20, 2023 15:59

jgongd requested a review from a team as a code owner October 20, 2023 15:59

cla-bot bot added the cla-signed label Oct 20, 2023

NicholasBlaskey reviewed Oct 20, 2023

View reviewed changes

NicholasBlaskey force-pushed the ft-hw-failure-e2e-cleanup branch from 4460f2a to 03b74de Compare October 25, 2023 19:36

NicholasBlaskey requested a review from a team as a code owner October 25, 2023 19:36

NicholasBlaskey requested review from wes-turner and removed request for a team October 25, 2023 19:36

NicholasBlaskey force-pushed the ft-hw-failure-e2e-cleanup branch from 51011e3 to 7957f22 Compare October 25, 2023 21:02

jgongd force-pushed the jgong-ft-capacity-check-rebased branch from d9fc86d to dbf1f4c Compare October 27, 2023 16:22

jgongd commented Oct 27, 2023

View reviewed changes

master/internal/rm/agentrm/resource_pool.go Show resolved Hide resolved

NicholasBlaskey reviewed Oct 27, 2023

View reviewed changes

cla-bot bot removed the cla-signed label Oct 31, 2023

jgongd force-pushed the jgong-ft-capacity-check-rebased branch from 4a2a6bd to 373bbfa Compare October 31, 2023 12:44

cla-bot bot added the cla-signed label Oct 31, 2023

NicholasBlaskey force-pushed the ft-hw-failure-e2e-cleanup branch from 89f109b to e06a350 Compare October 31, 2023 13:21

jgongd force-pushed the jgong-ft-capacity-check-rebased branch 2 times, most recently from aa61403 to a649b2a Compare October 31, 2023 15:12

NicholasBlaskey force-pushed the ft-hw-failure-e2e-cleanup branch 2 times, most recently from 68b9d66 to 5698fe6 Compare October 31, 2023 15:35

jgongd force-pushed the jgong-ft-capacity-check-rebased branch from a649b2a to b8a4395 Compare October 31, 2023 16:01

Base automatically changed from ft-hw-failure-e2e-cleanup to main October 31, 2023 16:10

jgongd force-pushed the jgong-ft-capacity-check-rebased branch from b8a4395 to 52848a2 Compare October 31, 2023 17:57

NicholasBlaskey reviewed Oct 31, 2023

View reviewed changes

jgongd force-pushed the jgong-ft-capacity-check-rebased branch 2 times, most recently from 6aadb8a to dfa9413 Compare October 31, 2023 20:51

NicholasBlaskey approved these changes Nov 1, 2023

View reviewed changes

jgongd force-pushed the jgong-ft-capacity-check-rebased branch from f18edd1 to f353484 Compare November 1, 2023 21:12

jgongd force-pushed the jgong-ft-capacity-check-rebased branch 2 times, most recently from d8bcd7d to 60b8e9d Compare November 6, 2023 18:05

jgongd added 10 commits November 7, 2023 09:44

chore: ft trial slot capacity check [DET-9897]

8c09608

chore: ft trial slot capacity check [DET-9897]

95681c0

chore: ft trial slot capacity check [DET-9897]

919a1b3

Address comments

chore: ft trial slot capacity check [DET-9897]

62e6a6d

lint

573c076

remove TODO

45aa878

Address comments

a7d7e07

lint and bugfix

6e07a83

cleanup TODO

048679d

Optimize

74e6ac5

jgongd force-pushed the jgong-ft-capacity-check-rebased branch from 60b8e9d to 74e6ac5 Compare November 7, 2023 14:44

jgongd merged commit 8418029 into main Nov 7, 2023
68 of 80 checks passed

jgongd deleted the jgong-ft-capacity-check-rebased branch November 7, 2023 15:17

stoksc reviewed Nov 7, 2023

View reviewed changes

dannysauer added this to the 0.26.4 milestone Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: ft slot capacity check for each trial [DET-9897] #8213

chore: ft slot capacity check for each trial [DET-9897] #8213

jgongd commented Oct 20, 2023 •

edited

Loading

NicholasBlaskey Oct 20, 2023

NicholasBlaskey Oct 20, 2023

NicholasBlaskey Oct 20, 2023

NicholasBlaskey Oct 20, 2023

NicholasBlaskey Oct 20, 2023

NicholasBlaskey Oct 20, 2023

NicholasBlaskey Oct 20, 2023

jgongd Oct 27, 2023

NicholasBlaskey Oct 27, 2023

NicholasBlaskey left a comment

NicholasBlaskey Oct 27, 2023

NicholasBlaskey Oct 27, 2023

jgongd Oct 31, 2023

NicholasBlaskey Oct 27, 2023

NicholasBlaskey Oct 27, 2023

jgongd Oct 31, 2023

NicholasBlaskey Oct 27, 2023

NicholasBlaskey Oct 27, 2023

cla-bot bot commented Oct 31, 2023

netlify bot commented Oct 31, 2023 •

edited

Loading

NicholasBlaskey Oct 31, 2023

NicholasBlaskey Oct 31, 2023

NicholasBlaskey Oct 31, 2023

jgongd Oct 31, 2023

NicholasBlaskey Oct 31, 2023

NicholasBlaskey left a comment

NicholasBlaskey Nov 1, 2023

jgongd Nov 1, 2023

NicholasBlaskey Nov 1, 2023

stoksc Nov 7, 2023

stoksc Nov 7, 2023

stoksc Nov 7, 2023

stoksc Nov 7, 2023

		@@ -459,6 +476,7 @@ func (t *trial) maybeAllocateTask() error {
		Debugf("starting new trial allocation")

		prom.AssociateJobExperiment(t.jobID, strconv.Itoa(t.experimentID), t.config.Labels())

		@@ -0,0 +1,728 @@
		// Code generated by mockery v2.20.0. DO NOT EDIT.

chore: ft slot capacity check for each trial [DET-9897] #8213

chore: ft slot capacity check for each trial [DET-9897] #8213

Conversation

jgongd commented Oct 20, 2023 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

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

NicholasBlaskey 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

cla-bot bot commented Oct 31, 2023

netlify bot commented Oct 31, 2023 • edited Loading

✅ Deploy Preview for determined-ui ready!

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

NicholasBlaskey 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

jgongd commented Oct 20, 2023 •

edited

Loading

netlify bot commented Oct 31, 2023 •

edited

Loading