Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-64420

Deprecated javaVersionChecker

    XMLWordPrintable

    Details

    • Similar Issues:
    • Released As:
      ssh-slaves-1.31.5

      Description

      Java is a requirement to run the remoting process on the Jenkins Agents, currently, there are three methods to find the Java command in the SSH Build Agents:

      • Java command is in the PATH.
      • Java PATH is set in the javaPath advanced setting.
      • Jenkins tries to guess the Java location using some fixed paths and the JENKINS_HOME environment variable.

      this last way to find Java try to fix something that should be configured in the Agent environment or in the Agent configuration (the other two options), it does not cover all the possibilities, it is odd, it is not OS independent, in general, is odd and error-prone. For all these, I want to deprecate the use of this configuration option, a warning will show on the agent logs at the start fo a few months, and at the end of 2021, I will remove this option.
       

        Attachments

          Issue Links

            Activity

            Hide
            ahammar Anders Hammar added a comment -

            This change is causing issues for our setup where we have java configured in the PATH. It is JavaVersionChecker that finds java in PATH (in addition to some other magic URLs), så it can't just be deprecated without rewriting the logic. As it is now we're seeing a warning log message for all our agents (quite a few) even though java is present on the agents' path. Very confusing.
            My vote is to revert the change and redo.

            Show
            ahammar Anders Hammar added a comment - This change is causing issues for our setup where we have java configured in the PATH. It is JavaVersionChecker that finds java in PATH (in addition to some other magic URLs), så it can't just be deprecated without rewriting the logic. As it is now we're seeing a warning log message for all our agents (quite a few) even though java is present on the agents' path. Very confusing. My vote is to revert the change and redo.
            Hide
            ifernandezcalvo Ivan Fernandez Calvo added a comment -

            this change only deprecates the class and adds a message on logs, it does not change any logic, you can omit the message by adding a logger for that class on level ERROR (Configure Jenkins/System log).
            If you see the message in logs is because the user you have configured to access the Agents does not have Java in the PATH, so this class is called to find where Java is. After the summer this class will disappear and your agents will stop working, the Java command in the PATH of the user is a requirement.

            Show
            ifernandezcalvo Ivan Fernandez Calvo added a comment - this change only deprecates the class and adds a message on logs, it does not change any logic, you can omit the message by adding a logger for that class on level ERROR (Configure Jenkins/System log). If you see the message in logs is because the user you have configured to access the Agents does not have Java in the PATH, so this class is called to find where Java is. After the summer this class will disappear and your agents will stop working, the Java command in the PATH of the user is a requirement.
            Hide
            ahammar Anders Hammar added a comment -

            I don't think that's correct. We do have java in the path and we still see the new warning log message.
            Checking the code, JavaVersionChecker is used in all cases but if javaPath is configured in the agent config. Even if java is in the path. And the first place it looks in is workingDir/jdk/bin/java. Which doesn't exist and then results in the warning message which incorrectly states that java is not in the path. Then java i checked in the path, where it it is found. See DefaultJavaProvider.getJavas().
            So even if the logic is not changed and everything still works, we get an incorrect warning log message. The correct way IMO would be to first check if java is in the path before falling back to the old logic in JavaVersionChecker.

            Show
            ahammar Anders Hammar added a comment - I don't think that's correct. We do have java in the path and we still see the new warning log message. Checking the code, JavaVersionChecker is used in all cases but if javaPath is configured in the agent config. Even if java is in the path. And the first place it looks in is workingDir/jdk/bin/java. Which doesn't exist and then results in the warning message which incorrectly states that java is not in the path. Then java i checked in the path, where it it is found. See DefaultJavaProvider.getJavas(). So even if the logic is not changed and everything still works, we get an incorrect warning log message. The correct way IMO would be to first check if java is in the path before falling back to the old logic in JavaVersionChecker.
            Hide
            ifernandezcalvo Ivan Fernandez Calvo added a comment - - edited

            when checkJavaVersion return java in that case java is in the path of the user that connects through SSH, if checkJavaVersion return a path, it means that java is not in the path

            Show
            ifernandezcalvo Ivan Fernandez Calvo added a comment - - edited when checkJavaVersion return java in that case java is in the path of the user that connects through SSH, if checkJavaVersion return a path, it means that java is not in the path
            Hide
            ahammar Anders Hammar added a comment -

            I don't follow.
            When launching, it's only checked if javaPath (the config field) is set, not path: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/SSHLauncher.java#L448
            If not, JavaVersionChecker.resolveJava() is called: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/SSHLauncher.java#L454
            Then checkJavaVersion() is called for each java command path that DefaultJavaProfiver.getJavas() returns: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/JavaVersionChecker.java#L77
            And here's the problem, getJavas() will return a list where the first element contains a java command path that is NOT "java" but it will be workingDir/jdk/bin/java: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/DefaultJavaProvider.java#L73
            "java" is added later: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/DefaultJavaProvider.java#L79

            So, even if java is in the path, workingDir/jdk/bin/java is tried first (with checkJavaVersion()) and could then (as in our case) cause a warning log message if it does not exist.

            Show
            ahammar Anders Hammar added a comment - I don't follow. When launching, it's only checked if javaPath (the config field) is set, not path: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/SSHLauncher.java#L448 If not, JavaVersionChecker.resolveJava() is called: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/SSHLauncher.java#L454 Then checkJavaVersion() is called for each java command path that DefaultJavaProfiver.getJavas() returns: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/JavaVersionChecker.java#L77 And here's the problem, getJavas() will return a list where the first element contains a java command path that is NOT "java" but it will be workingDir/jdk/bin/java: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/DefaultJavaProvider.java#L73 "java" is added later: https://github.com/jenkinsci/ssh-slaves-plugin/blob/master/src/main/java/hudson/plugins/sshslaves/DefaultJavaProvider.java#L79 So, even if java is in the path, workingDir/jdk/bin/java is tried first (with checkJavaVersion()) and could then (as in our case) cause a warning log message if it does not exist.
            Hide
            ifernandezcalvo Ivan Fernandez Calvo added a comment - - edited

            shit, the problem is the order in the list if your java path match with one before the simple "java" you will see the warning even though java is in the PATH. it is easy if you have JAVA_HOME defined. I am going to rethink how to show the warning without touch the logic.

            I have opened https://issues.jenkins.io/browse/JENKINS-65155

            Show
            ifernandezcalvo Ivan Fernandez Calvo added a comment - - edited shit, the problem is the order in the list if your java path match with one before the simple "java" you will see the warning even though java is in the PATH. it is easy if you have JAVA_HOME defined. I am going to rethink how to show the warning without touch the logic. I have opened https://issues.jenkins.io/browse/JENKINS-65155
            Hide
            ahammar Anders Hammar added a comment -

            I suggest adding a check if java is in the path in SSHLauncher.launch() before JavaVersionChecker is used. If java is not in the path, emit the warning log message. But you still need to use JavaVersionChecker to keep the current guess java behavior. (The warning log in JavaVersionChecker should then be removed.)
            Unfortunately v1.31.6 requires Jenkins 2.282 which means that anyone using Jenkins LTS won't be able to upgrade for at least 4 months. How this agressive requirement could be added in a bugfix I don't understand.

            Show
            ahammar Anders Hammar added a comment - I suggest adding a check if java is in the path in SSHLauncher.launch() before JavaVersionChecker is used. If java is not in the path, emit the warning log message. But you still need to use JavaVersionChecker to keep the current guess java behavior. (The warning log in JavaVersionChecker should then be removed.) Unfortunately v1.31.6 requires Jenkins 2.282 which means that anyone using Jenkins LTS won't be able to upgrade for at least 4 months. How this agressive requirement could be added in a bugfix I don't understand.

              People

              Assignee:
              ifernandezcalvo Ivan Fernandez Calvo
              Reporter:
              ifernandezcalvo Ivan Fernandez Calvo
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: