964880 - Add swap to linux build machines with <12GB RAM

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task)

Description

[Jordan Lund (:jlund)]

started on: https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=a581d53d51f4

other cases:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=4936f09590a0
https://tbpl.mozilla.org/php/getParsedLog.php?id=33691486&tree=Mozilla-Aurora

example log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33695402&tree=Mozilla-Aurora&full=1

error context:
collect2: error: ld terminated with signal 9 [Killed]
make[5]: *** [libxul.so] Error 1
make[5]: Leaving directory `/builds/slave/m-aurora-l64-00000000000000000/build/obj-firefox/toolkit/library'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/builds/slave/m-aurora-l64-00000000000000000/build/obj-firefox'
make[3]: *** [default] Error 2
make[3]: Leaving directory `/builds/slave/m-aurora-l64-00000000000000000/build/obj-firefox'
make[2]: *** [realbuild] Error 2
make[2]: Leaving directory `/builds/slave/m-aurora-l64-00000000000000000/build'
make[1]: *** [profiledbuild] Error 2
make[1]: Leaving directory `/builds/slave/m-aurora-l64-00000000000000000/build'
make: *** [build] Error 2
State Changed: unlock buildroot

Comment 1

[Jordan Lund (:jlund)]

I am worried that this could be a memory issue: collect2: ld terminated with signal 9 [Killed]

Could this be due to a difference in mem sizes on our EC2 machines?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

We just hit one on esr24 too.
https://tbpl.mozilla.org/php/getParsedLog.php?id=33700664&tree=Mozilla-Esr24

Comment 3

[Jordan Lund (:jlund)]

we have recently switched EC2 instance types from m3.xlarge to c3.xlarge resulting in a ram drop from 15GB RAM to 7.5GB RAM.

I have a feeling based off errors like this: collect2: error: ld terminated with signal 9 [Killed]

we are hitting memory limit issues on builds like linux64 opt builds.

Trees are currently down so it is hard to verfiy this with current builds. However one linux64 esr24 build is running against one of our 'in house' linux machines. These machines also have around 7.5gb ram but with an additional 4gb swap space.

Taking a recent snapshot of free memory from one of these in-house machines at similar point to where the builds have been failing (~85min), we can see that memory is getting pretty sparse and swap is being used a fair bit:

[[e-mail] ~]# free -m 
             total       used       free     shared    buffers     cached
Mem:          7864       7753        110          0          0         35
-/+ buffers/cache:       7717        146
Swap:         4095       2249       1846

Comment 4

[Rail Aliiev [:rail]]

it sounds like an oom kill. We may need to add swap (SSD instance storage for bld-linux64). I hope this is not an issue for the try-linux64-ec2 machines, otherwise we may need to use EBS for swap (instance storage is mounted as /builds/slave already).

Comment 5

[Chris AtLee [:catlee]]

Instead of re-partitioning to add swap, we could add a swap file on instance storage.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Bumping priority on this. We're hitting these OOMs on !trunk very frequently, and it takes 2+ hours on a retrigger before we know if it'll die again.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Even better, now a lot of the retriggers aren't even working. But hey, it's not like we have a last-second crash landing on esr24 that needs builds and tests or something.

Comment 8

[Justin Wood (:Callek)]

[23:03:08]	RyanVM	to recap: most !trunk linux64 builds OOM and die and we can't retrigger them
[23:03:25]	RyanVM	last_good_build is going to *love* this tomorrow
[23:03:31]	nthomas	so this was a new class of slave right ?
[23:03:34]	RyanVM	s/build/rev
[23:03:36]	RyanVM	yes
[23:03:38]	RyanVM	c3 or something

Is it worth reverting the c3.whatever instance type due to this bug?

Comment 9

[Mike Hommey [:glandium]]

My bet is this is a PGO-only problem.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Probably, otherwise we'd be seeing it on the non-unified trunk builds. Or trunk PGO builds where unified is saving our bacon.

Comment 11

[Phil Ringnalda (:philor)]

