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

chore: add mender-data-dir.service file #1695

Merged

Conversation

aduskett
Copy link
Contributor

@aduskett aduskett commented Nov 15, 2024

Both the mender-authd.service and mender-udpated.service files have
mender-client-data-dir.service in the "After=" line. However, this
service file isn't provided by the mender package. Instead, the
mender-client-data-dir.service file currently resides in the
meta-mender repository. This means that the mender-authd.service and
mender-updated.service files are incorrect, but currently "just work"
because of the WantedBy in the mender-data-dir.service file.

Even though "After" does not mean "required" it can still be confusing
to users as to where this file comes from. Furthermore, a user may not
want the service file to exist at all, as their build setup may create
the /data directory (or /var/lib/mender) during the image creation.

  • Move the mender-data-dir.service file into the mender project.
  • Add a new option "MENDER_DATA_DIR_SYSTEMD_UNIT" set to OFF by
    default. When enabled, the mender-data-dir.service file is
    added to the MENDER_DATA_DIR_SYSTEMD_UNIT list.
  • Rename mender-client-data-dir.service to mender-data-dir.service
    in the mender-authd.service and mender-updated.service files.

Changelog: Add systemd mender-data-dir.service optionally installed
with MENDER_DATA_DIR_SYSTEMD_UNIT CMake variable. This
service historically has been in meta-mender repository and
used elsewhere from there. By moving it to the source
repository we'll have it better aligned with authd and
updated services

Ticket: None

@mender-test-bot
Copy link

@aduskett, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".


my commands and options

You can trigger a pipeline on multiple prs with:

  • mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

  • mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

  • mentioning me and sync

You can cherry pick to a given branch or branches with:

  • mentioning me and:
 cherry-pick to:
 * 1.0.x
 * 2.0.x

@aduskett
Copy link
Contributor Author

@mender-test-bot start pipeline --pr mender/1695

@mender-test-bot
Copy link

Hello 😺 I created a pipeline for you here: Pipeline-1545075314

Build Configuration Matrix

Key Value
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
INTEGRATION_REV master
MENDER_ARTIFACT_REV master
MENDER_BINARY_DELTA_REV master
MENDER_CLI_REV master
MENDER_CONFIGURE_MODULE_REV master
MENDER_CONNECT_REV master
MENDER_CONVERT_REV master
MENDER_GATEWAY_REV master
MENDER_REV pull/1695/head
MENDER_SETUP_REV master
MENDER_SNAPSHOT_REV master
MONITOR_CLIENT_REV master
RUN_INTEGRATION_TESTS true
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true

@aduskett
Copy link
Contributor Author

I'm not sure why this failed as all of the links for the failed tests are not accessible to me.

@lluiscampos
Copy link
Member

@aduskett The mender-qa pipeline has access to internal secrets so we cannot make the CI logs public. Here the error:

NOTE: recipe mender-master-git-r0: task do_package: Started
ERROR: mender-master-git-r0 do_package: QA Issue: mender: Files/directories were installed but not shipped in any package:
  /usr/lib/systemd/system/mender-client-data-dir.service
Please set FILES such that these items are packaged. Alternatively if they are unneeded, avoid installing them or delete them within do_install.
mender: 1 installed and not shipped files. [installed-vs-shipped]
ERROR: mender-master-git-r0 do_package: Fatal QA errors were found, failing task.
ERROR: Logfile of failure stored in: /builds/Northern.tech/Mender/build-qemux86-64-uefi-grub/tmp/work/core2-64-poky-linux/mender/master-git/temp/log.do_package.114947
NOTE: recipe mender-master-git-r0: task do_package: Failed
ERROR: Task (/builds/Northern.tech/Mender/meta-mender/meta-mender-core/recipes-mender/mender-client/mender_git.bb:do_package) failed with exit code '1'

Let me know if you need help debugging it

@aduskett aduskett force-pushed the add-missing-service-file branch from 3e1435a to 89d511f Compare November 19, 2024 08:18
@aduskett
Copy link
Contributor Author

@aduskett The mender-qa pipeline has access to internal secrets so we cannot make the CI logs public. Here the error:

NOTE: recipe mender-master-git-r0: task do_package: Started
ERROR: mender-master-git-r0 do_package: QA Issue: mender: Files/directories were installed but not shipped in any package:
  /usr/lib/systemd/system/mender-client-data-dir.service
Please set FILES such that these items are packaged. Alternatively if they are unneeded, avoid installing them or delete them within do_install.
mender: 1 installed and not shipped files. [installed-vs-shipped]
ERROR: mender-master-git-r0 do_package: Fatal QA errors were found, failing task.
ERROR: Logfile of failure stored in: /builds/Northern.tech/Mender/build-qemux86-64-uefi-grub/tmp/work/core2-64-poky-linux/mender/master-git/temp/log.do_package.114947
NOTE: recipe mender-master-git-r0: task do_package: Failed
ERROR: Task (/builds/Northern.tech/Mender/meta-mender/meta-mender-core/recipes-mender/mender-client/mender_git.bb:do_package) failed with exit code '1'

Let me know if you need help debugging it

Hey! Thanks for the help! I have no idea what is going on, as a grep for say, mender-authd.service doesn't pull up anything in a FILES list or directory. Where am I supposed to add the new service file?

Thanks so much!

@lluiscampos lluiscampos self-assigned this Nov 19, 2024
@lluiscampos
Copy link
Member

I need to have a look into it at a later time (this week I am busy with something out of regular work). I assigned it to myself 👍

@lluiscampos
Copy link
Member

@aduskett This error is from meta-mender, because there we already have a mender-data-dir.service shipped with the recipe and the new one being added in this PR is not handled.

Can you elaborate on the motivation on having this file here? So far we consider it system specific, and that is why we have it in meta-mender for yocto and also install it in mender-convert for Debian images.

If you need it to be in the mender repo for other reason (buildroot?) we could have it conditionally included with CMake and off by default. But still will make more sense for us to keep it in the repo that does the system integration (iow the responsible for creating a /data partition).

@aduskett
Copy link
Contributor Author

aduskett commented Nov 26, 2024

@aduskett This error is from meta-mender, because there we already have a mender-data-dir.service shipped with the recipe and the new one being added in this PR is not handled.

Can you elaborate on the motivation on having this file here? So far we consider it system specific, and that is why we have it in meta-mender for yocto and also install it in mender-convert for Debian images.

If you need it to be in the mender repo for other reason (buildroot?) we could have it conditionally included with CMake and off by default. But still will make more sense for us to keep it in the repo that does the system integration (iow the responsible for creating a /data partition).

Understandable, but then why does mender-authd.service and mender-client.service require mender-client-data-dir.service? Especially if the data directory (or /var/lib/mender) may already be there. It doesn't seem necessary and more tailored to meta-mender specifically, which, in that case, there isn't any documentation about this requirement, or where mender-client-data-dir.service comes from.

@lluiscampos
Copy link
Member

Understandable, but then why does mender-authd.service and mender-client.service require mender-client-data-dir.service? (...)

Sorry for the delay in the response - I've been checking this also internally with some colleagues.

The documentation is not crystal clear, but from what I can understand from the man page (and from my local experiments), I believe that After= does not declare a hard dependency, but a soft one - "if the service is there, run it before you run this one". If that is correct, then mender-authd.service and mender-updated.service (I assume you meant "updated" above) do not require mender-client-data-dir.service.

From the sytemd man page:

systemd knows various kinds of dependencies, including positive and
negative requirement dependencies (i.e.  Requires= and Conflicts=) as well
as ordering dependencies (After= and Before=). NB: ordering and
requirement dependencies are orthogonal. If only (...)

With the above I am trying to justify that as it is now is "okey". But all in all @aduskett you have a very valid point and we could have designed this differently - having mender-client-data-dir.service in this repo, installed conditionally by CMAke on some variable, and then use opt-it in in meta-mender and mender-convert.

Can you prepare the work in the three repos (here + meta-mender + mender-convert)? I don't know how much experience you have on yocto, but there we will need to differentiate between old and newer recipes from where they can get the service. I can guide you and review your work but I am pretty busy otherwise as to do the job myself 😃

@lluiscampos
Copy link
Member

FYI @TheYoctoJester and @danielskinstad ☝️

@aduskett
Copy link
Contributor Author

aduskett commented Dec 2, 2024

@lluiscampos Absolutely I can. That sounds like an excellent solution. As for my Yocto experience, I have enough to be dangerous :)
What would you like the CMake option name to be?

@lluiscampos
Copy link
Member

(...)What would you like the CMake option name to be?

A boolean MENDER_DATA_DIR_SYSTEMD_UNIT, for example?

@aduskett aduskett force-pushed the add-missing-service-file branch 2 times, most recently from 7f828a1 to 7c61612 Compare December 3, 2024 14:04
@aduskett
Copy link
Contributor Author

aduskett commented Dec 3, 2024

(...)What would you like the CMake option name to be?

A boolean MENDER_DATA_DIR_SYSTEMD_UNIT, for example?

ok, this pull request has been updated to use an option. As the option is set to OFF by default, I will make a pull request with the other two repositories once this is merged. Sound good?

Copy link
Member

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

A part from my comment below,

I just realized that here the service files go after mender-client-data-dir.service while in meta-mender it was renamed to mender-data-dir.service. It "just works" because of the WantedBy in the mender-data-dir.service but it is wrong!

So, can we then rename the file you are introducing to mender-data-dir.service and update the updated and authd services?

Also, this PR definitely honors a Changelog entry 😃 Suggestion to write something like: "Changelog: Add systemd mender-data-dir.service optionally installed with MENDER_DATA_DIR_SYSTEMD_UNIT CMake variable. This service historically has been in meta-mender repository and used elsewhere from there. By moving it to the source repository we'll have it better aligned with authd and updated services"

ok, this pull request has been updated to use an option. As the option is set to OFF by default, I will make a pull request with the other two repositories once this is merged. Sound good?

Sounds very good! Thanks for doing this

@aduskett aduskett force-pushed the add-missing-service-file branch from 7c61612 to 579ce10 Compare December 5, 2024 10:33
@aduskett
Copy link
Contributor Author

aduskett commented Dec 5, 2024

A part from my comment below,

I just realized that here the service files go after mender-client-data-dir.service while in meta-mender it was renamed to mender-data-dir.service. It "just works" because of the WantedBy in the mender-data-dir.service but it is wrong!

So, can we then rename the file you are introducing to mender-data-dir.service and update the updated and authd services?

Also, this PR definitely honors a Changelog entry 😃 Suggestion to write something like: "Changelog: Add systemd mender-data-dir.service optionally installed with MENDER_DATA_DIR_SYSTEMD_UNIT CMake variable. This service historically has been in meta-mender repository and used elsewhere from there. By moving it to the source repository we'll have it better aligned with authd and updated services"

ok, this pull request has been updated to use an option. As the option is set to OFF by default, I will make a pull request with the other two repositories once this is merged. Sound good?

Sounds very good! Thanks for doing this

Sounds good. I submitted the changes as requested!

@aduskett aduskett force-pushed the add-missing-service-file branch from 579ce10 to 67020b7 Compare December 5, 2024 10:41
@aduskett aduskett changed the title chore: add mender-client-data-dir.service file chore: add mender-data-dir.service file Dec 5, 2024
@aduskett aduskett force-pushed the add-missing-service-file branch 2 times, most recently from d6a4c23 to 41d4961 Compare December 5, 2024 10:46
@lluiscampos
Copy link
Member

@aduskett Out commit linter is a bit picky, can you remove the blank spaces before the Changelog lines? Like:

Changelog: Add systemd mender-data-dir.service optionally installed
with MENDER_DATA_DIR_SYSTEMD_UNIT CMake variable. This
service historically has been in meta-mender repository and
used elsewhere from there. By moving it to the source
repository we'll have it better aligned with authd and
updated services

Thanks 🙌

Both the mender-authd.service and mender-udpated.service files have
mender-client-data-dir.service in the "After=" line. However, this
service file isn't provided by the mender package. Instead, the
mender-client-data-dir.service file currently resides in the
meta-mender repository. This means that the mender-authd.service and
mender-updated.service files are incorrect, but currently "just work"
because of the WantedBy in the mender-data-dir.service file.

Even though "After" does not mean "required" it can still be confusing
to users as to where this file comes from. Furthermore, a user may not
want the service file to exist at all, as their build setup may create
the /data directory (or /var/lib/mender) during the image creation.

  - Move the mender-data-dir.service file into the mender project.
  - Add a new option "MENDER_DATA_DIR_SYSTEMD_UNIT" set to OFF by
    default. When enabled, the mender-data-dir.service file is
    added to the MENDER_DATA_DIR_SYSTEMD_UNIT list.
  - Rename mender-client-data-dir.service to mender-data-dir.service
    in the mender-authd.service and mender-updated.service files.

Changelog: Add systemd mender-data-dir.service optionally installed
with MENDER_DATA_DIR_SYSTEMD_UNIT CMake variable. This
service historically has been in meta-mender repository and
used elsewhere from there. By moving it to the source
repository we'll have it better aligned with authd and
updated services

Ticket: None

Signed-off-by: Adam Duskett <[email protected]>
@aduskett aduskett force-pushed the add-missing-service-file branch from 41d4961 to 5dd629b Compare December 5, 2024 12:25
@mender-test-bot
Copy link

Merging these commits will result in the following changelog entries:

Changelogs

mender (add-missing-service-file)

New changes in mender since master:

  • Add systemd mender-data-dir.service optionally installed
    with MENDER_DATA_DIR_SYSTEMD_UNIT CMake variable. This
    service historically has been in meta-mender repository and
    used elsewhere from there. By moving it to the source
    repository we'll have it better aligned with authd and
    updated services

@aduskett
Copy link
Contributor Author

aduskett commented Dec 5, 2024

@lluiscampos done

@lluiscampos
Copy link
Member

@mender-test-bot start pipeline

@mender-test-bot
Copy link

Hello 😺 I created a pipeline for you here: Pipeline-1575204043

Build Configuration Matrix

Key Value
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
INTEGRATION_REV master
MENDER_ARTIFACT_REV master
MENDER_BINARY_DELTA_REV master
MENDER_CLI_REV master
MENDER_CONFIGURE_MODULE_REV master
MENDER_CONNECT_REV master
MENDER_CONVERT_REV master
MENDER_GATEWAY_REV master
MENDER_REV pull/1695/head
MENDER_SETUP_REV master
MENDER_SNAPSHOT_REV master
MONITOR_CLIENT_REV master
RUN_INTEGRATION_TESTS true
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true

@lluiscampos lluiscampos merged commit 043a1ae into mendersoftware:master Dec 6, 2024
16 checks passed
@mender-test-bot
Copy link

Hello 😺 This PR contains changelog entries. Please, verify the need of backporting it to the following release branches:
4.0.x (release 3.7.x) - 🤖 🍒

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