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

[aciba90]

Proposed Commit Message

fix(apt_configure): disable sources.list if rendering deb822
    
If deb822 sources format is used and a preexistent
/etc/apt/sources.list exists, then disable the old sources file.
This prevents user-facing warnings while operating with apt or
apt-get and other possible conflicts.
    
This is safe to do as cloud-init owns the base APT configuration which
tipically/historically involved overwriting /etc/apt/sources.list

Fixes GH-4691
LP: #2045086

Additional Context

I think it is safe to disable /etc/apt/source.list, because previously cloud-init override it, before #4437, or now if features.APT_DEB822_SOURCE_LIST_FILE is false or if the provided sources config has the classic style.
#4691

Test Steps

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

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[blackboxsw]

I'm not certain this is the fix we want. In noble, cloud-init doesn't appear to actually be writing /etc/apt/sources.list it seems an artifact of the image build process given that the timestamp on the file matches /etc/cloud/build.info not the actual instance launch time/date

[aciba90]

I'm not certain this is the fix we want. In noble, cloud-init doesn't appear to actually be writing /etc/apt/sources.list it seems an artifact of the image build process given that the timestamp on the file matches /etc/cloud/build.info not the actual instance launch time/date

That is true and has been like that for previous Ubuntu releases, I believe. Cloud-init did and is overwriting /etc/apt/sources.list if features.APT_DEB822_SOURCE_LIST_FILE == False. So the options I see for the case when cloud-init is going to write [1]ubuntu.sources but [2]sources.list exists are:

1. Write [1] and disable [2]. (This PR).
2. Overwrite [2], instead of [1] and warn about it. (Maintain old behavior).
3. Parse [2], merge with [1] and goto 1. (Complex and error-prone).

[blackboxsw]

That is true and has been like that for previous Ubuntu releases, I believe. Cloud-init did and is overwriting /etc/apt/sources.list if features.APT_DEB822_SOURCE_LIST_FILE == False. So the options I see for the case when cloud-init is going to write [1]ubuntu.sources but [2]sources.list exists are:

1. Write [1] and disable [2]. (This PR).
2. Overwrite [2], instead of [1] and warn about it. (Maintain old behavior).
3. Parse [2], merge with [1] and goto 1. (Complex and error-prone).

Agreed here, we know that there is work in progress to make sure CPC image builds do not leave this config artifact around on images with cloud-init installed. But, that would only cover the Ubuntu cloud image case for migration to an APT deb822 compatible system.

Since we know cloud-init needs to support any debian derivative, we still need to cover the case where new cloud-init which wants to write deb822 format is running in some debian image which contains an old /etc/apt/sources.list unified format.
In this case, it makes sense to disable the existing /etc/apt/sources.list, WARN on this behavior taken by cloud-init and allow cloud-init to emit the new deb822 sources format to /etc/cloud/cloud.cfg.d as you have listed.

As you mentioned, we don't want to perform 3. to parse old unified sources.list due to complexity, but also due to the fact that cloud-init has always blindly overwritten that file anyway, so no initial /etc/apt/sources.list file survives first boot with cloud-init anyway, so we don't need to support that case.

We also should rule out 2. for that same reason, cloud-init should own base APT configuration on a system (which typically involved overwriting /etc/apt/sources.list anyway) so no need to preserve this file on new deb822-compatible installs.

[blackboxsw]

@aciba90 thank you for clarifying the use-cases we want to cover here. I think a minor adjustment to the file suffix we use when renaming, and a breadcrumb comment in the file to notify that cloud-init migrated it would be helpful.
Thanks for covering the more generic cases outside of Ubuntu cloud image builds.

[blackboxsw]

I think we want to rename this .disabled instead of .old as a best practice for preserved and ignored apt sources files as we see apt ignores these files by default `

$ apt-config dump | grep disable
Dir::Ignore-Files-Silently:: "\.disabled$";

I think we may also want to add a header line # disabled by cloud-init to this file to leave a breadcrumb for why this rename happened

[blackboxsw]

General idea looks good, i think we need unittest coverage for this case and a minor addition to the existing integration test tests/integration_tests/modules/test_apt_functionality.py to assert that we don't see warnings about duplicate apt sources being found on any series as well as that we see the .disabled file being created.

[aciba90]

Thanks, @blackboxsw, for the comments and suggestions! I have addressed them and this PR is ready to be re-reviewed.

[blackboxsw]

Thanks for the integration test and unittest coverage. +1 with a minor nit in integration test.
I'll apply it and land it. it'd be good to get an upload from main into noble to help out customers affected by this warning.

[blackboxsw]

Just a minor improvement to error message traceback.

Suggested change

	assert (
	class_client.execute(f"ls {ORIG_SOURCES_FILE}").return_code
	!= 0
	)
	assert class_client.execute(
	f"test -f {ORIG_SOURCES_FILE}"
	).failed, f"Found unexpected {ORIG_SOURCES_FILE}"