JENKINS-56937 JCasC support for admin monitors #4552

timja · 2020-03-07T11:19:23Z

Example config:

jenkins:
  disabledAdministrativeMonitors:
    - "jenkins.security.s2m.MasterKillSwitchWarning"

Proposed changelog entries

Entry 1: JENKINS-56937, configuration-as-code plugin support for disable admin monitors
...

Proposed upgrade guidelines

N/A

Submitter checklist

JIRA issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a JIRA issue should exist and be labeled as lts-candidate

timja · 2020-03-07T11:20:53Z

core/src/main/java/hudson/model/AbstractCIBase.java

+     *
+     * @since TODO
+     */
+    public Set<String> getDisabledAdministrativeMonitors(){


https://github.com/search?q=org%3Ajenkinsci+getDisabledAdministrativeMonitors&type=Code

So what's buggy about the CopyOnWriteArraySet?

https://github.com/search?q=org%3Ajenkinsci+getDisabledAdministrativeMonitors&type=Code

As expected, given it was @Restricted.

So what's buggy about the CopyOnWriteArraySet?

JCasC doesn't support it

to avoid sharing the same data structure w/ the UI, why not just do something like this?

synchronized(this.disabledAdministrativeMonitors) { return new HashSet<String>(this.disabledAdministrativeMonitors); };

... then any client of this class doesn't need to worry about concurrency concerns in the internal data structure; those concerns remain .. internal to this implementation

The issue is that clients modify this set, so returning a copy prevents them updating it

ouch. any reason that clients can't be modified to invoke the setter instead of relying on directly mutating the internal state of this object? the current situation sounds like a pretty leaky design

I suggested modifying it a couple of comments up, will take a look

have changed, PTAL

suggest wrapping this return ... statement w/ synchronized(disabledAdministrativeMonitors) {} as suggested above, since the new HashSet<>(..) operation will iterate over the collection passed to it

jetersen

LGTM

res0nance · 2020-03-07T15:27:35Z

core/src/main/java/hudson/model/AbstractCIBase.java

+     * @since TODO
+     */
+    public void setDisabledAdministrativeMonitors(Set<String> disabledAdministrativeMonitors){
+        this.disabledAdministrativeMonitors = disabledAdministrativeMonitors;


Probably should be cloned here instead of taking a reference to an external object

Why’s that? It’s just a setter

both stapler and jcasc will handle creating a clone that should be safe for Jenkins core to use.

Spotbugs normally warns against taking a reference to an outside object

not on collections or simple object types.

Spotbugs would complain on date objects.

res0nance · 2020-03-07T15:30:19Z

core/src/main/java/hudson/model/AbstractCIBase.java

@@ -93,13 +93,26 @@ protected void killComputer(Computer c) {
    * Package-protected, but accessed API
    * ============================================================================================================== */

-    /*package*/ final CopyOnWriteArraySet<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();
+    private Set<String> disabledAdministrativeMonitors = new HashSet<>();


There is a gigantic comment above that you might want to consider removing

It was probably already obsolete after #3873

alecharp

I get the visibility change but I'm not so sure about the type change..

alecharp · 2020-03-12T10:04:45Z

core/src/main/java/hudson/model/AbstractCIBase.java

+     *
+     * @since TODO
+     */
+    public void setDisabledAdministrativeMonitors(Set<String> disabledAdministrativeMonitors){


Suggested change

public void setDisabledAdministrativeMonitors(Set<String> disabledAdministrativeMonitors){

public void setDisabledAdministrativeMonitors(Set<String> disabledAdministrativeMonitors) {

rsandell

Makes me slightly nervous removing the thread secure CopyOnWriteArraySet to a non thread safe HashSet

daniel-beck · 2020-03-12T10:33:53Z

Can we keep it a CopyOnWriteArraySet internally to not break when a badly timed https://github.com/nisarg1499/Jenkins/blob/b2f718fbf2965132aef5c1391803f71f5bc7ca05/core/src/main/java/hudson/model/AdministrativeMonitor.java#L121-L122 arrives?

timja · 2020-03-12T11:07:58Z

Can we keep it a CopyOnWriteArraySet internally to not break when a badly timed nisarg1499/Jenkins:core/src/main/java/hudson/model/AdministrativeMonitor.java@b2f718f#L121-L122 arrives?

probably, i'll take a look

timja · 2020-03-14T16:31:47Z

Can we keep it a CopyOnWriteArraySet internally to not break when a badly timed nisarg1499/Jenkins:core/src/main/java/hudson/model/AdministrativeMonitor.java@b2f718f#L121-L122 arrives?

Is what I've pushed what you meant?

daniel-beck · 2020-03-15T11:14:25Z

Is what I've pushed what you meant?

Sort of. I'd also sprinkle in some volatile and/or synchronized until it looks about right 😁

timja

how's this @daniel-beck ?

core/src/main/java/hudson/model/AbstractCIBase.java

timja · 2020-03-15T11:26:52Z

core/src/main/java/hudson/model/AbstractCIBase.java

-    * ============================================================================================================== */
-
-    /*package*/ final CopyOnWriteArraySet<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();
+    private Set<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();


Suggested change

private Set<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();

private volatile Set<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();

daniel-beck · 2020-03-15T11:44:22Z

Better to ask someone else.

timja · 2020-03-15T16:06:46Z

Better to ask someone else.

@jglick or @jvz any chance you could take a look at the concurrency concern?

jvz · 2020-03-16T16:11:48Z

If you want to make that correct, you'll need to make both the getter and setter synchronized as well as any other (public or externally-callable) methods that access or mutate the set. At that point, you can also just use HashSet since everything would be protected by the synchronization mutex.

Alternatively, you could work at being almost correct by changing the setter to do a clear() followed by an addAll() rather than changing the field itself. Then no synchronization or volatile is needed, and it only introduces a fairly insignificant race condition.

timja · 2020-03-16T16:34:07Z

If you want to make that correct, you'll need to make both the getter and setter synchronized as well as any other (public or externally-callable) methods that access or mutate the set. At that point, you can also just use HashSet since everything would be protected by the synchronization mutex.

Alternatively, you could work at being almost correct by changing the setter to do a clear() followed by an addAll() rather than changing the field itself. Then no synchronization or volatile is needed, and it only introduces a fairly insignificant race condition.

Thanks, minding taking a look at what I pushed to see if that would work

jvz · 2020-03-16T16:54:59Z

core/src/main/java/hudson/model/AbstractCIBase.java

-    * ============================================================================================================== */
-
-    /*package*/ final CopyOnWriteArraySet<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();
+    private volatile Set<String> disabledAdministrativeMonitors = new HashSet<>();


My mistake; this will likely still need to be either CopyOnWriteArraySet or use Collections.synchronizedSet() around this. Otherwise, a caller that iterates on the result of getDisabledAdministrativeMonitors() may get a ConcurrentModificationException when another thread calls disable() which modifies the set.

Alternatively, you can update getDisabledAdministrativeMonitors to wrap the set in a new HashSet to copy the set and prevent the issue entirely.

Alternatively, you can update getDisabledAdministrativeMonitors to wrap the set in a new HashSet to copy the set and prevent the issue entirely.

That doesn't work as the set is modified through the getter, by calling add on it.

jvz

Wait, just saw your comment. If you're also modifying the returned result, then this still won't work properly. What you can do instead would be to store a Collections.synchronizedSet(new HashSet<>()) in the field (make it final). Next, the methods that were previously synchronized should instead synchronized (disabledAdministrativeMonitors) in a block inside the method (remove synchronized from the method keywords). Then callers adding to the set will be synchronized on the same lock. Finally, in the setter, you should (inside the synchronized (disabledAdministrativeMonitors) { ... } block) clear followed by addAll.

core/src/main/java/hudson/model/AbstractCIBase.java

jvz · 2020-03-16T18:50:37Z

Thanks for your patience with my confusion here. I believe this last change I suggested should put things in the proper place!

core/src/main/java/hudson/model/AbstractCIBase.java

res0nance · 2020-03-17T00:17:42Z

core/src/main/java/hudson/model/AbstractCIBase.java

-    /* =================================================================================================================
-    * Package-protected, but accessed API
-    * ============================================================================================================== */
+    private final Set<String> disabledAdministrativeMonitors = Collections.synchronizedSet(new HashSet<>());


The docs of synchronizedSet says (I've never used this before)

Returns a synchronized (thread-safe) set backed by the specified set. In order to guarantee serial access, it is critical that all access to the backing set is accomplished through the returned set. It is imperative that the user manually synchronize on the returned set when iterating over it: Set s = Collections.synchronizedSet(new HashSet()); ... synchronized (s) { Iterator i = s.iterator(); // Must be in the synchronized block while (i.hasNext()) foo(i.next()); } Failure to follow this advice may result in non-deterministic behavior.

I'm not sure what it means by Failure to follow this advice may result in non-deterministic behavior. in here

There's no direct iteration of this field,

there's 4 usages of it,

enable / disable, calls add /remove

isEnabled, calls contains

the UI, which iterates over it, not sure if there's much we can do there,

jcasc, it will use the setter to update it which will clear the set and use what jcasc sets.

My worry is what is the worst that could happen? Will it throw an exception or will it just show old data?
If its the latter then I think its alright but the documentation doesn't seem perfectly clear

#3687 (comment) was the (un)funnest one I've seen. Way up there in my hall of shame ☹️

That's only relevant because iterating over a collection is technically n operations, not 1. You need to synchronize for the duration of the logical operation, not n separate synchronizations for n items in the collection. This is all about fencing the allowed execution timeline based on "happens-before" rules in Java.

So, in a sense, no, peppering synchronized and volatile everywhere isn't good enough to solve every concurrency issue! 😅

jdef · 2020-03-24T17:13:36Z

I suggested returning a copy of the Set. There are no concurrency concerns with the returned object

…

On Tue, Mar 24, 2020, 12:35 PM Matt Sicker ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/hudson/model/AbstractCIBase.java <#4552 (comment)>: > - @restricted(NoExternalUse.class) - public CopyOnWriteArraySet<String> getDisabledAdministrativeMonitors(){ + /** + * Get the disabled administrative monitors + * + * @SInCE TODO + */ + public Set<String> getDisabledAdministrativeMonitors(){ It's to prevent concurrent modification exceptions. If you modify a non-threadsafe collection while iterating, it throws an exception. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4552 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLFMJKBZA2KEAQMOKKTRJDOLNANCNFSM4LDO2YTQ> .

core/src/main/java/hudson/model/AdministrativeMonitor.java

jvz

This looks like it would work well enough, too. Thanks for synchronizing on a final field, too, which is the recommended approach in general.

timja · 2020-03-30T19:26:36Z

Thanks for the reviews all, marking as ready-for-merge, this will be merged after 24 hours if there's no negative feedback

JENKINS-56937 JCasC support for admin monitors

87adc90

timja commented Mar 7, 2020

View reviewed changes

timja mentioned this pull request Mar 7, 2020

How to configure administrative monitors? jenkinsci/configuration-as-code-plugin#774

Closed

jetersen approved these changes Mar 7, 2020

View reviewed changes

timja requested review from a team and oleg-nenashev March 7, 2020 11:32

res0nance reviewed Mar 7, 2020

View reviewed changes

daniel-beck self-requested a review March 7, 2020 22:29

alecharp reviewed Mar 12, 2020

View reviewed changes

rsandell reviewed Mar 12, 2020

View reviewed changes

timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Mar 14, 2020

timja added 3 commits March 14, 2020 16:06

Merge branch 'master' into disable-admin-monitors

f33d763

Removed out of date comment and small tweaks

1083c29

Formatting

6cfe46b

Use copy on write in setter

1a3f010

timja commented Mar 15, 2020

View reviewed changes

timja requested review from jvz and jglick March 15, 2020 16:04

synchronized

34ba209

jvz suggested changes Mar 16, 2020

View reviewed changes

Back to copyonwrite

57c47ab

jvz suggested changes Mar 16, 2020

View reviewed changes

Tweaks to thread safety

5301e58

timja requested a review from jvz March 16, 2020 18:43

jvz reviewed Mar 16, 2020

View reviewed changes

core/src/main/java/hudson/model/AbstractCIBase.java Outdated Show resolved Hide resolved

Last change hopefully

880d834

timja added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Mar 16, 2020

timja requested a review from jvz March 16, 2020 18:58

jvz approved these changes Mar 16, 2020

View reviewed changes

jetersen reviewed Mar 16, 2020

View reviewed changes

core/src/main/java/hudson/model/AbstractCIBase.java Outdated Show resolved Hide resolved

Remove synchronized on method

fc04644

timja requested review from rsandell, alecharp and res0nance and removed request for jglick March 16, 2020 20:12

res0nance reviewed Mar 17, 2020

View reviewed changes

timja requested a review from res0nance March 17, 2020 08:08

timja added 2 commits March 25, 2020 07:58

Merge branch 'master' into disable-admin-monitors

d53453e

Simplify code

51e1883

jetersen reviewed Mar 25, 2020

View reviewed changes

core/src/main/java/hudson/model/AdministrativeMonitor.java Show resolved Hide resolved

timja requested a review from jvz March 25, 2020 08:50

amuniz approved these changes Mar 25, 2020

View reviewed changes

Update sync

e8bc527

jvz approved these changes Mar 25, 2020

View reviewed changes

timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 30, 2020

timja merged commit 1a0f231 into jenkinsci:master Apr 1, 2020

timja deleted the disable-admin-monitors branch April 1, 2020 19:13

daniel-beck mentioned this pull request May 3, 2020

Update at-since up to May 3, 2020 #4705

Merged

	public void setDisabledAdministrativeMonitors(Set<String> disabledAdministrativeMonitors){
	public void setDisabledAdministrativeMonitors(Set<String> disabledAdministrativeMonitors) {

	private Set<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();
	private volatile Set<String> disabledAdministrativeMonitors = new CopyOnWriteArraySet<>();

JENKINS-56937 JCasC support for admin monitors #4552

JENKINS-56937 JCasC support for admin monitors #4552

Conversation

timja commented Mar 7, 2020 • edited Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

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

jetersen 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

Choose a reason for hiding this comment

jetersen Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

daniel-beck commented Mar 12, 2020

timja commented Mar 12, 2020

timja commented Mar 14, 2020

daniel-beck commented Mar 15, 2020

timja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Mar 15, 2020

timja commented Mar 15, 2020

jvz commented Mar 16, 2020 • edited Loading

timja commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz left a comment • edited Loading

Choose a reason for hiding this comment

jvz commented Mar 16, 2020

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

jdef commented Mar 24, 2020 via email

jvz left a comment

Choose a reason for hiding this comment

timja commented Mar 30, 2020

timja commented Mar 7, 2020 •

edited

Loading

jetersen Mar 8, 2020 •

edited

Loading

jvz commented Mar 16, 2020 •

edited

Loading

jvz left a comment •

edited

Loading