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

batou_ext.oci: Add support for podman #204

Merged
merged 2 commits into from
Feb 21, 2025
Merged

batou_ext.oci: Add support for podman #204

merged 2 commits into from
Feb 21, 2025

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 29, 2024

FC-37959

We mostly want this for healthchecks that must pass before the unit is actually active.

@Ma27 Ma27 force-pushed the FC-37959-podman branch 2 times, most recently from db0e430 to 3476623 Compare December 30, 2024 14:30
@Ma27 Ma27 marked this pull request as ready for review December 30, 2024 14:34
@Ma27 Ma27 requested a review from PhilTaken December 30, 2024 14:35
@PhilTaken
Copy link
Member

Considering how considerable the differences between podman and docker are I'd almost think about splitting it up into two Components 🤔 that might however introduce a bunch of copy-pasted code. This would also eliminate the need to specifiy the backend for every single container.

@Ma27
Copy link
Member Author

Ma27 commented Jan 9, 2025

Given that

I'd argue that it would be fine to keep it as-is.

This would also eliminate the need to specifiy the backend for every single container.

Hmm... I'd have to check whether the current approach allows podman & docker in parallel in the first place now that I think of it. You set the backend globally in NixOS.
So perhaps we need to structure that part in a different way anyways.

@Ma27
Copy link
Member Author

Ma27 commented Jan 17, 2025

Hmm... I'd have to check whether the current approach allows podman & docker in parallel in the first place now that I think of it. You set the backend globally in NixOS.

Yeah, backend is set globally, so doing it per-container here doesn't make sense.

@Ma27
Copy link
Member Author

Ma27 commented Jan 17, 2025

@PhilTaken the backend setting is now done via another component.

@PhilTaken
Copy link
Member

I like the podmanruntime component here, that simplifies usage a bunch. I still think theres almost too much complexity in the ocicontainer component here but I cannot think of a good solution.

Did you test this with a reasonably complex deployment?

FC-37959

We mostly want this for healthchecks that must pass before the unit is
actually active.

This is kept intentionally simple, only healthchecks via sd_notify are
supported. Mixing containers in systemd units with `healthy` and `conmon`
strategy leads to setups with conflicting requirements (linger vs no
linger) and it's far too easy to end up with an unsupported
configuration (and containers not behaving properly).
@@ -108,6 +172,12 @@ class Container(Component):
}

def configure(self):
self.backend = (
"podman"
if self.require("oci:podman", strict=False, host=self.host)
Copy link
Member

@frlan frlan Feb 19, 2025

Choose a reason for hiding this comment

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

Wondering whether we want to make it this implicit.
This might cause a docker assuming container being executed by podman.

Not considering it a blocker though

@frlan frlan merged commit 0aa39f4 into master Feb 21, 2025
6 checks passed
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