server: catch incomplete available memory detection by gosigar #15841

knz · 2017-05-10T12:15:25Z

If gosigar is told by a syscall there is 0 available memory, consider
this information as false even if there was no error.

Fixes #15839.

cockroach-teamcity · 2017-05-10T12:15:31Z

This change is

tbg · 2017-05-10T12:26:57Z

pkg/server/config.go

@@ -237,6 +237,12 @@ func GetTotalMemory() (int64, error) {
 			humanize.IBytes(mem.Total), humanize.Bytes(math.MaxInt64))
 	}
 	totalMem := int64(mem.Total)
+	checkTotal := func(x int64) error {


nit: consider making the signature here (int64, error). That way you can hold up the usual convention (that you return zero when there's an error) more explicitly.

oh good idea. Done.

tbg · 2017-05-10T12:31:40Z

pkg/server/config.go

 		}
-		if cgAvlMem > mem.Total {
+		if mem.Total > 0 && cgAvlMem > mem.Total {


mem.Total is (intuitively, not by type) equal to totalMem, right? I think it would add to clarity if mem went out of scope after totalMem was a thing (even at the cost of a type conversion). Just a suggestion, might be misreading this as well.

It makes the code slightly more readable perhaps. I changed it as you suggested.

tbg

sorry to have so many nits for such a small change - you're still free to opt out.

tbg · 2017-05-10T12:45:21Z

pkg/server/config.go

 		}
-		if cgAvlMem > mem.Total {
+		if totalMem > 0 && int64(cgAvlMem) > totalMem {


looks like we could overflow cgAvlMem here and in l. 284 during the conversion?

Ah nevermind! You check above.

tbg · 2017-05-10T12:46:20Z

pkg/server/config.go

 		}
 		var cgAvlMem uint64
 		if cgAvlMem, err = strconv.ParseUint(strings.TrimSpace(string(buf)), 10, 64); err != nil {
 			if log.V(1) {
 				log.Infof(context.TODO(), "can't parse available memory from cgroups (%s), using system memory %s instead", err,
 					humanizeutil.IBytes(totalMem))
 			}
-			return totalMem, nil
+			return checkTotal(totalMem)
 		}
 		if cgAvlMem > math.MaxInt64 {
 			if log.V(1) {
 				log.Infof(context.TODO(), "available memory from cgroups is too large and unsupported %s using system memory %s instead",


this sentence would look weird because cgAvlMem pops up randomly.

What do you mean "looks weird"?

the first %s is outside of the flow of the sentence. It would look better in parentheses, or moved up to read "available memory %s from cgroups is too large, ...". Still only a nit.

Oh now I understand. Thanks for clarifying. Done.

tamird · 2017-05-10T13:01:23Z

Has this been reported upstream? On May 10, 2017 8:15 AM, "kena" <[email protected]> wrote: @knz <https://github.com/knz> requested your review on: cockroachdb/cockroach#15841 <#15841> server: catch incomplete available memory detection by gosigar. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15841 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPPG45jJhp0HJ8n2j-8B79g05CXjXks5r4apegaJpZM4NWk3i> .

knz · 2017-05-10T13:16:54Z

@tamird yes elastic/gosigar#72

benesch · 2017-05-10T13:45:46Z

FWIW I wasn't encountering this on my OpenBSD VM. I'm going to do some more digging when I have some spare time. 👍 in the meantime.

knz · 2017-05-10T13:48:02Z

@benesch perhaps the syscall(s) used in gosigar is sensitive to some configuration, which the gosigar openbsd support's author didn't take into account. If I were you I'd make an inventory of the syscalls in sigar_openbsd.go and read the man pages thoroughly.

tamird · 2017-05-11T01:33:16Z

LGTM but we should like to the upstream issue since we don't want to keep this extra layer of paranoia on top of gosigar forever.

Thanks for fixing! Please consider nominating for 1.0.1.

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

pkg/server/config.go, line 284 at r4 (raw file):

			return checkTotal(totalMem)
		}
		totalMem = int64(cgAvlMem)

why not return checkTotal(..) in keeping with convention elsewhere?

Comments from Reviewable

If gosigar is told by a syscall there is 0 available memory, consider this information as false even if there was no error. Upstream issue: elastic/gosigar#72

knz · 2017-05-12T17:08:53Z

I have linked the upstream issue. TFYR!

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

pkg/server/config.go, line 284 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why not return checkTotal(..) in keeping with convention elsewhere?

Done.

Comments from Reviewable

tamird · 2017-05-12T17:41:11Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

Comments from Reviewable

knz requested a review from tamird May 10, 2017 12:15

knz force-pushed the check-gosigar-mem branch from c956ada to 09e9f33 Compare May 10, 2017 12:16

tbg approved these changes May 10, 2017

View reviewed changes

knz force-pushed the check-gosigar-mem branch from 09e9f33 to 37e7024 Compare May 10, 2017 12:28

tbg approved these changes May 10, 2017

View reviewed changes

knz force-pushed the check-gosigar-mem branch from 37e7024 to 77b8830 Compare May 10, 2017 12:35

tbg approved these changes May 10, 2017

View reviewed changes

tbg reviewed May 10, 2017

View reviewed changes

knz force-pushed the check-gosigar-mem branch from 77b8830 to 20826c3 Compare May 10, 2017 14:04

server: catch incomplete available memory detection by gosigar

f161689

If gosigar is told by a syscall there is 0 available memory, consider this information as false even if there was no error. Upstream issue: elastic/gosigar#72

knz force-pushed the check-gosigar-mem branch from 20826c3 to f161689 Compare May 12, 2017 17:08

knz added the 1.0.1 label May 12, 2017

knz merged commit be5890c into cockroachdb:master May 12, 2017

knz deleted the check-gosigar-mem branch May 12, 2017 17:35

mberhault mentioned this pull request May 15, 2017

cherrypick: server: catch incomplete available memory detection by gosigar #15914

Merged

mberhault added the 1.0.1-merged label May 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: catch incomplete available memory detection by gosigar #15841

server: catch incomplete available memory detection by gosigar #15841

knz commented May 10, 2017

cockroach-teamcity commented May 10, 2017

tbg May 10, 2017

knz May 10, 2017

tbg May 10, 2017

knz May 10, 2017

tbg left a comment

tbg May 10, 2017

tbg May 10, 2017

tbg May 10, 2017

knz May 10, 2017

tbg May 10, 2017

knz May 10, 2017

tamird commented May 10, 2017 via email

knz commented May 10, 2017

benesch commented May 10, 2017

knz commented May 10, 2017

tamird commented May 11, 2017

knz commented May 12, 2017

tamird commented May 12, 2017

server: catch incomplete available memory detection by gosigar #15841

server: catch incomplete available memory detection by gosigar #15841

Conversation

knz commented May 10, 2017

cockroach-teamcity commented May 10, 2017

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

tbg 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

tamird commented May 10, 2017 via email

knz commented May 10, 2017

benesch commented May 10, 2017

knz commented May 10, 2017

tamird commented May 11, 2017

knz commented May 12, 2017

tamird commented May 12, 2017