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.

Sign up for GitHub

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

Jump to bottom

Some changes to calls to Docker API for podman compatibility #463

Merged
merged 10 commits into from
Sep 27, 2022
Merged

Some changes to calls to Docker API for podman compatibility #463

merged 10 commits into from
Sep 27, 2022

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Sep 15, 2022 •
edited
Loading

  • Podman expects the network name specified when launching the container to match the name used when creating it even when the network ID is specified, the API spec is unclear about that
  • Adding labels when committing a container needs to use changes query parameter instead of a POST json, which is unspecified
  • Add a config to specify the hostname where Complement on the host can be reached when inside a container. Podman uses host.containers.internal

@MatMaul MatMaul requested review from a team as code owners September 15, 2022 09:57
@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 15, 2022 •
edited
Loading

With that and this change synapse side I can run complement with podman rootless using:

  • systemctl --user start podman.service to start the rootless API daemon (can also be enabled)
  • DOCKER_HOST=unix://$XDG_RUNTIME_DIR/podman/podman.sock BUILDAH_FORMAT=docker COMPLEMENT_HOSTNAME_RUNNING_COMPLEMENT=host.containers.internal ./scripts-dev/complement.sh
    Docker image format is needed because OCI format doesn't support the HEALTHCHECK directive unfortunately.

@MatMaul MatMaul force-pushed the mv/fix_podman branch 2 times, most recently from 9ea2e32 to c5a43c0 Compare September 15, 2022 16:09
reivilibre
reivilibre approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

internal/docker/builder.go Outdated Show resolved Hide resolved
@kegsay kegsay self-assigned this Sep 17, 2022
@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 19, 2022

It seems like the hack here was only needed because of the fact that the network name was not the one used when launching the container.

kegsay
kegsay requested changes Sep 20, 2022
View reviewed changes
Copy link
Member

@kegsay kegsay left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In theory LGTM, but I have outstanding questions on why seemingly unrelated things were modified. The most dangerous is the blunt removal of network disconnection code, which the comments explained why it existed, and yet this PR just removes without any reason.

EDIT: I see the reasoning now in your above comment.

internal/docker/builder.go Outdated Show resolved Hide resolved
internal/docker/builder.go Outdated Show resolved Hide resolved
internal/docker/deployer.go Show resolved Hide resolved
internal/docker/deployer.go Outdated
@@ -283,7 +275,7 @@ func deployImage(
Mounts: mounts,
}, &network.NetworkingConfig{
EndpointsConfig: map[string]*network.EndpointSettings{
contextStr: {
"complement_" + pkgNamespace + "_" + blueprintName: {
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we using the contextStr anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be the name of the network, which is not the case here.

We could also use the contextStr as the name of the network in createNetworkIfNotExists, or makes createNetworkIfNotExists returns both the ID and the network name and store both in Deployer.

The latter looks like the cleaner way to do it so leaning towards that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it seems like we can just use the name, ID is optional, at least with Podman.
I've push a change to return and use the network name instead of the ID, let's see if Docker is happy.

internal/docker/labels.go Outdated Show resolved Hide resolved
@MatMaul MatMaul requested a review from kegsay September 20, 2022 13:51
@MatMaul MatMaul mentioned this pull request Sep 21, 2022
Merged
@MatMaul MatMaul changed the base branch from main to mv/raise-synapse-timeout September 21, 2022 12:11
@MatMaul MatMaul changed the base branch from mv/raise-synapse-timeout to main September 21, 2022 12:11
Mathieu Velten added 9 commits September 23, 2022 00:31
@MatMaul
Some changes to calls to Docker API for podman compatibility
ed54884 
- Podman expects the network name specified when launching the container
to match the name used when creating it, the spec is unclear about that
- Adding labels when commiting a container needs to use "changes" query
parameter instead of a POST json, which is unspecified
- Add a config to specify the hostname where Complement on the host can
be reached when inside a container. Podman uses "host.containers.internal"
@MatMaul
Use quotes for labels in changes
8a07063
@MatMaul
Remove debug + makes AS labels on one line
9542f6c
@MatMaul
Remove hack, not needed anymore
107c3bc
@MatMaul
a bit too far...
23bce51
@MatMaul
Add doc
0162397
@MatMaul
Convert from labels to changes
a60d5cc
@MatMaul
Replace networkID by networkName
eb2bc15
@MatMaul
Add/fix some comments
9e88e44
kegsay
kegsay approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

internal/config/config.go Show resolved Hide resolved
internal/docker/builder.go Outdated Show resolved Hide resolved
internal/docker/builder.go Show resolved Hide resolved
@MatMaul
Copy link
Contributor Author

MatMaul commented Sep 26, 2022

@kegsay thanks for the review, I'll let you do a final round and merge. My Fedora is happy :) .

@MatMaul MatMaul requested a review from kegsay September 26, 2022 13:20
@kegsay kegsay merged commit 8e341d5 into main Sep 27, 2022
@kegsay kegsay deleted the mv/fix_podman branch September 27, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reivilibre reivilibre reivilibre approved these changes

@kegsay kegsay kegsay approved these changes

Assignees

@MatMaul MatMaul

@kegsay kegsay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MatMaul @kegsay @reivilibre