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

add CLHM helper to warn about modularity on F39 systems #2510

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

dustymabe
Copy link
Member

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

It's notable that there's nothing CoreOS specific about this actually. Having a warning on the console won't be very visible to desktop users e.g.

One minor downside is that now every single boot for all CoreOS systems, including those not using modularity (client side) will have to spend CPU cycles on this.

Combining these two things: the check would perhaps be better implemented in rpm-ostree; we could write out a journal message when we detect this with a dedicated MESSAGE_ID, and then clients (such as CLHM but also gnome-software etc.) could scrape it if present. In that case we only do the check if rpm-ostree is already being invoked, note.

Anyways, I'm OK with this.

@dustymabe
Copy link
Member Author

dustymabe commented Jul 17, 2023

It's notable that there's nothing CoreOS specific about this actually. Having a warning on the console won't be very visible to desktop users e.g.

True, though non-CoreOS users (I think) typically upgrade interactively and thus can evaluate problems at the time of attempted upgrade, which makes the warning less necessary IMO.

One minor downside is that now every single boot for all CoreOS systems, including those not using modularity (client side) will have to spend CPU cycles on this.

Indeed. The way I've implemented this PR means it only gets applied until we switch to F39, which means it's timeboxed.

Combining these two things: the check would perhaps be better implemented in rpm-ostree; we could write out a journal message when we detect this with a dedicated MESSAGE_ID, and then clients (such as CLHM but also gnome-software etc.) could scrape it if present. In that case we only do the check if rpm-ostree is already being invoked, note.

Yeah. I guess this cross cuts with us maybe removing modularity support from rpm-ostree too. For this I was trying not to require work being done in other components since it was easy enough for us to just check with a single CLI call.

Anyways, I'm OK with this.

+1 - I'm happy for this to happen at another level if a better solution emerges.

@travier
Copy link
Member

travier commented Jul 18, 2023

Some questions I had and thoughts intermixed:

  • Should we include that in https://github.com/coreos/console-login-helper-messages directly?

    • Given that it's only until F39 lands and won't be needed after, here should be fine and easier to do.
    • We should maybe make sure to order the unit properly given the CLHM constraints?
  • Could we make that only run once?

    • Agree that it's not great as this will run on every boot as rpm-ostree status does a fair amount of I/O.
    • If we make it run only once, then users that start using modules after the run and before F39 won't be notified.
    • I could not find a better solution. Not sure if we could find the information from the RPMDB or if that would effectively be less I/O expansive.

So overall, I'm inclined to say that this should be OK as it's limited in time and we probably don't want to invest development time in rpm-ostree for that given that we'll potentially remove all the code for modules support once F39 comes.

Maybe if we send a message about the modularity support drop on coreos-status we can also include a note for non-modularity users to mask this unit to avoid taking the I/O hit?

F39 release is in october so about 3 months away.

@dustymabe
Copy link
Member Author

Some questions I had and thoughts intermixed:

* Should we include that in [coreos/console-login-helper-messages](https://github.com/coreos/console-login-helper-messages) directly?
  
  * Given that it's only until F39 lands and won't be needed after, here should be fine and easier to do.
  * We should maybe make sure to order the unit properly given the CLHM constraints?

I don't know of other OSTree variants that use CLHM. Does IoT? I don't Silverblue does.

The unit works now I just copied coreos-check-cgroups example that's already in our code base.

* Could we make that only run once?
  
  * Agree that it's not great as this will run on every boot as `rpm-ostree status` does a fair amount of I/O.
  * If we make it run only once, then users that start using modules after the run and before F39 won't be notified.
  * I could not find a better solution. Not sure if we could find the information from the RPMDB or if that would effectively be less I/O expansive.

The unit currently writes to /run. If we "cache" the result then it could be wrong if either the user later layers a module OR the user removes the modular layer. I don't think the added complexity would be worth the benefit that would be gained.

The warning that is printed today instructs the user how to disable the warning from being displayed again.

So overall, I'm inclined to say that this should be OK as it's limited in time and we probably don't want to invest development time in rpm-ostree for that given that we'll potentially remove all the code for modules support once F39 comes.

Maybe if we send a message about the modularity support drop on coreos-status we can also include a note for non-modularity users to mask this unit to avoid taking the I/O hit?

That seems reasonable. Though really, it's one unit on boot. It doesn't run more than once a boot. Is rpm-ostree status really that intensive? Maybe we should try to make it less intensive?

F39 release is in october so about 3 months away.

@travier
Copy link
Member

travier commented Jul 19, 2023

I don't know of other OSTree variants that use CLHM. Does IoT? I don't Silverblue does.

I think we're the only ostree variant using it. IoT does not: https://pagure.io/fedora-iot/ostree/blob/main/f/fedora-iot-base.json

The unit works now I just copied coreos-check-cgroups example that's already in our code base.

👍🏻

@dustymabe dustymabe merged commit 62eb778 into coreos:testing-devel Jul 19, 2023
1 check passed
@dustymabe dustymabe deleted the dusty-clhm-modularity branch July 19, 2023 16:39
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