Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ubuntu/devel: Clean up postinst and preinst scripts #5567

Open
wants to merge 2 commits into
base: ubuntu/devel
Choose a base branch
from

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Jul 29, 2024

Drop version gated code from preinst.
Add version gates to old code in preinst and postinst.

Proposed Commit Message

chore(debian): Clean up postinst and preinst scripts

Drop version gated code from preinst.
Add version gates to old code in preinst and postinst.

Additional Context

Test Steps

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>)

debian/cloud-init.postinst Outdated Show resolved Hide resolved
@blackboxsw blackboxsw added the packaging Supplemental package review requested label Jul 29, 2024
@holmanb holmanb force-pushed the ubuntu/devel branch 3 times, most recently from 085f532 to 2e53fe3 Compare August 5, 2024 19:15
Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Aug 20, 2024
@holmanb holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Aug 20, 2024
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments got moved where they shouldn't, and I think we could remove even more, but generally looks good

debian/cloud-init.preinst Outdated Show resolved Hide resolved
grep DataSourceOracle /var/lib/cloud/instance/datasource > /dev/null 2>&1 || return 0
rm -f /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg
}

cleanup_user_scripts() {
local oldver="$1" last_bad_ver="24.3~1g6e4153b3-0ubuntu1"
dpkg --compare-versions "$oldver" le "$last_bad_ver" || return 0 # from Oracle now that datasource honors system cfg above datasource cfg.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment shouldn't be here.

debian/cloud-init.preinst Outdated Show resolved Hide resolved
debian/cloud-init.preinst Outdated Show resolved Hide resolved
@TheRealFalcon TheRealFalcon self-assigned this Aug 26, 2024
Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to document these changes in a new UNRELEASED d/changelog entry.
Given that we had released to oracular, we'll probably need to rebase on latest upstream/ubuntu/devel

shellcheck warnings on the scripts don't generate any new warning conditions for non-POSIX patterns we don't already use in other functions.

Upgrade path on oracular doesn't generate any error conditions.

Unrelated to this PR, lintian is not thrilled with a now bogus/deprecated/retired override.
lintian (2.117.0) unstable; urgency=low dropped package-supports-alternative-init-but-no-init.d-script Feb 2024

 lintian  cloud-init_24.4~2g2e4c39b7-0ubuntu2_all.deb
E: cloud-init: alien-tag package-supports-alternative-init-but-no-init.d-script [usr/share/lintian/overrides/cloud-init:7]
lintian -i cloud-init_24.4~2g2e4c39b7-0ubuntu2_all.deb
N:
E: cloud-init: alien-tag package-supports-alternative-init-but-no-init.d-script [usr/share/lintian/overrides/cloud-init:7]
N: 
N:   The given override refers to an unknown tag.
N: 
N:   Please refer to Format of override files (Section 2.4.1) in the Lintian
N:   User's Manual for details.
N: 
N:   Visibility: error
N:   Show-Always: yes
N:   Check: debian/lintian-overrides/mystery

We can delete that speciifc lintian override in this PR if you want (and we may need to talk separately about lintian warnings

W: cloud-init: systemd-service-file-refers-to-unusual-wantedby-target cloud-init.target [lib/systemd/system/cloud-init-network.service]
N:
W: cloud-init: systemd-service-file-shutdown-problems [lib/systemd/system/cloud-init-main.service]
N: 
N:   The specified systemd .service file contains both DefaultDependencies=no
N:   and Conflicts=shutdown.target directives without Before=shutdown.target.
N:   
N:   This can lead to problems during shutdown because the service may linger
N:   until the very end of shutdown sequence as nothing requests to stop it
N:   before (due to DefaultDependencies=no).
N:   
N:   There is race condition between stopping units and systemd getting a
N:   request to exit the main loop, so it may proceed with shutdown before all
N:   pending stop jobs have been processed.
N:   
N:   Please add Before=shutdown.target.
N: 
N:   Please refer to https://github.com/systemd/systemd/issues/11821 for
N:   details.
N: 
N:   Visibility: warning
N:   Show-Always: no
N:   Check: systemd
N: 
N: 0 hints overridden; 1 unused override

debian/cloud-init.postinst Outdated Show resolved Hide resolved
debian/cloud-init.postinst Outdated Show resolved Hide resolved
@@ -7,81 +7,4 @@ set -e
set -f # disable pathname expansion
db_capb escape # to support carriage return / multi-line values

fix_lp1889555() {
local oldver="$1" last_bad_ver="20.3-2-g371b392c-0ubuntu1"
Copy link
Member Author

@holmanb holmanb Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jammy's first release was 22.1-14-g2e17a0d6-0ubuntu1~22.04.5. This should be safe to drop. Also, since cloud style grub initialization landed in noble already this is a no-op.

rename_hook_hotplug_udev_rule() {
local oldver="$1" last_bad_ver="24.3~1g6e4153b3-0ubuntu1"
dpkg --compare-versions "$oldver" le-nl "$last_bad_ver" || return 0
# Avoids LP: #1946003 see commit: b519d861aff8b44a0610c176cb34adcbe28df144
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 21.4, so anything that upgraded through Jammy (22.1-14-g2e17a0d6-0ubuntu1~22.04.5) should have gotten this fix already.

}

update_maas_spelling(){
local oldver="$1" last_bad_ver="24.3~1g6e4153b3-0ubuntu1"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maas spelling has existed since xenial (maybe before), so this shouldn't be needed anymore.

@@ -5,10 +5,6 @@ set -e
case "$1" in
purge)
rm -f /etc/cloud/cloud.cfg.d/90_dpkg.cfg
rm -f /etc/apt/apt.conf.d/90cloud-init-pipelining
Copy link
Member Author

@holmanb holmanb Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloud-init used to have a matching postinst which installed this file (it ran cloud-init single from the postinst). Cloud-init no longer automatically installs it, so we can drop this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack. Dropped in cloud-init 18.5, which means any package upgrades coming from bionic++ will already have this patch.

#!/bin/sh

set -e
rm -f /etc/cron.d/cloudinit-updates
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't been used in a very long time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, pre version 0.7.7

@holmanb
Copy link
Member Author

holmanb commented Sep 3, 2024

W: cloud-init: systemd-service-file-refers-to-unusual-wantedby-target cloud-init.target [lib/systemd/system/cloud-init-network.service]
N:
W: cloud-init: systemd-service-file-shutdown-problems [lib/systemd/system/cloud-init-main.service]
N: 
N:   The specified systemd .service file contains both DefaultDependencies=no
N:   and Conflicts=shutdown.target directives without Before=shutdown.target.
N:   
N:   This can lead to problems during shutdown because the service may linger
N:   until the very end of shutdown sequence as nothing requests to stop it
N:   before (due to DefaultDependencies=no).
N:   
N:   There is race condition between stopping units and systemd getting a
N:   request to exit the main loop, so it may proceed with shutdown before all
N:   pending stop jobs have been processed.
N:   
N:   Please add Before=shutdown.target.
N: 
N:   Please refer to https://github.com/systemd/systemd/issues/11821 for
N:   details.
N: 
N:   Visibility: warning
N:   Show-Always: no
N:   Check: systemd
N: 
N: 0 hints overridden; 1 unused override

@blackboxsw I addressed this in #5659

@holmanb
Copy link
Member Author

holmanb commented Sep 3, 2024

@blackboxsw @TheRealFalcon Thanks for the reviews. I just updated the PR to address your comments. I also spotted a couple of more things that I think we can drop which I've added in a separate commit.

@aciba90
Copy link
Contributor

aciba90 commented Sep 4, 2024

Sorry for the conflicts, some lintian issues were addressed in #5652 and I did not see the overlap here.

@holmanb
Copy link
Member Author

holmanb commented Sep 4, 2024

Sorry for the conflicts, some lintian issues were addressed in #5652 and I did not see the overlap here.

all good, no worries

@TheRealFalcon
Copy link
Member

+1 to changes, but still needs a d/changelog entry.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve pending d/changelog entry

@holmanb
Copy link
Member Author

holmanb commented Sep 4, 2024

I just squashed the extra commits. It looks like your "request changes" review hasn't been dismissed by Github yet @TheRealFalcon so I can't merge yet.

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@blackboxsw
Copy link
Collaborator

@holmanb cleared to land this in ubuntu/devel. oracular accepted our 24.4~3+really24.3.1 so we can rebase and land this unreleased content above that.

@blackboxsw blackboxsw added this to the cloud-init-24.4 milestone Sep 6, 2024
@holmanb holmanb added the wip Work in progress, do not land label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Supplemental package review requested wip Work in progress, do not land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants