[JENKINS-25106] Support minimumNoInstances #343

erikhakansson · 2019-05-04T18:28:44Z

PR not finished yet. Just publishing to allow commenting on it.

erikhakansson · 2019-05-04T18:35:28Z

I would like some tips on how to properly test this. Any way to mock calls against ec2 api?

erikhakansson · 2019-07-07T23:34:42Z

Finished the PR now. Should work fine!

erikhakansson · 2019-07-12T20:46:11Z

@thoulen sorry to ping you, I just saw you had some recent commits so I guess you're one of the maintainers.

Do you think you could have a look at this PR?

snallami · 2019-07-14T14:31:39Z

src/main/java/hudson/plugins/ec2/EC2Cloud.java

@@ -751,7 +793,11 @@ private static AmazonWebServicesCredentials getCredentials(@Nullable String cred

    private AmazonEC2 reconnectToEc2() throws IOException {
        synchronized(this) {
-            connection = connect(createCredentialsProvider(), getEc2EndpointUrl());
+            if (AmazonEC2Cloud.isTestMode()) {


I am not maintainer or owner of this plugin. Just happen to see this and adding my review comments.

I am not a big fan of mixing mock code in original code. Some suggestions here https://8thlight.com/blog/dariusz-pasciak/2014/12/19/dont-mix-test-code-with-production-code.html

Well, no, generally you don't want that, but as you can see in the code this methodology is already used, and frankly I'm not sure Jenkins supports injecting and/or replacing (test) classes in a running Jenkins instance.

That blog post deals mostly with unit testing.

We should clean up this test mode stuff, but I'd be happy seeing it done in a separate PR since it's both pervasive already in this plugin, and fixing it here would make the diff gigantic.

snallami · 2019-07-14T14:33:08Z

src/main/java/hudson/plugins/ec2/EC2Cloud.java

@@ -784,12 +830,22 @@ public AmazonEC2 connect() throws AmazonClientException {
     * @return {@link AmazonEC2} client
     */
    public static AmazonEC2 connect(AWSCredentialsProvider credentialsProvider, URL endpoint) {
+        if (AmazonEC2Cloud.isTestMode()) {


Same comment like above abt mixing test code and production code

jvz · 2019-07-15T15:12:22Z

src/main/java/hudson/plugins/ec2/EC2Cloud.java

        awsCredentialsProvider = credentialsProvider;
        AmazonEC2 client = new AmazonEC2Client(credentialsProvider, createClientConfiguration(endpoint.getHost()));
        client.setEndpoint(endpoint.toString());
        return client;
    }

+    public synchronized static AmazonEC2 mockConnect(AWSCredentialsProvider credentialsProvider, URL endpoint, EC2PrivateKey privateKey) {


Test code should be either package-private or annotated with @Restricted(NoExternalUse.class).

This does not seem to be a good way to introduce test code here anyways, though restricting its visibility will make it simpler to refactor.

src/main/java/hudson/plugins/ec2/util/MockAmazonEC2.java

erikhakansson · 2019-07-15T18:38:36Z

@jvz maybe that refactoring you mentioned is actually necessary before this is really testable.

Perhaps some stuff can be made to use DI instead. As I understand it, Jenkins uses Guice. I've never used Guice specifically, but using any DI it should be possible to have a named injection, and perhaps that name can be read from a JVM parameter (or a default). Thus, when running tests we could just always pass a certain parameter that would ensure test (mock) classes would be loaded instead of real implementations. That way, test classes can remain firmly as test code and stay out of main source.

Thoughts on that?

jvz · 2019-07-15T18:47:51Z

Any class that's annotated with @Extension is already wired into Guice, so you can @Inject there at least.

erikhakansson · 2019-07-15T18:51:16Z

@jvz

I found this: https://stackoverflow.com/questions/7651980/google-guice-and-varying-injections-at-runtime (second answer)

Using that logic it might be possible to have a custom provider do something like:

@Override 
protected void configure() {
  String implementationClass = System.getProperty("my.implementation");
  if (implementationClass == null) {
    implementationClass = DEFAULT_IMPLEMENTATION_CLASS;
   }
   bind(MyInterface.class).to(Class.forName(implementationClass));
  }

Obviously with proper catches for ClassNotFound and such, but anyway, that way you can easily load a different implementation if running in test mode.

erikhakansson · 2019-07-17T20:07:24Z

@jvz please see #379

erikhakansson · 2019-08-02T09:43:10Z

Merged in changes from #379, refactored based on those changes, and squashed commits.

erikhakansson · 2019-08-02T09:48:46Z

I.e, this depends on #379 being approved and merged.

naxhh · 2019-08-12T12:09:12Z

Hi #379 has been merged! We where looking into implementing this feature so this is a +1 from us.
If there's something we can do to drive this forward..

erikhakansson · 2019-08-12T16:53:02Z

@jvz Do you think you can have a look at this again now, if you have time? :)

@naxhh If you would try building and running it locally I think it would be great. If the maintainers get reports of successful real-world usage I'm sure they will feel safer accepting the PR. Also, real-world is usually better at exposing bugs than unit tests :)

jvz · 2019-08-13T19:09:26Z

src/main/java/hudson/plugins/ec2/util/EC2SlaveFactory.java

+        return instance;
+    }
+
+    EC2OndemandSlave createOnDemandSlave(String name, String instanceId, String description, String remoteFS, int numExecutors, String labelString, Node.Mode mode, String initScript, String tmpDir, List<? extends NodeProperty<?>> nodeProperties, String remoteAdmin, String jvmopts, boolean stopOnTerminate, String idleTerminationMinutes, String publicDNS, String privateDNS, List<EC2Tag> tags, String cloudName, boolean useDedicatedTenancy, int launchTimeout, AMITypeData amiType, ConnectionStrategy connectionStrategy, int maxTotalUses)


Would it make more sense to use some sort of config class object here? Having more than a couple method parameters in Java gets really hard to deal with.

Yeah I suppose. I just went with the lowest effort change to just retain the current parameters.

But how about a config object with a builder pattern that throws an exception for missing parameters?

That sounds like a great way to approach it!

I pushed a change here.

I decided against throwing Exceptions since I don't really know if some of those parameters should in fact be nullable.

jvz · 2019-08-13T19:10:34Z

src/test/java/hudson/plugins/ec2/EC2PrivateKeyTest.java

-                + "9ontJin0nlHPk+AOmV8xt3yYD+wPAJy5MjUco7tS4Ix6bmvxcpZi2ZcHT1GwkiIzgKWE\n"
-                + "-----END RSA PRIVATE KEY-----");
+    private static EC2PrivateKey getPrivateKey() {
+        return new EC2PrivateKey(TestConstants.PRIVATE_KEY);


Not really in scope here, but this key could be replaced with a KeyPairGenerator instance that creates a new key for each test run.

Ah, I was not aware of the existence of that. :)

Want me to implement it in this PR despite being out of scope?

That's up to you. I've updated a test in jsch-plugin a while ago to do this. There are similar tests in ssh-credentials-plugin that hard code the keys, too. This only came to my attention before due to some sort of static analysis complaining about embedding private keys in the source code.

I've managed to solve this locally for most tests.

However, the tests in EC2PrivateKeyTest specifically are a bit cumbersome because they rely on comparison against a hard-coded key fingerprint.

I could of course calculate the fingerprint of the generated key, but that generation is basically what the test already tests. So I'd more or less be replicating the production code in the test, and then testing whether my test replication functions the same as the production code.

I could of course revert the EC2PrivateKeyTest.java and retain the hard-coded key there, and just use the generated key for the rest of the tests, but then we'll still have the hard-coded key.

Suggestion?

Sounds like you'd be better off using the hard-coded key in that specific test at least.

I pushed a change with that implemented now!

I also squashed the commits.

jvz · 2019-08-13T19:11:38Z

Looks like this is coming along nicely. @fcojfernandez can you or a teammate review?

jvz · 2019-08-14T18:30:36Z

By the way, DevOps World/Jenkins World is going on this week, so review might be delayed.

res0nance

Looking real good

res0nance · 2019-09-17T00:36:54Z

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

@@ -170,7 +174,7 @@
    public SlaveTemplate(String ami, String zone, SpotConfiguration spotConfig, String securityGroups, String remoteFS,
            InstanceType type, boolean ebsOptimized, String labelString, Node.Mode mode, String description, String initScript,
            String tmpDir, String userData, String numExecutors, String remoteAdmin, AMITypeData amiType, String jvmopts,
-            boolean stopOnTerminate, String subnetId, List<EC2Tag> tags, String idleTerminationMinutes,
+            boolean stopOnTerminate, String subnetId, List<EC2Tag> tags, String idleTerminationMinutes, String minimumNumberOfInstancesStr,


Could you fix this bit @erikhakansson

res0nance · 2019-09-17T00:38:03Z

src/main/java/hudson/plugins/ec2/util/MinimumInstanceChecker.java

+public class MinimumInstanceChecker {
+
+  public static int countCurrentNumberOfSlaves(@Nonnull SlaveTemplate slaveTemplate) {
+    return (int) Arrays.stream(Jenkins.get().getComputers()).filter(computer -> {


Nit: I'd prefer if it was 4 spaces like the rest of the project

I agree. Just the default settings from my IDE that came through. Fixed!

fcojfernandez · 2019-09-18T12:29:03Z

Relaunching the CI

erikhakansson · 2019-09-18T21:38:39Z

Seems like unrelated errors on CI side. But for some reason it doesn't rebuild when I close and open

erikhakansson · 2019-09-19T19:34:50Z

I can't figure this out.
https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fec2-plugin/detail/PR-343/35/pipeline/11

The build obviously works, but on Windows the CI fails to clean up the workspace after.

Fixes #JENKINS-25106

Co-Authored-By: Francisco Fernández <31063239+fcojfernandez@users.noreply.github.com>

erikhakansson · 2019-09-20T16:05:30Z

Ok I'm even more confused now. I rebased against master and added a commit disabling my new tests, just to see what might cause the workdir to be undeletable. Now CI says

[2019-09-20T16:01:01.848Z] Merging remotes/origin/master commit bc2c4f09236e40a64ac66ce86f4fabdda8387b76 into PR head commit e5383223c2d5f18ce03ae2cf99b9882c6f2f8856

Cannot checkout from unborn branch

erikhakansson · 2019-09-20T18:22:08Z

I created a new PR (#401) with the exact same changes and it builds (although it's currently stalled at a later step, waiting for a new slave).

Should I just close this and continue on the other one?

jvz · 2019-09-20T18:29:36Z

Yeah, let's continue on the new PR. It seems as though this branch's git history might have caused an issue in CI.

erikhakansson · 2019-09-20T18:36:59Z

Yeah seems that way. I'm closing this.

erikhakansson · 2019-09-20T18:37:15Z

I had a spelling mistake in the branch name anyway :)

erikhakansson force-pushed the minimumNoIntances branch 2 times, most recently from 9cbdfaa to 4303fef Compare July 7, 2019 22:30

erikhakansson changed the title ~~[WIP] Support minimumNoInstances~~ Support minimumNoInstances [Solves #JENKINS-25106] Jul 7, 2019

erikhakansson changed the title ~~Support minimumNoInstances [Solves #JENKINS-25106]~~ [JENKINS-25106] Support minimumNoInstances Jul 7, 2019

snallami reviewed Jul 14, 2019

View reviewed changes

thoulen requested review from jvz and thoulen July 14, 2019 19:53

jvz reviewed Jul 15, 2019

View reviewed changes

jvz requested changes Jul 15, 2019

View reviewed changes

src/main/java/hudson/plugins/ec2/util/MockAmazonEC2.java Outdated Show resolved Hide resolved

erikhakansson mentioned this pull request Jul 17, 2019

Remove test code in production code #379

Merged

erikhakansson force-pushed the minimumNoIntances branch from 2b30515 to 92c33a2 Compare August 2, 2019 09:41

erikhakansson force-pushed the minimumNoIntances branch from 92c33a2 to 8a95d81 Compare August 10, 2019 18:18

jvz self-requested a review August 13, 2019 17:14

jvz reviewed Aug 13, 2019

View reviewed changes

erikhakansson force-pushed the minimumNoIntances branch from a737bd8 to 7f2c638 Compare August 15, 2019 20:12

res0nance approved these changes Sep 17, 2019

View reviewed changes

fcojfernandez closed this Sep 18, 2019

fcojfernandez reopened this Sep 18, 2019

erikhakansson closed this Sep 18, 2019

erikhakansson reopened this Sep 18, 2019

res0nance closed this Sep 19, 2019

res0nance reopened this Sep 19, 2019

erikhakansson closed this Sep 19, 2019

erikhakansson reopened this Sep 19, 2019

erikhakansson closed this Sep 20, 2019

erikhakansson reopened this Sep 20, 2019

erikhakansson and others added 5 commits September 20, 2019 18:00

Support for minimum number of instances.

1ae8404

Fixes #JENKINS-25106

Update src/main/java/hudson/plugins/ec2/SlaveTemplate.java

7630835

Co-Authored-By: Francisco Fernández <31063239+fcojfernandez@users.noreply.github.com>

Added null check for slaveTemplate

632fff1

Change minimumNumberOfInstances parameter to int. Also fixed indentation

a561b93

Temp commit with commented-out tests

e538322

erikhakansson force-pushed the minimumNoIntances branch from 04b6848 to e538322 Compare September 20, 2019 16:00

erikhakansson closed this Sep 20, 2019

erikhakansson reopened this Sep 20, 2019

erikhakansson mentioned this pull request Sep 20, 2019

[JENKINS-25106] Support minimumNoInstances #401

Merged

erikhakansson closed this Sep 20, 2019

erikhakansson deleted the minimumNoIntances branch September 27, 2019 16:37

[JENKINS-25106] Support minimumNoInstances #343

[JENKINS-25106] Support minimumNoInstances #343

Conversation

erikhakansson commented May 4, 2019

erikhakansson commented May 4, 2019

erikhakansson commented Jul 7, 2019

erikhakansson commented Jul 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikhakansson commented Jul 15, 2019

jvz commented Jul 15, 2019

erikhakansson commented Jul 15, 2019

erikhakansson commented Jul 17, 2019

erikhakansson commented Aug 2, 2019

erikhakansson commented Aug 2, 2019

naxhh commented Aug 12, 2019 • edited Loading

erikhakansson commented Aug 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz commented Aug 13, 2019

jvz commented Aug 14, 2019

res0nance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcojfernandez commented Sep 18, 2019

erikhakansson commented Sep 18, 2019

erikhakansson commented Sep 19, 2019

erikhakansson commented Sep 20, 2019

erikhakansson commented Sep 20, 2019

jvz commented Sep 20, 2019

erikhakansson commented Sep 20, 2019

erikhakansson commented Sep 20, 2019

naxhh commented Aug 12, 2019 •

edited

Loading