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

fix(delete): [JENKINS-70252] delete WorkflowRun before WorkflowJob #463

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a1k0u
Copy link

@a1k0u a1k0u commented Sep 6, 2024

We use the repository scanning feature in the cloudbees-folder-plugin with Multi Branch Pipeline project to delete non-existent branches. When the folder plugin finds out which branch should be deleted, it calls the delete method in the I class (AbstractFolder<I>), in our case it is WorkflowJob. WorkflowJob only overrides the performDelete method, where it disables the job, exits the queue and calls AbstractItem.performDelete(). This way WorkflowJob only recursively deletes files under it. There is no logic for deleting artifacts under a WorkflowRun. This is why, for example, artifacts are not deleted from S3 after deleting a branch (second case in the linked issue).

But if you delete only the WorkflowRun (for example logRotator in pipeline options), it will delete artifacts because the parent class calls deleteArtifacts in the delete method (fisrt case in the linked issue).

I believe it would be correct to update hudson.model.Job, where to add logic to remove builds (hudson.model.Run) from getBuilds. But it was much easier to make changes to the plugin and test it in a production environment.

Testing done

I really don't know how to test this properly. I just made the changes and tested it in our Jenkins instance. Everything is working as I expected. I will be glad to know how to test this fix.

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 demonstrates feature works or fixes the issue

https://issues.jenkins.io/browse/JENKINS-70252

@a1k0u a1k0u requested a review from a team as a code owner September 6, 2024 07:49
@@ -663,6 +663,11 @@ public void replaceAction(@NonNull Action a) {
@Override protected void performDelete() throws IOException, InterruptedException {
setDisabled(true);
Jenkins.get().getQueue().cancel(this);

for (WorkflowRun workflowRun : getBuilds()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trouble is that this will make performance worse for those not using artifact-manager-s3 (or those using it in its default and only supported mode, which never deletes artifacts), since loading potentially thousands of WorkflowRun objects from so many little XML files is expensive. Anyway deleting those S3 directories one by one would be inefficient.

Fixing this properly would have to take a different approach I think. https://javadoc.jenkins.io/jenkins/model/ArtifactManager.html#delete() cannot handle this since the whole object is scoped per build. https://javadoc.jenkins.io/jenkins/model/ArtifactManagerFactory.html could have a delete(Job) method, perhaps. Or JCloudsArtifactManager (or something else in the plugin) could check ItemListener.onDeleted and delete the whole directory with one API call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants