Async jobs add endtime #16

ernjvr · 2018-06-26T07:45:17Z

Description

Overview:
There is currently no functional mechanism that captures or persists the end time of when an asynchronous job has finished. As a result, users are not able to do any reporting about the duration of various asynchronous jobs in Cloudstack.

Data Layer changes:
The time stamp of when an asynchronous job has finished should be persisted to the 'removed' column of the async_job database table. This is an existing column which is not being used for any other functionality.
QueryAsyncJobResult API changes:
The queryAsyncJobResult API should now include an 'end_time' response tag that will display the value of the 'removed' database column.
Management Server process changes:
When a Cloudstack management server starts up, it finds in the database all the asynchronous jobs it is the owner of, whose statuses are in an 'in_progress' state and updates them to a 'failed' status. At the same time, it should now also mark them as 'removed' by updating the 'removed' field with the current timestamp.
Garbage collection changes:
Currently, the garbage collector uses the 'created' database column to find in the database completed asynchronous job records that are older than the 'job.expire.minutes' global setting's timestamp and, then, deletes these records. It should no longer use the 'created' column to do this. It should use the 'removed' column instead.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Start a virtual machine to initiate an asynchronous job. Query the cloud.async_job table to find the record that was created in response to the 'start virtual machine' job. When the job finishes, verify that the 'removed' column has been correctly updated with the end timestamp of the relevant record.
Using cloudmonkey, make an API call to queryAsyncJobResult using the uuid as the jobid and verify that the 'endtime' field returns the correct timestamp value of the relevant database record's 'removed' column.
Verify that the garbage collector deletes the correct expired asynchronous job records from the cloud.async_job table.
Hypervisor: KVM

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

rohityadavcloud

Some changes request, overall LGTM good work. I've not tested/checked your marvin test cases though.

rohityadavcloud · 2018-06-27T09:14:12Z

api/src/main/java/org/apache/cloudstack/api/response/AsyncJobResponse.java

    @Param(description = "  the created date of the job")
    private Date created;

+    @SerializedName(ApiConstants.END_TIME)


This could be finished or completed?

rohityadavcloud · 2018-06-27T09:14:45Z

framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/dao/AsyncJobDaoImpl.java

        expiringUnfinishedAsyncJobSearch.done();

        expiringCompletedAsyncJobSearch = createSearchBuilder();
-        expiringCompletedAsyncJobSearch.and("created", expiringCompletedAsyncJobSearch.entity().getCreated(), SearchCriteria.Op.LTEQ);


This change is okay only if the 'created' it not used in any of the methods to query the result.

This is only used by the Asyncjob garbage collector.

I verified; it is only used by the async jobs garbage collector.

rohityadavcloud · 2018-06-27T09:15:26Z

framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/dao/AsyncJobDaoImpl.java

-        SearchCriteria<AsyncJobVO> sc = expiringCompletedAsyncJobSearch.create();
-        sc.setParameters("created", cutTime);
+    public List<AsyncJobVO> getExpiredCompletedJobs(final Date cutTime, final int limit) {
+        final SearchCriteria<AsyncJobVO> sc = expiringCompletedAsyncJobSearch.create();


We'll need to discuss this bit over our syncup call when you're back.

Suggestion: during the upgrade 4.11.1->4.12.0 -> set all the removed field of the async_jobs -> NOW()?

Update added to schema-41110to41200.sql

Update added.

rohityadavcloud · 2018-06-27T09:17:28Z

framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java

-                job.setLastUpdated(DateUtil.currentGMTTime());
+                final Date currentGMTTime = DateUtil.currentGMTTime();
+                job.setLastUpdated(currentGMTTime);
+                job.setRemoved(currentGMTTime);


rohityadavcloud · 2018-06-27T09:18:06Z

framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java

-    public AsyncJob queryJob(long jobId, boolean updatePollTime) {
-        AsyncJobVO job = _jobDao.findById(jobId);
+    public AsyncJob queryJob(final long jobId, final boolean updatePollTime) {
+        final AsyncJobVO job = _jobDao.findByIdIncludingRemoved(jobId);


This may have some API response changes, wrt who uses this method and if it can cause any regression (as the method will return a removed job).

borisstoyanov · 2018-07-03T09:32:33Z

test/integration/smoke/test_privategw_acl.py

        minimum_vms_to_pass = 2
        for vm_ip in vms_ips:
-            ssh_command = "ping -c 5 %s" % vm_ip
+            ssh_command = "ping -c 10 %s" % vm_ip


@ernjvr is this related change? if it's a bugfix can we have it in a separate PR?

borisstoyanov

@ernjvr I see lots of other commits in your PR, can you pull and rebase your code against master and resubmit the pull request

… response tag name.

rohityadavcloud · 2018-07-04T09:26:35Z

api/src/main/java/org/apache/cloudstack/api/response/AsyncJobResponse.java

    private Date created;

+    @SerializedName(ApiConstants.COMPLETED)
+    @Param(description = "  the removed date of the job")


Change the description and variable to completed?

rohityadavcloud · 2018-07-04T09:27:46Z

framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/dao/AsyncJobDao.java

 public interface AsyncJobDao extends GenericDao<AsyncJobVO, Long> {
+
+    @Override
+    AsyncJobVO findById(Long id);


Why is this necessary?

Because it was referenced by interface and not implementation. But as discussed, I will remove the override and use findByIdIncludingRemoved.

rohityadavcloud · 2018-07-04T09:32:53Z

framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/dao/AsyncJobDaoImpl.java

    }

+    @Override
+    public AsyncJobVO findById(Long id) {


The findByIdIncluding removed previous usage was okay.

rohityadavcloud · 2018-07-04T09:49:49Z

test/integration/smoke/test_async_job.py

+        cmd.jobid = self.virtual_machine.jobid
+        cmd_response = self.apiclient.queryAsyncJobResult(cmd)
+        endtime = cmd_response.endtime
+        self.assertIsNotNone(endtime, "Expected 'endtime' field of queryAsyncJobResult to have a timestamp of when the job finished.")


cmd_response -> get the complete time, then use the dbclient -> async_jobs table -> job id and verify that completed matches what you got in the API response.

In the API, jobstatus -> success/failure -> check that against db as well.

rohityadavcloud · 2018-07-04T09:50:49Z

test/integration/smoke/test_async_job.py

+            "basic",
+            "sg"],
+        required_hardware="true")
+    def test_async_job_table_values(self):


Squash the above two test cases into one, asset equal for the job id's removed/completed time.

Honestly I'm not a fan of combining separate tests into one, this is not a good practice since it's limiting the test granularity. But in this case maybe it does makes sense to do it since the api result is depended on the DB value and could not work without it. In this case it'll save us the time to deploy a VM which is a lot and it does makes sense to combine.

rohityadavcloud · 2018-07-04T09:51:54Z

test/integration/smoke/test_async_job.py

+    def test_async_job_table_values(self):
+        """
+        Test async job db table for valid values
+        """


Add another failure test case. Destroy something twice, etc. job status -> failed (int 2?).

I've tried this case and I'm not sure it baheves as expected. I've successfully destroyed a VM and tried to do it again and the job result was successful same as the first try. Should this behave like that? I think it should return an error saying the VM is already in Destroyed state.

I get the same result as you @borisstoyanov

@ernjvr do you think we should refactor the service if it tries to bring a VM into a state which it is, to prompt an error? It kinda makes sense to me.

@borisstoyanov I investigated the code for the scenario of starting a VM. It also returns a success status when I make a request to start a VM that has already been started. This success status is returned by the VM job handler code. The async job status gets updated with whatever status is returned by the VM job handler code. So, I think changing how the VM handler code deals with duplicate commands for the same resource is out of scope for this feature. I think it should be dealt with in a separate bug fix ticket. What do you think?

Sound the right way to do it, thanks for the investigation @ernjvr, let me log a ticket in community. When we have time we could get back to this.

apache#2736

rohityadavcloud

Please see comments. For integration tests, make use of both the API respones and db client/query result to verify both the job status and the removed time.

… fix API description

rohityadavcloud · 2018-07-11T08:50:37Z

test/integration/smoke/test_async_job.py

+        except Exception as e:
+            self.debug("Warning! Exception in tearDown: %s" % e)
+
+    @attr(


This can be formatted to a single line.

rohityadavcloud · 2018-07-11T08:51:21Z

test/integration/smoke/test_async_job.py

+            "advancedns",
+            "basic",
+            "sg"],
+        required_hardware="true")


This could be set to required_hardware="false".

rohityadavcloud · 2018-07-11T08:52:45Z

test/integration/smoke/test_async_job.py

+            VirtualMachine.RUNNING)
+        self.assertEqual(response[0], PASS, response[1])
+
+        result = self.dbclient.execute("select * from async_job where uuid='%s'" % self.virtual_machine.jobid)


