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.

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

Improved Windows compatibility for machine command #15404

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

arixmkii
Copy link
Contributor

@arixmkii arixmkii commented Aug 22, 2022

Extracting more changes done during experiment in #13006 and continuation of #15372 (split into independent PRs as the first one is already reviewed and they are pretty independent).

It was checked to compile on MacOS and Windows and as it is purely refactoring, no behavior changes are expected.

List of changes

Fixing command line genarated

The only non platform specific change is

append(cmd, []string{"-qmp", monitor.Network + ":" + monitor.Address.GetPath() + ",server=on,wait=off"}...)

This is a correction, because Qemu is using unix: (also tcp: for TCP) not unix:/ to set the protocol. It was working before, because address is absolute and additional leading / is harmless).

Windows aware runtime directory

On windows the only option is to use either user specific or system wide AppData directories

Extracting unix specific parts from machine.go

Parts, which only can compile on unix are moved to _unix version and _windows alternatives (taking into account some limitations, because of lack of signaling) are provided.

Does this PR introduce a user-facing change?

None

[NO NEW TESTS NEEDED]

@arixmkii arixmkii force-pushed the win_compat2 branch 4 times, most recently from 0a17ada to 32f787a Compare August 22, 2022 10:15
@arixmkii
Copy link
Contributor Author

This doesn't enable Qemu for windows. So, no new functionality added and refactorings should be covered by existent tests, thus no tests provided for now.

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2022

@arixmkii Builds are failing, I added the [NO NEW TESTS NEEDED] flag.

@mheon
Copy link
Member

mheon commented Aug 22, 2022

/approve
@n1hility PTAL

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arixmkii, mheon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022
@n1hility
Copy link
Member

I have some notes that I will paste shortly, and will post some thoughts on #13006

pkg/machine/qemu/machine_windows.go Outdated Show resolved Hide resolved
pkg/machine/qemu/machine_windows.go Outdated Show resolved Hide resolved
@arixmkii arixmkii marked this pull request as draft August 23, 2022 10:19
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022
@arixmkii arixmkii marked this pull request as ready for review August 23, 2022 10:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022
@rhatdan rhatdan added the windows issue/bug on Windows label Aug 24, 2022
@arixmkii arixmkii requested a review from n1hility August 25, 2022 11:53
Copy link
Member

@n1hility n1hility left a comment

Choose a reason for hiding this comment

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

Thanks for the update to the PR! I have one more follow-up.

pkg/machine/qemu/machine_windows.go Outdated Show resolved Hide resolved
@n1hility
Copy link
Member

Also looks like some lingering compile issues

@arixmkii arixmkii force-pushed the win_compat2 branch 3 times, most recently from 2a3d6c3 to f24f6fc Compare August 29, 2022 13:11
@arixmkii
Copy link
Contributor Author

I believe a single failed test for rootless ubuntu could be just flaky one or a glitch.

@arixmkii arixmkii requested a review from n1hility August 29, 2022 15:53
@n1hility
Copy link
Member

LGTM

I'll rerun the flake

@arixmkii
Copy link
Contributor Author

arixmkii commented Sep 2, 2022

@n1hility do you expect some more work with this one (like me rebasing it, etc)? It says it still is waiting for your approval.

@rhatdan
Copy link
Member

rhatdan commented Sep 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022
@rhatdan
Copy link
Member

rhatdan commented Sep 2, 2022

Thanks @arixmkii

@openshift-merge-robot openshift-merge-robot merged commit 4cbc477 into containers:main Sep 2, 2022
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
@arixmkii arixmkii deleted the win_compat2 branch March 1, 2024 13:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none windows issue/bug on Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants