Digital Ocean integration plugin - need help in testing/code review

Hello community,

I'm working on digital ocean integration plugin, here it is: https://github.com/beolnix/tcdop .
It works but ... it is my first plugin for teamcity and I'm sure that there might be potential problems in the way of how I used cloud API or somwhere else.
I followed vmware plugin example, as it is recommended in the documentation, but I have that kind of feeling.. you know, that I missed something.

I will appriciate if you try to use it and tell me if it works for you or not.
Or if you have time for the code review it would be just perfect.

I think there might be a lot of different issues in the beginning, so it would be great if you create an issue on a github in case of any problem so I could keep it orginized and fix issues one by one accordig to their priority.

Please keep in mind that released 0.2 version is still very early build and I don't recommend you to keep it activated on your Teamcity instance for a long time to avoid any non-expected charges from Digital Ocean.

Thanks in advance.

Yes, of course it is the opensource project. So, if you would like to participate in it's development, who I am to stop you? Push requests are very welcome.

7 comments

Hi Danila,

It's great news!
Thank you for developing the plugin.

We will look into the code and file issues as you suggested. This can take some time, though. Feel free to ping us about it if we do not produce the results within a week.

0

Hi Yegor,

Thanks a lot!

I'm looking forward for the feedback.

0

Hi Danila,

In general, it's a good start and the concept in general looks good, but there are some major things that you need to take care of, before it becomes a production-ready. It's very good that you've contacted us on an early stage, so it's easier to make major changes.

I haven't yet had a chance to try the binaries, but here's some feedback based on the code:

Minor:
1) DOCloudClientFactory:93 Executors.newFixedThreadPool(backgroundThreadsLimit) can prevent server from shutting down, if any thread is stuck.
You can actually use jetbrains.buildServer.util.executors.ExecutorsFactory#newFixedScheduledDaemonExecutor from TC common-api.jar and create an scheduled daemon executor, which you can later use in CloudImageStorageImpl to avoid having a permanent thread.
2) DOCloudClient#canStartNewInstance will be called quite often. LOG.error here will result in huge # of messages.
3) DOClientServiceImpl#createInstance - if instance creation fails due to whatever reason, it is actually better to create an instance with state Error and non-empty
   CloudErrorInfo - this simplifies diagnostics, because these problems will be visible from UI.
4) Waiting for instance initialization in a thread with 20 mins timeout can be a serious bottleneck preventing from starting many instances at once.
5) Actually, the the cloud client is invalidated, when cloud profile is changed, so usually there's no need to detect new images (DOClientServiceImpl#getImages). You can populate them during CloudClient initialization.

Major:
6) However, the plugin should detect stopped/disappeared instances and reflect their state correspondingly in cloud state (CloudImage.getInstances()).
7) The CloudClient during initialization should be able to detect launched instances and catch them. CloudClient is disposed and created once again every time you update the cloud profile. Usually, this is done via tagging instances,
   if cloud provider supports such functionality.
8) The plugin has to do something with instances in ERROR and ERROR_CANNOT_STOP states. For instance, if the stop command failed due to network issue, the instance will remain in ERROR_CANNOT_STOP state until the
   CloudClient is disposed. Non-critical errors should be cleared after a certain period to let TC try the stop procedure once again.
9) The DOCloudClient#isInitialized should return true only, when all already started instances are caught properly (see #7 above).
10) DOCloudInstance#containsAgent doesn't look to be reliable. The best way is to match some agent property and started instance (see #11-12 below). See:
    https://github.com/JetBrains/teamcity-vmware-plugin/blob/master/cloud-vmware-agent/src/main/java/jetbrains/buildServer/clouds/vmware/VMWarePropertiesReader.java on the agent side
 and
 https://github.com/JetBrains/teamcity-vmware-plugin/blob/master/cloud-vmware-server/src/main/java/jetbrains/buildServer/clouds/vmware/VmwareCloudInstance.java,
    https://github.com/JetBrains/teamcity-vmware-plugin/blob/master/cloud-vmware-server/src/main/java/jetbrains/buildServer/clouds/vmware/connector/VMWareApiConnectorImpl.java (reconfigureInstance) 
 on the server side
11) in DOCloudClient#startNewInstance CloudInstanceUserData is ignored, which is not good. CloudInstanceUserData contains specific values that should be set in agent properties to make the behaviour correct.
    These properties include: idleTimeout, serverAddress, profileId and optional parameter indicating that instance should be terminated after first build (if this property is not set in agent parameters, instance won't be stopped after first build).
12) DOAgentPropsUpdater  doesn't read any VM-specific properties as well as CloudInstanceUserData mentioned above. Here's the example of how it can work:
    https://github.com/JetBrains/teamcity-vmware-plugin/blob/master/cloud-vmware-agent/src/main/java/jetbrains/buildServer/clouds/vmware/VMWarePropertiesReader.java
13) If the CloudClient looses the instance, it can run forever consuming money. Which is undesirable. Here's what can be done to avoid this (this feature requires reading CloudInstanceUserData mentioned above):
    https://github.com/JetBrains/teamcity-azure-plugin/blob/master/teamcity-azure-plugin-agent/src/main/java/jetbrains/buildServer/clouds/azure/IdleShutdown.java

0

Hi Sergey,

First of all, thanks a lot for your time. It is really very helpful to see such a detailed code review feedback.

I created corresponding issues on github for each of your notes. I'm going to fix it one by one starting from major ones, but it may take a while.

I'll keep monitor this thread just in case you would like to add something else.

0

Here are my notes based on trying the binaries which Sergey linked (but that link does not work for non-JetBrainer's):

Some quick/raw notes based on tc-digitalocean-plugin-0.2.zip:

Cloud profile settings form is broken on edit (bottom-most fields are left-shifted) and Save does nothing or it not displayed at all

Agent is not authorized automatically on connection from the DigitalOcean droplet

It makes sense to push data to the agent, like server URL, make the timeout be processed on the agent like other cloud plugins do, etc.

Is there a way to disable DigitalOcean emails an new droplet creation?

Usability.less important:
Makes sense to change labels: In Could Type section (shown in UI on configuration): Digital ocean type => DigitalOcean
"Digital Ocean integration settings" title -> is the title needed at all?

Mandatory fields on the form should be marked as such

Token field needs a help icon linking the place describing how to find it, the same for "Image Name"
Would be good to allow to configure several images per profile

0

Yegor, thanks a lot for your feedback. I created issues in github according to your notes, will fix it one by one as well.

As for the agents authorization - I'll double check, it worked fine for me. But anyway, based on your and Sergey's feedback, I think the implementation of this mechanism must be changed significantly.

As for the emails - they are sent by Digital Ocean, as far as I could see there is no way to disable new droplet creation email notification in the settings on their portal.

0

Thank you for the replies.

BTW, I just came across another plugin which sounds alike: https://github.com/graf-abolmasov/digitalocean-teamcity-plugin

0

Please sign in to leave a comment.