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

ds-identify: FreeBSD fixes #4485

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Conversation

igalic
Copy link
Collaborator

@igalic igalic commented Oct 4, 2023

Proposed Commit Message

ds-identify: Allow disable service and override environment

This PR allows disabling the ds-identify service on FreeBSD, without
having to delete the service file.
It also overrides two environment variables when running the service
to ensure that on FreeBSD, paths are where they are to be expected.

Fixes GH-4481
Sponsored by: The FreeBSD Foundation
Co-authored-by: Dave Cottlehuber <[email protected]>

Additional Context

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

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! Does @dch want to review too?

igalic and others added 2 commits October 6, 2023 22:06
sometimes it's preferable to disable ds-identify, for example when the
results it returns are bogus, or when we know the results can set them
ourselves, because they are unchanging.

Allow disabling the service, rather than having to delete it.

Sponsored by: The FreeBSD Foundation
override PATH_DI_CONFIG and PATH_ETC_CLOUD when executing ds-identify \
on startup.

Inconviniently, we can't just override PATH_ROOT, because that would set
/run to /usr/local/run and /var/lib/cloud to /usr/local/var/lib/cloud.

In the future we will want to override /run, but we can't do that just
yet without also ensuring that the python code can relocate /run.

Fixes: canonicalGH-4481
Sponsored by: The FreeBSD Foundation
Co-authored-by: Dave Cottlehuber <[email protected]>
@dch
Copy link

dch commented Oct 6, 2023

thanks @igalic for the help on this, and the better solution than my original one.
This works in practice; I am not entirely sure whether all the vars from different
rc files should be mixed or not, it's not something I've seen in other scripts.
Viz,

load_rc_config 'cloudinit' <- only loads the cloudinit_* vars

: ${cloudinit_enable="NO"} <- ok
: ${cloudinit_dsidentify_enable="YES"} <- ok too
: ${dsidentify_env="PATH_DI_CONFIG={{ prefix }}/etc/cloud/ds-identify.cfg PATH_ETC_CLOUD={{ prefix }}/etc/cloud"} <- script won't work without it

dsidentify_env prefix has to match the ${name} in the script to be loaded, as
we found. I suspect that it's not possible to override dsidentify_env this way,
for example. Not that anybody should want to do this, ofc.

Anyway, +1 for merging this as-is, the disabling functionality will work, and
the fix for previous issue is resolved.

@TheRealFalcon TheRealFalcon merged commit 9fde069 into canonical:main Oct 9, 2023
27 checks passed
@igalic igalic deleted the fix/ds-identify-fbsd branch October 10, 2023 08:00
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.

3 participants