Content Cache Charm

Merge ~hloeung/content-cache-charm:sysctl into content-cache-charm:master

  1. Git
  2. lp:~hloeung/content-cache-charm
  3. sysctl
  4. Merge into master
Proposed by Haw Loeung
Status: Merged
Approved by: Haw Loeung
Approved revision: 9930e9ede4f12297cedef3dc240a7fc292d05847
Merged at revision: d024b96dbe824ce4abae7c2b7a24dc8349688e58
Proposed branch: ~hloeung/content-cache-charm:sysctl
Merge into: content-cache-charm:master
Diff against target: 198 lines (+55/-18)
8 files modified
dev/null (+0/-1)
lib/utils.py (+23/-0)
reactive/content_cache.py (+2/-10)
tests/unit/files/sysctl_net_tcp_congestion_control.txt (+1/-0)
tests/unit/files/sysctl_net_tcp_congestion_control_bbr2.txt (+1/-0)
tests/unit/files/sysctl_net_tcp_congestion_control_no_bbr.txt (+1/-0)
tests/unit/test_content_cache.py (+6/-7)
tests/unit/test_utils.py (+21/-0)
Reviewer Review Type Date Requested Status
Joel Sing (community) +1 Approve
Canonical IS Reviewers Pending
Review via email: mp+409339@code.launchpad.net

Commit message

Fix detection and selection of TCP congestion control algorithm

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
Joel Sing (jsing) :
review: Needs Information
Revision history for this message
Haw Loeung (hloeung) :
Revision history for this message
Joel Sing (jsing) wrote :

LGTM, see minor comment inline.

review: Approve (+1)
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision d024b96dbe824ce4abae7c2b7a24dc8349688e58

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/utils.py b/lib/utils.py
2index f06effc..7e481c0 100644
3--- a/lib/utils.py
4+++ b/lib/utils.py
5@@ -218,3 +218,26 @@ def package_version(package):
6 if not version:
7 return None
8 return version
9+
10+
11+_SYSCTL_NET_IPV4_CONGESTION_CONTROL = '/proc/sys/net/ipv4/tcp_congestion_control'
12+
13+
14+def select_tcp_congestion_control(preferred_tcp_cc, tcp_avail_path=_SYSCTL_NET_IPV4_CONGESTION_CONTROL):
15+ if not os.path.exists(tcp_avail_path):
16+ return None
17+
18+ for pref_cc in preferred_tcp_cc:
19+ # We need to try set the TCP congestion control algorithm and
20+ # see if it's successfully set.
21+ cmd = ['sysctl', 'net.ipv4.tcp_congestion_control={}'.format(pref_cc)]
22+ subprocess.check_call(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
23+
24+ # Now check to see if it's set in '/proc/sys/net/ipv4/tcp_congestion_control'
25+ # and use if so.
26+ with open(tcp_avail_path) as f:
27+ ccs = f.read()
28+ if pref_cc in ccs.split():
29+ return pref_cc
30+
31+ return None
32diff --git a/reactive/content_cache.py b/reactive/content_cache.py
33index 93c72ca..0bed6de 100644
34--- a/reactive/content_cache.py
35+++ b/reactive/content_cache.py
36@@ -465,7 +465,6 @@ def configure_nagios():
37
38
39 _SYSCTL_CORE_DEFAULT_QDISC = '/proc/sys/net/core/default_qdisc'
40-_SYSCTL_NET_IPV4_CONGESTION_CONTROL = '/proc/sys/net/ipv4/tcp_available_congestion_control'
41
42
43 @reactive.when_not('content_cache.sysctl.configured')
44@@ -478,15 +477,8 @@ def configure_sysctl():
45 if os.path.exists(_SYSCTL_CORE_DEFAULT_QDISC):
46 context['net_core_default_qdisc'] = 'fq'
47
48- if os.path.exists(_SYSCTL_NET_IPV4_CONGESTION_CONTROL):
49- congestion_control = None
50- with open(_SYSCTL_NET_IPV4_CONGESTION_CONTROL) as f:
51- ccs = f.read()
52- if 'bbr2' in ccs.split():
53- congestion_control = 'bbr2'
54- elif 'bbr' in ccs.split():
55- congestion_control = 'bbr'
56- context['net_ipv4_tcp_congestion_control'] = congestion_control
57+ preferred_tcp_cc = ['bbr2', 'bbr']
58+ context['net_ipv4_tcp_congestion_control'] = utils.select_tcp_congestion_control(preferred_tcp_cc)
59
60 # Set or lower tcp_notsent_lowat to optimise HTTP/2 prioritisation.
61 # https://blog.cloudflare.com/http-2-prioritization-with-nginx/
62diff --git a/tests/unit/files/sysctl_net_tcp_available_congestion_control.txt b/tests/unit/files/sysctl_net_tcp_available_congestion_control.txt
63deleted file mode 100644
64index 371ac85..0000000
65--- a/tests/unit/files/sysctl_net_tcp_available_congestion_control.txt
66+++ /dev/null
67@@ -1 +0,0 @@
68-reno cubic bbr
69diff --git a/tests/unit/files/sysctl_net_tcp_available_congestion_control_bbr2.txt b/tests/unit/files/sysctl_net_tcp_available_congestion_control_bbr2.txt
70deleted file mode 100644
71index a810c9d..0000000
72--- a/tests/unit/files/sysctl_net_tcp_available_congestion_control_bbr2.txt
73+++ /dev/null
74@@ -1 +0,0 @@
75-reno bbr bbr2 cubic dctcp
76diff --git a/tests/unit/files/sysctl_net_tcp_available_congestion_control_no_bbr.txt b/tests/unit/files/sysctl_net_tcp_available_congestion_control_no_bbr.txt
77deleted file mode 100644
78index f5a7b00..0000000
79--- a/tests/unit/files/sysctl_net_tcp_available_congestion_control_no_bbr.txt
80+++ /dev/null
81@@ -1 +0,0 @@
82-cubic reno
83diff --git a/tests/unit/files/sysctl_net_tcp_congestion_control.txt b/tests/unit/files/sysctl_net_tcp_congestion_control.txt
84new file mode 100644
85index 0000000..be094f2
86--- /dev/null
87+++ b/tests/unit/files/sysctl_net_tcp_congestion_control.txt
88@@ -0,0 +1 @@
89+bbr
90diff --git a/tests/unit/files/sysctl_net_tcp_congestion_control_bbr2.txt b/tests/unit/files/sysctl_net_tcp_congestion_control_bbr2.txt
91new file mode 100644
92index 0000000..4a67ec6
93--- /dev/null
94+++ b/tests/unit/files/sysctl_net_tcp_congestion_control_bbr2.txt
95@@ -0,0 +1 @@
96+bbr2
97diff --git a/tests/unit/files/sysctl_net_tcp_congestion_control_no_bbr.txt b/tests/unit/files/sysctl_net_tcp_congestion_control_no_bbr.txt
98new file mode 100644
99index 0000000..2e551e1
100--- /dev/null
101+++ b/tests/unit/files/sysctl_net_tcp_congestion_control_no_bbr.txt
102@@ -0,0 +1 @@
103+cubic
104diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
105index 4d2a8e8..83917f6 100644
106--- a/tests/unit/test_content_cache.py
107+++ b/tests/unit/test_content_cache.py
108@@ -1154,7 +1154,8 @@ site1.local:
109
110 @mock.patch('charms.reactive.set_flag')
111 @mock.patch('subprocess.call')
112- def test_configure_sysctl(self, call, set_flag):
113+ @mock.patch('lib.utils.select_tcp_congestion_control')
114+ def test_configure_sysctl(self, tcp_cc, call, set_flag):
115 sysctl_conf_path = os.path.join(self.tmpdir, '90-content-cache.conf')
116
117 with mock.patch('reactive.content_cache.SYSCTL_CONF_PATH', sysctl_conf_path):
118@@ -1172,8 +1173,8 @@ site1.local:
119 'reactive.content_cache',
120 SYSCTL_CONF_PATH=sysctl_conf_path,
121 _SYSCTL_CORE_DEFAULT_QDISC=sysctl_conf_path,
122- _SYSCTL_NET_IPV4_CONGESTION_CONTROL='some-file-does-not-exist',
123 ):
124+ tcp_cc.return_value = None
125 content_cache.configure_sysctl()
126 # Check contents
127 with open('tests/unit/files/sysctl_core_default_qdisc.conf', 'r') as f:
128@@ -1185,7 +1186,6 @@ site1.local:
129 'reactive.content_cache',
130 SYSCTL_CONF_PATH=sysctl_conf_path,
131 _SYSCTL_CORE_DEFAULT_QDISC='some-file-does-not-exist',
132- _SYSCTL_NET_IPV4_CONGESTION_CONTROL='some-file-does-not-exist',
133 ):
134 content_cache.configure_sysctl()
135 # Check contents
136@@ -1199,8 +1199,8 @@ site1.local:
137 with mock.patch.multiple(
138 'reactive.content_cache',
139 SYSCTL_CONF_PATH=sysctl_conf_path,
140- _SYSCTL_NET_IPV4_CONGESTION_CONTROL='tests/unit/files/sysctl_net_tcp_available_congestion_control.txt',
141 ):
142+ tcp_cc.return_value = 'bbr'
143 content_cache.configure_sysctl()
144 # Check contents
145 with open('tests/unit/files/sysctl_net_tcp_congestion_control.conf', 'r') as f:
146@@ -1211,8 +1211,8 @@ site1.local:
147 with mock.patch.multiple(
148 'reactive.content_cache',
149 SYSCTL_CONF_PATH=sysctl_conf_path,
150- _SYSCTL_NET_IPV4_CONGESTION_CONTROL='tests/unit/files/sysctl_net_tcp_available_congestion_control_bbr2.txt',
151 ):
152+ tcp_cc.return_value = 'bbr2'
153 content_cache.configure_sysctl()
154 # Check contents
155 with open('tests/unit/files/sysctl_net_tcp_congestion_control_bbr2.conf', 'r') as f:
156@@ -1220,12 +1220,11 @@ site1.local:
157 with open(sysctl_conf_path, 'r') as f:
158 got = f.read()
159 self.assertEqual(got, want)
160- sysctl_file = 'tests/unit/files/sysctl_net_tcp_available_congestion_control_no_bbr.txt'
161 with mock.patch.multiple(
162 'reactive.content_cache',
163 SYSCTL_CONF_PATH=sysctl_conf_path,
164- _SYSCTL_NET_IPV4_CONGESTION_CONTROL=sysctl_file,
165 ):
166+ tcp_cc.return_value = None
167 content_cache.configure_sysctl()
168 # Check contents
169 with open('tests/unit/files/sysctl_net_tcp_congestion_control_no_bbr.conf', 'r') as f:
170diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
171index b2d7348..268e3f4 100644
172--- a/tests/unit/test_utils.py
173+++ b/tests/unit/test_utils.py
174@@ -208,3 +208,24 @@ class TestLibUtils(unittest.TestCase):
175 self.assertEqual(utils.package_version('haproxy'), None)
176
177 self.assertEqual(utils.package_version('some-package-doesnt-exist'), None)
178+
179+ def test_select_tcp_congestion_control(self):
180+ sysctl_tcp_cc_path = 'some-file-does-not-exist'
181+ preferred = ['bbr']
182+ want = None
183+ self.assertEqual(utils.select_tcp_congestion_control(preferred, sysctl_tcp_cc_path), want)
184+
185+ sysctl_tcp_cc_path = 'tests/unit/files/sysctl_net_tcp_congestion_control.txt'
186+ preferred = ['bbr2', 'bbr']
187+ want = 'bbr'
188+ self.assertEqual(utils.select_tcp_congestion_control(preferred, sysctl_tcp_cc_path), want)
189+
190+ sysctl_tcp_cc_path = 'tests/unit/files/sysctl_net_tcp_congestion_control_bbr2.txt'
191+ preferred = ['bbr2', 'bbr']
192+ want = 'bbr2'
193+ self.assertEqual(utils.select_tcp_congestion_control(preferred, sysctl_tcp_cc_path), want)
194+
195+ sysctl_tcp_cc_path = 'tests/unit/files/sysctl_net_tcp_congestion_control_no_bbr.txt'
196+ preferred = ['bbr2', 'bbr']
197+ want = None
198+ self.assertEqual(utils.select_tcp_congestion_control(preferred, sysctl_tcp_cc_path), want)

Subscribers

People subscribed via source and target branches