-
Notifications
You must be signed in to change notification settings - Fork 442
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
[collector] provide support for windows #792
Conversation
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.
I'm concerned that we don't have a good way to test this setting since all our CI runs on linux. Were you able to test locally?
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.
I'm concerned that we don't have a good way to test this setting since all our CI runs on linux. Were you able to test locally?
{{- if not .Values.isWindows }} | ||
root_path: /hostfs | ||
{{- end }} |
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.
This is still needed for windows, just under different path, right?
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.
When I tried to set path I got this error -> https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/4697200b4ea01a96f5d737d6836dc5ee4b0a6a7b/receiver/hostmetricsreceiver/hostmetrics_others.go#L14
Not sure how it works on windows, but it seems we don’t
I am testing this in Amazon EKS with Windows nodes together with Linux ones :) Colleague is also looking to contribute the Docker images in open-telemetry/opentelemetry-collector-releases#339 Really valid concerns. Not sure how feasible is this to do some testing with ci as might be complicated to spin up windows. But can look into this. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
I've looked at ability to run windows in CI and couldn't find anything that runs out of the box. Maybe this tool could be adapated https://github.com/kubernetes-sigs/sig-windows-dev-tools but it would probably quite a lot of effort. So closing for now. |
Allow users to set
isWindows
flag to true, which will configure mounts and various presets to use windows paths.This change will also add a new node selector for all existing users:
AFAIK this shouldn't be breaking change?
Added example with windows: