fix: when cluster fails ensure end time of open allocation is set to the last cluster heartbeat [DET 6509] #3657

nrajanee · 2022-02-23T23:37:08Z

Description

fix: when cluster fails ensure end time of open allocation is set to the last cluster heartbeat [DET 6509]

Test Plan

Added an integration test which tests to ensure that the cluster heartbeat is added to the cluster id table accurately. It also checks whether open allocations have the correct cluster heartbeat as their endTime. The integration test covers the new functions added to postgres_cluster.go and the change made in CloseOpenAllocaitons in postgres_tasks.go. Manually check and ensure that the cluster heartbeat is getting updated every 10 minutes. This manual check will ensure that the go routine added to core.go works correctly.

Checklist

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.

cla-bot · 2022-02-23T23:37:10Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

netlify · 2022-02-23T23:37:14Z

✔️ Deploy Preview for determined-ui canceled.

🔨 Explore the source changes: 2f8bbac

🔍 Inspect the deploy log: https://app.netlify.com/sites/determined-ui/deploys/62195cc025d95400082b8905

stoksc · 2022-02-24T14:58:11Z

master/internal/core.go

@@ -807,6 +820,8 @@ func (m *Master) Run(ctx context.Context) error {
 		return err
 	}

+	go updateClusterHeartbeat(m.db)


Can you drop a comment like explaining that this must happen after CloseOpenAllocations above?

stoksc · 2022-02-24T15:00:01Z

master/static/migrations/20220215164216_add-cluser-id-cluser-heartbeat.up.sql

@@ -0,0 +1 @@
+ALTER TABLE public.cluster_id ADD COLUMN cluster_heartbeat timestamp not null DEFAULT (DATE_TRUNC('millisecond', now() at time zone 'utc')::timestamp);


can you add new line endings to the sql files and format them a bit?

stoksc · 2022-02-24T15:00:12Z

master/internal/db/postgres_tasks.go

+	cluster_heartbeat, err := db.GetClusterHeartBeat()
+
+	if err != nil {
+		return err
+	}


Suggested change

cluster_heartbeat, err := db.GetClusterHeartBeat()

if err != nil {

return err

}

cluster_heartbeat, err := db.GetClusterHeartBeat()

if err != nil {

return err

}

stoksc · 2022-02-24T15:11:19Z

master/internal/db/postgres_tasks.go

 	if _, err := db.sql.Exec(`
 UPDATE allocations
-SET end_time = current_timestamp AT TIME ZONE 'UTC'
+SET end_time = $1
 WHERE end_time IS NULL
-`); err != nil {
+`, cluster_heartbeat); err != nil {


I would prefer something like

UPDATE allocations SET end_time = cluster_heartbeat FROM cluster_id;

and dropping the code above this, with the advantages mainly being GetClusterHeartBeat becomes unused and less code is always better (and this becomes much less code), this is 1 db call instead of 2 (but this is not a big deal since it's every 10m, but applies elsewhere so it makes sense to try to be consistent).

Yes! I'm not sure why I did it this way in the first place. Getting directly from the cluster_id makes a lot more sense.

I changed the sql query to this: UPDATE allocations
SET end_time = cluster_heartbeat FROM cluster_id
WHERE end_time IS NULL. We need the end_time is NULL bit to ensure that only the open allocations end_time is set to cluster heartbeat, correct?

And I got rid fo the GetClusterHeartBeat method from postgres_cluster.go

stoksc · 2022-02-24T15:13:45Z

master/internal/db/postgres_cluster.go

+	var uuidVal []string
+	if err := db.sql.Select(&uuidVal, `SELECT cluster_id FROM cluster_id`); err != nil {
+		return errors.Wrapf(err, "error reading cluster_id from cluster_id table")
+	}
+	if len(uuidVal) != 1 {
+		return errors.Errorf(
+			"expecting exactly one cluster_id from cluster_id table, %d values found", len(uuidVal),
+		)
+	}


this is all basically unused, I think we can just get rid of this. if it is to check the 1 row invariant is upheld, then i would say the check should happen elsewhere because there is nothing we can do about it here, we may as well still do what we need to do (set cluster heartbeat).

I agree. done!

stoksc · 2022-02-24T15:16:34Z

master/internal/db/postgres_cluster.go

+	var layout = "2006-01-02T15:04:05.000Z"
+	cluster_heartbeat, err := time.Parse(layout, row_heartbeat[0])
+	return cluster_heartbeat, err


i commented elsewhere i dont think we need this func, but i'm also curious, i would think this should just work if you use time.Time rather than string + time.Parse. is that not the case?

Do you mean something like this:

var rowHeartbeat time.Time
if err := db.sql.Select(&rowHeartbeat, SELECT cluster_heartbeat FROM cluster_id); err != nil {
return time.Time{}, errors.Wrapf(err, "error reading cluster_heartbeat from cluster_id table")
}

Yes, I'll have to check if that works. I added this to the integration test so that we can ensure that we're updating the cluster heartbeat correctly.

stoksc · 2022-02-24T15:17:32Z

master/internal/core.go

+	t := time.NewTicker(10 * time.Minute)
+	defer t.Stop()
+	current_cluster_heartbeat := time.Now().UTC().Truncate(time.Millisecond)
+	for range t.C {
+		err := db.UpdateClusterHeartBeat(current_cluster_heartbeat)
+		if err != nil {
+			log.Error(err.Error())
+		}
+
+	}


to get the first tick immediately

Suggested change

t := time.NewTicker(10 * time.Minute)

defer t.Stop()

current_cluster_heartbeat := time.Now().UTC().Truncate(time.Millisecond)

for range t.C {

err := db.UpdateClusterHeartBeat(current_cluster_heartbeat)

if err != nil {

log.Error(err.Error())

}

}

t := time.NewTicker(10 * time.Minute)

defer t.Stop()

current_cluster_heartbeat := time.Now().UTC().Truncate(time.Millisecond)

for {

err := db.UpdateClusterHeartBeat(current_cluster_heartbeat)

if err != nil {

log.Error(err.Error())

}

<-t.C

}

also just because making sure we can gracefully shutdown is generally useful, can you go ahead pass a context.Context to this?

stoksc · 2022-02-24T15:20:15Z

master/internal/db/postgres_cluster_intg_test.go

+	current_time := time.Now().UTC().Truncate(time.Millisecond)
+	db.UpdateClusterHeartBeat(current_time)
+
+	cluster_heartbeat, err := db.GetClusterHeartBeat()


require.NoError after this

stoksc · 2022-02-24T15:23:18Z

master/internal/db/postgres_cluster_intg_test.go

+	_, err := db.GetOrCreateClusterID()
+
+	require.NoError(t, err, "failed to get or create cluster id")


i prefer we keep the require.NoError and the call that created it side by side to signal to the reader 'this is just an err check for the above call'. similar to how most go code reads

x, err := doThing() if err != nil { return err } y, err := doOtherThing() ...

not

x, err := doThing() if err != nil { return err } y, err := doOtherThing() ...

as another example (not just me), go source https://github.com/golang/go/blob/master/src/io/fs/readdir.go#L22-L47 usually does this, i find it makes it a lot easily to quickly skim code. as an extension of this, there's also https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow which helps too.

Yes it does make it easier to read. done!

stoksc · 2022-02-24T15:29:57Z

master/internal/db/postgres_cluster_intg_test.go

+	aOut, err := db.AllocationByID(aIn.AllocationID)
+
+	if *aOut.EndTime != cluster_heartbeat {
+		errors.Errorf("Expected end time of open allocation is = %q but it is = %q instead", cluster_heartbeat.String(), (*aOut.EndTime).String())


use require.Equal, errors.Errorf doesn't do anthing here

Oops yes thank you. I missed this one! Changed it.

cla-bot · 2022-02-24T21:52:51Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T17:48:45Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T18:37:10Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

stoksc · 2022-02-25T19:17:40Z

@nrajanee when you are done addressing feedback, could you reassign me so i know to re-review (or request a re-review and re-assign, too)

stoksc · 2022-02-25T19:18:09Z

@cla-bot[bot] check

cla-bot · 2022-02-25T19:18:10Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T19:18:11Z

The cla-bot has been summoned, and re-checked this pull request!

stoksc · 2022-02-25T19:19:57Z

@nrajanee you're added to contributors but cla-bot is still failing so you'll need to follow the instructions it commented likely. fyi, make sure to sign your commits with your @hpe email.

nrajanee · 2022-02-25T20:46:52Z

@cla-bot[bot] check

cla-bot · 2022-02-25T20:46:54Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T20:46:55Z

The cla-bot has been summoned, and re-checked this pull request!

nrajanee · 2022-02-25T20:51:00Z

@cla-bot[bot] check

cla-bot · 2022-02-25T20:51:02Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T20:51:03Z

The cla-bot has been summoned, and re-checked this pull request!

nrajanee · 2022-02-25T21:02:43Z

@cla-bot[bot] check

cla-bot · 2022-02-25T21:02:44Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T21:02:45Z

The cla-bot has been summoned, and re-checked this pull request!

nrajanee · 2022-02-25T21:11:00Z

@cla-bot[bot] check

cla-bot · 2022-02-25T21:11:02Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T21:11:03Z

The cla-bot has been summoned, and re-checked this pull request!

cla-bot · 2022-02-25T21:12:51Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

stoksc

lgtm, just a few final comments

stoksc · 2022-02-25T21:14:55Z

master/internal/db/postgres_cluster_intg_test.go

+	err = db.AddAllocation(aIn)
+	require.NoError(t, err, "failed to add allocation")
+
+	//Don't complete the above allocation and call CloseOpenAllocations


nit here and elsewhere

Suggested change

//Don't complete the above allocation and call CloseOpenAllocations

// Don't complete the above allocation and call CloseOpenAllocations

master/internal/db/postgres_cluster_intg_test.go

stoksc · 2022-02-25T21:15:53Z

master/static/migrations/20220215164216_add-cluser-id-cluser-heartbeat.down.sql

@@ -0,0 +1 @@
+ALTER TABLE public.cluster_id DROP COLUMN IF EXISTS cluster_heartbeat;  


nrajanee · 2022-02-25T21:22:12Z

@cla-bot[bot] check

cla-bot · 2022-02-25T21:22:14Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T21:22:15Z

The cla-bot has been summoned, and re-checked this pull request!

cla-bot · 2022-02-25T21:28:26Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T21:38:37Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2022-02-25T21:53:56Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Nikita Rajaneesh.
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@example.com
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

… is set to the last cluster heartbeat in cluster_id table

stoksc

lgtm. Great work!

nrajanee requested a review from stoksc February 23, 2022 23:37

stoksc reviewed Feb 24, 2022

View reviewed changes

nrajanee requested a review from stoksc February 25, 2022 20:55

stoksc reviewed Feb 25, 2022

View reviewed changes

nrajanee force-pushed the DET-6509_accurate_allocation_time branch from 59f4317 to 727c09d Compare February 25, 2022 21:28

nrajanee requested a review from stoksc February 25, 2022 21:38

nrajanee force-pushed the DET-6509_accurate_allocation_time branch from 50cdbcb to 3d0b138 Compare February 25, 2022 21:53

fix: [DET 6509] when cluster fails ensure end time of open allocation…

2f8bbac

… is set to the last cluster heartbeat in cluster_id table

nrajanee force-pushed the DET-6509_accurate_allocation_time branch from 3d0b138 to 2f8bbac Compare February 25, 2022 22:48

cla-bot bot added the cla-signed label Feb 25, 2022

stoksc approved these changes Feb 25, 2022

View reviewed changes

nrajanee changed the title ~~fix: [DET 6509] when cluster fails ensure end time of open allocation is set to the last cluster heartbeat~~ fix: when cluster fails ensure end time of open allocation is set to the last cluster heartbeat [DET 6509] Feb 28, 2022

nrajanee merged commit ecea441 into determined-ai:master Feb 28, 2022

dannysauer modified the milestones: 0.0.102, 0.17.11 Feb 6, 2024

		@@ -0,0 +1 @@
		ALTER TABLE public.cluster_id ADD COLUMN cluster_heartbeat timestamp not null DEFAULT (DATE_TRUNC('millisecond', now() at time zone 'utc')::timestamp);

		_, err := db.GetOrCreateClusterID()

		require.NoError(t, err, "failed to get or create cluster id")

	//Don't complete the above allocation and call CloseOpenAllocations
	// Don't complete the above allocation and call CloseOpenAllocations

		@@ -0,0 +1 @@
		ALTER TABLE public.cluster_id DROP COLUMN IF EXISTS cluster_heartbeat;

fix: when cluster fails ensure end time of open allocation is set to the last cluster heartbeat [DET 6509] #3657

fix: when cluster fails ensure end time of open allocation is set to the last cluster heartbeat [DET 6509] #3657

Conversation

nrajanee commented Feb 23, 2022 • edited

Description

Test Plan

Checklist

cla-bot bot commented Feb 23, 2022

netlify bot commented Feb 23, 2022 • edited

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

Choose a reason for hiding this comment

nrajanee Feb 24, 2022 • edited

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

nrajanee Feb 24, 2022 • edited

Choose a reason for hiding this comment

cla-bot bot commented Feb 24, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

stoksc commented Feb 25, 2022

stoksc commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

stoksc commented Feb 25, 2022 • edited

nrajanee commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

nrajanee commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

nrajanee commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

nrajanee commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

stoksc 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

nrajanee commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

stoksc left a comment

Choose a reason for hiding this comment

nrajanee commented Feb 23, 2022 •

edited

netlify bot commented Feb 23, 2022 •

edited

nrajanee Feb 24, 2022 •

edited

nrajanee Feb 24, 2022 •

edited

stoksc commented Feb 25, 2022 •

edited