Skip to content

properly get displayname of node #38

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.

Sign up for GitHub

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

Merged
jakub-bochenski merged 2 commits into from
Dec 22, 2017
Merged

properly get displayname of node #38

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
371e27b
properly get displayname of node
mwinter69 Dec 11, 2017
67bc346
displayname of node in pipeline
mwinter69 Dec 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
package jenkins.plugins.logstash.persistence;

import hudson.model.Action;
import hudson.model.Computer;
import hudson.model.Environment;
import hudson.model.Result;
import hudson.model.AbstractBuild;
Expand Down Expand Up @@ -138,7 +139,7 @@ public TestData(Action action) {
public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener listener) {
initData(build, currentTime);

Node node = build.getBuiltOn();
Node node = build.getExecutor().getOwner().getNode();
Copy link

@jakub-bochenski jakub-bochenski Dec 12, 2017

Choose a reason for hiding this comment

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

how about we try to use getBuiltOn() first and fall back to getExecutor().getOwner().getNode() if the first one is null? Just to be on the safe side

Copy link
Author

@mwinter69 mwinter69 Dec 12, 2017

Choose a reason for hiding this comment

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

It must be null if you follow the logic.
In the method execute of class Run you will see that the BuildWrappers are initialized from method createBuildListener. But the builtOn is only set when later in the execute the RunExecution.run is called (which actually is AbstractBuild.AbstractBuildExecution.run)

Copy link

@jakub-bochenski jakub-bochenski Dec 13, 2017

Choose a reason for hiding this comment

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

I get that. However this is relying on implementation details. With the current low test coverage I'd prefer to be as conservative as possible.

Copy link

@jakub-bochenski jakub-bochenski Dec 13, 2017

Choose a reason for hiding this comment

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

Another way to look at it is this.
It seems pointless for anyone to call getBuiltOn() if getExecutor().getOwner().getNode() can give you the same result and do it earlier.

I see there is an @Exported getBuiltOnStr() method so remote API is the only sane use case.
Is getBuiltOn() purely a legacy method then?

Copy link
Author

@mwinter69 mwinter69 Dec 13, 2017

Choose a reason for hiding this comment

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

It is not pointless.
getExecutor() will only return something when a build is running. getBuiltOn() will also return the node when the build is finished when the node with this name still exists. Internally only the name of the node is stored and when you call getBuiltOn() it searches for the node with the name.
Maybe its time to write some tests with JenkinsRule where a real job is executed on a node.

Copy link

@jakub-bochenski jakub-bochenski Dec 13, 2017

Choose a reason for hiding this comment

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

Thanks for the explanation.

Based on that I'd still prefer to keep the fallback logic in case somebody somewhere tries to pass a non-running build here.

Maybe its time to write some tests with JenkinsRule where a real job is executed on a node.

Yeah this project definitely needs a heavy makeover (including this class).

I started to port the integ test we do on travis with docker in #36 but there is always something more urgent.
I also need to get more familiar with the jenkins test harness, hopefully #40 will be an opportunity to learn by example

Copy link

@jakub-bochenski jakub-bochenski Dec 13, 2017

Choose a reason for hiding this comment

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

Another thing: how does this compare to the pipeline logic in lines 190-194? Seems to to the same thing but using displayName?

Copy link
Author

@mwinter69 mwinter69 Dec 13, 2017

Choose a reason for hiding this comment

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

This is wrong, it will return something like Executor-#0, but definitely not the host.
For pipelines using getExecutor().getOwner().getNode() will just get the host where the logstashSend is called, but the build could have run on many different hosts before.
Anyway I'm also looking into a more pipeline like logstash that accepts a block

logstash {
  node('label') {
    do something
    do anotherthing
  }
}

but I have to find a way to inject the host after the logstashwriter is initialized.

Copy link

@jakub-bochenski jakub-bochenski Dec 15, 2017

Choose a reason for hiding this comment

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

will just get the host where the logstashSend is called, but the build could have run on many different hosts before.

I get that.
One final question: for now, do you think we should change the pipeline handling to use the getExecutor().getOwner().getNode() instead of the current executor display name?

Copy link
Author

@mwinter69 mwinter69 Dec 16, 2017

Choose a reason for hiding this comment

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

I fixed also the pipeline handling to display the name of the node and not the executor. I also added some integration tests using JenkinsRule.

if (node == null) {
buildHost = "master";
buildLabel = "master";
Expand Down
12 changes: 10 additions & 2 deletions src/test/java/jenkins/plugins/logstash/LogstashWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import hudson.EnvVars;
import hudson.model.AbstractBuild;
import hudson.model.Computer;
import hudson.model.Executor;
import hudson.model.Project;
import hudson.model.Result;
import hudson.model.TaskListener;
Expand Down Expand Up @@ -75,6 +77,9 @@ String getJenkinsUrl() {

@Mock BuildData mockBuildData;
@Mock TaskListener mockListener;
@Mock Computer mockComputer;
@Mock Executor mockExecutor;


@Captor ArgumentCaptor<List<String>> logLinesCaptor;

Expand All @@ -85,7 +90,6 @@ public void before() throws Exception {
when(mockBuild.getDisplayName()).thenReturn("LogstashNotifierTest");
when(mockBuild.getProject()).thenReturn(mockProject);
when(mockBuild.getParent()).thenReturn(mockProject);
when(mockBuild.getBuiltOn()).thenReturn(null);
when(mockBuild.getNumber()).thenReturn(123456);
when(mockBuild.getTimestamp()).thenReturn(new GregorianCalendar());
when(mockBuild.getRootBuild()).thenReturn(mockBuild);
Expand All @@ -97,6 +101,10 @@ public void before() throws Exception {
when(mockBuild.getLog(3)).thenReturn(Arrays.asList("line 1", "line 2", "line 3", "Log truncated..."));
when(mockBuild.getLog(Integer.MAX_VALUE)).thenReturn(Arrays.asList("line 1", "line 2", "line 3", "line 4"));
when(mockBuild.getEnvironment(null)).thenReturn(new EnvVars());
when(mockBuild.getExecutor()).thenReturn(mockExecutor);
when(mockExecutor.getOwner()).thenReturn(mockComputer);
when(mockComputer.getNode()).thenReturn(null);


when(mockTestResultAction.getTotalCount()).thenReturn(0);
when(mockTestResultAction.getSkipCount()).thenReturn(0);
Expand Down Expand Up @@ -142,7 +150,7 @@ public void constructorSuccess() throws Exception {
verify(mockBuild).getDescription();
verify(mockBuild).getUrl();
verify(mockBuild).getAction(AbstractTestResultAction.class);
verify(mockBuild).getBuiltOn();
verify(mockBuild).getExecutor();
verify(mockBuild, times(2)).getNumber();
verify(mockBuild).getTimestamp();
verify(mockBuild, times(4)).getRootBuild();
Expand Down
32 changes: 24 additions & 8 deletions src/test/java/jenkins/plugins/logstash/persistence/BuildDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@

import hudson.EnvVars;
import hudson.model.AbstractBuild;
import hudson.model.Computer;
import hudson.model.Environment;
import hudson.model.EnvironmentList;
import hudson.model.Executor;
import hudson.model.Node;
import hudson.model.Project;
import hudson.model.Result;
Expand Down Expand Up @@ -61,6 +63,8 @@ public class BuildDataTest {
@Mock Environment mockEnvironment;
@Mock Date mockDate;
@Mock TaskListener mockListener;
@Mock Computer mockComputer;
@Mock Executor mockExecutor;


@Before
Expand Down Expand Up @@ -97,6 +101,8 @@ public void before() throws Exception {
when(mockRootProject.getFullName()).thenReturn("parent/RootBuildDataTest");

when(mockDate.getTime()).thenReturn(60L);
when(mockBuild.getExecutor()).thenReturn(mockExecutor);
when(mockExecutor.getOwner()).thenReturn(mockComputer);
}

@After
Expand Down Expand Up @@ -124,7 +130,7 @@ private void verifyMocks() throws Exception
verify(mockBuild).getStartTimeInMillis();
verify(mockBuild).getUrl();
verify(mockBuild).getAction(AbstractTestResultAction.class);
verify(mockBuild).getBuiltOn();
verify(mockBuild).getExecutor();
verify(mockBuild).getNumber();
verify(mockBuild).getTimestamp();
verify(mockBuild, times(4)).getRootBuild();
Expand All @@ -133,6 +139,8 @@ private void verifyMocks() throws Exception
verify(mockBuild).getEnvironments();
verify(mockBuild).getEnvironment(mockListener);

verify(mockExecutor).getOwner();

verify(mockRootProject).getName();
verify(mockRootProject).getFullName();

Expand All @@ -150,9 +158,15 @@ private void verifyTestResultActions() {
verify(mockTestResultAction, times(1)).getFailedTests();
}

private void verifiyNodeActions(int labelCount) {
verify(mockComputer).getNode();
verify(mockNode, times(2)).getDisplayName();
verify(mockNode, times(labelCount)).getLabelString();
}

@Test
public void constructorSuccessBuiltOnNull() throws Exception {
when(mockBuild.getBuiltOn()).thenReturn(null);
when(mockComputer.getNode()).thenReturn(null);

// Unit under test
BuildData buildData = new BuildData(mockBuild, mockDate, mockListener);
Expand All @@ -170,7 +184,7 @@ public void constructorSuccessBuiltOnNull() throws Exception {

@Test
public void constructorSuccessBuiltOnMaster() throws Exception {
when(mockBuild.getBuiltOn()).thenReturn(mockNode);
when(mockComputer.getNode()).thenReturn(mockNode);

when(mockNode.getDisplayName()).thenReturn("Jenkins");
when(mockNode.getLabelString()).thenReturn("");
Expand All @@ -187,11 +201,12 @@ public void constructorSuccessBuiltOnMaster() throws Exception {

verifyMocks();
verifyTestResultActions();
verifiyNodeActions(1);
}

@Test
public void constructorSuccessBuiltOnSlave() throws Exception {
when(mockBuild.getBuiltOn()).thenReturn(mockNode);
when(mockComputer.getNode()).thenReturn(mockNode);

when(mockNode.getDisplayName()).thenReturn("Test Slave 01");
when(mockNode.getLabelString()).thenReturn("Test Slave");
Expand All @@ -208,6 +223,7 @@ public void constructorSuccessBuiltOnSlave() throws Exception {

verifyMocks();
verifyTestResultActions();
verifiyNodeActions(2);
}

@Test
Expand Down Expand Up @@ -252,8 +268,7 @@ public void constructorSuccessWithEnvVars() throws Exception {
when(mockBuild.getEnvironments()).thenReturn(new EnvironmentList(Arrays.asList(mockEnvironment)));
when(mockBuild.getBuildVariables()).thenReturn(new HashMap<String, String>());

when(mockBuild.getBuiltOn()).thenReturn(mockNode);

when(mockComputer.getNode()).thenReturn(mockNode);
when(mockNode.getDisplayName()).thenReturn("Jenkins");
when(mockNode.getLabelString()).thenReturn("");

Expand Down Expand Up @@ -289,6 +304,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable {

verifyMocks();
verifyTestResultActions();
verifiyNodeActions(1);
}

@Test // JENKINS-41324
Expand All @@ -297,8 +313,7 @@ public void constructorSuccessWithChangedEnvVars() throws Exception {
when(mockBuild.getBuildVariables()).thenReturn(new HashMap<String, String>());
when(mockBuild.getSensitiveBuildVariables()).thenReturn(new HashSet<String>());

when(mockBuild.getBuiltOn()).thenReturn(mockNode);

when(mockComputer.getNode()).thenReturn(mockNode);
when(mockNode.getDisplayName()).thenReturn("Jenkins");
when(mockNode.getLabelString()).thenReturn("");

Expand Down Expand Up @@ -327,6 +342,7 @@ public Void answer(InvocationOnMock invocation) throws Throwable {

verifyMocks();
verifyTestResultActions();
verifiyNodeActions(1);
}

@Test
Expand Down