Skip to content

executor/resource: stub out NewSysSampler on Windows#4040

Merged
jedevc merged 1 commit intomoby:masterfrom
thaJeztah:windows_no_proc
Jul 20, 2023
Merged

executor/resource: stub out NewSysSampler on Windows#4040
jedevc merged 1 commit intomoby:masterfrom
thaJeztah:windows_no_proc

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jul 20, 2023

Commit 509cfa3 (#3860) introduced the SysSampler, which measures resource consumption. However, for this it depends on prometheus' procfs. That package does not have build-tags but is a Linux-only implementation, which (by default) attempts to access /proc; https://github.com/prometheus/procfs/blob/v0.9.0/fs.go#L26-L33 https://github.com/prometheus/procfs/blob/v0.9.0/internal/fs/fs.go#L23-L24

This patch splits the implementation of "resource" into platform-specific files, and stubs out the NewSysSampler() on non-Linux platforms.

Comment on lines +11 to +9
func newSysSampler() (*Sampler[*types.SysSample], error) {
return NewSampler(2*time.Second, 20, func(_ time.Time) (*types.SysSample, error) { return nil, nil }), nil
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TBH; wasn't sure if return nil, nil would work, but looks like it's happy

Also was hoping to make this function return nil, but not sure if that would work, as it's called unconditionally;

usage := s.sysSampler.Record()
defer usage.Close(false)

I was initially thinking along the lines of

var usage some.Type
if s.sysSampler != nil {
    usage := s.sysSampler.Record()
    defer usage.Close(false)
}

But honestly, generics are not yet "my thing", and I got confused what to use for some.Type (.Record() has a func (s *Sampler[T]) Record() *Sub[T] signature)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The type should just be *Sub[*types.SysSample].

@thaJeztah thaJeztah marked this pull request as ready for review July 20, 2023 10:23
@thaJeztah
Copy link
Copy Markdown
Member Author

FWIW; also testing these changes in moby/moby#46035 (well, currently from a locally patched "vendor", but didn't want to restart it). So it may be worth keeping an eye on that one to see if all works as expected.

@thaJeztah thaJeztah marked this pull request as draft July 20, 2023 12:19
Commit 509cfa3 introduced the SysSampler,
which measures resource consumption. However, for this it depends on
prometheus' procfs. That package does not have build-tags but is a Linux-only
implementation, which (by default) attempts to access `/proc`;
https://github.com/prometheus/procfs/blob/v0.9.0/fs.go#L26-L33
https://github.com/prometheus/procfs/blob/v0.9.0/internal/fs/fs.go#L23-L24

This patch splits the implementation of "resource" into platform-specific
files, and stubs out the NewSysSampler() on non-Linux platforms.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@crazy-max
Copy link
Copy Markdown
Member

crazy-max commented Jul 20, 2023

Not for this PR but FreeBSD has linprocfs and linsysfs modules that are the equivalent of the Linux ones. Mounted in /compat/linux/proc and /compat/linux/sys. Not sure if prometheus/procfs supports this. Anyway should be considered after #2376 merged (cc @akhramov).

@thaJeztah
Copy link
Copy Markdown
Member Author

Yes I was considering to use _unix.go, but saw other files in the same package used _linux.go and _nolinux.go so I kept with that.

@thaJeztah thaJeztah marked this pull request as ready for review July 20, 2023 13:03
@jedevc jedevc merged commit fa0dbb1 into moby:master Jul 20, 2023
@thaJeztah thaJeztah deleted the windows_no_proc branch July 20, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants