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

Add some Unix system metrics and configuration contents #22

Merged
merged 2 commits into from May 12, 2015

Conversation

Projects
None yet
5 participants
@ydubreuil
Copy link
Contributor

commented Feb 8, 2015

@ndeloof

This comment has been minimized.

Copy link
Member

commented Feb 8, 2015

discussed about this with Yoann and made me a demo of the related Linux system stats he'd like to get in all support bundles,
so 👍

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

Pretty sure this won't work on OS X, which is considered to be a Unix.

@ndeloof

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

@daniel-beck osx definitively is more unix-friendly than windows, but I won't consider it a Unix :-P
see hudson.tools.JDKInstaller.Platform
Anyway the target here is Linux.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

@ndeloof Am I missing something here? Both Launcher and SlaveComputer think OS X isUnix due to its File.pathSeparatorChar, which means it passes the checks in this code. Only to then fail to read from /proc as that does not exist there.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

… which means any support bundle will contain eight files per OS X node that consist of a warning message and mostly identical 5KB stack trace.

--- WARNING: Could not attach /proc/cpuinfo as it cannot currently be found ---

java.io.FileNotFoundException: /proc/cpuinfo (No such file or directory)
    at java.io.FileInputStream.open(Native Method)
    at java.io.FileInputStream.<init>(FileInputStream.java:146)
    at hudson.util.IOUtils.copy(IOUtils.java:29)
    at com.cloudbees.jenkins.support.api.FileContent.writeTo(FileContent.java:63)
    at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:342)
    at com.cloudbees.jenkins.support.SupportAction.doDownload(SupportAction.java:160)
    ...

This does not look like it's deliberate, and is just asking for a bug report to be filed about it.

@ndeloof

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

indeed, must just skip producing content if platform does not match expectations

@ydubreuil ydubreuil force-pushed the ydubreuil:system-metrics branch from 3458500 to fe2b171 Feb 9, 2015

@ydubreuil

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2015

@daniel-beck you're right, it's better to detect the platform. It's fixed now.

I'm also limiting the dmesg output size and adding the output of 'id -a'

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

No longer generates useless data on OS X.

FWIW /bin/sh -c "id -a" would also work on OS X.

String line;
while ((line = bufferedReader.readLine()) != null) {
writer.println(line);
}

This comment has been minimized.

Copy link
@jglick

jglick Feb 10, 2015

Member

Overkill. If you were starting with a StringWriter and an InputStreamReader, there is no need to look up line boundaries. A simple IOUtils.copyStream would suffice.

This comment has been minimized.

Copy link
@ydubreuil

ydubreuil Feb 16, 2015

Author Contributor

You're right, I'm simplifying the code

}

for (Map.Entry<String,String> procDescriptor : UNIX_PROC_CONTENTS.entrySet()) {
container.add(new FileContent("nodes/" + name + "/proc/" + procDescriptor.getValue(), new File(procDescriptor.getKey())));

This comment has been minimized.

Copy link
@jglick

jglick Feb 10, 2015

Member

This is already covered by another component IIRC.

This comment has been minimized.

Copy link
@ydubreuil

ydubreuil Feb 16, 2015

Author Contributor

I found a implementation in JDKInstaller, but nothing really reusable.

This comment has been minimized.

Copy link
@jglick

jglick Feb 23, 2015

Member

I meant another com.cloudbees.jenkins.support.api.Component. Specifically, FileDescriptorLimit, which has listAllOpenFileDescriptors. There is no actual overlap with that component (it lists /proc/self/fd/* whereas this lists other things under /proc/self/), but they are clearly closed related, so it would seem reasonable to merge them into one Component to reduce the number of choices the user needs to make when picking things to include in the bundle.

UNIX_PROC_CONTENTS.put("/proc/swaps", "swaps.txt");
UNIX_PROC_CONTENTS.put("/proc/cpuinfo", "cpuinfo.txt");
UNIX_PROC_CONTENTS.put("/proc/mounts", "mounts.txt");
UNIX_PROC_CONTENTS.put("/proc/sys/kernel/random/entropy_avail", "kernel_entropy_avail.txt");

This comment has been minimized.

Copy link
@jglick

jglick Feb 10, 2015

Member

Probably irrelevant after the fix of JENKINS-20108.

This comment has been minimized.

Copy link
@ydubreuil

ydubreuil Feb 16, 2015

Author Contributor

True, system entropy is now only required to feed the initial seed

final LogRecord lr = new LogRecord(Level.FINE, "Could not retrieve command content from {0}");
lr.setParameters(new Object[]{getNodeName(node)});
lr.setThrown(e);
LOGGER.log(lr);

This comment has been minimized.

Copy link
@jglick

jglick Feb 23, 2015

Member

By the way you can save yourself some effort and just write

LOGGER.log(Level.FINE, "Could not retrieve command content from " + getNodeName(node), e);

Yes this does string concatenation when FINE is not being logged, but that is going to be trivial compared to the work that went into the remote procedure call (or even stack trace creation), and we hope such exceptional cases are unusual to begin with. It is only worth bothering with the message format overloads of log for calls that are expected to be frequent.

@@ -0,0 +1,93 @@
package com.cloudbees.jenkins.support.api;

This comment has been minimized.

Copy link
@jglick

jglick Feb 23, 2015

Member

Missing license/copyright header in this and other files.

@jglick

This comment has been minimized.

Copy link
Member

commented May 12, 2015

👍 despite some minor unaddressed comments; will merge.

jglick added a commit that referenced this pull request May 12, 2015

Merge pull request #22 from ydubreuil/system-metrics
Add some Unix system metrics and configuration contents

@jglick jglick merged commit 040f5a1 into jenkinsci:master May 12, 2015

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.