Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to implicitly rely on the default FCOS release schedule meaning we'll have restarted before we hit this timeout by default. Now, zincati is pretty FCOS specific but still, this seems...slightly unclean at best.
I think the most obvious bigger fix here is to not run as a persistent daemon at all, but to run as a timer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That said, I'm OK with this...but it does seem probably better done via a drop-in in fedora-coreos-config instead of hardcoded in source here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chosen period is FCOS-informed, but isn't tightly coupled at all. Whether for a given environment updates come in faster or slower than that, it shouldn't hurt to restart the service. I suggested making it longer so that at least in the context of FCOS (by far the most prevalent, if not only, use of Zincati), it's more of a last resort than a primary line of defense. But yes, also fine with having it be a dropin override in the FCOS overlays.
Yes, @travier suggested this as well, but it requires a larger rework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had originally suggested 2w but @jlebon suggested 3w. TBH it could be 1 day, though I'm not sure how well that would work with our periodic update strategy that people can set.
I'd be OK setting this in FCOS configs if we think it would be better placed there. Theoretically if zincati had other consumers this would be useful to them too, though those users might prefer a different cadence for restart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it will be racy with DBus today.
But my larger (yet still minor) concern is this restart will appear as an error, when it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I actually was unaware that Zincati had a D-Bus interface until debugging #1119 (comment) today. It still seems pretty hidden currently at least, but indeed if we were to develop it more down the line as part of #498, having it be seemingly randomly force restarted would be odd and we'd probably want to rework/drop this directive.