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

feat(DockerCloud): option to use docker hostname as slave display name #299

Closed
wants to merge 1 commit into from
Closed

feat(DockerCloud): option to use docker hostname as slave display name #299

wants to merge 1 commit into from

Conversation

menski
Copy link
Contributor

@menski menski commented Aug 22, 2015

Adds an option to the docker cloud to use the hostname of the docker
host as slave display name. If the hostname cannot be resolved the IP
address is used.

This is useful for setups with docker swarm. The display name of the
slave will then contain the information on which actual docker host
the job is running.

Limitation: The docker container must have a port mapping to
get the hostname of the slave.

Review on Reviewable

@lanwen
Copy link
Member

lanwen commented Aug 23, 2015

Think it should be added to page "Built on Docker" and not slave name. Docker swarm is abstraction layer to avoid such interaction with each concrete host. But the information should be available by request.

If this information will be on page for https://github.com/jenkinsci/docker-plugin/blob/master/docker-plugin/src/main/java/com/nirima/jenkins/plugins/docker/action/DockerBuildAction.java - is it solves your problem?

@menski
Copy link
Contributor Author

menski commented Aug 23, 2015

We like to see the actual host of the build while it runs to easily spot problems. This way we can already see in the Jenkins executor overview when something goes wrong with the scheduling or a host isn't accepting any jobs.

As this is an option it is the decision of the user if he wants to see this information or not.

@lanwen
Copy link
Member

lanwen commented Aug 23, 2015

What kind of problems it helps to solve? Maybe just extend logging? Think hostname exposing is wrong way to research slave provisioning problems

@menski
Copy link
Contributor Author

menski commented Aug 23, 2015

We just want to quickly see where a job is actually running. This helped us to spot problems early and it adds some useful information to the slave name.

@lanwen
Copy link
Member

lanwen commented Aug 23, 2015

With this logic it will be useful to add image name, exposed ports, image hash, attached volumes etc :) IMHO If you get regular problems and spot them with help of hostname, probably you do something wrong.

@KostyaSha
Copy link
Member

Sorry, but i wouldn't merge it now, because i plan review swarm integration separately. I will add it to roadmap. Cloud name + container hash is really unique identifier, if you want see more info, then it should be displayed in slave view page (in TODO review this API provided by jenkins: afair you just need extended configure.jelly for slave object). Also plugin has docker cloud page, then is in bad state and may show more advanced info.

@KostyaSha
Copy link
Member

@menski if you want you can help me with #299 ;)

@KostyaSha KostyaSha mentioned this pull request Aug 23, 2015
21 tasks
@menski
Copy link
Contributor Author

menski commented Aug 23, 2015

@KostyaSha do you mean #235? Or any specific issue? As #299 is the current one.

@KostyaSha
Copy link
Member

@menski sorry, wrong url in buffer, docker-java/docker-java#299 (seems we have two PRs with the same id?)

Adds an option to the docker cloud to use the hostname of the docker
host as slave display name. If the hostname cannot be resolved the IP
address is used.

This is useful for setups with docker swarm. The display name of the
slave will then contain the information on which actual docker host
the job is running.

Limitation: The docker container must have a port mapping to
get the hostname of the slave.
@KostyaSha KostyaSha modified the milestone: 0.13.0 Aug 25, 2015
@KostyaSha
Copy link
Member

(in TODO review this API provided by jenkins: afair you just need extended configure.jelly for slave object).

@menski ^^

@nnordrum
Copy link

Because I love to derail things... wouldn't it be better to create a field with that is evaluated with the Token-Macro plugin and then also publish some additional tokens that could be used?

I just can't imagine we could possibly come up with a strategy that makes everybody happy, and this way you could create a "sensible default", and then let @menski use hostname if desired, and I could use the name of the committer (since for us, commit = container, and toss them after each job), which I've had at least 5 people independently ask for...

@magnayn
Copy link
Contributor

magnayn commented Oct 21, 2015

So anyone who has used docker in production will know precisely why knowing
the actual running host is important. That's the whole reason the slave
name is prepended with the cloud name in the first place. So I reject the
notion that 'if you want to know the name, you're doing it wrong' or that
it should be hidden from view. It's a useful feature.

That said, where there's 2 options, there's more than likely >2. I quite
like the @nnordrum idea of an expandable token macro name. Best solution
would probably be a dropdown list of 'naming strategy', with perhaps
options of
default - automatically choose (we may detect later that it's a swarm and
choose automatically)
cloud - as it is today
host - as the PR does it

then later: other - provides text box to provide a string (expandable
macro).

then if someone comes up with something un-thought-of, we don't need
another boolean flag to select it.

@menski / @nnordrum - would you be interested in re-spinning the feature in
this way?

On Tue, Oct 20, 2015 at 10:48 PM, Noah Nordrum notifications@github.com
wrote:

Because I love to derail things... wouldn't it be better to create a field
with that is evaluated with the Token-Macro plugin and then also publish
some additional tokens that could be used?

I just can't imagine we could possibly come up with a strategy that makes
everybody happy, and this way you could create a "sensible default", and
then let @menski https://github.com/menski use hostname if desired, and
I could use the name of the committer (since for us, commit = container,
and toss them after each job), which I've had at least 5 people
independently ask for...


Reply to this email directly or view it on GitHub
#299 (comment)
.

@magnayn
Copy link
Contributor

magnayn commented Oct 21, 2015

@nnordrum BTW - I don't think you'll be able to name the slave based on the commit (or author) that it's going to build, as that isn't the order of things in Jenkins. Slaves that are provisioned have no knowledge of the build that they are going to execute.

@magnayn magnayn removed the pause label Oct 21, 2015
@nnordrum
Copy link

@magnayn I completely agree at the time of provision, but is there some reason why the slave couldn't rename itself? I'm far from an expert on the internals of jenkins slave naming...

I'm basing this almost completely on the fact that the Build Executor Status box's entire innerHTML is replaced from ajaxExecutors, so we should be able to rename the computer/slave/whatever it's called and then it would show up "soon".

@magnayn
Copy link
Contributor

magnayn commented Oct 21, 2015

Sure, you could do that - the slave's name can be changed from within the
build; you'd need to add a build step within the project configuration that
does that.

Such a build step though wouldn't be tied neccesarily to docker-plugin
however, you could equally well apply 'expand tokens in this string and set
the slave name' to any project.

On Wed, Oct 21, 2015 at 11:18 PM, Noah Nordrum notifications@github.com
wrote:

@magnayn https://github.com/magnayn I completely agree at the time of
provision, but is there some reason why the slave couldn't rename itself?
I'm far from an expert on the internals of jenkins slave naming...

I'm basing this almost completely on the fact that the Build Executor
Status box's entire innerHTML is replaced from ajaxExecutors, so we should
be able to rename the computer/slave/whatever it's called and then it would
show up "soon".


Reply to this email directly or view it on GitHub
#299 (comment)
.

@nnordrum
Copy link

I could just add a system groovy command then I bet.

any pointers on how to rename a slave? :)

Edit: nevermind, was stupidly easy... Jenkins.instance.slaves.first().name = 'test-rename'

didn't even look at the API... got it right on the first guess in the Script Console

@nnordrum
Copy link

ok that didn't work as expected... created a Freestyle Job with an "Execute system Groovy script" step

out.println build.builtOn.name
build.builtOn.name = "${build.builtOn.name}-RENAMED"
out.println build.builtOn.name

and an Execute shell step

sleep 120

Fails almost immediately

[EnvInject] - Loading node environment variables.
Building remotely on BuildSwarm-f944ad2dfabd (maven docker groovy java) in workspace /home/jenkins/workspace/test-rename
BuildSwarm-f944ad2dfabd
BuildSwarm-f944ad2dfabd-RENAMED
ERROR: Build step failed with exception
java.lang.NullPointerException: no workspace from node DockerSlave{name=BuildSwarm-f944ad2dfabd-RENAMED, containerId=f944ad2dfabd, template=DockerTemplate{configVersion=2, labelString='maven java docker groovy', launcher=com.nirima.jenkins.plugins.docker.launcher.DockerComputerSSHLauncher@1286d72e, remoteFsMapping='', remoteFs='/home/jenkins', instanceCap=2147483647, mode=EXCLUSIVE, retentionStrategy=com.nirima.jenkins.plugins.docker.strategy.DockerOnceRetentionStrategy@4094db2b, numExecutors=1, dockerTemplateBase=DockerTemplateBase{image=jenkins_slave}, removeVolumes=false, pullStrategy=PULL_NEVER}} which is computer null and has channel null
    at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:76)
    at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
    at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
    at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
    at hudson.model.Build$BuildExecution.build(Build.java:205)
    at hudson.model.Build$BuildExecution.doRun(Build.java:162)
    at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:537)
    at hudson.model.Run.execute(Run.java:1744)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:374)
