Use attribute 'temporarilyOffline' instead 'offline' for enabling/disabling nodes.

[manox]

Using the 'offline' attribute to decide whether to toggle the state of nodes on activation/deactivation results in incorrect behavior. The attribute 'temporarilyOffline' must be used instead.

[pbelskiy]

Hi! Thanks for PR.

I cannot understand your problem, do you mean that when node disconnected you cannot understand if it toggled offline temporary? Or what you wanna detect here?

When node is disconnected:

  "offline" : True,
  "temporarilyOffline" : False,

After click on Mark this node temporarily offline:

  "offline" : True,
  "temporarilyOffline" : True,

When click on Bring this node back online:

  "offline" : False,
  "temporarilyOffline" : False,

Version of Jenkins is 2.357

[manox]

Hi, the states you describe are not the actual behavior of Jenkins agents for me. For me it looks like this when disabling/enabling directly in Jenkins (and with ujenkins after my PR).

When disconnected node:

  "offline" : True,
  "temporarilyOffline" : False,

When click on Mark this node temporarily offline / deactivate via ujenkins:

  "offline" : True,
  "temporarilyOffline" : True,

When click on Bring this node back online / activate via ujenkins:

  "offline" : True,
  "temporarilyOffline" : False,

Without the PR it looks like this:

When disconnected node:

  "offline" : True,
  "temporarilyOffline" : False,

When deactivate via ujenkins:

  "offline" : True,
  "temporarilyOffline" : False,

When activate via ujenkins:

  "offline" : True,
  "temporarilyOffline" : True,

[pbelskiy]

Sorry I edited my table states message above, now it's correct (Version of Jenkins is 2.357). Which version of Jenkins are you using? I just want to reproduce your issue.

Same approach are using in python-jenkins package: https://opendev.org/jjb/python-jenkins/src/branch/master/jenkins/__init__.py (1610 line)

But jenkinsapi uses some another approach, which I think is more interesting: https://github.com/pycontribs/jenkinsapi/blob/master/jenkinsapi/node.py (207 line)

As we see on source code of Jenkins CI https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/model/Computer.java

@Exported
public boolean isOffline() {
    return temporarilyOffline || getChannel() == null;
}

/**
 * Returns true if this node is marked temporarily offline by the user.
 *
 * <p>
 * In contrast, {@link #isOffline()} represents the actual online/offline
 * state. For example, this method may return false while {@link #isOffline()}
 * returns true if the agent failed to launch.
 *
 * @deprecated
 *      You should almost always want {@link #isOffline()}.
 *      This method is marked as deprecated to warn people when they
 *      accidentally call this method.
 */
@Exported
@Deprecated
public boolean isTemporarilyOffline() {
    return temporarilyOffline;
}

So temporarilyOffline field is using when user toggle the button and offline field is using when temporarilyOffline or disconnected.

So there is no problem as I understand. 1) You cannot enable your node because 'offline is False' but node is disabled or disconnected? 2) Or you cannot disable your node because 'offline is True' but node is enabled?

[manox]

My version of Jenkins is 2.332.3.

Here is a concrete example of what is one of the problems with the current implementation of ujenkins: If a previously activated node is offline, e.g. because there is no connection, and you use the ujenkins enable function, it will be actually deactivated.

[pbelskiy]

Okay, it's really toggles always when nodes.enable() using and node is disconnected.

[pbelskiy]

@manox you can do same PR for https://github.com/pbelskiy/aiojenkins

So, anyway I suggest to use same approach as jenkinsapi do:

AssertionError: Node is offline and not marked as temporarilyOffline, check client connection: offline = True, temporarilyOffline = False

And also add some method which run agent like jenkins.node.launch_agent what do you think?

[manox]

I will test aiojenkins and do same PR.

With the jenkinsapi method, you have no way to disable nodes that are not currently connected. For my purposes the current implementation is sufficient.