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

Fix SmartOS build #1017 #1018

Merged
merged 1 commit into from Aug 23, 2018
Merged

Fix SmartOS build #1017 #1018

merged 1 commit into from Aug 23, 2018

Conversation

dfredell
Copy link
Contributor

@dfredell dfredell commented Jul 23, 2018

This is a fix for issue #1017
@discordianfish You reviewed my previous SmartOS PR #762 Can you take a look again?

I apologize if I created a pile of notifications trying to get my commit username correct.

Signed-off-by: Dan Fredell <Dan.Fredell@gmail.com>
@discordianfish
Copy link
Member

I have actually 0 experience with smartos and last time I touched solaris was 10 years ago.. Are you sure this doesn't break other solarises? I don't know why these lines were added before.

@dfredell
Copy link
Contributor Author

I'm not sure if it breaks others, it fixes it for me though. I can now run the exporter and get the metrics.

[root@smartos-fun ~]# go/bin/node_exporter 
INFO[0000] Starting node_exporter (version=, branch=, revision=)  source="node_exporter.go:82"
INFO[0000] Build context (go=go1.10, user=, date=)       source="node_exporter.go:83"
INFO[0000] Enabled collectors:                           source="node_exporter.go:90"
INFO[0000]  - loadavg                                    source="node_exporter.go:97"
INFO[0000]  - textfile                                   source="node_exporter.go:97"
INFO[0000]  - time                                       source="node_exporter.go:97"
INFO[0000] Listening on :9100                            source="node_exporter.go:111"
[root@smartos-fun ~]# bg
[1]+ go/bin/node_exporter &
[root@smartos-fun ~]# curl localhost:9100/metrics
# HELP go_gc_duration_seconds A summary of the GC invocation durations.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 0
go_gc_duration_seconds{quantile="0.25"} 0
...
# HELP node_load1 1m load average.
# TYPE node_load1 gauge
node_load1 0.37890625
# HELP node_load15 15m load average.
# TYPE node_load15 gauge
node_load15 0.4453125
# HELP node_load5 5m load average.
# TYPE node_load5 gauge
node_load5 0.46875

My little research points to golang/go#23749 which is a security change in golang the prevents unknown gcc flags.

@discordianfish
Copy link
Member

@kpettijohn You've added this collector. Can you confirm that removing this doesn't break other solarises?

@mamash
Copy link

mamash commented Aug 22, 2018

These cgo flags might have been required for a specific GCC compiler provided with a specific SunOS platform. They should not be hardcoded here, because A) -fno-stack-protector is a compiler flag and not on the linker flag whitelist in golang (recent security measures made to tackle golang/go#23672); and B) only compilers should set these if requested at compile time, as SunOS platforms, compilers and user security preferences may differ.

@kpettijohn
Copy link
Contributor

@mamash thanks for the additional information! When making the original PR I was using SmartOS (joyent_20150820T062742Z) and it "seemed" required to add the flags for a successful build.

@discordianfish sorry for not jumping in sooner but based on @mamash comments it sounds like dropping the -fno-stack-protector is the correct action and might actually help support more SunOS platforms. Unfortunately I no longer have an environment for testing these changes.

Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

Ok, thanks for the feedback. That sounds reasonable, so LGTM.

@SuperQ wdyt?

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@discordianfish discordianfish merged commit c52e0d3 into prometheus:master Aug 23, 2018
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
Signed-off-by: Dan Fredell <Dan.Fredell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants