Deb822 default sources list d #4437

blackboxsw · 2023-09-15T01:19:13Z

This PR is separated into distinct commits which could stand on their own for readability

Proposed Commit Message

    apt_configure: add deb822 support for default sources file

    When features.APT_DEB822_SOURCE_LIST_FILE is set, cloud-init
    will generate an /etc/apt/sources.list.d/<distro>.source file
    in Deb822 format instead of /etc/apt/sources.list.
    
    Introduce a package Recommends dependency on python3-apt.
    If apt_pkg module is available use it to read APT config settings for
    Dir::Etc, Dir::Etc::sourceparts and Dir::Etc::sourcelist.
    
    Add deb822 default templates per distro:
    - templates/sources.list.debian.deb822.tmpl
    - templates/sources.list.ubuntu.deb822.tmpl
    
    When features.APT_DEB822_SOURCE_LIST_FILE is set, cloud-init will
    - read the distro-specific deb822 formatted template file
    - When apt_pkg module is present read the appropriate APT config
      "Dir::Etc::sourceparts" ortherwise use DEFAULT_APT_CFG
    - render /etc/apt/sources.list.d/<distro>.source file from the tmpl
    
    Add function is_deb822_sources_format to check for expected deb822
    keys in rendered APT rendered content before writing deb822 source
    format files. If APT source content is not deb822 format, fallback to
    writing /etc/apt/sources.list.

    The feature flag and format check retains original behavior on stable
    releases where custom cloud-config user-data could contain non-deb822
    APT source content for the keys:
      custom_apt_sources and apt_sources

Additional Context

We would like to target this change for mantic.
When this PR lands, we will also add a Recommends: python3-apt to debian/control files.

Test Steps

./tools/run-container --package ubuntu/mantic
CLOUD_INIT_KEEP_INSTANCE=1 CLOUD_INIT_OS_IMAGE=mantic CLOUD_INIT_CLOUD_INIT_SOURCE=cloud-init_23.3-*~bddeb_all.deb tox -e integration-tests tests/integration_tests/modules/test_apt.py

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

blackboxsw · 2023-09-15T03:00:10Z

tests/unittests/config/test_apt_configure_sources_list_v1.py

@@ -14,15 +14,6 @@
 EXAMPLE_TMPL = """\
 ## template:jinja
 ## Note, this file is written by cloud-init on first boot of an instance
-## modifications made here will not survive a re-bundle.


Just reducing vertical space dedicated to unnecessary extra boilerplate test strings

blackboxsw · 2023-09-15T03:03:37Z

templates/sources.list.ubuntu.deb822.tmpl

+Types: deb
+URIs: {{mirror}}
+Suites: {{codename}} {{codename}}-updates {{codename}}-backports


Please pay extra attention to the Suites and Types here and compare to the active default types/suites we
have in the non-deb822 format templates in templates/sources.list.(ubuntu|debian).tmpl to double check we didn't unintentionally activate or disable some type or suite.

tests/unittests/config/test_apt_source_v3.py

blackboxsw · 2023-09-15T15:19:40Z

CC: @enr0n just FYI as I know you recently added changes to ubuntu-release-ugprader to support upgrade paths and migration to deb822 format. You may have some ideas on shared design we should look at long term within cloud-init to better support U-R-U with shared libraries/functions etc. And please confirm that cloud-init's writing of /etc/apt/sources.list.d/ubuntu.sources doesn't collide with any upgrade migration logic in place. Given that cloud-init could run in systems without python3-apt installed, any shared libs/functions we rely would need to rely on working in the potential absence of aptsources/apt_pkg python modules.

TheRealFalcon

First pass review. Overall looks really good. I left inline comments. I still need to grok the disable code a little deeper and look at tests.

cloudinit/config/cc_apt_configure.py

tests/unittests/helpers.py

cloudinit/subp.py

cloudinit/config/cc_apt_configure.py

enr0n · 2023-09-18T13:36:32Z

CC: @enr0n just FYI as I know you recently added changes to ubuntu-release-ugprader to support upgrade paths and migration to deb822 format. You may have some ideas on shared design we should look at long term within cloud-init to better support U-R-U with shared libraries/functions etc. And please confirm that cloud-init's writing of /etc/apt/sources.list.d/ubuntu.sources doesn't collide with any upgrade migration logic in place. Given that cloud-init could run in systems without python3-apt installed, any shared libs/functions we rely would need to rely on working in the potential absence of aptsources/apt_pkg python modules.

I have not had time to read the code yet, but FYSA last week we decided we are not going to have deb822 by default for 23.10, so u-r-u will not do the migration on upgrades for this release. I was on travel last week so I haven't updated u-r-u or sent an update on ubuntu-devel yet, but I will today. All of the deb822 logic will still be needed of course, but we will not enable it by default for 23.10.

Provide a feature flag to allow writing deb822 format sources files to /etc/apt/sources.list.d/<distro>.sources. This feature will be disabled on stable releases to prevent change in behavior.

TheRealFalcon

I left some minor inline comments.

templates/sources.list.ubuntu.deb822.tmpl

tests/unittests/config/test_apt_configure_sources_list_v1.py

cloudinit/config/cc_apt_configure.py

When features.APT_DEB822_SOURCE_LIST_FILE is true, cloud-init will generate an /etc/apt/sources.list.d/<distro>.source file in Deb822 format instead of /etc/apt/sources.list. Add deb822 default templates per distro: - templates/sources.list.debian.deb822.tmpl - templates/sources.list.ubuntu.deb822.tmpl When features.APT_DEB822_SOURCE_LIST_FILE is set, cloud-init will - read the appropriate deb822 formatted template file per distro - read the appropriate apt configuration from apt_pkg python module to determine the right "Dir::Etc::sourceparts" directory to emit deb822 sources files - render /etc/apt/sources.list.d/<distro>.source file from the tmpl Also add validation function is_deb822_format to assert all required deb822 keys exist in the apt configuration rendered content before writing deb822 source format files. If content is not deb822 format, fallback to writing /etc/apt/sources.list. This retains original behavior on stable releases for custom cloud-config user-data containing non-deb822 formatted content for the keys: custom_apt_sources and apt_sources

Provide two new functions to handle disabling deb822 format files disable_suites_deb822 and disable_deb822_section_without_suites. When features.APT_DEB822_SOURCE_LIST_FILE is set, deb822 format apt sources will be rendered and written to /etc/apt/source.list.d/. A deb822 source file can have suites defined in a single entry: Types: deb URIs: https://ppa.launchpadcontent.net/something Suites: mantic mantic-updates Components: main When disable_suites matches any suite in a deb822 source, disable_suites_deb822 will preserve the original Suites line as a comment and redact any active configured Suites from the commented line. The result when user-data provides disable_suites: [mantic] is: Types: deb URIs: https://ppa.launchpadcontent.net/something # cloud-init disable_suites redacted: Suites: mantic mantic-updates Suites: mantic Components: main If all applicable Suites are disabled by cloud-init for an entry, cloud-init will disabled the entire entry like the following: ## Entry disabled by cloud-init, due to disable_suites # disabled by cloud-init: Types: deb # disabled by cloud-init: URIs: https://ppa.launchpadcontent.net/... # disabled by cloud-init: Suites: mantic mantic-updates # disabled by cloud-init: Components: main

deb822 sources files require minimally Types, Components, Suites and URIs keys otherwise errors are raised by apt update regarding malformed sources entry. If any of these keys exists, return is_deb822_sources_format as our best guess for the type of output format desired. This check is only a basic smell-test for custom `sources_list` optional value provided as part of cloud-config data. It is expected that an admin overriding cloud-init's defaults will prefer deb822 sources format or not when customizing behavior. is-deb822_sources_format function is more specific than the general deb822 spec, because is designed around the seeing the minimal required key names Types, Components, Suites or URIs as an indicator that this format is intended as a deb822 sources file. Any deb822 sources file that doesn't contain any one of those keys would generate format errors from apt update.

TheRealFalcon

LGTM! I'll leave it to you to merge.

holmanb

@blackboxsw Nice work on this PR. I left a few comments inline, mostly related to how the commits are.

holmanb · 2023-09-27T19:02:58Z

cloudinit/config/cc_apt_configure.py

+    """Return a dict of applicable apt configuration or defaults.
+
+    Prefer python apt_pkg if present.
+    Fallback to apt-config dump command if present out output parsed


