Add foreman_ygg_migration package #11317

adamruzicka · 2024-10-04T12:30:36Z

This package should get pulled in when yggdrasil >=0.4.z is installed.

When installed, it checks whether yggdrasild.service is enabled. If not, it does nothing. If enabled, this package tweaks yggdrasil's config file to account for the newly needed config options. Additionally, it disables the yggdrasild service and enables yggdrasil.service.

Our worker is left to be activated through dbus.

adamruzicka · 2024-10-15T12:00:52Z

[test rpm-copr]

ekohl

Just gathering my thoughts on this.

This is because today we ship packages/client/yggdrasil but future EL versions will ship this instead. Those future EL versions are 0.4 which has a different architecture, but most importantly a different service name.

I'm worried that if systemctl is-enabled yggdrasild could be unreliable to detect this. I don't know exactly what the ordering will be, but perhaps the old service file is already cleaned up and systemd doesn't know about it anymore.

ekohl · 2024-10-15T13:25:43Z

packages/client/foreman_ygg_migration/foreman_ygg_migration.spec

+
+	if grep -q FOREMAN_YGG_WORKER_WORKDIR /etc/systemd/system/yggdrasild.service.d/* 2>/dev/null; then
+		mkdir -p /etc/systemd/system/com.redhat.Yggdrasil1.Worker1.foreman.service.d/
+		cp -r /etc/systemd/system/yggdrasild.service.d/* /etc/systemd/system/com.redhat.Yggdrasil1.Worker1.foreman.service.d/


After this, you may need to run systemctl daemon-reload.

ekohl · 2024-10-15T13:27:43Z

packages/client/foreman_ygg_migration/foreman_ygg_migration.spec

+		cp -r /etc/systemd/system/yggdrasild.service.d/* /etc/systemd/system/com.redhat.Yggdrasil1.Worker1.foreman.service.d/
+	fi
+
+	systemctl disable --now yggdrasild


Shouldn't the old service get uninstalled and thus removed?

From what I've seen, we're missing systemd scriptlets in our yggdrasil spec, so while the old package gets uninstalled and the yggdrasild service definition goes away, it seems to stay enabled.

ekohl · 2024-10-15T13:29:24Z

packages/client/foreman_ygg_migration/foreman_ygg_migration.spec

+%post
+if systemctl is-enabled yggdrasild >/dev/null 2>/dev/null; then
+	grep -Pq '^server'    /etc/yggdrasil/config.toml || sed -i 's/broker.*=/server =/' /etc/yggdrasil/config.toml
+	grep -Pq '^prefix'    /etc/yggdrasil/config.toml || echo 'prefix = "yggdrasil"' >>/etc/yggdrasil/config.toml


This doesn't look idempotent. Perhaps grep should be stricter to check if the prefix isn't correct? Same below for data-host

I was operating under the assumption that the configuration was generated by foreman. That would mean that the data-host and prefix either aren't there at all or are generated correctly (after theforeman/foreman#10340 gets in). I could tighten it down, but I'm not sure if that's worth it

I think I misread it and you're right, it is idempotent: either the line is there or it's added.

ekohl · 2024-10-15T13:29:55Z

packages/client/foreman_ygg_migration/foreman_ygg_migration.spec

+	fi
+
+	systemctl disable --now yggdrasild
+	systemctl enable  --now yggdrasil


It may need to restart yggdrasil if it's already running.

That would mean that someone installed new-enough yggdrasil, started it and then installed the migration package, correct?

Yes, though I also wonder if that use case would be one to error out on. And you really shouldn't error out on RPM scriptlets. Still not entirely sure what's best here and currently my mind is exploring the various paths a user could take.

adamruzicka · 2024-10-15T14:12:41Z

This is because today we ship packages/client/yggdrasil but future EL versions will ship this instead. Those future EL versions are 0.4 which has a different architecture, but most importantly a different service name.

Correct

I'm worried that if systemctl is-enabled yggdrasild could be unreliable to detect this. I don't know exactly what the ordering will be, but perhaps the old service file is already cleaned up and systemd doesn't know about it anymore.

I tested it and it seemed to work, probably because of us not being diligent enough about service handling in the specfile of yggdrasil we ship.

ekohl · 2024-10-15T14:51:44Z

I tested it and it seemed to work, probably because of us not being diligent enough about service handling in the specfile of yggdrasil we ship.

IIRC with EL9 there were some serious changes how systemd RPM packaging worked. The operations are now much more batched than on EL8 with the use /usr/lib/systemd/systemd-update-helper. Perhaps we're benefiting from that?

adamruzicka · 2024-10-21T11:22:59Z

[test rpm-copr]

pondrejk

ack from the functional side, tested together with theforeman/katello-pull-transport-migrate#8

evgeni · 2024-10-30T11:43:16Z

packages/client/foreman_ygg_migration/foreman_ygg_migration.spec

+License: MIT
+
+%if 0%{?rhel} >= 8
+Supplements: yggdrasil >= 0.4.0


People can (and do 😢) disable weak dependencies.
For such setups it will mean that this package won't be automatically installed once they upgrade to 0.4.

This is not a blocker (and there is no better package relationship to fix this), but something we should keep in mind and document somewhere.

What exactly should we document and where?

We should document that if people have weak dependencies disabled, they should install this package explicitly. As to where, I'm not sure if we already have a procedure or something where this would fit well or we'll need to write a new one

Yeah, I am not aware of a good place for this (maybe at least release notes?)

evgeni · 2024-10-30T11:45:48Z

packages/client/foreman_ygg_migration/foreman_ygg_migration.spec

@@ -0,0 +1,34 @@
+Name: foreman_ygg_migration


I am debating with myself whether this needs to be an own package, vs a sub-package of foreman_ygg_worker (smth like foreman_ygg_worker_migration or so).

That would be cleaner, but at the same time it is not a migration of foreman_ygg_worker

This package should get pulled in when yggdrasil >=0.4.z is installed. When installed, it checks whether yggdrasild.service is enabled. If not, it does nothing. If enabled, this package tweaks yggdrasil's config file to account for the newly needed config options and migrates service overrides from the old service to the new worker. Additionally, it disables the yggdrasild service and enables yggdrasil.service. Our worker is left to be activated through dbus.

We don't expect yggdrasil >= 0.4.0 to land there

in an attempt to pass repoclosure

pr-processor bot added Not yet reviewed RPM labels Oct 4, 2024

adamruzicka force-pushed the rpm/develop-ygg-migration branch 2 times, most recently from d9b72c8 to 7eb8bf7 Compare October 4, 2024 12:59

adamruzicka mentioned this pull request Oct 14, 2024

Update foreman_ygg_worker to 0.3.0 #11296

Open

adamruzicka force-pushed the rpm/develop-ygg-migration branch from 4fdcd1d to 2d765be Compare October 15, 2024 12:58

ekohl reviewed Oct 15, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 15, 2024

pondrejk mentioned this pull request Oct 16, 2024

testing foreman_ygg_worker, etc. SatelliteQE/robottelo#16690

Draft

pondrejk approved these changes Oct 24, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

adamruzicka force-pushed the rpm/develop-ygg-migration branch from 84698c4 to 7be2074 Compare October 24, 2024 14:41

evgeni reviewed Oct 30, 2024

View reviewed changes

adamruzicka marked this pull request as ready for review November 12, 2024 12:17

adamruzicka requested a review from a team as a code owner November 12, 2024 12:17

adamruzicka added 3 commits November 19, 2024 04:54

Do not build foreman_ygg_migration on RHEL 7

a875af2

We don't expect yggdrasil >= 0.4.0 to land there

Conflict with older yggdrasil

bc6a38a

in an attempt to pass repoclosure

adamruzicka force-pushed the rpm/develop-ygg-migration branch from 7be2074 to bc6a38a Compare November 19, 2024 09:55

evgeni approved these changes Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add foreman_ygg_migration package #11317

Add foreman_ygg_migration package #11317

adamruzicka commented Oct 4, 2024

adamruzicka commented Oct 15, 2024

ekohl left a comment

ekohl Oct 15, 2024

ekohl Oct 15, 2024

adamruzicka Oct 15, 2024

ekohl Oct 15, 2024

adamruzicka Oct 15, 2024

ekohl Oct 15, 2024

ekohl Oct 15, 2024

adamruzicka Oct 15, 2024

ekohl Oct 15, 2024

adamruzicka commented Oct 15, 2024

ekohl commented Oct 15, 2024

adamruzicka commented Oct 21, 2024

pondrejk left a comment

This comment was marked as duplicate.

evgeni Oct 30, 2024

ehelms Nov 20, 2024

adamruzicka Nov 20, 2024

evgeni Nov 26, 2024

evgeni Oct 30, 2024

adamruzicka Nov 12, 2024

Add foreman_ygg_migration package #11317

Are you sure you want to change the base?

Add foreman_ygg_migration package #11317

Conversation

adamruzicka commented Oct 4, 2024

adamruzicka commented Oct 15, 2024

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented Oct 15, 2024

ekohl commented Oct 15, 2024

adamruzicka commented Oct 21, 2024

pondrejk left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment