-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[WIP] Support remote podman #2235
[WIP] Support remote podman #2235
Conversation
potential fix for white space in source path
make kindnetd tolerate everything
don't allow _ in cluster names
Gather info about the provider using `podman info` instead of looking on the host directly. This allows starting a cluster when the host is remote, for instance running in a VM on a MacOS or Windows host. Not all the info available via `docker info` is available in `podman info`, so logging a warning when using podman. Signed-off-by: Andrea Frittoli <[email protected]>
Welcome @afrittoli! |
Hi @afrittoli. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: afrittoli The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
info := &providers.ProviderInfo{ | ||
Rootless: euid != 0, | ||
} | ||
if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil { |
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.
and .... do we stop handling the local podman case?
is there a way to know if podman is working in remote or local mode? |
info := &providers.ProviderInfo{ | ||
Rootless: euid != 0, | ||
} | ||
if _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil { |
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.
maybe, we just need to skip this check if we can detect is a remote podman
if !podman_remote() && _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers"); err == nil {
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.
let me summon the expert,
@vrothberg can I know from the podman cli if is running remote or local?
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 yet but I opened containers/podman#10289. @baude, do you know a workaround?
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.
@afrittoli can we try with this approach?
containers/podman#10289 (comment)
something like
// podman_remote returns true if podman is using a remote client
// xref: https://github.com/containers/podman/issues/10289
func podman_remote() bool{
... check https://github.com/containers/podman/issues/10289#issuecomment-838302394 ...
}
This change needs to be compatible with older podman versions. I suspect instead of trying to detect if podman is remote or not it needs to check the podman version anyhow before selecting the format string to request, since go text templates explode on unknown fields. |
?
|
I mean the fields available to the format string depend on the podman version. So we should just read the podman version and if it's new enough we request the new fields and if not we collect the other fields from podman and the rest from reading local as we already support? You don't need to check if it's local or not, either it's new enough to get this info for remote and we do that, or it's too old and we should still get the info for local. That's purely based on version. |
If we want to support older versions of podman we do indeed check the version. In terms of local vs. remote, the To sum up, based on the various comments, my proposal would be to:
Does this make sense? |
podman info will provide the "remote" info, it has merged in master containers/podman#10300 I honestly will not try to support older podman versions, we have too many |
@afrittoli: PR needs rebase. 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 would suggest a slightly more specific order:
@aojea I think time has shown that we will need to switch on the version anyhow as we cannot depend on format structs to be stable. We already do this for e.g. port mapping. |
THIS ^^^
sounds great |
Does anyone know if this general approach is still required? I think podman has made some improvements here 🤞 |
I tried recently kind with podman on an M1 Mac and it all worked well, so this can be closed now. |
not at all, thank you! |
Gather info about the provider using
podman info
instead oflooking on the host directly. This allows starting a cluster
when the host is remote, for instance running in a VM on
a MacOS or Windows host.
Not all the info available via
docker info
is available inpodman info
, so logging a warning when using podman.Signed-off-by: Andrea Frittoli [email protected]