What does "if present out output parsed" mean?

holmanb · 2023-09-27T19:11:37Z

cloudinit/config/cc_apt_configure.py


 def get_apt_cfg() -> Dict[str, str]:
-    """Return a dict of applicable apt configuration or defaults."""
-    return {
-        "sourcelist": apt_pkg.config.find_file(


The import for apt_pkg still exists at 39c5c06, which conflicts with the commit message. Please fix this if you want this to be a rebase commit.

Also it feels a bit odd to introduce code (the import for example) in one commit in a series, only to relocate / refactor the same code in a followup commit in the serious. Not a huge issue in this case, since in this case (as you mentioned) it makes the review easier due to the size of this PR, but I think that's something we want to avoid when we can for rebase commits.

Agreed. If I had stuck with the desire to rebase merge the PR this commit set would have needed tidying up.

holmanb · 2023-09-27T19:24:23Z

cloudinit/config/cc_apt_configure.py

@@ -575,8 +575,9 @@ def is_deb822_sources_format(apt_src_content: str) -> bool:


 DEFAULT_APT_CFG = {
-    "sourcelist": "/etc/apt/sources.list",
-    "sourceparts": "/etc/apt/sources.list.d/",
+    "Dir::Etc": "etc/apt",


Refactoring code that hasn't landed yet adds churn / noise in the commit log. To avoid this, please squash the changes in this commit into the commit that introduced these lines of code prior to merging.

yes agreed on this point, which had prompted my overall squash merge for this PR given some of the overlap and churn I had introduced when handling review comments and iterations. I left these separate commits in a less than desirable state after handling initial reviews which made separate commits for this effort a bit more work than I wanted to spend on this particular PR.

holmanb · 2023-09-27T19:25:21Z

cloudinit/config/cc_apt_configure.py

@@ -18,8 +18,6 @@
 from textwrap import dedent, indent
 from typing import Dict

-import apt_pkg


This import removal doesn't belong in this commit if we are merging these commits individually.

holmanb · 2023-09-27T19:28:47Z

cloudinit/config/cc_apt_configure.py

@@ -511,7 +509,7 @@ def disable_suites(disabled, src, release) -> str:
        return src

    retsrc = src
-    if is_deb822_format(src):
+    if is_deb822_sources_format(src):


Why are we renaming a function that is introduced in this commit series?

To avoid unnecessary code churn / review burden (reviewing the "same code" multiple times), it would be better to name it as you want it in the commit that introduces the function.

holmanb · 2023-09-27T19:41:48Z

cloudinit/config/cc_apt_configure.py

@@ -562,16 +560,18 @@ def is_deb822_format(apt_src_content: str) -> bool:
    Return True if content looks like it is deb822 formatted APT source.
    """
    # TODO(At jammy EOL: use aptsources.sourceslist.Deb822SourceEntry.invalid)
-    try:
-        apt_src = safeyaml.load(apt_src_content)


This is more refactoring within the series that we don't need in the commit history.

+1 on these points here @holman. I had actually squash merged this PR during merge instead of a rebase merge because of the way I had dealt with review comments and refactoring code after the fact. When I handle some of the review comments, I didn't squash the content into the correct commit in some cases and rather that presenting 10 separate commits for this effort, I thought it was going to be more appropriate to squash merge and represent the set of changes in the single commit message.

blackboxsw force-pushed the deb822-default-sources-list-d branch 2 times, most recently from 6a53cc0 to db6767a Compare September 15, 2023 02:37

blackboxsw commented Sep 15, 2023

View reviewed changes

tests/unittests/config/test_apt_source_v3.py Show resolved Hide resolved

blackboxsw force-pushed the deb822-default-sources-list-d branch 2 times, most recently from 7c3437c to 2862251 Compare September 15, 2023 15:13

blackboxsw force-pushed the deb822-default-sources-list-d branch from 2862251 to 753c9dc Compare September 15, 2023 15:25

TheRealFalcon self-assigned this Sep 15, 2023

TheRealFalcon requested changes Sep 15, 2023

View reviewed changes

blackboxsw force-pushed the deb822-default-sources-list-d branch from d24ecd2 to a1823a8 Compare September 25, 2023 03:31

features: add APT_DEB822_SOURCE_LIST_FILE allowing deb822 sources

5cbf17d

Provide a feature flag to allow writing deb822 format sources files to /etc/apt/sources.list.d/<distro>.sources. This feature will be disabled on stable releases to prevent change in behavior.

blackboxsw force-pushed the deb822-default-sources-list-d branch from c1e2111 to 35e9fae Compare September 25, 2023 15:40

blackboxsw requested a review from TheRealFalcon September 25, 2023 18:44

TheRealFalcon reviewed Sep 26, 2023

View reviewed changes

templates/sources.list.ubuntu.deb822.tmpl Show resolved Hide resolved

tests/unittests/config/test_apt_configure_sources_list_v1.py Show resolved Hide resolved

cloudinit/config/cc_apt_configure.py Show resolved Hide resolved

blackboxsw added 9 commits September 26, 2023 16:25

make python apt_pkg a recommends instead of strict dependency

39c5c06

integration tests: exercise deb822 feature in test_apt

383193d

docs: apt_configure represent deb822 sources behavior and example

203d04f

drop unneeded target check

c7ac464

apt: call apk_pkg.init_config to initialize before queries

b56fb0c

refactor get_apt_cfg and DEFAULT_APT_CFG to avoid conditionals

38cfd5d

blackboxsw force-pushed the deb822-default-sources-list-d branch from 35e9fae to 38cfd5d Compare September 26, 2023 22:26

blackboxsw requested a review from TheRealFalcon September 27, 2023 18:11

TheRealFalcon approved these changes Sep 27, 2023

View reviewed changes

blackboxsw merged commit 494bb1a into canonical:main Sep 27, 2023

holmanb requested changes Sep 27, 2023

View reviewed changes

holmanb mentioned this pull request Sep 29, 2023

Ubuntu/devel #4472

Merged

3 tasks

This was referenced Dec 14, 2023

fix(apt_configure): disable source.list if rendering deb822 #4699

Merged

Ubuntu/mantic disable deb822 #4700

Merged

Ubuntu/lunar disable deb822 #4698

Merged

Ubuntu/jammy disable deb822 #4697

Merged

Ubuntu/focal disable deb822 #4696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deb822 default sources list d #4437

Deb822 default sources list d #4437

blackboxsw commented Sep 15, 2023 •

edited

Loading

blackboxsw Sep 15, 2023

blackboxsw Sep 15, 2023

blackboxsw commented Sep 15, 2023

TheRealFalcon left a comment

enr0n commented Sep 18, 2023

TheRealFalcon left a comment

TheRealFalcon left a comment

holmanb left a comment

holmanb Sep 27, 2023

holmanb Sep 27, 2023

blackboxsw Sep 27, 2023

holmanb Sep 27, 2023

blackboxsw Sep 27, 2023

holmanb Sep 27, 2023

holmanb Sep 27, 2023

holmanb Sep 27, 2023

blackboxsw Sep 27, 2023

Deb822 default sources list d #4437

Deb822 default sources list d #4437

Conversation

blackboxsw commented Sep 15, 2023 • edited Loading

This PR is separated into distinct commits which could stand on their own for readability

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackboxsw commented Sep 15, 2023

TheRealFalcon left a comment

Choose a reason for hiding this comment

enr0n commented Sep 18, 2023

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackboxsw commented Sep 15, 2023 •

edited

Loading