[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sstosh
Once this PR has been reviewed and has the lgtm label, please assign giuseppe for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

I don't see how this is a breaking change right now? It all depends on how it would be implemented in podman.
However right now this is missing the active_service field as this will also be written by podman system connection.

Arguably writing to containers.conf.d/connections.conf would be a breaking change as users might already have such file created themselves.

But I totally I agree with @vrothberg this needs a proper design first with a migration part. System connections are a fundamental part of podman machine so we should not cause any regressions.

Lets set up a google doc to design this out.

@Luap99 @vrothberg could one of you take the lead on this. I think this is fundamentally a good idea, I believe we should break this out, and stop rewriting containers.conf. The name should be made to be less likely to hit

__connections.conf or something like that. We need to remove the content from containers.conf man page or at least tell users to look in the connections.conf file. The buildfarm tool should use the new format as well. The output should mention that this comments in this file would not survive machine editing.

I think we setup tooling to read from existing containers.conf files and write __connections.conf file if the __connections.conf file does not exist. With the way containers.conf works this should not be a big deal, other then users changes to connections data in continers.conf would no longer be used if the __connections.conf file exists.

I believe we need to make this change before podman 4.7.

We need to write down exactly what we need first.

There are other problems besides just rewriting user files. Assume we have containers.conf.d/foo.conf which sets certain connections. A podman system connection rm will not remove them as it's always looking at one specific path. It's very broken and we'll likely introduce regressions, so we may very well use this opportunity to move this stuff entirely out of containers.conf into another DB that users cannot touch (plus migration off containers.conf?).

I believe we need to make this change before podman 4.7.

I don't think we need to. We've been living with this behavior for a long time and we have set priorities for Q3. I wished we never introduced this behavior but I don't think it should block buildfarm. I really don't want to spend time on it as I strongly opposed abusing containers.conf as a DB in the first place.

As long as we guarantee __connections.conf is read after foo.conf, then we are fine. Once __connections.conf exists, it is the only conf for connections to be used.

As long as we guarantee __connections.conf is read after foo.conf, then we are fine. Once __connections.conf exists, it is the only conf for connections to be used.

It depends on which problems we want to solve, see my other comment:

There are other problems besides just rewriting user files. Assume we have containers.conf.d/foo.conf which sets certain connections. A podman system connection rm will not remove them as it's always looking at one specific path. It's very broken and we'll likely introduce regressions, so we may very well use this opportunity to move this stuff entirely out of containers.conf into another DB that users cannot touch (plus migration off containers.conf?).

We have the issue you describe now. If a user creates a foo.conf file and we do a removal of connections, the foo.conf file will not be touched even if the connection within it is removed.

By moving to a __connnections.conf file that is always processed last we eliminate that issue.

PR needs rebase.

This is going to be worked on in a different PR, closing.