-
Notifications
You must be signed in to change notification settings - Fork 52
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
Use podman machine marker instead deprecated enabled machine config #676
Use podman machine marker instead deprecated enabled machine config #676
Conversation
With containers/common@0db57c8 `machine_enabled=true/false` setting in the containers.conf is deprecated and now there is `podman-machine` marker file, which is used for this setting. - https://github.com/containers/common/blob/main/pkg/machine/machine.go#L21
/hold need to test the bundle. |
[engine] | ||
machine_enabled=true | ||
EOF | ||
touch /etc/containers/podman-machine |
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.
It's expected to contain qemu
(possible values are qemu
or wsl
)
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.
@cfergeau no that is not required, only having this marker file is enough qemu
and wsl
only require to run gvisor stack or not, for us we want to run gvisor stack for both mac/windows.
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.
Hmm the implementation supports empty files indeed, though podman-machine does not use an empty file: https://github.com/containers/podman/blob/3e44a7afed7a5751ef700729f97b9f6e955fcc5e/pkg/machine/ignition.go#L421-L435
Probably best to stay as close as possible to podman-machine?
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.
podman-machine does not use an empty file
@cfergeau it is not using empty file because for them they have different implementation for qemu and wsl side, for us we don't have anything different where we dont' want to run the gvisor vsock. So machineEnabled
as of now just this maker file. In future if we need different implementation then we might need to add the respective provider info.
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.
In future if we need different implementation then we might need to add the respective provider info.
We are not the ones parsing this file, the file is something intended for podman use. And in the current podman code, I don't think podman ever creates an empty podman-machine
file, hence my suggestion to stay as close as possible to podman.
Sure, nothing breaks now because podman is always checking against the wsl
value. Hopefully they'll never start testing for qemu
in other code.
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.
Having qemu
in the marker file is wrong for us because we are not even using qemu
for mac/windows. What we want is just letting podman know if it have machine enabled or not and having this file without any content make more sense to me to replicate what is is deprecated now.
Support for this landed in podman 4.2.0 through podman commit e247f02a4f |
@praveenkumar: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed 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.
Not going to fight for qemu
, even if the PR could be more future-proof.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau 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 |
/cherry-pick release-4.13 |
@praveenkumar: once the present PR merges, I will cherry-pick it on top of release-4.13 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@praveenkumar: new pull request created: #680 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
With crc-org/snc#676, we updated the marker file for podman-machine. This PR is to make sure the marker file is removed in case of systemd mode networking.
With crc-org/snc#676, we updated the marker file for podman-machine. This PR is to make sure the marker file is removed in case of systemd mode networking.
With containers/common@0db57c8
machine_enabled=true/false
setting in the containers.conf is deprecated and now there ispodman-machine
marker file, which is used for this setting.