Quadlet - allow deleting the network when stopping the service #25844
Conversation
openshift-ci
bot
added
release-note
approved
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
ygalblum
force-pushed
the
quadlet-network-delete
branch
2 times, most recently
from
bf391d8 to
a343554
Compare
pkg/systemd/quadlet/quadlet.go
Outdated
| KeyIPv6: true, | ||
| KeyInternal: true, | ||
| KeyNetworkName: true, | ||
| KeyNetworkDeleteWhenStop: true, |
There was a problem hiding this comment.
Would NetworkDeleteOnStop sound better?
There was a problem hiding this comment.
Also I suppose since this is already part of the network section we might also be able to get rid of the Network prefix?
There was a problem hiding this comment.
Sorry, I missed the second comment. The reason I kept Network is so that it won't be used in other unit types
ygalblum
force-pushed
the
quadlet-network-delete
branch
from
a343554 to
b16d5a9
Compare
pkg/systemd/quadlet/quadlet.go
Outdated
| @@ -940,6 +942,12 @@ func ConvertNetwork(network *parser.UnitFile, name string, unitsInfoMap map[stri | |||
| // Need the containers filesystem mounted to start podman | |||
| service.Add(UnitGroup, "RequiresMountsFor", "%t/containers") | |||
|
|
|||
| if network.LookupBooleanWithDefault(NetworkGroup, KeyNetworkDeleteOnStop, false) { | |||
| serviceStopPostCmd := createBasePodmanCommand(network, NetworkGroup) | |||
| serviceStopPostCmd.add("network", "rm", "--force", networkName) | |||
There was a problem hiding this comment.
Sorry should have checked this earlier, do we really want force in this case? Per systemd ordering it will stopped after container are stopped but not when one runs systemctl stop manually on the network right?
It could be very unexpected if the removing the unit causes podman to force remove all container that are on the network.
Having the command error in case there still are other containers running might be a good thing.
There was a problem hiding this comment.
I tested this scenario: #23678 (comment)
From what I could tell, stopping the network unit will first stop the dependent container units and only then stop the network. So, the force flag is expected to be a noop as all containers are supposed to stop before the network is deleted.
Having said that, it will remove containers that were added to this network without a proper connection in a Quadlet unit.
There was a problem hiding this comment.
Ack, thanks. So then I think it is even better to not add --force, anything else would mean we have a outside container on this network which is not managed by quadlet so I would prefer to not force remove that.
Returning an error in this case seems like the best option.
There was a problem hiding this comment.
Concur, force-removal seems too aggressive - likely to cause some collateral damage.
There was a problem hiding this comment.
OK, I removed the --force flag and also enhanced the test to make sure dependency works
|
LGTM after all comments addressed |
Signed-off-by: Ygal Blum <[email protected]>
ygalblum
force-pushed
the
quadlet-network-delete
branch
from
b16d5a9 to
0d4a148
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@baude I made some small changes since your review |
|
/lgtm |
Does this PR introduce a user-facing change?
Yes
Resolves: #23678