landscape-client: handle already registered client #4784
Conversation
There was a problem hiding this comment.
Thank you @chifac08 for continued support and contributions to cloud-init.
I do think we probably want to pull the landscape-config --is-registered check up before we invoke the full landscape-config --silent command with all the cmd_params. Otherwise, I'm not quite certain how landscape-config is expected to behave when not registered, but when it's also provided with all necessary config options to register.
I think we'll also want to check with @Perfect5th on the estimated SRU verification for the currently queued SRU back to Jammy for landscape-client v. 23 before we land this as we'd have to back out those changes on stable releases for cloud-init which also targets back to Jammy.
|
Implemented changes you requested. Furthermore, I added some unit tests. Please review again. thx |
There was a problem hiding this comment.
Thanks @chifac08 for the update and unit tests. In testing on a live system we can see that the rcs=[5] handling isn't working as your code expects. I've proposed an alternative that you may want to take a peek at.
Also I know the landscape-client is proposed for update into Focal and Jammy so that landscape-config --is-registered should be available on all supported Ubuntu releases that cloud-init updates.
But, we will need to wait on releasing this in cloud-init until landscape-client 23.02 published to focal. The current status of landscape-client SRU is present in blocked state here https://ubuntu-archive-team.ubuntu.com/pending-sru.html. rmadison landscape-client can also tell us once that SRU publication is officially accepted.
cloudinit/config/cc_landscape.py
Outdated
| try: | ||
| subp.subp(["landscape-config", "--silent", "--is-registered"], rcs=[5]) | ||
| subp.subp(["landscape-config", "--silent"] + cmd_params) | ||
| util.write_file(LS_DEFAULT_FILE, "RUN=1\n") |
There was a problem hiding this comment.
I actually think writing this file a feature that is now obsolete and unnecessary given that we are running landscape-config directly which will start the landscape-client services up and ensure they are active at boot. It was first introduced in 2012 when this module was writing directly to /etc/landscape/client.conf and performing a service landscape-client restart after writing configs. I don't think the recent landscape-client on any supported Ubuntu series from Focal onward consumes this RUN=1 setting in /etc/default/landscape-client anymore. So, let's drop the LS_DEFAULT_FILE variable and this write_file as well.
There was a problem hiding this comment.
Just validated that all services are appropriately started on Ubuntu focal with landscape-client 18.01-0ubuntu4.3 and no RUN=1 needed in /etc/default/landscape-client. Yes this can be dropped/
There was a problem hiding this comment.
Approve the changeset, but I'd prefer to avoid landing this until https://bugs.launchpad.net/landscape-client/+bug/2006402 lands and SRUs to Focal so we don't have to stub out the --is-registered parameter logic.
github-actions
bot
added
the
stale-pr
|
Still waiting on SRU resolution for landscape-client before this lands. |
|
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
bot
added
the
stale-pr
TheRealFalcon
removed
the
stale-pr
|
@blackboxsw , looks like the last comment on that bug is from 2023-10-20 . Do we expect that to be SRUed anytime soon? |
github-actions
bot
added
the
stale-pr
Yep. I had circled back with the team and was assured they had it on their planning and it looks like verification just got unblocked yesterday. We expect this release within a week now that verification is complete for Focal/Jammy. https://bugs.launchpad.net/landscape-client/+bug/2006402/comments/53 |
github-actions
bot
added
the
stale-pr
|
Checked again on this landscape-client SRU. All tests are verified, SRU review queue is saturated given demand for Noble release reviews. Giving this another week to allow the Noble release week testing to pass this week which should free up SRU vanguards to unblock landscape-client publication. |
|
chifac08 looks like landscape-client published yesterday. I just validated your PR works great on both Ubuntu focal and jammy with the newly published landscape-client --is-registered param. Finally landing this without further delay. |
landscape-client: handle already registered client
Additional Context
The parameter was introduced to landscape-client in version 23.02 (canonical/landscape-client#103) and has been available in "
release (main)" repository since Lunar. The package is currently in "proposed" repository for Focal as well as Jammy and will hopefully be released in near future. So please keep in mind, if this PR should make it's way in the upstream we have to take care about the current release status of all supported Ubuntu releases. Please have a look at launchpadTest Steps
Checklist
Merge type