Regarding integration of Advanced Build Discarder(GSoC Idea) with Multi-Branch Pipeline Plugin

[Nisarg Shah]

Hello everyone,

I am Nisarg Shah and I have prepared an application on Advanced Build Discarder for GSoC-2019.

I have included one feature of integrating Advanced Build Discarder plugin to be integrated with Multi-Branch Pipeline plugin which is not there in the current version.

I have a doubt in the integration part, whether the delete() method currently used in logRotator.java(current Discarder) will work for discarding builds from Multi-Branch plugin or a new delete method() which will fetch details of root directory  and should be implemented for better and efficient implementation.

Also, I have added few steps of implementation of features in the proposal.

Reviews and feedback regarding implementation would be helpful. And also few points regarding better implementation will be appreciated.

Thank you.

Regards,

Nisarg Shah

[Jesse Glick]

Did not see this mentioned in the doc so replying here.

On Thu, Apr 4, 2019 at 9:30 AM Nisarg Shah <nissh...@gmail.com> wrote:
> I have a doubt in the integration part, whether the delete() method currently used in logRotator.java(current Discarder) will work for discarding builds from Multi-Branch plugin

It should. Did you have some specific reason to think it would not?

[Nisarg Shah]

Actually I thought of making a new delete() method for integration with External Workspace Manager. As in External Workspace Manager the details of workspace and root directory won't be accessible by current methods. For that Fingerprints will be used.

So I was just confused that, would this be the same case. But yes, this should work for discarding builds from Multi-Branch plugin.

And regarding your comment in the proposal : A new plugin will be implemented and for accessing methods of new plugin, AbstractStepImpl class can be used for "Step" as per my study. But this class is deprecated. 

I need to configure the new filter methods via "Options" or "Properties". So how can I ? Or this is not needed, and only "buildDiscarder" can be extended with suitable '@Symobl' (what I meant from your comment in the proposal).

Thank you.

[Jesse Glick]

On Thu, Apr 4, 2019 at 12:41 PM Nisarg Shah <nissh...@gmail.com> wrote:
> I thought of making a new delete() method for integration with External Workspace Manager.

Not exactly sure where this is going, but perhaps that plugin could
simply implement `RunListener.onDeleted`? That would do the right
thing when a build is deleted for whatever reason (not just rotation).

> Or this is not needed, and only "buildDiscarder" can be extended with suitable '@Symobl'

As in for example:

https://github.com/jenkinsci/jenkins/blob/6a3886e459556d479a2f1c08afd53d3e1015392b/core/src/main/java/hudson/tasks/LogRotator.java#L256-L257

[Nisarg Shah]

Okay. So you are suggesting to implement RunListener.onDeleted in External Workspace Manager, if I am not wrong.

But this method is used when any action is to be performed after some deletion operation. Here I want to delete builds,artifacts and workspaces created by External Workspace Manager.

And for this Fingerprints can be used as we can get workspaceId, using which externalWorkspace object can be fetched. And using that path of workspace can be obtained which can be used for discarding workspaces. 

I wonder whether RunLinstener.onDeleted can be used here or not. Can you please elaborate on usage on RunListener.onDeleted for discarding builds and workspaces.

[Jesse Glick]

On Thu, Apr 4, 2019 at 2:22 PM Nisarg Shah <nissh...@gmail.com> wrote:
> Here I want to delete builds,artifacts and workspaces created by External Workspace Manager.

Artifacts are normally deleted alongside a build. (Unless you are
using `artifact-manager-s3`, by design.) Unless I am missing
something, it is the responsibility of the
`external-workspace-manager` plugin to implement deletion of (unused?)
external workspaces when builds are deleted, and that is orthogonal to
your proposal. `RunListener.onDeleted` is simply the hook plugins
receive to notify them to do any additional cleanup they need to,
however they see fit.

[Nisarg Shah]

So according to you, discarding of workspaces when builds are discarded is responsibility of 'external-workspace-manager'. And advanced Build Discarder should be notified to do the additional cleanup ? 

And one more thing, deleting of builds should be done advanced build discarder. So how that can be implemented in a better way?

My idea is: Using RunListener extension point for communication and filter methods would be accessible in the plugin. And after configuring the filter methods, whenever required delete() method would be called and it would delete the build.

[Jesse Glick]

On Thu, Apr 4, 2019 at 2:51 PM Nisarg Shah <nissh...@gmail.com> wrote:
> discarding of workspaces when builds are discarded is responsibility of 'external-workspace-manager'.

Yes, I would think so.

> And advanced Build Discarder should be notified to do the additional cleanup ?

Not quite sure what that means. The build discarder just needs to pick
some builds to delete, and delete them. Deleting a build triggers some
listeners.

[Nisarg Shah]

Ya, deleting a build would trigger listener and for that RunListener.onDeleted() can be used.

But for deleting builds from external workspace manager, few builds will be picked according to the filters. And that builds will be deleted by calling delete() method, if I am not wrong.

[Oleg Nenashev]

I agree with Jesse here. As we discussed at the meetings, Advanced Build Discarder plugin should be just invoking run deletion which will trigger RunListeners then. EXWS plugin can be releasing workspaces upon receiving a notification from RunListener, but there should be an implementation in the EXWS plugin for it (now it relies on Workspace Cleanup plugin). Section 3 in your current proposal actually describes it in such way.

There may be a use-case for deeper integration between Advanced Build discarder and EXWS if you want to do it.

So I was just confused that, would this be the same case. But yes, this should work for discarding builds from Multi-Branch plugin.

When we were discussing Multi-branch support, the whole discussion was about advanced Build Discard strategies fine tuned for the Multi-Branch use-cases. For example, defining different retention strategies for branches, PRs and release tags. As a user, I may want to keep the release tag builds forever while I unlikely want it for merged pull requests. AFAIK the standard deletion API should be working for Multi-Branch well, so I do not think there is any specifics for the deletion logic.

[Nisarg Shah]

I have added both the possibilities in implementation of External Workspace Manager.

1. Invoking run deletion assuming that discarding of workspaces is already implemented.

2. And if it is not implemented, then I have given a way for discarding workspaces.

I hope this is fine.

I have updated my Proposal.

Further reviews and comments regarding implementation would be appreciated.

Thank you.