(In reply to Rail Aliiev [:rail] from comment #4)
> I hope this is not an issue for the try-linux64-ec2 machines

Doesn't appear to be - I ran 49 (because I can't count to 50) PGO non-unified trunk builds on try last night, and didn't hit this once.

Comment 12

[(dormant account)]

(In reply to Justin Wood (:Callek) from comment #8)
> [23:03:08]	RyanVM	to recap: most !trunk linux64 builds OOM and die and we
> can't retrigger them
> [23:03:25]	RyanVM	last_good_build is going to *love* this tomorrow
> [23:03:31]	nthomas	so this was a new class of slave right ?
> [23:03:34]	RyanVM	s/build/rev
> [23:03:36]	RyanVM	yes
> [23:03:38]	RyanVM	c3 or something
> 
> Is it worth reverting the c3.whatever instance type due to this bug?
No. These new c3 nodes are slightly faster, have ssds and are much cheaper. Sorry this came up. We didn't notice this during testing due to unified builds.

catlee will add a swap file tomorrow.

catlee, ideally you'd use the fallocate util to create the file. it's faster and better to do that than use dd or sparse file: https://ausinfotech.net/blog/fallocate-linux-utility-to-preallocate-space-to-a-file/

Comment 13

[Chris AtLee [:catlee]]

and indeed our build machines do have fallocate installed.

I watched a pair of machines through a build cycle on aurora after added 4G swap, and neither ended up using more than 500k of swap. We're really close to the edge here!

Let's add 4G just to be safe for the future.

Comment 14

[(dormant account)]

(In reply to Chris AtLee [:catlee] from comment #13)
> and indeed our build machines do have fallocate installed.
> 
> I watched a pair of machines through a build cycle on aurora after added 4G
> swap, and neither ended up using more than 500k of swap. We're really close
> to the edge here!
> 
> Let's add 4G just to be safe for the future.

sounds good. However once we have proper pooling, lets pool builds that need more memory onto machines with swap files and keep other machines swap-free.

Comment 15

[Rail Aliiev [:rail]]

Massimo and I are going to poke this issue today. The idea is to add a swap partition or file. If this approach doesn't work we may consider switching back to older hardware. :/

Comment 16

[Massimo Gervasini [:massimo]]

Attachment: adding swap file to centos/xenhvm instances

Added a script to create and mount a swap file for CentOS/xenhvm machines.

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8368632 [details] [diff] [review]
adding swap file to centos/xenhvm instances

Review of attachment 8368632 [details] [diff] [review]:
-----------------------------------------------------------------

Rail just made some comments on bug 924430 regarding use of 'xenhvm' to indicate an AWS instance.  It doesn't look like it's the best indicator.  Let's land this with the suggested modifications, and I'll file a new bug to add an aws_instance_type fact that can be used instead.

::: modules/tweaks/manifests/swap.pp
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +class tweaks::swap {

The name of this class suggests "I enable swap", when in fact for the vast majority of hosts, it does no such thing.  Can you rename it to something like tweaks::swap_on_instance_storage?  That at least makes it clear what it's doing.  An extra comment explaining this would help, too, and pointing to this bug.  

The second bit will be to only include this class where necessary.  I think that will be easier to fix with an `aws_instance_type` fact, which can be used in toplevel::slave::build::mock to only include the class on `c3.*` instances.  Once that's in place, both of the case statements in this class can have defaults which fail(), instead of simply doing nothing.

@@ +19,5 @@
> +                    exec {
> +                        "/sbin/service add_swap start || true":
> +                            subscribe   => File["/etc/init.d/add_swap"],
> +                            refreshonly => true;
> +                    }

This bit shouldn't be necessary if you use ensure => running.  It might be good to add a 'status' command, or make starting idempotent.  I don't know what happens if you delete a swapfile and then run swapon on the new version..

Comment 18

[Rail Aliiev [:rail]]

Comment on attachment 8368632 [details] [diff] [review]
adding swap file to centos/xenhvm instances

https://hg.mozilla.org/build/puppet/rev/235d596dfcae with some changes

Comment 19

[Rail Aliiev [:rail]]

I see swap enabled on freshly rebooted/started slaves. \o/

In a couple of hours the errors should disappear.

Comment 20

[(dormant account)]

(In reply to Rail Aliiev [:rail] from comment #18)
> Comment on attachment 8368632 [details] [diff] [review]
> adding swap file to centos/xenhvm instances
> 
> https://hg.mozilla.org/build/puppet/rev/235d596dfcae with some changes

Why is this pattern-matching on instance type vs amount of ram < 12gb? Seems fragile

Comment 21

[Rail Aliiev [:rail]]

(In reply to Taras Glek (:taras) from comment #20)
> Why is this pattern-matching on instance type vs amount of ram < 12gb? Seems
> fragile

This is a good idea.

Massimo, can we use RAM, so we don't even rely on the fact whether a slave is in AWS or not?

Comment 22

[Massimo Gervasini [:massimo]]

Attachment: add_swap.patch

Hi Rail,

this patch removes the amazon box check; a swap file is created if MemTotal (from /proc/meminfo) is smaller than 12 GB

Comment 23

[Rail Aliiev [:rail]]

Comment on attachment 8373299 [details] [diff] [review]
add_swap.patch

Review of attachment 8373299 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/tweaks/files/add_swap
@@ +17,5 @@
>  PATH="/sbin:/bin:/usr/sbin:/usr/bin"
>  
>  DEFAULT_SWAP_FILE="/swap_file"
> +MIN_MEMORY_SIZE_GB=12
> +MIN_MEMORY_SIZE_KB="(($MIN_MEMORY_SIZE_GB * 1024 * 1024))"

You forgot "$" -- it should be $((...))

@@ +18,5 @@
>  
>  DEFAULT_SWAP_FILE="/swap_file"
> +MIN_MEMORY_SIZE_GB=12
> +MIN_MEMORY_SIZE_KB="(($MIN_MEMORY_SIZE_GB * 1024 * 1024))"
> +MEMORY_SIZE_KB="$(cat /proc/meminfo | grep MemTotal | sed -n 's/^.* \([[:digit:]]*\) .*$/\1/p')"

It's hard to parse this... Can you use something like 

MEMORY_SIZE_KB="$(grep ^MemTotal: /proc/meminfo | awk '{print $2}')"

@@ +24,5 @@
>  
>  set -e
>  
> +# Do we need a swap file? If not, exit without error.
> +if (($MEMORY_SIZE_KB > $MIN_MEMORY_SIZE_KB)); then exit 0; fi

I don't think this will work, you need to use [..] for conditions..

I'd suggest something like this:

if [ -z $MEMORY_SIZE_KB ]; then
    log "Unable to detect RAM available, adding swap..."
elif [ $MEMORY_SIZE_KB -gt $MIN_MEMORY_SIZE_KB ]; then
    # check if we need a swap file; if not, exit without error
    log "No swap required, $MEMORY_SIZE_KB KB mem available"
    exit 0
fi

Comment 24

[Massimo Gervasini [:massimo]]

Attachment: 964880.patch

Hi Rail,

I've updated the patch as requested, here is the new one.

Comment 25

[Massimo Gervasini [:massimo]]

In bug 966070, we are moving from m1.large to m3.medium; we need to enable the same swap file mechanism on masters too.

Comment 26

[Massimo Gervasini [:massimo]]

Attachment: 964880.master.patch

Enabling swap on buildbot master nodes too.

add_swap will be available on every buildbot master but the script will create a swap file (4GB) only if the machine has less than 12 GB ram as defined in the script itself.

Comment 27

[Rail Aliiev [:rail]]

Comment on attachment 8382250 [details] [diff] [review]
964880.master.patch

Can you use the following file instead:
modules/toplevel/manifests/server/buildmaster/mozilla.pp

just to follow the same pattern as we introduced in modules/toplevel/manifests/slave/build/mock.pp

Additionally, can you also wrap it with | if $::virtual == "xenhvm" | (just being super cautious).

Comment 28

[Massimo Gervasini [:massimo]]

Attachment: 964880.master.patch

thanks, rail!

here's the updated patch

Comment 29

[Massimo Gervasini [:massimo]]

Tested on buildbot-master70. It works.