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-39465] - Fix the AgentProtocol settings persistency handling #2621

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Nov 7, 2016

Well, everybody likes XStream magic...

  • Fix XStream serialization/deserializaation (See Data Format Notes below)
  • Users may have to reconfigure their Settings to make them persistent. Seems to be fine since the original configuration is vroken. Needs to be changelogged in any case
  • Invalidate agentProtocols cache on the configuration reload
  • Add some functional tests for single and multiple protocol redefinition

Data Format Notes

Due to whatever reason, without a definition of an array recipient field the data goes to the disk in the following way:

<enabledAgentProtocol>JNLP3-connect</enabledAgentProtocol>
<enabledAgentProtocol>JNLP4-connect</enabledAgentProtocol>

It is supposed to processed by Implicit array correctly thanks to addImplicitCollection(), but it does not actually happen. With the fix the data is being stored in another format:

  <enabledAgentProtocols>
    <string>JNLP3-connect</string>
    <string>JNLP4-connect</string>
  </enabledAgentProtocols>

This data now works correctly and gets deserialized correctly. readResolve() just adds a fallback for the case when Implicit array handling starts behaving correctly (?).

https://issues.jenkins-ci.org/browse/JENKINS-39465

@reviewbybees, esp. @stephenc

…in Jenkins instance

Due to whatever reason, without a definition of an array recipient field the data goes to the disk in the following way:

```
<enabledAgentProtocol>JNLP3-connect</enabledAgentProtocol>
<enabledAgentProtocol>JNLP4-connect</enabledAgentProtocol>
```

It is supposed to processed by Implicit array correctly, but it does not actually happen.
With a fix the data is being stored in another format:

```
  <enabledAgentProtocols>
    <string>JNLP3-connect</string>
    <string>JNLP4-connect</string>
  </enabledAgentProtocols>
```

This data now works correctly and gets deserialized correctly. readResolve() just adds a fallback for the case when Implicit array handling starts behaving correctly (?).
…en we reload the configuration
@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Nov 7, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@rsandell

This comment has been minimized.

Copy link
Member

rsandell commented Nov 7, 2016

🐝

1 similar comment
@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Nov 7, 2016

🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 7, 2016

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Nov 7, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Nov 7, 2016

@jenkinsci/code-reviewers It would be great to get the review. We want to merge it into the tomorrow's out-of-order release (discussion)

@jglick
jglick approved these changes Nov 7, 2016
Copy link
Member

jglick left a comment

🐝

public void agentProtocols_singleEnable_roundtrip() throws Exception {
final Set<String> defaultProtocols = Collections.unmodifiableSet(j.jenkins.getAgentProtocols());
Assume.assumeThat("We assume that JNLP3-connect is disabled",
defaultProtocols, not(hasItem("JNLP3-connect")));

This comment has been minimized.

Copy link
@jglick

jglick Nov 7, 2016

Member

Is there a reason you are not just using assertEquals to specify the exact set of protocols? So that the test does not need to be updated if and when new protocols are added? But that seems like a trivial amount of work compared to the work of actually integrating the new protocol, and compared to the benefit of having a test which is both stronger (catches more mistakes) and simpler (easier to read).

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Nov 7, 2016

Author Member

I propose to follow-up in a separate PR. BTW, I'm pretty sure that JNLP3-connect will be always disabled. The test depending on JNLP4 status is more flaky

@christ66

This comment has been minimized.

Copy link
Member

christ66 commented Nov 7, 2016

🐝

@oleg-nenashev oleg-nenashev merged commit 3e2e017 into jenkinsci:master Nov 7, 2016
2 checks passed
2 checks passed
Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
oleg-nenashev added a commit that referenced this pull request Nov 7, 2016
olivergondza added a commit that referenced this pull request Nov 8, 2016
…#2621)

* [JENKINS-39465] - Tweak processing of enabled and disabled protocols in Jenkins instance

Due to whatever reason, without a definition of an array recipient field the data goes to the disk in the following way:

```
<enabledAgentProtocol>JNLP3-connect</enabledAgentProtocol>
<enabledAgentProtocol>JNLP4-connect</enabledAgentProtocol>
```

It is supposed to processed by Implicit array correctly, but it does not actually happen.
With a fix the data is being stored in another format:

```
  <enabledAgentProtocols>
    <string>JNLP3-connect</string>
    <string>JNLP4-connect</string>
  </enabledAgentProtocols>
```

This data now works correctly and gets deserialized correctly. readResolve() just adds a fallback for the case when Implicit array handling starts behaving correctly (?).

* [JENKINS-39465] - Add configuration roundtrip tests

* [JENKINS-39465] - Jenkins#agentProtocols cache must be invalidated when we reload the configuration

* [JENKINS-39465] - Remove obsolete comment from Tests

(cherry picked from commit 3e2e017)
Copy link
Member

jglick left a comment

a note for @oleg-nenashev

Assume.assumeThat("We assume that JNLP3-connect is disabled",
defaultProtocols, not(hasItem("JNLP3-connect")));
Assume.assumeThat("We assume that JNLP4-connect is disabled",
defaultProtocols, not(hasItem("JNLP4-connect")));

This comment has been minimized.

Copy link
@jglick

jglick Sep 27, 2017

Member

This makes no sense: the test is skipped.

  <testcase name="agentProtocols_multipleEnable_roundtrip" classname="jenkins.model.JenkinsTest" time="0">
    <skipped message="We assume that JNLP4-connect is disabled: got: &lt;[CLI-connect, CLI2-connect, JNLP-connect, JNLP2-connect, JNLP4-connect, Ping]&gt;, expected: not a collection containing &quot;JNLP4-connect&quot;"/>
    <system-out><![CDATA[=== Starting agentProtocols_multipleEnable_roundtrip(jenkins.model.JenkinsTest)
]]></system-out>
    <system-err><![CDATA[Sep 27, 2017 2:31:48 PM org.jvnet.hudson.test.JenkinsRule createWebServer
INFO: Running on http://localhost:45889/jenkins/
Sep 27, 2017 2:31:48 PM jenkins.InitReactorRunner$1 onAttained
INFO: Started initialization
Sep 27, 2017 2:31:48 PM jenkins.InitReactorRunner$1 onAttained
INFO: Listed all plugins
Sep 27, 2017 2:31:48 PM jenkins.InitReactorRunner$1 onAttained
INFO: Prepared all plugins
Sep 27, 2017 2:31:48 PM jenkins.InitReactorRunner$1 onAttained
INFO: Started all plugins
Sep 27, 2017 2:31:48 PM jenkins.InitReactorRunner$1 onAttained
INFO: Augmented all extensions
Sep 27, 2017 2:31:49 PM jenkins.InitReactorRunner$1 onAttained
INFO: Loaded all jobs
Sep 27, 2017 2:31:49 PM jenkins.slaves.DeprecatedAgentProtocolMonitor initializerCheck
WARNING: This Jenkins instance uses deprecated Remoting protocols: CLI-connect,CLI2-connect,JNLP-connect,JNLP2-connectIt may impact stability of the instance. If newer protocol versions are supported by all system components (agents, CLI and other clients), it is highly recommended to disable the deprecated protocols.
Sep 27, 2017 2:31:49 PM jenkins.InitReactorRunner$1 onAttained
INFO: Completed initialization
Sep 27, 2017 2:31:49 PM jenkins.model.Jenkins cleanUp
INFO: Stopping Jenkins
Sep 27, 2017 2:31:49 PM jenkins.model.Jenkins cleanUp
INFO: Jenkins stopped
]]></system-err>
  </testcase>

Why would these tests need to Assume anything? There should be nothing environmental that would affect whether or not the test could be run: we are controlling this aspect of the test environment.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 28, 2017

Author Member

agreed, the test needs to be reworked.

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.