Pin the last Jenkins package which uses sysvinit scripts. #119

nuclearsandwich · 2022-12-02T16:35:54Z

Earlier this year the packaged versions of Jenkins migrated to systemd units from sysvinit scripts. The current versions of the Jenkins chef cookbook and this cookbook both expect Jenkins to run using sysvinit and make configuration changes using the associated configuration files which are not used by the systemd service.

As a result, newer versions of the Jenkins will not initialize correctly with this cookbook.

Until the upstream Jenkins cookbook is updated to use systemd or we stop using the upstream cookbook entirely, we can't move forward from this Jenkins version.

It is possible for an already-configured farm to continue working after an upgrade and Jenkins cannot be safely downgraded so this version pin is a breaking change and additional work to protect from an unintended downgrade may be warranted.

nuclearsandwich · 2022-12-02T21:55:16Z

#120 fixes the problem with the repo CI scripts. I may rebase this PR if that merges first.

nuclearsandwich · 2022-12-03T01:28:03Z

attributes/jenkins.rb

+# Last version supporting Java 8
+#default['jenkins']['master']['version'] = '2.346.1'


This is a bit of a carry on from where this change originated (#117) but I'm interested in leaving it in as a signifier for other build farm maintainers. I expect that a PR and release between now and Java 11 support will raise our "high water level" to 2.346.1.

nuclearsandwich · 2022-12-03T01:28:30Z

recipes/jenkins.rb

+# However Jenkins cannot be safely downgraded and some configurations of
+# Jenkins will continue to function once initially configured even after the
+# systemd switch.
+# To protect users from unintentional downgrades we're going to do something really ugly here.


The rest of this gets rough. I'm very sorry but also a little proud.

nuclearsandwich · 2022-12-03T01:56:51Z

recipes/jenkins.rb

+# To protect users from unintentional downgrades we're going to do something really ugly here.
+package 'jenkins' do
+  action :lock
+  only_if { -> do


The { opens the Ruby block that chef will eval at convergence time and then within it I immediately start a lambda literal with -> do (affectionately known as the "stabby lambda" back in my day) creating a block within a block.

I need to do this in order to use early returns in the code below since using return or break in a block literal will result in a LocalJumpError. I'll call the lambda as soon as I close it passing the result to the outer block.

nuclearsandwich · 2022-12-03T01:57:29Z

recipes/jenkins.rb

+  only_if { -> do
+    jenkins_info = `dpkg -s jenkins`
+    if $?.exitstatus != 0
+       return false


If Jenkins isn't installed the command above will fail and we can proceed nothing to worry about.

nuclearsandwich · 2022-12-03T01:58:42Z

recipes/jenkins.rb

+
+    version_line = jenkins_info.lines.select{|line| line =~ /^Version: /}.first
+    if version_line.nil?
+      return false


I'm not actually sure that this code is reachable, I can't think of a situation where the command above would exit successfully but there wouldn't be any version info from it.

I may decide to take this out or turn it into a hard error because it's "wrong"

Eh, you're parsing output from a command line tool. I think it's totally reasonable to have a protection like this.

Not that you should support the scenario, but I could see something like localization resulting in nil here.

I could see something like localization resulting in nil here.

Yeah, I think you're right and on consideration if we get here we should make the user stop and figure out how and what to do about it. 9fb0a78

nuclearsandwich · 2022-12-03T02:00:05Z

recipes/jenkins.rb

+    if version_components[0].to_i > 2 or version_components[1].to_i >= 332
+      node.run_state[:jenkins_package_version_lock] = version_components.join('.')
+      Chef::Log.warn("Chef detected this Jenkins version: #{node.run_state[:jenkins_package_version_lock]}")
+      return true


Earlier attempts at this change tried to just override the node attribute within this block but due to the compile-then-converge nature of chef the attribute value has already been read by the recipe I'm trying to change before this converge-time code runs so instead I put a sigil value in the run_state and get nasty later.

nuclearsandwich · 2022-12-03T02:00:51Z

recipes/jenkins.rb

+    end
+    # Jenkins is installed but is older than the maximum and can be upgraded.
+    return false
+  end.call }


This is calling the lambda literal and closing the only_if block value argument. I'm not happy this is necessary but it's the cleanest way I could phrase the code block within and that is what mattered to me.

nuclearsandwich · 2022-12-03T02:04:21Z

recipes/jenkins.rb

+ruby_block 'prevent jenkins downgrade' do
+  block do
+    if node.run_state[:jenkins_package_version_lock]
+      if node['jenkins']['master']['version'] != node.run_state[:jenkins_package_version_lock]
+        Chef::Log.fatal("Before this cookbook continues. please set the node['jenkins']['master']['version'] attribute to #{node.run_state[:jenkins_package_version_lock]} or this cookbook will attempt to downgrade your Jenkins version.")
+        Chef::Log.fatal("See https://github.com/ros-infrastructure/cookbook-ros-buildfarm/issues/121 for more information")
+        raise
+      end
+    end
+  end
+end


By the time we know that we want to, we can't affect the attribute value used by the jenkins cookbook. If we did nothing here, the user would actually get a failure within the Jenkins cookbook when they try to downgrade a package marked for holding. But if we let that happen we risk them not knowing why that is happening and potentially releasing the hold.
So we'll check our sigil value and instead recommend they set the jenkins version that they are currently on which will keep them at their current state until we can make the necessary changes in this cookbook (and the upstream Jenkins one) to unblock further updates.

This is still suboptimal because anyone who is provisioning from scratch using an existing config will experience failures unless they revert back to our new default version for the first provisioning.

cottsay

Minor comments, but looks good.

Disclaimer: IANAR (I Am Not A Rubist)

cottsay · 2024-03-29T15:41:07Z

recipes/jenkins.rb

+
+    version_line = jenkins_info.lines.select{|line| line =~ /^Version: /}.first
+    if version_line.nil?
+      return false


Eh, you're parsing output from a command line tool. I think it's totally reasonable to have a protection like this.

Not that you should support the scenario, but I could see something like localization resulting in nil here.

cottsay · 2024-03-29T15:44:55Z

recipes/jenkins.rb

+    # Transform "Version: 2.319.1\n" to ["2", "319", "1"]
+    version_components = version_line.chomp.split(": ")[1].split(".")
+    # The first systemd versions are 2.335 (weekly) and 2.332.1 (LTS)
+    if version_components[0].to_i > 2 or version_components[1].to_i >= 332


Could this be cleaned up with the "spaceship operator"?

<=> implements three-way comparison for a class. I don't actually think it helps us unless we parse these version components into an actual class and implement the component level comparisons. Although you could be seeing something tidy that I'm not.

The logic here is complex enough that I was optimizing for plain readability rather than conciseness. I suppose that if we had a Comparable version class here we would be able to write

version_installed = Version(version_line.chomp.split(": ")[1].split(".")) if version >= Version([2,332,1])

recipes/jenkins.rb

Co-authored-by: Scott K Logan <[email protected]>

This version of the Jenkins cookbook updates the keys used to authenticate the jenkins apt repository.

I don't think this code is reachable in normal circumstances, Scott pointed out that it could be a result of a change in CLI output or localization differences in output. With that in mind, it makes more sense to error in these cases since if the package is installed but at an unknowable version we should stop and make the user figure it out.

nuclearsandwich self-assigned this Dec 2, 2022

nuclearsandwich mentioned this pull request Dec 2, 2022

Use a test public key with no expiration date. #120

Closed

nuclearsandwich mentioned this pull request Dec 3, 2022

Switch to systemd in Jenkins packages breaks our cookbook recipes. #121

Open

nuclearsandwich commented Dec 3, 2022

View reviewed changes

nuclearsandwich requested a review from Crola1702 March 21, 2024 04:06

nuclearsandwich mentioned this pull request Mar 21, 2024

Use cinc for test kitchen. #133

Merged

cottsay approved these changes Mar 29, 2024

View reviewed changes

nuclearsandwich and others added 4 commits April 25, 2024 10:31

Pin the last Jenkins package which uses sysvinit scripts.

5b15022

Add a convoluted hold and runtime check to prevent Jenkins downgrade.

c7ac2e0

Co-authored-by: Scott K Logan <[email protected]>

Update jenkins cookbook to fix upstream GPG key change.

51227c9

This version of the Jenkins cookbook updates the keys used to authenticate the jenkins apt repository.

nuclearsandwich force-pushed the supported-jenkins-version branch from 9fb0a78 to be95d8d Compare April 25, 2024 17:32

nuclearsandwich merged commit eb00797 into latest Apr 25, 2024
4 checks passed

nuclearsandwich deleted the supported-jenkins-version branch April 25, 2024 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pin the last Jenkins package which uses sysvinit scripts. #119

Pin the last Jenkins package which uses sysvinit scripts. #119

nuclearsandwich commented Dec 2, 2022

nuclearsandwich commented Dec 2, 2022

nuclearsandwich Dec 3, 2022

nuclearsandwich Dec 3, 2022

nuclearsandwich Dec 3, 2022

nuclearsandwich Dec 3, 2022

nuclearsandwich Dec 3, 2022

cottsay Mar 29, 2024

nuclearsandwich Apr 25, 2024

nuclearsandwich Dec 3, 2022

nuclearsandwich Dec 3, 2022

nuclearsandwich Dec 3, 2022

cottsay left a comment

cottsay Mar 29, 2024

cottsay Mar 29, 2024

nuclearsandwich Apr 25, 2024

		# Last version supporting Java 8
		#default['jenkins']['master']['version'] = '2.346.1'

Pin the last Jenkins package which uses sysvinit scripts. #119

Pin the last Jenkins package which uses sysvinit scripts. #119

Conversation

nuclearsandwich commented Dec 2, 2022

nuclearsandwich commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment