CleanLostNodesWork to only delete orphans belonging to current controller

[tweirtx]

PR adds jenkins_server_url and jenkins_cloud_name based filtering in CleanLostNodesWork - thus not touching orphan VMs belonging to other controller. All agent VMs have labels identifying the controller and Jenkins cloud where it belongs, if any orphan VM that got leftover for cases - where the cloud definition is deleted in Jenkins, or controller itself is permanently out of service - should be deleted by tools outside of Jenkins.

---

My team's Jenkins setup has multiple hosts utilizing the same GCP project, and the changes in #503 have led to an uptick in incidents where one Jenkins host terminates another's build job in the lost node cleanup routine. I understand the original criticism that led to this change, and I propose that instead of using the previous behavior which could cause issues, we add an option to have a configurable label to restrict the cleanup job's targeted GCE VMs. This may also provide a solid solution to #157 and https://issues.jenkins.io/browse/JENKINS-68244 as well.

Testing done

Aside from the unit tests, I deployed this in our QA environment and thoroughly tested the behavior of the clean lost nodes routine with the changes I made. At this time, I have not run the integration tests due to security restrictions, but I will be completing that task as soon as I can. (and moving this PR out of draft status). EDIT: Integration tests have been successfully completed.

---

Tested by integration tests.

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[gbhat618]

Hi, have you checked the follow up PR that fixed the bug #514? The problem you describe shouldn't be happening if you are using the version with the follow-up that fixed the issue.
Which version of the plugin are you using?

[tweirtx]

Yes, I did check that PR, but we're still seeing the issue on versions newer than the one that included that fix.

[gbhat618]

Yes, I did check that PR, but we're still seeing the issue on versions newer than the one that included that fix.

I think it's better to confirm where the bug is coming from. Some thoughts,

1. Confirm that another controller is actually the one responsible for deletion of agent VMs of some other controller: if you have separate service accounts configured in each of the Jenkins controllers, could you validate from the GCP Cloud Logging that indeed the principalEmail was the other Jenkins controller account?

2. Enable FINEST on only on the class com.google.jenkins.plugins.computeengine.com.google.jenkins.plugins.computeengine.CleanLostNodesWork by adding a new log recorder in $JENKINS_URL/manage/log/ -> Add recorder. (Note: do not enable FINEST on the package level, which will start logging across the GCE plugin, that will simply be causing too much logging). Then confirm the deleted VM of controller-A is showing up in say the logs of controller-B. You would be looking for

   google-compute-engine-plugin/src/main/java/com/google/jenkins/plugins/computeengine/CleanLostNodesWork.java

   Lines 116 to 119 in 9ade970

   	logger.log(
   	Level.FINEST,
   	() -> "Instance " + remote.getName() + " last_refresh label value: " + nodeLastRefresh + ", isOrphan: "
   	+ isOrphan);

   
   

   google-compute-engine-plugin/src/main/java/com/google/jenkins/plugins/computeengine/CleanLostNodesWork.java

   Line 125 in 9ade970

   logger.log(Level.INFO, "Removing orphaned instance: " + instanceName);

   . Note: the INFO log you can confirm it now itself.

The way the CleanLostNodesWork works is,

• when an agent VM is created the label jenkins_node_last_refresh contains the current time of the creation time.
• then, every 1hr, when the CleanLostNodesWork runs, it updates the jenkins_node_last_refresh timestamp for all the it's connected agent VMs -> therefore any agent that is part of a long running build (several hours) keeps getting refreshed for this label timestamp. And it only deletes the VMs - whose jenkins_node_last_refresh > 3h.

  google-compute-engine-plugin/src/main/java/com/google/jenkins/plugins/computeengine/CleanLostNodesWork.java

  Lines 162 to 184 in 9ade970

  	/**
  	* Updates the label of the local instances to indicate they are still in use. The method makes N network calls
  	* for N local instances, couldn't find any bulk update apis.
  	*/
  	private void updateLocalInstancesLabel(
  	ComputeClientV2 clientV2, Set<String> localInstances, List<Instance> remoteInstances) {
  	var remoteInstancesByName =
  	remoteInstances.stream().collect(Collectors.toMap(Instance::getName, instance -> instance));
  	var labelToUpdate = ImmutableMap.of(NODE_IN_USE_LABEL_KEY, getLastRefreshLabelVal());
  	for (String instanceName : localInstances) {
  	var remoteInstance = remoteInstancesByName.get(instanceName);
  	if (remoteInstance == null) {
  	continue;
  	}
  	try {
  	clientV2.updateInstanceLabels(remoteInstance, labelToUpdate);
  	logger.log(Level.FINEST, () -> "Updated label for instance " + instanceName);
  	} catch (IOException e) {
  	logger.log(Level.WARNING, "Error updating label for instance " + instanceName, e);
  	}
  	}
  	}
  	}

