Creating JenkinsLocationConfiguration when the instance is ready

[joseblas]

No description provided.

[basil]

Hello @joseblas, thank you for your contribution to the open-source Jenkins test harness. I do not doubt you are facing a legitimate problem, and I would love to see a solution integrated. However, the problem you are facing is not clear to me, even after reading BEE-16540. It is also important to note that other members of the open-source Jenkins community do not have access to this issue tracker, so it may be better for you to file an issue in this repository's issue tracking system as well as to avoid using CloudBees-specific terminology in your code comments. At any rate, I seem to vaguely recall having some issues relating to JenkinsLocationConfiguration testing open-source Jenkins plugins in the past, so I am curious to hear more about your problem to see if it jogs my memory. Once we have a clear understanding of the problem you are facing, we can evaluate the design and implementation of possible solutions, including the one you have proposed.

[joseblas]

Hi @basil,

first of all, apologies. That slipped bc my day to day work.

My problem is when starting a Jenkins instance using RealJenkinsRule JenkinsLocationConfiguration is not yet created.

This change, as mentioned in test testJenkinsLocationConfiguration, makes sure that is created after the port is available.

This proposal writes that file just after the port is available when using supportsPortFileName. I was considering to add a boolean named createJenkinsLocationConfiguration to use this just when needed.
Also, bc this will provide flexibility for other kid of tests.

[basil]

Thanks @joseblas, I think we need to understand a few more things:

• When and how is JenkinsLocationConfiguration normally created in a production scenario? We want RealJenkinsRule to be as close to a production scenario as possible for the most realistic testing. Somehow I doubt that the production scenario is writing out a string of XML to a file as you are doing here.
• What are the consequences of running without a JenkinsLocationConfiguration (i.e., what is the use case for needing the test harness to do this)? Based on this use case we could write a more realistic test that verifies the goal (the actual use case) rather than the mechanism (writing the XML file).
• A related question is: is this already handled by JenkinsRule? If so, we might draw some inspiration from that implementation for the RealJenkinsRule version.

[joseblas]

Sure @basil

• There are two options: manually or with jcasc. In this case is simulating jcasc functionality.
• The instance cannot provide a deterministic URL of itself. Is the Jenkins url in the Configuration. Therefore, cannot provide info re other external systems to connect unto.
• I don't know if has been addresses by JenkinsRule as this use case needs RealJenkinsRule bc of the external process.

[timja]

When and how is JenkinsLocationConfiguration normally created in a production scenario

It's created in the setupwizard or with jcasc

[basil]

I don't know if has been addresses by JenkinsRule as this use case needs RealJenkinsRule bc of the external process.

The implication was that you should do some research to find out, since the answer to this question is important context to have in mind when evaluating the design of this proposed fix. Had you done this research, you would have seen that not only does JenkinsRule create this file in https://github.com/jenkinsci/jenkins-test-harness/blob/0038ef9b8a0bb2fec2ef4be67d1124f51db53076/src/main/java/org/jvnet/hudson/test/JenkinsRule.java#L438= but also that it creates it in a less fragile way than writing out a string of hard-coded XML to disk (rather, it uses XStream to serialize the Java object to disk).

And in fact, so does RealJenkinsRule in https://github.com/jenkinsci/jenkins-test-harness/blob/0038ef9b8a0bb2fec2ef4be67d1124f51db53076/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java#L807= And putting a breakpoint on that line and running RealJenkinsRuleTest#smokes clearly does create jenkins.model.JenkinsLocationConfiguration.xml. But running your new RealJenkinsRuleTest#testJenkinsLocationConfiguration test in a debugger, we do not hit the same breakpoint. This points to the proximate cause of your problem: somehow, your new RealJenkinsRuleTest#testJenkinsLocationConfiguration (and presumably your actual use case) which only calls RealJenkinsRule#startJenkins is unexpectedly not executing https://github.com/jenkinsci/jenkins-test-harness/blob/0038ef9b8a0bb2fec2ef4be67d1124f51db53076/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java#L807= but RealJenkinsRuleTest#smokes (and presumably not your actual use case) which calls RealJenkinsRule#then is expectedly executing https://github.com/jenkinsci/jenkins-test-harness/blob/0038ef9b8a0bb2fec2ef4be67d1124f51db53076/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java#L807=.

The next step from here is to answer the question: "Why?" I would suggest going through both code paths in the debugger and examining what is the key difference between the two execution scenarioes that results in the expected behavior in one and unexpected behavior in the other. When doing this exercise, think about what the expected calling convention (e.g., #then, #startJenkins, etc) is and what the expected behavior is for each calling convention. Then observe what actually happens with regard to hitting https://github.com/jenkinsci/jenkins-test-harness/blob/0038ef9b8a0bb2fec2ef4be67d1124f51db53076/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java#L807=. You may discover that #then (which calls #startJenkins, #runRemotely, and #stopJenkins) is creating jenkins.model.JenkinsLocationConfiguration.xml in the #runRemotely method. That might motivate you to change your code to conform to the expected calling convention (e.g., using #then or adding a #runRemotely call between your #startJenkins and #stopJenkins calls), or it might motivate changing the semantics of the existing methods to match your expectations (e.g., moving the creation of jenkins.model.JenkinsLocationConfiguration.xml from #runRemotely to #startJenkins).

You requested a second review from me without addressing the feedback about CloudBees-specific terminology that I provided in my first review and without doing the research (key to understanding the nature of the problem) that I suggested in my first review. That is no big deal, but I would ask that in the future you go through my review feedback systematically before requesting subsequent reviews. In particular, before requesting a third review I would ask that you go through the exercise I described above to take the proximate cause of the problem to a root cause, describing the two code paths (preferably with stack traces) and explaining in writing the reason why one code path results in the expected result and the other code path results in the unexpected result. Once this root cause analysis has been provided, I would be happy to review this PR a third time.

[joseblas]

hi @basil,

Apologies for not creating the issue here, already done, I did not take that as wanted. Again, apologies.

1. JenkinsRule creates the file itself, but RealJenkinsRule cannot do it because is a fork and there is no longer access to the Jenkins.get() instance. That's way it uses XStream, in this PR cannot be done that way as the Jenkins.get() is not available.
2. In this case, I already tested those options, but didn't mention. Apologies again. then cannot be used because there are another two (Jenkins) instances in the process. I did not even try as the code looked unreadable. Also an extra runRemotely doesn't work as the process occurs with jcasc which means the jcasc code is execute before the runRemotely.
3. I have created an issue in this repo, I hope that is what you mean as avoid Cloudbees-terminology, which btw is only the initiative that created this change, not the change itself.

[basil]

JenkinsRule creates the file itself, but RealJenkinsRule cannot do it

RealJenkinsRule clearly can do it, as it does when using #then (which you noted in #467).

I did not even try as the code looked unreadable

Seems perfectly readable to me. I suggest spending more time stepping through the code in a debugger, as mentioned in my previous post.

I hope that is what you mean as avoid Cloudbees-terminology

No, I meant these code comments in your PR which refer to concepts that do not exist in open-source Jenkins:

This file is needed before for ConnectedMasters connection to OCs.

The comments in my previous post about root-cause analysis still apply.

[joseblas]

Hi @basil,

thanks for the feedback. I've already changed the comments.

I've spent time with debugger and code analysis and, to be honest, I think using startJenkins should have the same functionality as then. Otherwise, makes no sense to have startJenkins accessible from the outside if all the cases can fit within then.

In my particular case I have three instances running with class inheritance which makes the hardly readably, but that could be unrelevant, but I think is important to have the same functionality with both then and start/stopJenkins alternatives.

[jglick]

I am also unclear on what problem is being solved here. #467 says something about port assignment (#348 I presume) but that should be transparent to tests. Is there something which worked before which broke—what? And what is the relationship to JCasC exactly? Is it really reading $JENKINS_HOME/jenkins.model.JenkinsLocationConfiguration.xml directly rather than going through the normal API methods? If not, then you should be able to demonstrate the actual problem you have in a minimal, self-contained way, perhaps with the help of a mock plugin that does something earlier in startup than a runRemotely block would capture.

[jglick]

Use the Matchers import. Anyway I think you would want to assert the actual XML content.

[jglick]

the code looked unreadable

I am not even sure what code is being discussed here. One common way to demonstrate concretely why an API or other change is desirable, even if not strictly necessary, is to first write a passing test using a supposedly awkward workaround, commit it, then introduce the proposed change in a commit alongside a simplification to the test case.

[basil]

I've spent time with debugger and code analysis and, to be honest, I think using startJenkins should have the same functionality as then. Otherwise, makes no sense to have startJenkins accessible from the outside if all the cases can fit within then.

That seems plausible but I would want to better understand the use case for not using #then to be sure. And if we do want to go in this direction, it would be preferable to extract the implementation from #then into common code that could be invoked by both #then and #startJenkins rather than adding a second and fragile implementation that manipulates XML outside of the XStream framework.

I am not even sure what code is being discussed here.

I suspect the author of this PR may be referring to the code in RealJenkinsRule that invokes JenkinsLocationConfiguration.get().setUrl(url.toExternalForm()) in the controller JVM, but I concur the author of this PR is not communicating clearly so it is difficult to be sure.

[joseblas]

Let me try to explain all history behind this.

A few weeks I started developing some testing classes for Jenkins, involving several instances. As a matter of fact, three instances which two of them had to get a connection file from the other one. As the testing is complex, I did not use then. Which apparently was the only way of having JenkinsLocationConfiguration (JLC from now on) file.

So I tried to get a workaround for this. As JCasC was involved in my testing and before 2.339 you could get the port before starting Jenkins I could pass it through JCasC,

        cc.extraEnv("CC_LOCATION_URL", cc.getUrl().toString());
        cc.startJenkins();

After upgrading to +2.339 this stopped working as the port is not available beforehand. So I start to check again why. The reason was the same: JLC file is not created.

So I try another way of creating a JLC file. After startJenkins running a runRemotely did not work as the instances was already up and running. And the previous workaround wasn't available as the URL was not available before calling startJenkins.

So it had to stay within startJenkins method, but could not use XStream because the RealJenkinsRule was executed in an external JVM (the main point if this implementation). The code that runs startJenkins is not executed inside a Jenkins instance.

I've added three tests, with a new method in RealJenkinsRule: createJenkinsLocationConfigurationFileWhenStartingRealJenkinsRule()

which is a boolean to create the JLC file (or not) when stating Jenkins.

First one:

     @Test public void testJenkinsLocationConfigurationIsNotCreated() throws Throwable {
        rr.startJenkins();
        String[] jenkinsLocations = rr.getHome().list(
                (dir, name) -> name.equalsIgnoreCase("jenkins.model.JenkinsLocationConfiguration.xml"));
        assertThat(jenkinsLocations, arrayWithSize(0));
    }

This test shows that the JLC file is not created when starting Jenkins with RealJenkinsRule.

Then the second one for showing that JLC file is indeed created when using then,

    @Test public void testJenkinsLocationConfigurationIsCreatedWithThen() throws Throwable {
        rr.then(jr -> {
            String[] jenkinsLocations = jr.jenkins.getRootDir().list(
                    (dir, name) -> name.equalsIgnoreCase("jenkins.model.JenkinsLocationConfiguration.xml"));
            assertThat(jenkinsLocations, arrayWithSize(1));

            File file = new File(jr.jenkins.getRootDir(), jenkinsLocations[0]);
            assertThat(file, anExistingFile());
            String expected = "<?xml version='1.1' encoding='UTF-8'?>\n"
                                      + "<jenkins.model.JenkinsLocationConfiguration>\n"
                                      + "  <jenkinsUrl>"+jr.jenkins.getRootUrl()+"</jenkinsUrl>\n"
                                      + "</jenkins.model.JenkinsLocationConfiguration>";;
            assertThat(FileUtils.readFileToString(file, Charset.defaultCharset()), is(expected));
        });
    }

note: with then is created bc of this:

        startJenkins();
        try {
            return runRemotely(s);
        } finally {
            stopJenkins();
        }
    }