Move this to line 127, closer to the access on result[0][17], probably name this as db_result to make it more obvious.

rohityadavcloud · 2018-07-11T08:53:39Z

test/integration/smoke/test_async_job.py

+        removed = timezone('UTC').localize(result[0][17])
+        removed = removed.astimezone(timezone('CET'))
+        removed = removed.strftime("%Y-%m-%dT%H:%M:%S%z")
+        self.assertEqual(completed, removed, "Expected 'completed' tag value to be equal to 'removed' db column value.")


Remove the word usage tag and use timestamp/datetime instead, it's good enough otherwise. Same at line 137.

rohityadavcloud

Left minor nits, otherwise LGTM. I did not test it, which the internal testing would approve or advise bugs.

rohityadavcloud · 2018-07-11T09:01:43Z

test/integration/smoke/test_async_job.py

+        jobstatus_db = result[0][8]
+        self.assertEqual(jobstatus_api, jobstatus_db, "Expected 'jobstatus' tag value to be equal to 'job_status' db column value.")
+
+        return


You don't return at the end .

rohityadavcloud · 2018-07-11T09:02:04Z

test/integration/smoke/test_async_job.py

+            domainid=self.domain.id
+        )
+        self.cleanup = [self.account]
+        return


Remove the return.

rohityadavcloud · 2018-07-11T09:02:29Z

test/integration/smoke/test_async_job.py

+from marvin.lib.base import *
+from marvin.lib.common import *
+from pytz import timezone
+


Run pep8 or pylint on this file and fix and lint related issues.

Fixes #11 Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* Changed lease time to inifinity * message around DHCP lease infinity Co-authored-by: Andrija Panic <45762285+andrijapanicsb@users.noreply.github.com>

ernjvr requested review from DaanHoogland and rohityadavcloud June 26, 2018 07:56

rohityadavcloud requested changes Jun 27, 2018

View reviewed changes

borisstoyanov reviewed Jul 3, 2018

View reviewed changes

ernjvr added 6 commits July 3, 2018 12:32

initial commit: api, service, db access

f3f2adb

add AsyncJobJoinDaoTest

c81989d

added licence to AsyncJobJoinDaoTest

8514602

refactor AsyncJobJoinDaoTest

74b30a8

add marvin test

9c6ff0d

include removed in findById search

98ddc26

ernjvr force-pushed the async_jobs_add_endtime branch from 01ee941 to 98ddc26 Compare July 3, 2018 10:36

ernjvr added 2 commits July 3, 2018 17:08

Override findById to exclude 'removed IS NULL' sql clause. Rename API…

f685024

… response tag name.

revert to findById

58044ee

rohityadavcloud requested changes Jul 4, 2018

View reviewed changes

rohityadavcloud reviewed Jul 4, 2018

View reviewed changes

rohityadavcloud requested changes Jul 4, 2018

View reviewed changes

ernjvr added 4 commits July 4, 2018 16:30

revert findById override; fix nullPointer during cleanupLeftOverJobs;…

a12b5ff

… fix API description

add sql update for 41110to41200 upgrade

5de2bc6

refactored test case to compare api field value against db column value

d8fca2d

job status check added

3020cf3

rohityadavcloud reviewed Jul 11, 2018

View reviewed changes

rohityadavcloud approved these changes Jul 11, 2018

View reviewed changes

rohityadavcloud reviewed Jul 11, 2018

View reviewed changes

ernjvr added 2 commits July 11, 2018 11:45

formatting and wording changes

6a36f78

pylint changes

4ac5796

ernjvr closed this Jul 11, 2018

rohityadavcloud added a commit that referenced this pull request Jan 20, 2021

package: update dependencies and add core-js (#16)

4f7dda1

Fixes #11 Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

shwstppr pushed a commit that referenced this pull request Jul 27, 2021

Changed lease time to inifinity (#16)

0dacb2c

* Changed lease time to inifinity * message around DHCP lease infinity Co-authored-by: Andrija Panic <45762285+andrijapanicsb@users.noreply.github.com>

shwstppr pushed a commit that referenced this pull request Jun 20, 2024

kvm: run bash scripts with single quotes (#16)

ae7f414

Async jobs add endtime #16

Async jobs add endtime #16

Uh oh!

Conversation

ernjvr commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

rohityadavcloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud left a comment

Choose a reason for hiding this comment

Uh oh!

ernjvr commented Jun 26, 2018 •

edited

Loading