So unless the controller which is facing the agent VM deletion by other controllers is running an older of the plugin that doesn't have the current version of the code of CleanLostNodesWork - I am not able to think of another scenario that is causing this wrong deletion.

[gbhat618]

But if you wanted to disable the CleanLostNodesWork - beware you will need to disable it from all controller pointing to the same GCP project, otherwise whichever controller still has it enabled will mark the VMs whose jenkins_node_last_refresh is > 3hr as orphan.

1. A way to disable in the current Jenkins JVM - will not persist across restarts, could be,
   From script console - $JENKINS_URL/manage/script,

   import com.google.jenkins.plugins.computeengine.CleanLostNodesWork
   ExtensionList.lookupSingleton(CleanLostNodesWork.class).cancel()

2. As a hack if you just want to disable the CleanLostNodesWork you can set this system property to a really large number, like 100yr or so — -Dcom.google.jenkins.plugins.computeengine.CleanLostNodesWork.recurrencePeriod=3153600000 (unit is millis - 100yr = 3600L * 24 * 365 * 100;)

But as already mentioned if you are attempting to do either of these, it will need to be done in all controllers.

[gbhat618]

Now, coming to how can disable deletion of agent VMs created by a specific controller,

Perhaps instead of introducing a new label jenkins_node_cleanup, we can introduce a magic value to the existing label jenkins_node_last_refresh which will be set to a value like 0 and then put a filter in the code to both — exclude the VM from updating the value of the label, and exclude from deletion the VM with label value 0 - and just do a FINE logging. And expose it via a system property -Dcom.google.jenkins.plugins.computeengine.CleanLostNodesWork.disableUpdatingNodeLastRefresh = true (which is false by default)

Edit: I will think a bit more about this, and get back. I think this will greatly reduce the diff for this PR.

[gbhat618]

Perhaps instead of introducing a new label [...] a magic value [...] 0 [...]

This has a problem, it won't delete the orphans of the current controller as well.

Then we need an identifier for the instances created by the current controller's, which can be the instanceIds of the GCE clouds,

google-compute-engine-plugin/src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineCloud.java

Lines 163 to 165 in 9ade970

	// Apply a label that associates an instance configuration with
	// this cloud provider
	configuration.appendLabel(CLOUD_ID_LABEL_KEY, getInstanceId());

And we can have a system property -Dcom.google.jenkins.plugins.computeengine.CleanLostNodesWork.deleteOnlyVMsOfThisController=true (or a similar name) and have the logic around that. We won't have to introduce a new label then also, and diff would be smaller too.

Note: a small gotcha (that only affects a CasC based controller though) if somebody doesn't set the instanceId field for a GCE cloud in their CasC configuration, and after the restart when the CasC config is redone - it will compute new random uuid. This could be avoided in by setting a UUID in the CasC yaml file. Otherwise after the controller restart it won't recognize it's own created VMs before the restart and leave them be hanging around. We can document it, and if someone misses to set this up, it is still acceptable in that rarely some VMs would be hanging around if there were existing during that specific restart time, which I think is timing dependent as well, so likely be rare -

google-compute-engine-plugin/src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineCloud.java