and run remotely calls a remote method for execution RealJenkinsRule.doStep

 try (CustomJenkinsRule rule = new CustomJenkinsRule(url); ACLContext ctx = ACL.as(ACL.SYSTEM)) {
                        return s.run(rule);
                    } catch (Throwable t) {
                        throw new RuntimeException(t);
                    }

and CustomJenkinsRule constructor is:

 public CustomJenkinsRule(URL url) throws Exception {
            this.jenkins = Jenkins.get();
            this.url = url;
            jenkins.setNoUsageStatistics(true); // cannot use JenkinsRule._configureJenkinsForTest earlier because it tries to save config before loaded
            JenkinsLocationConfiguration.get().setUrl(url.toExternalForm());
            testDescription = Description.createSuiteDescription(System.getProperty("RealJenkinsRule.description"));
            env = new TestEnvironment(this.testDescription);
            env.pin();
        }

And also is created when using the new feature added:

    @Test public void testJenkinsLocationConfigurationIsCreated() throws Throwable {
        rr
            .createJenkinsLocationConfigurationFileWhenStartingRealJenkinsRule()
            .startJenkins();

        String[] jenkinsLocations = rr.getHome().list(
                (dir, name) -> name.equalsIgnoreCase("jenkins.model.JenkinsLocationConfiguration.xml"));
        assertThat(jenkinsLocations, arrayWithSize(1));

        File file = new File(rr.getHome(), jenkinsLocations[0]);
        assertThat(file, anExistingFile());
        String expected = "<?xml version='1.1' encoding='UTF-8'?>\n"
                                                 + "<jenkins.model.JenkinsLocationConfiguration>\n"
                                                 + "  <jenkinsUrl>"+rr.getUrl()+"</jenkinsUrl>\n"
                                                 + "</jenkins.model.JenkinsLocationConfiguration>";;
        assertThat(FileUtils.readFileToString(file, Charset.defaultCharset()), is(expected));
    }

