Merge lp:~fginther/charms/trusty/jenkins-remote-slave/cloud-worker-swap-fix into lp:~canonical-ci-engineering/charms/trusty/jenkins-remote-slave/cloud-worker

Proposed by Francis Ginther on 2015-10-06
Status: Merged
Approved by: Francis Ginther on 2015-10-07
Approved revision: 37
Merged at revision: 35
Proposed branch: lp:~fginther/charms/trusty/jenkins-remote-slave/cloud-worker-swap-fix
Merge into: lp:~canonical-ci-engineering/charms/trusty/jenkins-remote-slave/cloud-worker
Diff against target: 34 lines (+14/-11)
1 file modified
hooks/install.d/0200-swap.sh (+14/-11)
To merge this branch: bzr merge lp:~fginther/charms/trusty/jenkins-remote-slave/cloud-worker-swap-fix
Reviewer Review Type Date Requested Status
Para Siva (community) Approve on 2015-10-07
Joe Talbott (community) 2015-10-06 Approve on 2015-10-06
Review via email: mp+273611@code.launchpad.net

Commit message

Add swapfile to /etc/fstab to make it permanent.

Description of the change

Add swapfile to /etc/fstab to make it permanent.

To post a comment you must log in.
Joe Talbott (joetalbott) wrote :

Typo

review: Needs Fixing
36. By Francis Ginther on 2015-10-06

Fix typo in SWAPFILE.

Francis Ginther (fginther) wrote :

Joe, Thanks for catching that typo. I tested the changes by copy-n-pasting just the new bits to a test file and missed the edit for SWAPFILE. I've retested this snippet after fixing the typo.

Joe Talbott (joetalbott) wrote :

LGTM, thanks.

review: Approve
Para Siva (psivaa) wrote :

Thanks a lot Francis for working on this.

I'm not sure if a charm upgrade with this change will fix currently deployed slaves. This is because the code execution would not reach the code where you edit /etc/fstab. Since there is already a swapfile at /.

New charm deployments will be certainly free of this bug.

I'm marking this as 'Needs fixing' but feel free to ignore, since this fix may not be intended to be applied to all the slaves deployed everywhere.

review: Needs Fixing
37. By Francis Ginther on 2015-10-07

Refactor 0200-swap.sh to continue to update fstab file if the swapfile already exists.

Francis Ginther (fginther) wrote :

Siva, thanks for catching that. I've refactored the script to continue if the swapfile is defined and still only create it if it's not there. I've tested this locally from a fresh deployment and an upgrade from a deployment made prior to this change.

Para Siva (psivaa) wrote :

Thanks for addressing the comment. Looks perfect. Thanks again for fixing this issue.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/install.d/0200-swap.sh'
2--- hooks/install.d/0200-swap.sh 2015-07-23 16:06:44 +0000
3+++ hooks/install.d/0200-swap.sh 2015-10-07 13:14:22 +0000
4@@ -1,16 +1,19 @@
5 #!/bin/bash
6-set -ex
7+set -eux
8
9 SWAPFILE=/swapfile
10 SWAP_SIZE=4G
11
12-if [ -f "${SWAPFILE}" ]; then
13- exit 0
14-fi
15-
16-echo "Creating swapfile ${SWAPFILE} of ${SWAP_SIZE}"
17-
18-fallocate -l "${SWAP_SIZE}" "${SWAPFILE}"
19-chmod 600 "${SWAPFILE}"
20-mkswap "${SWAPFILE}"
21-swapon "${SWAPFILE}"
22+if [ ! -f "${SWAPFILE}" ]; then
23+ echo "Creating swapfile ${SWAPFILE} of ${SWAP_SIZE}"
24+
25+ fallocate -l "${SWAP_SIZE}" "${SWAPFILE}"
26+ chmod 600 "${SWAPFILE}"
27+ mkswap "${SWAPFILE}"
28+ swapon "${SWAPFILE}"
29+fi
30+
31+# Make the new swap permanent by adding it to /etc/fstab
32+if ! grep -q ${SWAPFILE} /etc/fstab; then
33+ echo "${SWAPFILE} none swap sw 0 0" >> /etc/fstab
34+fi

Subscribers

People subscribed via source and target branches