Lines 181 to 184 in 9ade970

	public void setInstanceId(String instanceId) {
	if (Strings.isNullOrEmpty(instanceId)) {
	this.instanceId = UUID.randomUUID().toString();
	} else {

.

[gbhat618]

• #558 (comment) is much simpler implementation leveraging the existing labels and implementation.
• worth debugging to know if there is an actual bug or it's infra problem - #558 (comment)

[tweirtx]

We did extensive verification to ensure the plugin was the cause, including validation of running versions and logging active systems. We don't want to completely disable lost node detection, but we do want to ensure that the system is not picking up nodes belonging to another Jenkins host. We have a hotfix in place that sets the recurrence period to 24 hours, and that solves the problem, but the better fix is to remove the bad behavior in the first place.

Regarding the use of the instanceId label, I saw that you had mentioned in #503 the potential for a misconfiguration to occur based on that cloud ID. I took that into account when designing my solution, which originally sought to allow for the option to revert to the old behavior (versions prior to that PR did not exhibit the issue), however I decided a new implementation was in order to address the root cause of the original PR.

[gbhat618]

extensive verification to ensure the plugin was the cause, including validation of running versions and logging active systems

did you track down the bug ?

Edit: I have gone through the code again, and did some more analysis. I don't think there is a way for controller-A to delete the VMs of controller-B, unless,

• controller-B faced exceptions while updating the timestamps
• controller-B has a longer recurrencePeriod configured by system property, and thus it didn't refresh the timestamp.

The 24h recurrencePeriod hotfix also leans towards this asymmetric config, or exception facing, causing delay in refreshing the timestamp.

[gbhat618]

This test will now be failing due to

cloud.setLostNodeCleanupRestriction(true);

?

[gbhat618]

Please help explain what would be the need for configurable label for lost node restriction at individual cloud level. Perhaps having a system property to decide and hardcoding the label name is easier. Also requesting to not use the mock based tests, rather use the IT test, something like - adding another test testSecondControllerDoesNotCleanUpLostNodeWhenRestrictionEnabled in the existing CleanLostNodeWorkIT would provide the coverage.

[gbhat618]

Given #558 (comment), I still think we should track down the actual bug and think of reproducible scenarios (preferably via an IT test) and arrive at the solution accordingly.

If unable to come up with a reproducer at all, I think it is better to just introduce a system property alone, rather introducing configuration properties, adding new configurable properties means we will need to support it, and they cannot be taken out in future releases without appropriate backward compatibility support - which will just complicate the whole thing.

[tweirtx]

I haven't been able to track down the root cause of the problem, but it was observed before the recurrence periods were reconfigured (which is why they were configured to 24 hours in the first place). I will make the requested changes to the tests, though I will say that the integration tests currently all pass. The log message was mistakenly left in from debugging and I will remove it.

The reasoning for a configurable label is to differentiate which Jenkins master node created the VMs, and consequently, filter out which ones actually apply to the master node when it runs cleanup. I will refactor the code to use a system property instead of a per-cloud configuration as requested.

[tweirtx]

I'm getting a bit frustrated because this is now the third different implementation of the same feature. I reworked it twice based on review feedback, the PR received two approvals, and now we're discussing another substantial change.

I'd really prefer to keep things the way they are with the system property, as suddenly changing the behavior in a backwards-incompatible way is what caused the problem in the first place.

If the design expectations have changed, that's okay, but I'd like us to agree on the desired implementation before I spend more time rewriting it. Otherwise, it feels like the goalposts keep moving.

[gbhat618]

Hi @tweirtx,

reworked it twice based on review feedback, the PR received two approvals, and now we're discussing another substantial change

Apologies for not handling it well. I should have been more explicit about some of my comments being just possible ideas, or could have moved the conversation to an issue instead. Will keep that in mind in future.

Given that there is still a possible corner case of one controller wrongly deleting other controller's agent VMs - the plugin is better off not touching other controller's agents. And we may re-offer it as an opt-in (if required in future), rather than default. The feature still seems good idea in being helpful to clean-up orphans of other controllers; but clearly causes problems to running builds when bugs creep in, and can have edge cases with config or version incompatibilities across controllers.

I will push a commit to this PR in sometime, also cleanup/needed rewrites to the integration tests. Thanks for being patient with it, and also for reporting the problem.

[gbhat618]

Jenkins.get().removeNode(agentNode) internally calls this method, which fires a GCP api request to delete the VM.

I couldn't think of some other ways to make the VM exist in the cloud, but remove the agent definition from Jenkins.

So introducing a special field skipGcpTerminateForTesting for test usage.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

[gbhat618]

such kind of migrations are rare, the CleanLostNodesWork is designed for when API errors or something makes the Jenkins node get deleted, but leaves behind a VM running.

[gbhat618]

due to the integration test package doesn't match class's package, need to make it public.

[gbhat618]

Added new cases to assert VMs are retrieved by new label filter - passing cloud name label (cloud-a, and cloud-b). so did minor refactoring to improve the readability of this test.

[gbhat618]

while I was updating the readme, mentioning about this sysprop was necessary - then thought why not just also update it to use SystemProperties.getDuration - which retains it's original behavior of ms if no units mentioned.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[gbhat618]

This PR now looks complete from my side.

• ran all the IT tests, and one IT needed fixing - done.
• also did manual testing with 2 clouds, and confirmed regular builds work correctly.
• thought it's good to update the README due to frequent questions arising around how orphan cleanup works.

Need help with review, and after I will merge it.

[gbhat618]

Hi @jtnord, made changes after the last review, can you help check again once. I will merge it after that.