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

[JENKINS-33983] Using correct git installation when fetching heads #424

Merged
merged 1 commit into from Aug 14, 2016

Conversation

Projects
None yet
3 participants
@jcechace
Copy link

commented Jul 21, 2016

Fix for JENKINS-33983.
A correct git client (for either selected git tool installation or global default installation) is now created when fetching git heads instead of always using jGit.

@olivergondza olivergondza reopened this Jul 21, 2016

@jcechace jcechace force-pushed the jcechace:JENKINS-33983 branch from 1b4cf61 to 6065b05 Jul 22, 2016

PowerMockito.doReturn(git).when(Git.class, "with", Mockito.any(), Mockito.any());

// Partial mock our AbstractGitSCMSourceImpl
gitSCMSource = PowerMockito.spy(new AbstractGitSCMSourceImpl());

This comment has been minimized.

Copy link
@olivergondza

olivergondza Jul 27, 2016

Member

Why not just PowerMockito.mock(AbstractGitSCMSource.class)? That way we do not need the dummy implementation (as mock is not limited to classes).

This comment has been minimized.

Copy link
@jcechace

jcechace Jul 27, 2016

Author

Some of the methods on AbstractGitSCMSourceImpl are called internally. This would mean adding return values to the mock (default return values are not suitable -- e.g. mock will return null instead of empty String). I figured it's more readable this way than adding bunch of mock calls.

@olivergondza

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

👍 aside from the nit in comments.

pom.xml Outdated
@@ -53,6 +54,12 @@
</pluginManagement>
<plugins>
<plugin>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<argLine>-XX:MaxPermSize=${permgen.size}</argLine>

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Jul 27, 2016

This drops the -Djava.awt.headless=true that is inherited from the parent pom. Is that intentional?

This comment has been minimized.

Copy link
@jcechace

jcechace Aug 1, 2016

Author

Thanks, this should be fixed now by overriding the ${argLine} property from parent pom instead of the entire surefire configuration.

@jcechace jcechace force-pushed the jcechace:JENKINS-33983 branch from 6065b05 to 89c5ba6 Aug 1, 2016

@MarkEWaite

This comment has been minimized.

Copy link

commented Aug 11, 2016

This seems to silently change the default git implementation used for existing jobs. For example, I had a multi-branch pipeline job defined which used the JGit implementation to scan for branches. When I installed the plugin built with this pull request, the configure screen for the job showed that it was selecting the "Default" git implementation.

I expected it would show that it was using the JGit implementation, since that was the implementation that had been used before the pull request.

In general, JGit capability in the git plugin is a subset of the functionality already available from the command line git in the plugin. Thus, I don't think this change of behavior is likely to be harmful to the general population. It will probably be closer to general expectations of new users, since the default used everywhere else is the command line git implementation.

Users who configured JGIt as an additional git implementation and never installed command line git on their master server will likely see a problem with this change. I assume there are very few of those types of users, even among the 80 000+ installations of the plugin.

@@ -26,6 +26,8 @@
<java.level>6</java.level>
<!-- Use the same configuration than before. -->
<concurrency>2</concurrency>
<permgen.size>128m</permgen.size>

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Aug 11, 2016

I don't think this is needed based on the current 160 MB setting already in the master branch.

@MarkEWaite MarkEWaite merged commit 5a44833 into jenkinsci:master Aug 14, 2016

1 check passed

Jenkins This pull request looks good
Details
@MarkEWaite

This comment has been minimized.

Copy link

commented Aug 16, 2016

@jcechace the change has been merged, but there is at least one problem which will block me from releasing it. Any hints you can offer to diagnose the failure?

I define a multi-branch pipeline job named "git-plugin-freestyle-multi-branch" in my lts-with-plugins Docker instance.

When I start that Docker instance, open that job (under the git-client-plugin tab, it is named "Git Client Branches - Maven"), and index that job ("Branch Indexing" then "Run Now"), it fails with the message:

Creating git repository in /var/jenkins_home/caches/git-50a6d984703ef2ee4f093ba9c6503eec
 > git init /var/jenkins_home/caches/git-50a6d984703ef2ee4f093ba9c6503eec # timeout=10
FATAL: Failed to recompute children of Git Client Branches - Maven
hudson.plugins.git.GitException: Could not init /var/jenkins_home/caches/git-50a6d984703ef2ee4f093ba9c6503eec
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$5.execute(CliGitAPIImpl.java:652)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.init(CliGitAPIImpl.java:181)
    at hudson.plugins.git.GitAPI.init(GitAPI.java:217)
    at jenkins.plugins.git.AbstractGitSCMSource.retrieve(AbstractGitSCMSource.java:275)
    at jenkins.scm.api.SCMSource.fetch(SCMSource.java:146)
    at jenkins.branch.MultiBranchProject.computeChildren(MultiBranchProject.java:294)
    at com.cloudbees.hudson.plugins.folder.computed.ComputedFolder.updateChildren(ComputedFolder.java:157)
    at com.cloudbees.hudson.plugins.folder.computed.FolderComputation.run(FolderComputation.java:122)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:410)