[joseblas]

After some digging I think is not possible to use then to run several Jenkins instances at the same time.

If one instance is created at top level (as @rule or @ClassRule), then the second one has to be started. And that then call has to be done within the first then, which means to pass the second RealJenkinsRule to the first instance as param in runRemotely. Which I think doesn't work.

This, doesn't work bc you cannot create a RealJenkinsRule within then

            String[] jenkinsLocations = s.jenkins.getRootDir().list((dir, name) -> name.equalsIgnoreCase(
                    "jenkins.model.JenkinsLocationConfiguration.xml"));
            System.out.println(" First One "+ jenkinsLocations);
            System.out.println(" First One "+ cc2);

            cc2.then(s2 -> {
                String[] jenkinsLocations2 = s2.jenkins.getRootDir().list((dir, name) -> name.equalsIgnoreCase(
                        "jenkins.model.JenkinsLocationConfiguration.xml"));
                System.out.println(" First One2 "+ jenkinsLocations2);

            });

        });

Throws a NPE here,
RealJenkinsRule.java:516 (startJenkins method)

pb.environment().put("JENKINS_HOME", home.getAbsolutePath());

[jglick]

you cannot create a RealJenkinsRule within then

Of course not; the Step is run on a different JVM.

What none of the above discussion, nor the new test coverage, actually explains is the purpose of all this. Why do you expect a file named jenkins.model.JenkinsLocationConfiguration.xml to exist at a certain time to begin with? To all appearances, and for all tests that I know about (including plenty of CloudBees tests involving multiple controllers), the right Jenkins URL is available when code asks for it. I can only emphasize the second bullet point of #464 (comment).

My impression is that this is somehow related to CloudBees proprietary code (not jenkinsci/configuration-as-code-plugin per se) but if so what this PR should contain (repeating #464 (review)) is a demonstration of an actual scenario involving an actual plugin (a fake one in src/test/resources/plugins/*.jpi if necessary), one or more controllers as appropriate, and some sort of logic flow which implicitly requires the Jenkins root URL to be set in some form at a particular (early?) point that models the problem you really had. This allows us to focus conversation on the best way to solve that problem—writing out raw XML text at some point, some other change to RJR, or redesigning your test to work with existing versions of RJR. (And at that point the current test cases are redundant, overly low-level, and could be deleted.)

[jglick]

As previously discussed, and looks like this is currently in a draft condition anyway.

[jglick]

Currently unused.

[jglick]

Do not pass a lambda or anonymous inner class to runRemotely.

[joseblas]

Closing this PR bc what this wanted to solve was not a real need.