Merge ~vultaire/charm-sysconfig:lp1841809 into charm-sysconfig:master
- Git
- lp:~vultaire/charm-sysconfig
- lp1841809
- Merge into master
Status: | Merged |
---|---|
Approved by: | Jeremy Lounder |
Approved revision: | d732035d8aff8c3a74d715fa9e4f88a435f0422c |
Merged at revision: | cb5240074d70796e3c06b257756b00013a0ffda4 |
Proposed branch: | ~vultaire/charm-sysconfig:lp1841809 |
Merge into: | charm-sysconfig:master |
Diff against target: |
473 lines (+171/-83) 6 files modified
src/config.yaml (+6/-0) src/lib/lib_sysconfig.py (+48/-18) src/reactive/sysconfig.py (+9/-1) src/templates/etc.systemd.resolved.conf.j2 (+27/-0) src/tests/functional/test_deploy.py (+40/-30) src/tests/unit/test_lib.py (+41/-34) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeremy Lounder (community) | Approve | ||
Alvaro Uria (community) | Approve | ||
Review via email: mp+378630@code.launchpad.net |
Commit message
Adding resolved-cache-mode config option
Description of the change
* Added the feature
* Added tests for the feature
* Removed 'cosmic' option from test_deploy.py since it adds significant test overhead time (10 minutes on certain tests), due to there not being a cosmic charm available to deploy with.
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Alvaro Uria (aluria) wrote : | # |
* typo: Default value on config.yaml
* nitpick: code style
* Behavior around updating the resolved.conf file (systemd-resolved service should be restarted instead of notifying that the unit needs a reboot)
Paul Goins (vultaire) wrote : | # |
Noting my responses. Will make changes.
Paul Goins (vultaire) : | # |
Paul Goins (vultaire) wrote : | # |
@aluria: Care to give this another look? I think I've addressed everything.
Alvaro Uria (aluria) wrote : | # |
Hey Paul. I've added a comment inline, but I will repeat it here.
* checksum verification can be achieved via any_file_changed [1], helper from the reactive framework, A list of file(s) needs to be passed.
* "render" already checks if new content is going to be written, or it will skip the write
* As a suggestion of a diff approach, set_flag(
And think the above will make more sense in the comment inline.
Other than this, the change (and tests) look great.
1. https:/
Paul Goins (vultaire) wrote : | # |
Most comments addressed. Did not use set_flag(...) since cpufrequtils also handles a service restart in the same way (in the lib module rather than via a flag-based reactive hook); if we change it for resolved, I think we should change it for cpufrequtils as well - and that starts to work its way out of scope in my opinion.
Alvaro Uria (aluria) wrote : | # |
Hey Paul, Thank you! I've added a minor suggestion for readability purpose, but I won't block it for merging :)
Alvaro Uria (aluria) wrote : | # |
Hey Paul. I suggested the use of a list for [ONE_FILE], but failed to check the change. Good catch. I also failed to mention that "import hashlib" may not be needed anymore. Other than that, all looks good to me.
Jeremy Lounder (jldev) : | # |
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Change successfully merged at revision cb5240074d70796
Preview Diff
1 | diff --git a/src/config.yaml b/src/config.yaml |
2 | index bd60238..4b93f3c 100644 |
3 | --- a/src/config.yaml |
4 | +++ b/src/config.yaml |
5 | @@ -122,3 +122,9 @@ options: |
6 | Recommended option when Bios control power is set to the OS. |
7 | - 'performance' |
8 | - 'powersave' |
9 | + resolved-cache-mode: |
10 | + type: string |
11 | + default: "" |
12 | + description: | |
13 | + Sets the "Cache" configuration value in /etc/systemd/resolved.conf. Valid |
14 | + values are '' (system default), 'yes', 'no', and 'no-negative'. |
15 | diff --git a/src/lib/lib_sysconfig.py b/src/lib/lib_sysconfig.py |
16 | index 51bf049..73e1311 100644 |
17 | --- a/src/lib/lib_sysconfig.py |
18 | +++ b/src/lib/lib_sysconfig.py |
19 | @@ -2,7 +2,6 @@ |
20 | |
21 | Manage grub, systemd, coufrequtils and kernel version configuration. |
22 | """ |
23 | - |
24 | import os |
25 | import subprocess |
26 | from datetime import datetime, timedelta, timezone |
27 | @@ -11,15 +10,18 @@ from charmhelpers.contrib.openstack.utils import config_flags_parser |
28 | from charmhelpers.core import hookenv, host, unitdata |
29 | from charmhelpers.core.templating import render |
30 | from charmhelpers.fetch import apt_install, apt_update |
31 | +from charms.reactive.helpers import any_file_changed |
32 | |
33 | GRUB_DEFAULT = 'Advanced options for Ubuntu>Ubuntu, with Linux {}' |
34 | CPUFREQUTILS_TMPL = 'cpufrequtils.j2' |
35 | GRUB_CONF_TMPL = 'grub.j2' |
36 | SYSTEMD_SYSTEM_TMPL = 'etc.systemd.system.conf.j2' |
37 | +SYSTEMD_RESOLVED_TMPL = 'etc.systemd.resolved.conf.j2' |
38 | |
39 | CPUFREQUTILS = '/etc/default/cpufrequtils' |
40 | GRUB_CONF = '/etc/default/grub.d/90-sysconfig.cfg' |
41 | SYSTEMD_SYSTEM = '/etc/systemd/system.conf' |
42 | +SYSTEMD_RESOLVED = '/etc/systemd/resolved.conf' |
43 | KERNEL = 'kernel' |
44 | |
45 | |
46 | @@ -155,25 +157,30 @@ class SysConfigHelper: |
47 | |
48 | @property |
49 | def systemd_config_flags(self): |
50 | - """Return grub-config-flags config option.""" |
51 | + """Return systemd-config-flags config option.""" |
52 | return parse_config_flags(self.charm_config['systemd-config-flags']) |
53 | |
54 | @property |
55 | def kernel_version(self): |
56 | - """Return grub-config-flags config option.""" |
57 | + """Return kernel-version config option.""" |
58 | return self.charm_config['kernel-version'] |
59 | |
60 | @property |
61 | def update_grub(self): |
62 | - """Return grub-config-flags config option.""" |
63 | + """Return update-grub config option.""" |
64 | return self.charm_config['update-grub'] |
65 | |
66 | @property |
67 | def governor(self): |
68 | - """Return grub-config-flags config option.""" |
69 | + """Return governor config option.""" |
70 | return self.charm_config['governor'] |
71 | |
72 | @property |
73 | + def resolved_cache_mode(self): |
74 | + """Return resolved-cache-mode config option.""" |
75 | + return self.charm_config['resolved-cache-mode'] |
76 | + |
77 | + @property |
78 | def config_flags(self): |
79 | """Return parsed config-flags into dict. |
80 | |
81 | @@ -187,9 +194,14 @@ class SysConfigHelper: |
82 | |
83 | def _render_boot_resource(self, source, target, context): |
84 | """Render the template and set the resource as changed.""" |
85 | - render(source=source, templates_dir='templates', target=target, context=context) |
86 | + self._render_resource(source, target, context) |
87 | self.boot_resources.set_resource(target) |
88 | |
89 | + @staticmethod |
90 | + def _render_resource(source, target, context): |
91 | + """Render the template.""" |
92 | + render(source=source, templates_dir='templates', target=target, context=context) |
93 | + |
94 | def _is_kernel_already_running(self): |
95 | """Check if the kernel version required by charm config is equal to kernel running.""" |
96 | configured = self.kernel_version |
97 | @@ -208,18 +220,15 @@ class SysConfigHelper: |
98 | """Validate config parameters.""" |
99 | valid = True |
100 | |
101 | - if self.reservation not in ['off', 'isolcpus', 'affinity']: |
102 | - hookenv.log('reservation not valid. Possible values: ["off", "isolcpus", "affinity"]', hookenv.DEBUG) |
103 | - valid = False |
104 | - |
105 | - if self.raid_autodetection not in ['', 'noautodetect', 'partitionable']: |
106 | - hookenv.log('raid-autodetection not valid. ' |
107 | - 'Possible values: ["off", "noautodetect", "partitionable"]', hookenv.DEBUG) |
108 | - valid = False |
109 | - |
110 | - if self.governor not in ['', 'powersave', 'performance']: |
111 | - hookenv.log('governor not valid. Possible values: ["", "powersave", "performance"]', hookenv.DEBUG) |
112 | - valid = False |
113 | + for config_key, value, valid_values in ( |
114 | + ('reservation', self.reservation, ['off', 'isolcpus', 'affinity']), |
115 | + ('raid-autodetection', self.raid_autodetection, ['', 'noautodetect', 'partitionable']), |
116 | + ('governor', self.governor, ['', 'powersave', 'performance']), |
117 | + ('resolved-cache-mode', self.resolved_cache_mode, ['', 'yes', 'no', 'no-negative']), |
118 | + ): |
119 | + if value not in valid_values: |
120 | + hookenv.log('{} not valid. Possible values: {}'.format(config_key, repr(valid_values)), hookenv.DEBUG) |
121 | + valid = False |
122 | |
123 | return valid |
124 | |
125 | @@ -272,6 +281,14 @@ class SysConfigHelper: |
126 | self._render_boot_resource(SYSTEMD_SYSTEM_TMPL, SYSTEMD_SYSTEM, context) |
127 | hookenv.log('systemd configuration updated') |
128 | |
129 | + def update_systemd_resolved(self): |
130 | + """Update /etc/systemd/resolved.conf according to charm configuration.""" |
131 | + context = {} |
132 | + if self.resolved_cache_mode: |
133 | + context['cache'] = self.resolved_cache_mode |
134 | + self._update_systemd_resolved(context) |
135 | + hookenv.log('systemd-resolved configuration updated') |
136 | + |
137 | def install_configured_kernel(self): |
138 | """Install kernel as given by the kernel-version config option. |
139 | |
140 | @@ -341,6 +358,19 @@ class SysConfigHelper: |
141 | hookenv.DEBUG |
142 | ) |
143 | |
144 | + def remove_resolved_configuration(self): |
145 | + """Remove systemd's resolved configuration. |
146 | + |
147 | + Will render resolved config with defaults. |
148 | + """ |
149 | + self._update_systemd_resolved({}) |
150 | + hookenv.log('deleted resolved configuration at '.format(SYSTEMD_RESOLVED), hookenv.DEBUG) |
151 | + |
152 | + def _update_systemd_resolved(self, context): |
153 | + self._render_resource(SYSTEMD_RESOLVED_TMPL, SYSTEMD_RESOLVED, context) |
154 | + if any_file_changed([SYSTEMD_RESOLVED]): |
155 | + host.service_restart('systemd-resolved') |
156 | + |
157 | def remove_cpufreq_configuration(self): |
158 | """Remove cpufrequtils configuration. |
159 | |
160 | diff --git a/src/reactive/sysconfig.py b/src/reactive/sysconfig.py |
161 | index eba0916..6c70fba 100644 |
162 | --- a/src/reactive/sysconfig.py |
163 | +++ b/src/reactive/sysconfig.py |
164 | @@ -28,7 +28,7 @@ from charms.reactive import ( |
165 | when_not, |
166 | ) |
167 | |
168 | -from lib_sysconfig import CPUFREQUTILS, GRUB_CONF, KERNEL, SYSTEMD_SYSTEM, SysConfigHelper |
169 | +from lib_sysconfig import CPUFREQUTILS, GRUB_CONF, KERNEL, SYSTEMD_SYSTEM, SYSTEMD_RESOLVED, SysConfigHelper |
170 | |
171 | |
172 | @when_none('sysconfig.installed', 'sysconfig.unsupported') |
173 | @@ -54,6 +54,7 @@ def install_sysconfig(): |
174 | syshelper.update_cpufreq() |
175 | syshelper.update_grub_file() |
176 | syshelper.update_systemd_system_file() |
177 | + syshelper.update_systemd_resolved() |
178 | set_flag('sysconfig.installed') |
179 | update_status() |
180 | |
181 | @@ -103,6 +104,12 @@ def config_changed(): |
182 | )) or helpers.any_file_changed([SYSTEMD_SYSTEM]): |
183 | syshelper.update_systemd_system_file() |
184 | |
185 | + # systemd resolved |
186 | + if any(syshelper.charm_config.changed(flag) for flag in ( |
187 | + 'resolved-cache-mode', |
188 | + )) or helpers.any_file_changed([SYSTEMD_RESOLVED]): |
189 | + syshelper.update_systemd_resolved() |
190 | + |
191 | update_status() |
192 | |
193 | |
194 | @@ -142,5 +149,6 @@ def remove_configuration(): |
195 | syshelper.remove_cpufreq_configuration() |
196 | syshelper.remove_grub_configuration() |
197 | syshelper.remove_systemd_configuration() |
198 | + syshelper.remove_resolved_configuration() |
199 | clear_flag('sysconfig.installed') |
200 | clear_flag('sysconfig.unsupported') |
201 | diff --git a/src/templates/etc.systemd.resolved.conf.j2 b/src/templates/etc.systemd.resolved.conf.j2 |
202 | new file mode 100644 |
203 | index 0000000..ab8832b |
204 | --- /dev/null |
205 | +++ b/src/templates/etc.systemd.resolved.conf.j2 |
206 | @@ -0,0 +1,27 @@ |
207 | +# This file is part of systemd. |
208 | +# |
209 | +# systemd is free software; you can redistribute it and/or modify it |
210 | +# under the terms of the GNU Lesser General Public License as published by |
211 | +# the Free Software Foundation; either version 2.1 of the License, or |
212 | +# (at your option) any later version. |
213 | +# |
214 | +# Entries in this file show the compile time defaults. |
215 | +# You can change settings by editing this file. |
216 | +# Defaults can be restored by simply deleting this file. |
217 | +# |
218 | +# See resolved.conf(5) for details |
219 | + |
220 | +[Resolve] |
221 | +#DNS= |
222 | +#FallbackDNS= |
223 | +#Domains= |
224 | +#LLMNR=no |
225 | +#MulticastDNS=no |
226 | +#DNSSEC=no |
227 | +#DNSOverTLS=no |
228 | +#Cache=yes |
229 | +{% if cache -%} |
230 | +Cache={{ cache }} |
231 | +{% endif -%} |
232 | +#DNSStubListener=yes |
233 | +#ReadEtcHosts=yes |
234 | diff --git a/src/tests/functional/test_deploy.py b/src/tests/functional/test_deploy.py |
235 | index 968288b..2900e06 100644 |
236 | --- a/src/tests/functional/test_deploy.py |
237 | +++ b/src/tests/functional/test_deploy.py |
238 | @@ -2,9 +2,11 @@ |
239 | |
240 | import asyncio |
241 | import os |
242 | +import re |
243 | import subprocess |
244 | |
245 | import pytest |
246 | +import websockets |
247 | |
248 | |
249 | # Treat all tests as coroutines |
250 | @@ -14,12 +16,12 @@ charm_build_dir = os.getenv('CHARM_BUILD_DIR', '..').rstrip('/') |
251 | |
252 | series = ['xenial', |
253 | 'bionic', |
254 | - pytest.param('cosmic', marks=pytest.mark.xfail(reason='canary')), |
255 | ] |
256 | |
257 | sources = [('local', '{}/builds/sysconfig'.format(charm_build_dir))] |
258 | |
259 | TIMEOUT = 600 |
260 | +MODEL_ACTIVE_TIMEOUT = 10 |
261 | GRUB_DEFAULT = 'Advanced options for Ubuntu>Ubuntu, with Linux {}' |
262 | PRINCIPAL_APP_NAME = 'ubuntu-{}' |
263 | |
264 | @@ -269,14 +271,19 @@ async def test_wrong_reservation(app, model): |
265 | ) |
266 | |
267 | |
268 | -async def test_wrong_raid_autodetection(app, model): |
269 | - """Tests wrong raid-autodetection value is used. |
270 | +@pytest.mark.parametrize('key,bad_value,good_value', [ |
271 | + ('raid-autodetection', 'changeme', ''), |
272 | + ('governor', 'changeme', ''), |
273 | + ('resolved-cache-mode', 'changeme', ''), |
274 | +]) |
275 | +async def test_invalid_configuration_parameters(app, model, key, bad_value, good_value): |
276 | + """Tests wrong config value is used. |
277 | |
278 | Expect application is blocked until correct value is set. |
279 | """ |
280 | await app.set_config( |
281 | { |
282 | - 'raid-autodetection': 'changeme' |
283 | + key: bad_value |
284 | } |
285 | ) |
286 | await model.block_until( |
287 | @@ -289,7 +296,7 @@ async def test_wrong_raid_autodetection(app, model): |
288 | |
289 | await app.set_config( |
290 | { |
291 | - 'raid-autodetection': '' |
292 | + key: good_value |
293 | } |
294 | ) |
295 | await model.block_until( |
296 | @@ -298,33 +305,32 @@ async def test_wrong_raid_autodetection(app, model): |
297 | ) |
298 | |
299 | |
300 | -async def test_wrong_governor(app, model): |
301 | - """Tests wrong governor value is used. |
302 | +@pytest.mark.parametrize('cache_setting', [ |
303 | + 'yes', |
304 | + 'no', |
305 | + 'no-negative', |
306 | +]) |
307 | +async def test_set_resolved_cache(app, model, jujutools, cache_setting): |
308 | + """Tests resolved cache settings.""" |
309 | + def is_model_settled(): |
310 | + return app.units[0].workload_status == 'active' and app.units[0].agent_status == 'idle' |
311 | |
312 | - Expect application is blocked until correct value is set. |
313 | - """ |
314 | - await app.set_config( |
315 | - { |
316 | - 'governor': 'changeme' |
317 | - } |
318 | - ) |
319 | - await model.block_until( |
320 | - lambda: app.status == 'blocked', |
321 | - timeout=TIMEOUT |
322 | - ) |
323 | - assert app.status == 'blocked' |
324 | - unit = app.units[0] |
325 | - assert 'configuration parameters not valid.' in unit.workload_status_message |
326 | + await model.block_until(is_model_settled, timeout=TIMEOUT) |
327 | |
328 | - await app.set_config( |
329 | - { |
330 | - 'governor': '' |
331 | - } |
332 | - ) |
333 | - await model.block_until( |
334 | - lambda: app.status == 'active', |
335 | - timeout=TIMEOUT |
336 | - ) |
337 | + await app.set_config({'resolved-cache-mode': cache_setting}) |
338 | + # NOTE: app.set_config() doesn't seem to wait for the model to go to a |
339 | + # non-active/idle state. |
340 | + try: |
341 | + await model.block_until(lambda: not is_model_settled(), timeout=MODEL_ACTIVE_TIMEOUT) |
342 | + except websockets.ConnectionClosed: |
343 | + # It's possible (although unlikely) that we missed the charm transitioning from |
344 | + # idle to active and back. |
345 | + pass |
346 | + |
347 | + await model.block_until(is_model_settled, timeout=TIMEOUT) |
348 | + |
349 | + resolved_conf_content = await jujutools.file_contents('/etc/systemd/resolved.conf', app.units[0]) |
350 | + assert re.search('^Cache={}$'.format(cache_setting), resolved_conf_content, re.MULTILINE) |
351 | |
352 | |
353 | async def test_uninstall(app, model, jujutools, series): |
354 | @@ -365,6 +371,10 @@ async def test_uninstall(app, model, jujutools, series): |
355 | systemd_content = await jujutools.file_contents(systemd_path, unit) |
356 | assert 'CPUAffinity=1,2,3,4' not in systemd_content |
357 | |
358 | + resolved_path = '/etc/systemd/resolved.conf' |
359 | + resolved_content = await jujutools.file_contents(resolved_path, unit) |
360 | + assert not re.search('^Cache=', resolved_content, re.MULTILINE) |
361 | + |
362 | cpufreq_path = '/etc/default/cpufrequtils' |
363 | cpufreq_content = await jujutools.file_contents(cpufreq_path, unit) |
364 | assert 'GOVERNOR' not in cpufreq_content |
365 | diff --git a/src/tests/unit/test_lib.py b/src/tests/unit/test_lib.py |
366 | index b7ac651..be25eb4 100644 |
367 | --- a/src/tests/unit/test_lib.py |
368 | +++ b/src/tests/unit/test_lib.py |
369 | @@ -1,13 +1,14 @@ |
370 | #!/usr/bin/python3 |
371 | """Unit tests for SysConfigHelper and BootResourceState classes.""" |
372 | - |
373 | - |
374 | +import os |
375 | import subprocess |
376 | +import tempfile |
377 | from datetime import datetime, timezone |
378 | |
379 | import lib_sysconfig |
380 | |
381 | import mock |
382 | +import pytest |
383 | |
384 | |
385 | class TestBootResourceState: |
386 | @@ -479,45 +480,22 @@ class TestLib: |
387 | ) |
388 | restart.assert_called() |
389 | |
390 | + @pytest.mark.parametrize("invalid_config_key", [ |
391 | + "reservation", "raid-autodetection", "governor", "resolved-cache-mode"]) |
392 | @mock.patch("lib_sysconfig.hookenv.config") |
393 | - def test_wrong_reservation(self, config): |
394 | - """Test wrong reservation value. |
395 | - |
396 | - Expect that is_config_valid() return false |
397 | - """ |
398 | - config.return_value = { |
399 | - "reservation": "wrong", |
400 | - "raid-autodetection": "", |
401 | - "governor": "" |
402 | - } |
403 | - sysh = lib_sysconfig.SysConfigHelper() |
404 | - assert not sysh.is_config_valid() |
405 | - |
406 | - @mock.patch("lib_sysconfig.hookenv.config") |
407 | - def test_wrong_raid(self, config): |
408 | - """Test wrong raid autodetection value. |
409 | - |
410 | - Expect that is_config_valid() return false |
411 | - """ |
412 | - config.return_value = { |
413 | - "reservation": "off", |
414 | - "raid-autodetection": "wrong", |
415 | - "governor": "" |
416 | - } |
417 | - sysh = lib_sysconfig.SysConfigHelper() |
418 | - assert not sysh.is_config_valid() |
419 | - |
420 | - @mock.patch("lib_sysconfig.hookenv.config") |
421 | - def test_wrong_governor(self, config): |
422 | - """Test wrong governor value. |
423 | + def test_wrong_config(self, config, invalid_config_key): |
424 | + """Test wrong configuration value. |
425 | |
426 | Expect that is_config_valid() return false |
427 | """ |
428 | - config.return_value = { |
429 | + return_value = { |
430 | "reservation": "off", |
431 | "raid-autodetection": "", |
432 | - "governor": "wrong" |
433 | + "governor": "", |
434 | + "resolved-cache-mode": "", |
435 | + invalid_config_key: 'wrong', # Will override the selected key with an invalid value |
436 | } |
437 | + config.return_value = return_value |
438 | sysh = lib_sysconfig.SysConfigHelper() |
439 | assert not sysh.is_config_valid() |
440 | |
441 | @@ -530,3 +508,32 @@ class TestLib: |
442 | sysh = lib_sysconfig.SysConfigHelper() |
443 | |
444 | assert sysh.enable_container |
445 | + |
446 | + @mock.patch("lib_sysconfig.any_file_changed") |
447 | + @mock.patch("lib_sysconfig.hookenv.config") |
448 | + @mock.patch("lib_sysconfig.host.service_restart") |
449 | + @mock.patch("lib_sysconfig.render") |
450 | + def test_update_resolved_file_unchanged(self, render, restart, config, file_changed): |
451 | + file_changed.return_value = True |
452 | + self._test_update_resolved_common(render, config) |
453 | + restart.assert_not_called() |
454 | + |
455 | + @mock.patch("lib_sysconfig.any_file_changed") |
456 | + @mock.patch("lib_sysconfig.hookenv.config") |
457 | + @mock.patch("lib_sysconfig.host.service_restart") |
458 | + @mock.patch("lib_sysconfig.render") |
459 | + def test_update_resolved_file_changed(self, render, restart, config, file_changed): |
460 | + file_changed.return_value = True |
461 | + self._test_update_resolved_common(render, config) |
462 | + restart.assert_called() |
463 | + |
464 | + def _test_update_resolved_common(self, render, config): |
465 | + config.return_value = {"resolved-cache-mode": "no-negative"} |
466 | + sysh = lib_sysconfig.SysConfigHelper() |
467 | + sysh.update_systemd_resolved() |
468 | + render.assert_called_with( |
469 | + source=lib_sysconfig.SYSTEMD_RESOLVED_TMPL, |
470 | + target=lib_sysconfig.SYSTEMD_RESOLVED, |
471 | + templates_dir="templates", |
472 | + context={"cache": "no-negative"}, |
473 | + ) |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.