Merge ~oddbloke/cloud-init/+git/cloud-init:apt-pipelining into cloud-init:master

Proposed by Dan Watkins
Status: Merged
Approved by: Ryan Harper
Approved revision: 22b4dcb425ba3ee426c3f30b1d711770d6f5df80
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~oddbloke/cloud-init/+git/cloud-init:apt-pipelining
Merge into: cloud-init:master
Diff against target: 56 lines (+30/-2)
2 files modified
cloudinit/config/cc_apt_pipelining.py (+2/-2)
cloudinit/config/tests/test_apt_pipelining.py (+28/-0)
Reviewer Review Type Date Requested Status
Ryan Harper Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+363660@code.launchpad.net

Commit message

cc_apt_pipelining: stop disabling pipelining by default

This was introduced due to Ubuntu using S3 mirrors, and S3 having a
buggy pipelining implementation. Those Ubuntu mirrors are no longer in
production and, furthremore, apt has also grown the ability to handle
servers with broken pipelining.

As such, we can stop disabling pipelining, which should result in
improved apt download speeds.

LP: #1794982

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:8e6211bb508c9549cf9ac358aff900ca1cd714c1
https://jenkins.ubuntu.com/server/job/cloud-init-ci/600/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/600/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

On which releases can we set this to 'os' ?

Further, does apt automatically do this or does one have to opt-in via 'os' ?

review: Needs Information
Revision history for this message
Dan Watkins (oddbloke) wrote :

> On which releases can we set this to 'os' ?

We no longer run the broken mirrors for any release, and the apt pipelining fix has landed in xenial and later. (Debian stable has a more recent version of apt than xenial, so I infer it's also landed there.)

> Further, does apt automatically do this or does one have to opt-in via 'os'?

The previous default of False would cause us to write out a configuration file which would definitely disable pipelining. Using one of "os", "none" or "unchanged" as the value will cause us to _not_ write out that configuration file, giving us the default behaviour configured in the underlying OS image.

Revision history for this message
Ryan Harper (raharper) wrote :

That sounds good.

Shall we add a unittest for this one?

Revision history for this message
Dan Watkins (oddbloke) wrote :

On Tue, Feb 26, 2019 at 02:26:50PM -0000, Ryan Harper wrote:
> Shall we add a unittest for this one?

Yep, I'll look at that now.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:7748cbb445be96ee903f2072264b4b2ce62f770f
https://jenkins.ubuntu.com/server/job/cloud-init-ci/601/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/601/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) :
review: Needs Fixing
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:22b4dcb425ba3ee426c3f30b1d711770d6f5df80
https://jenkins.ubuntu.com/server/job/cloud-init-ci/602/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/602/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/cloudinit/config/cc_apt_pipelining.py b/cloudinit/config/cc_apt_pipelining.py
index cdf28cd..459332a 100644
--- a/cloudinit/config/cc_apt_pipelining.py
+++ b/cloudinit/config/cc_apt_pipelining.py
@@ -49,7 +49,7 @@ APT_PIPE_TPL = ("//Written by cloud-init per 'apt_pipelining'\n"
4949
50def handle(_name, cfg, _cloud, log, _args):50def handle(_name, cfg, _cloud, log, _args):
5151
52 apt_pipe_value = util.get_cfg_option_str(cfg, "apt_pipelining", False)52 apt_pipe_value = util.get_cfg_option_str(cfg, "apt_pipelining", 'os')
53 apt_pipe_value_s = str(apt_pipe_value).lower().strip()53 apt_pipe_value_s = str(apt_pipe_value).lower().strip()
5454
55 if apt_pipe_value_s == "false":55 if apt_pipe_value_s == "false":
@@ -59,7 +59,7 @@ def handle(_name, cfg, _cloud, log, _args):
59 elif apt_pipe_value_s in [str(b) for b in range(0, 6)]:59 elif apt_pipe_value_s in [str(b) for b in range(0, 6)]:
60 write_apt_snippet(apt_pipe_value_s, log, DEFAULT_FILE)60 write_apt_snippet(apt_pipe_value_s, log, DEFAULT_FILE)
61 else:61 else:
62 log.warn("Invalid option for apt_pipeling: %s", apt_pipe_value)62 log.warn("Invalid option for apt_pipelining: %s", apt_pipe_value)
6363
6464
65def write_apt_snippet(setting, log, f_name):65def write_apt_snippet(setting, log, f_name):
diff --git a/cloudinit/config/tests/test_apt_pipelining.py b/cloudinit/config/tests/test_apt_pipelining.py
66new file mode 10064466new file mode 100644
index 0000000..2a6bb10
--- /dev/null
+++ b/cloudinit/config/tests/test_apt_pipelining.py
@@ -0,0 +1,28 @@
1# This file is part of cloud-init. See LICENSE file for license information.
2
3"""Tests cc_apt_pipelining handler"""
4
5import cloudinit.config.cc_apt_pipelining as cc_apt_pipelining
6
7from cloudinit.tests.helpers import CiTestCase, mock
8
9
10class TestAptPipelining(CiTestCase):
11
12 @mock.patch('cloudinit.config.cc_apt_pipelining.util.write_file')
13 def test_not_disabled_by_default(self, m_write_file):
14 """ensure that default behaviour is to not disable pipelining"""
15 cc_apt_pipelining.handle('foo', {}, None, mock.MagicMock(), None)
16 self.assertEqual(0, m_write_file.call_count)
17
18 @mock.patch('cloudinit.config.cc_apt_pipelining.util.write_file')
19 def test_false_disables_pipelining(self, m_write_file):
20 """ensure that pipelining can be disabled with correct config"""
21 cc_apt_pipelining.handle(
22 'foo', {'apt_pipelining': 'false'}, None, mock.MagicMock(), None)
23 self.assertEqual(1, m_write_file.call_count)
24 args, _ = m_write_file.call_args
25 self.assertEqual(cc_apt_pipelining.DEFAULT_FILE, args[0])
26 self.assertIn('Pipeline-Depth "0"', args[1])
27
28# vi: ts=4 expandtab

Subscribers

People subscribed via source and target branches