Caused by: hudson.plugins.git.GitException: Error performing command: git init /var/jenkins_home/caches/git-50a6d984703ef2ee4f093ba9c6503eec
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1730)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1699)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1695)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1317)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$5.execute(CliGitAPIImpl.java:650)
    ... 9 more
Caused by: java.io.IOException: Cannot run program "git" (in directory "/var/jenkins_home/caches/git-50a6d984703ef2ee4f093ba9c6503eec"): error=2, No such file or directory
    at java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)
    at hudson.Proc$LocalProc.<init>(Proc.java:240)
    at hudson.Proc$LocalProc.<init>(Proc.java:212)
    at hudson.Launcher$LocalLauncher.launch(Launcher.java:815)
    at hudson.Launcher$ProcStarter.start(Launcher.java:381)
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1719)
    ... 13 more
Caused by: java.io.IOException: error=2, No such file or directory
    at java.lang.UNIXProcess.forkAndExec(Native Method)
    at java.lang.UNIXProcess.<init>(UNIXProcess.java:248)
    at java.lang.ProcessImpl.start(ProcessImpl.java:134)
    at java.lang.ProcessBuilder.start(ProcessBuilder.java:1029)
    ... 18 more
Finished: FAILURE

Other jobs do not fail to index, even in that same Docker instance, but there are at least two (both multi-branch pipeline jobs) which show that failure.

@olivergondza

This comment has been minimized.

Copy link
Member

commented Aug 16, 2016

@MarkEWaite. I guess you have gone through the usual suspects. Is git installed? On the path configured in tool configuration? If there is not full path, is git on PATH?

@jcechace

This comment has been minimized.

Copy link
Author

commented Aug 16, 2016

@MarkEWaite Probably a silly question... Does the docker image have command line git installed? After the change, the default git tool will be selected if no other was selected -- and IIRC the default git tool (provided no configuration was done) is command line client.

@MarkEWaite

This comment has been minimized.

Copy link

commented Aug 16, 2016

@jcechace the image includes command line git. Other jobs in the same Docker image use command line git and are running successfully.

I created an instance of that Docker image at home earlier this morning and saw the problem. I then confirmed on a different computer at the office that the problem is repeatable.

I'll do more investigation later tonight or early tomorrow morning. I assume there is some issue with the call to git that is used to initialize a repository in that context.

My first hunch is that the directory does not exist when the call is made to init the repository. That's just a guess based on a quick experiment over lunch. If that's the case, it will be easy to resolve by checking for existence of the directory inside the git API init call. If the directory doesn't exist, the Java code can attempt to create it recursively and throw an exception if the create fails.

@MarkEWaite

This comment has been minimized.

Copy link

commented Aug 17, 2016

The directory is created by JGit and not created by command line git. I adapted the command line git implementation to match the JGit implementation.. That seems much easier than changing the multi-branch plugin.

Refer to pull request 3 in my fork. I'll watch the automated tests in multiple configurations, and if they are well behaved, will include that change in the master branch

@olivergondza

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

Good catch, linux seems to make no difference if the binary does not exists or the cwd does not. I guess Launcher can be made smarter to either auto create it or provide better error message.

@MarkEWaite

This comment has been minimized.

Copy link

commented Aug 17, 2016

Creating the directory in the CliGitAPIImpl.init() method allowed most of the jobs to complete successfully. That change is worthwhile.

Unfortunately, I found an additional case (or possibly two) which still needs further investigation before the change is released.

I have a repository, jenkins-bugs-private, which has a branch per bug. Each branch has a Jenkinsfile that includes groovy calls to verify that bug is resolved. I have a multi-branch pipeline job for that private repository which creates a job for each of the branches in the repository.

When that repository is configured to use JGit (previously the only option), with https username and password, the branch indexing works as expected. The same credentials with CliGit fail, even though those credentials work well with other command line git jobs in the same docker instance.

I will need some additional time to investigate that failure. While the original failure was visible in a public docker instance (because it didn't actually require credentials), this failure is intentionally limited to a private docker instance because it includes real credentials. I'll keep you informed as I learn more.

@MarkEWaite

This comment has been minimized.

Copy link

commented Aug 19, 2016

That repository and pipeline job seems to have exposed a long-standing bug in the CliGitAPIImpl.prune() method. When I run git remote prune origin with some ssh debugging enabled, it shows that ssh is invoked . However, the CliGitAPIImpl.prune() method does not provide credentials.

The JGit implementation adds credentials to the prune call as well.

I've added credentials to CliGitAPIImpl.prune() and am evaluating it now in automated and interactive tests. I suspect there will yet be other problems discovered before this pull request can be delivered to users.

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.