Build step 'Execute shell' marked build as failure
Finished: FAILURE

Any ideas?

@KostyaSha
Copy link
Member

@nnordrum you can't do this.

@nnordrum
Copy link

@KostyaSha I didn't think you could, but I wasn't really in a position to argue...

@magnayn
Copy link
Contributor

magnayn commented Oct 23, 2015

You might be able to get it to work, but it's likely to be a certain amount of pain.

The build stores a string, which is the name of the slave that it is being built on (String builtOn)

build.builtOn returns Jenkins.getInstance().getNode(builtOn);. In the jenkins instance, it has a hashmap of names of nodes to their instances.

By changing the slave's name, you're actually changing its whole identity (equals(), hashCode). You might be able to get around this by

  • changing the actual builtOn string in the build, as well as the name of the slave
  • adding the renamed node into the jenkins node map

Cleanup at the end is likely to be a pig, and you'll either have to change everything that was using the old node id to the new one, or live with 2 slaves until the end (I.E: most objects store ID - String - references to objects that are looked up, rather than direct references)

Sounds like too much trouble to me. Things really don't expect the ID to change once it's set, and the slave is always instantiated without any knowledge of what it's about to build.

As an aside - you may then see the attractive 'description' field in Slave, only to discover that it's final. (that's right kids. The identity is mutable,and the description is immutable 👊 )

You might want to ask on the ML if it's possible to extend the UI items in the build executor status list (LHS) in the same way that you can add extra status columns for builds, I think that woiuld be generally useful (e.g: not only is this build #117 but it's of PR #32).

@nnordrum
Copy link

what about Node.getDisplayName()? (i.e. DockerSlave.getDisplayName())

    public String getDisplayName() {
        return getNodeName(); // default implementation
    }

That shouldn't have any of those side effects--right?

@KostyaSha
Copy link
Member

Display names will be the right direction.

@magnayn
Copy link
Contributor

magnayn commented Oct 26, 2015

It won't have those side-effects, and sadly it won't work (in that I don't think it will give you what you want).

You can override both getDisplayName() and getNodeDescription().

The former isn't used in the UI (It'll simply continue to display cloudname-xxxxxxx). The latter appears if you click the node for it's details (appears in brackets), but after the machine is deleted of course even that disappears.

@KostyaSha
Copy link
Member

Description is different.
Hint: queue should support display names in the same way as job/folder.

@magnayn
Copy link
Contributor

magnayn commented Oct 26, 2015

Yes, it should, but it doesn't appear to.

I think a wider Jenkins change is the way to go.

There are few use-cases here.

  1. I use docker-swarm. When a machine is provisioned (I.E: before the
    build starts
    ), I would like to see the actual host it was provisioned on
    (particularly for diagnosis features).
  2. I would like to see, for the builds in progress, details about that
    build such as
  • the SHA1
  • the author / owner
  • the PR number
  • etc etc etc

#1 can probably be done with either changing the naming policy for the ID,
or the display name if this can be added to the build executor status view.
#2 smells like a hack. Whilst I'm sure it could be done, it'd be much nicer
if the build executor status list could show additional properties of the
things that it's building other than just the build number, perhaps from
existing actions that have been added into the build objects. People using
clouds other than docker would also likely find this useful.

On Mon, Oct 26, 2015 at 9:33 AM, Kanstantsin Shautsou <
notifications@github.com> wrote:

Description is different.
Hint: queue should support display names in the same way as job/folder.


Reply to this email directly or view it on GitHub
#299 (comment)
.

@kmadel kmadel mentioned this pull request Oct 31, 2015
@KostyaSha
Copy link
Member

@magnayn for displayNames i filled https://issues.jenkins-ci.org/browse/JENKINS-31560 , that should be pretty simple change.

for 2) It should be done via additional page in jenkins because this small executors list widget will be too overloaded. Showing detailed info makes sense only for those who wants see it. Then such page can operate in the same way as 'Views'. At all it can be just one more view.

@KostyaSha
Copy link
Member

Lol, it already supported. Just Override getDisplayName and additional field in DockerComputer, then set it whenever you want, if you need attach something to build for history, then just append action onTaskCompleted to Run :)

@ndeloof
Copy link
Contributor

ndeloof commented Sep 6, 2017

doesn't seem to be relevant now docker swarm mode supersede docker-swarm

@ndeloof ndeloof closed this Sep 6, 2017
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.

6 participants