Add hostfs option to Host and Process functions, fix OperatingSystem()
#226
Conversation
fearful-symmetry
changed the title
hostfs option to Host and Process functions, add HostFS functionality to OperatingSystem()hostfs option to Host and Process functions, fix OperatingSystem()
system.go
Outdated
| // in the container: | ||
| // | ||
| // - /proc:/hostfs/proc | ||
| // - /sys/fs/cgroup:/hostfs/sys/fs/cgroup |
There was a problem hiding this comment.
Considering all things that are "HostFS" aware, are there any other mounts that a user would need to add? What about some combination of /etc/{os,lsb}-release for the OS detection?
The machine ID code isn't host FS aware, and think that creates an inconsistency for users. Do you think that should also be made aware of hostfs?
There was a problem hiding this comment.
Ah, good catch. Lemme poke around...
There was a problem hiding this comment.
Alright, added the hostfs arg to MachineID. Did some poking around, it seems like everything else in the linux provider should be be hostfs aware if it can't be. One exception is NativeArchitecture(), which shouldn't matter.
There was a problem hiding this comment.
Can you please list in the godoc all of the necessary mounts for this example to work.
This fixes a subtle issue in agent's metadata processor where the OS reported by agent will reflect the OS of the container and not the OS of the container host. This is confusing to some users. So, we add an option type that a user can pass to the various metrics methods, as well as alter
OperatingSystem()so it can take an alternate HostFS value.This also cleans up the error handling a bit, since we had lots of unwrapped errors.
So, I designed this in a way that required the least amount of disruption to the code, but it's at the point where the registry is kind of complicated, and now juggles multiple types around. I was tempted to refactor the whole thing to make it a tad simpler and to avoid casting bare interfaces everywhere, but that felt a little outside the scope of the issue.