Improve job hostname detection on Windows#448
Conversation
nikita-tkachenko-datadog
added
the
changelog/Fixed
drodriguezhdez
left a comment
drodriguezhdez
left a comment
There was a problem hiding this comment.
Dropped some comments
| return hostname; | ||
| if (computer != null) { | ||
| try { | ||
| EnvVars computerEnvironment = computer.getEnvironment(); |
There was a problem hiding this comment.
What's the difference between EnvVars coming as the method parameter and the EnvVars coming from the Computer object? Are those not the same?
Also, how do we know the "computer" envvars have prevalence over "global?" envvars?
There was a problem hiding this comment.
The ones coming from the computer are relevant for that specific node (could be either the controller or a worker node). The ones coming as method parameter are the ones associated with the job. Debugging shows they're different.
| String envVarsHostname = getNodeHostname(envVars); | ||
| if (isValidHostname(envVarsHostname)) { | ||
| return envVarsHostname; | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems we're duplicating this piece of logic with computer EnvVars and other EnvVars.
Have we considered the following approach to avoid this duplicated code?
EnvVars finalEnvVars = computer != null ? compute.getEnvironment() : envVars
...
String envVarsHostname = getNodeHostname(envVars);
if (isValidHostname(envVarsHostname)) {
return envVarsHostname;
}
There was a problem hiding this comment.
I would like to examine both, as I couldn't find a clear definition of what the "job-level" vars are: I suspect they might include whatever's being set in the Job config UI or in the pipeline DSL. The computer-level ones, however, include variables set externally on the worker node that executes the job.
| if (isUnix(rootPath)) { | ||
| command = new String[]{"hostname", "-f"}; | ||
| } else { | ||
| command = new String[]{"hostname"}; |
There was a problem hiding this comment.
Does this command work in all Windows environments? If not, what would happen in that case? Would it throw an exception?
There was a problem hiding this comment.
Not sure about every possible version, but it does seem to be a standard command. If it doesn't work an exception will be thrown, then caught in this method and logged. null hostname will be returned. Given that this is the last possible fallback, seems like an acceptable outcome
2 participants
Requirements for Contributing to this repository
What does this PR do?
Improves job hostname detection logic:
Computerare examined to see if manually set hostname is providedhostnameshell command is executed remotely (now for Windows machines too, as Windows shell support this command)Description of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.