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

manifest: override insights-client-boot.service (HMS-4827) #1192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

loadtheaccumulator
Copy link

@loadtheaccumulator loadtheaccumulator commented Feb 4, 2025

Enable insights client.service to collect at boot to fix issue with rpm-ostree updates not being collected and reported via insights client at reboot.

Fixes: HMS: 4827

@loadtheaccumulator loadtheaccumulator requested a review from a team as a code owner February 4, 2025 22:07
@loadtheaccumulator loadtheaccumulator force-pushed the insights_enabled_boot branch 2 times, most recently from 8a291d7 to 381a87c Compare February 5, 2025 01:32
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Thanks! This LGTM but there's a couple of small issues below.

pkg/manifest/anaconda_installer_iso_tree.go Outdated Show resolved Hide resolved
pkg/manifest/anaconda_installer_iso_tree.go Outdated Show resolved Hide resolved
@achilleas-k achilleas-k changed the title manifest: add insights-client.service enable manifest: add insights-client.service enable (HMS-4827) Feb 5, 2025
@loadtheaccumulator
Copy link
Author

loadtheaccumulator commented Feb 5, 2025

@achilleas-k It looks like the service override file is written on condition of serviceOptions.InsightsOnBoot being true in subscription.go. I'm not sure the preferred way to pull that check in here so it only writes to the kickstart. Could also just write a bash check in the kickstart to check for the service, but the check is cleaner.

if insightsOnBoot {
icDir, icFile, err := runInsightsClientOnBoot()
if err != nil {
return nil, nil, nil, nil, err
}
dirs = append(dirs, icDir)
files = append(files, icFile)

@thozza
Copy link
Member

thozza commented Feb 6, 2025

Wouldn't it make more sense to conditionally add Wants=insights-client.service if insightsOnBoot with the proper ordering, to the osbuild-subscription-register.service unit

Wants: []string{"network-online.target"},
?

@loadtheaccumulator
Copy link
Author

Wouldn't it make more sense to conditionally add Wants=insights-client.service if insightsOnBoot with the proper ordering, to the osbuild-subscription-register.service unit

I tried this out with an override and the insights client does run at boot even when the ConditionPathExists condition is false, but execution at first boot errors. I haven't chased the root cause down yet, but assuming it's because it's running before registration and needs to be added to the After in the insights client service. I haven't tested it, but with the After, I would expect collection to happen twice on first boot. Once with the Wants and once with the registration.

The insights-client.service status does reflect it is disabled, so wondering if it running could cause confusion. Disabling will require overriding the Wants in the register unit or removing the insights client override file.

@loadtheaccumulator
Copy link
Author

The registration service needs to be added to the insights-client After directive anyway. I added an if insights-client override file exists conditional to the kickstart post and everything worked as expected.

@thozza
Copy link
Member

thozza commented Feb 7, 2025

@loadtheaccumulator Wants= just ensures the unit is pulled into the transaction, and After= ensures the proper order. Even if multiple units Wants the same unit, it is still started only once at boot.

@thozza
Copy link
Member

thozza commented Feb 7, 2025

Could you describe the problem and the expected behavior in more detail? The solution seems too messy to me. Since our code creates the override drop-in and the registration unit, I have difficulty understanding why this can't be solved in the units themselves. Adding a %post script to the KS to enable the unit does not look ideal.

@loadtheaccumulator
Copy link
Author

loadtheaccumulator commented Feb 7, 2025

Hey @thozza .For Edge OS upgrades to reflect in Insights, they have to collect at boot. Insights normally collects periodically, so insights-client.service handles the collection at boot. The kickstart file was originally injected into the ISO by Edge API three years or so ago, and we both configured and enabled the service there because we didn't have access to the ostree image at that time. When we moved that functionality to Image Builder last year, the service was configured, but wasn't enabled.

For me, the problem with the Wants is that it doesn't reflect the actual status of the service, and the registration service is designed for first boot. Maybe we just need to document it in the override.conf file, but if collection errors on boot at some point, my first step is to check the service. It will reflect disabled even though it runs via the Wants in another service.

The first boot registration service is also enabled via the kickstart.

EDIT: another issue is around disabling the insights-client service, due to ostree being immutable, requires either deleting/renaming the override.conf file or overriding the registration service Wants.

@loadtheaccumulator
Copy link
Author

I just ran across an insights-client-boot.service that is enabled by default. Testing overriding it now instead of insights-client.service.

@loadtheaccumulator
Copy link
Author

Overriding insights-client-boot.service works, and I updated this PR to reflect the change.

  • service is already enabled by default (so no need for additional enable step in ks file or other)
  • runs and collects at every boot

@loadtheaccumulator loadtheaccumulator changed the title manifest: add insights-client.service enable (HMS-4827) manifest: override insights-client-boot.service (HMS-4827) Feb 8, 2025
@loadtheaccumulator
Copy link
Author

I updated the title and description accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants