[JENKINS-35696] - Allow registering BC on the slave #10
Conversation
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| public static boolean registerBCOnSlave(@Nonnull Channel channel) throws Exception { | ||
| if (channel.getProperty(BOUNCYCASTLE_REGISTERED) == null) { | ||
| // pre-loading the bouncyclastle jar to make sure the JVM reconizes the signature | ||
| channel.preloadJar(PEMEncodable.class.getClassLoader(), BouncyCastleProvider.class); |
There was a problem hiding this comment.
I've never used preloadJar. Is the plugin classloader the right one to use here?
There was a problem hiding this comment.
I think so, AFIK you need a class loader that know where the correct class is loaded from, in this case BouncyCastleProvider.class
|
🐝 |
|
I'd include some synchronization on the channel (indirectly, like the guava cache using weak references in the keys) in case more than one plugin tries to register in parallel. Apart from that, if the class loader used is the correct one 🐝 |
|
@andresrc yes it can be done, but the addProvider is an innocuous operation, the second time does nothing. And the preloadJar is just a preload... so maybe it's better to keep it simple. Thanks for the review |
|
The local-side property value can be a Future for completion. That way if it is missing you have an almost idempotent write and worst case make two remoting calls before completing the future. All subsequent code that sees the future can just get the future ... and for the win we can rethrow the fiture's exception if we failed so that the failure reason is available to all, not burried in the logs that were rotated away because the system has been up too long |
| /** | ||
| * Callable to start the remote agent. | ||
| */ | ||
| public class BCRegisterer extends MasterToSlaveCallable<Boolean, Exception> { |
There was a problem hiding this comment.
@restricted as you have a utility method to manage the call
|
🐛 the static utility method is not in an intuitive class |
|
Also see my for the win comment wrt exceptions |
|
@stephenc I've moved the method to the Callable and made the constructor private. I'm returning the Future from the remote call. I'll have to leave the Exception re-throwing for a future improvement as I don't have time right now and we need to cut a version. I've created JENKINS-35914. I will involve creating the future (not returning the one from the asyncCall) and execute the preloadJar and the remote call all together. |
|
@alvarolobato alvarolobato#1 is the improvements that should be made IMHO. I hope you can see that this is not as hard... basically you get the re-throw for free and using ChannelProperty means that we:
|
|
Thanks @stephenc, looks great. I don't have that knowledge about the API (yet), commiting |
This is better
|
🐝 |
New method on the API to allow registering BC on the slave. The plugin is responsible for calling the method with a Channel to the slave.
Multiple calls to register BC on the same Channel will have no effect.
JENKINS-35696