You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/02/04 08:37:17 UTC
[GitHub] [cloudstack] soreana opened a new pull request #4649: add creation date as a value for domains and accounts.
soreana opened a new pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649
### Description
This PR enables the cloudstack to store the creation time of accounts and domains. We already implemented and used this feature in our platforms. We thought it might be useful for the community too.
The changes in this PR are as small as adding some fields and the setter/getter method. Although the database changes might seem scary, they are minor changes. Except defining the `add_created_column_in_domain_and_account` procedure, I only added the following lines in `account_view` and `domain_view`. Generally speaking, in the mentioned procedure, I added the `created` column to the `account` and `domain` tables, and I ignore its creation if they have already existed.
```
`account`.`created` AS `created`,
...
`domain`.`created` AS `created`,
```
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### 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)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] soreana commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
soreana commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779730376
@DaanHoogland You mean advanced zone and isolated networks?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-774014858
looks usefull for auditing but travis shows some sql syntax error. can you have a look @soreana ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] soreana commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
soreana commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-774821282
@DaanHoogland I think it was fixed. I will check the Travis later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#discussion_r576783045
##########
File path: engine/schema/src/main/resources/META-INF/db/schema-41510to41600.sql
##########
@@ -19,3 +19,285 @@
-- Schema upgrade from 4.15.1.0 to 4.16.0.0
--;
+
+--;
+-- Stored procedure to do idempotent column add;
+-- This is copied from schema-41000to41100.sql
+--;
+DROP PROCEDURE IF EXISTS `cloud`.`IDEMPOTENT_ADD_COLUMN`;
+
+CREATE PROCEDURE `cloud`.`IDEMPOTENT_ADD_COLUMN` (
Review comment:
@soreana FYI see https://github.com/apache/cloudstack/commit/543f9827ff83e1b8f433d98fbe4d5518387b440d#r47173483
@sureshanaparti has concocted a solution:
```
DELIMITER $$
CREATE PROCEDURE `cloud`.`IDEMPOTENT_ADD_COLUMN` (
IN in_table_name VARCHAR(200),
IN in_column_name VARCHAR(200),
IN in_column_definition VARCHAR(1000) )
BEGIN
DECLARE CONTINUE HANDLER FOR 1060 BEGIN END; SET @ddl = CONCAT('ALTER TABLE ', in_table_name); SET @ddl = CONCAT(@ddl, ' ', 'ADD COLUMN') ; SET @ddl = CONCAT(@ddl, ' ', in_column_name); SET @ddl = CONCAT(@ddl, ' ', in_column_definition); PREPARE stmt FROM @ddl; EXECUTE stmt; DEALLOCATE PREPARE stmt;
END $$
DELIMITER ;
```
but more important, this code already exists in the system at https://github.com/apache/cloudstack/blob/master/engine/schema/src/main/resources/META-INF/db/schema-41000to41100.sql#L27
why did you add it again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] soreana commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
soreana commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779709753
Sure, I will do that! Thanks :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779705116
the issue is with upgrade @rhtyd trillian wouldn't show it, would it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] soreana commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
soreana commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779708200
@rhtyd Could you please let me know what is the issue and how I can reproduce it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland edited a comment on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland edited a comment on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779727647
@soreana , can you also try running your upgrade code in isolation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779708926
@soreana can you build pkgs and deploy database (using cloudstack-setup-databases) in a test env and then start mgmt server - the mgmt server fails while running the sql file (the specific one changed in this PR).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] soreana edited a comment on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
soreana edited a comment on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779730376
@DaanHoogland You mean advanced zone (isolated networks)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-778120831
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779733508
no, i mean run the upgrade script, to see if it is really idem potent. on a 4.16 env that is stopped.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] soreana commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
soreana commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-774075705
@DaanHoogland Yes, sure! I'm going to do that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache edited a comment on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
weizhouapache edited a comment on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779900936
@DaanHoogland @rhtyd
I have setup a testing environment. it looks good.
might it because travis/jenkins use simulator for testing ?
```
mysql> select * from version;
+----+----------+---------------------+----------+
| id | version | updated | step |
+----+----------+---------------------+----------+
| 1 | 4.0.0 | 2021-02-16 14:58:19 | Complete |
| 2 | 4.1.0 | 2021-02-16 14:58:55 | Complete |
| 3 | 4.2.0 | 2021-02-16 14:59:05 | Complete |
| 4 | 4.2.1 | 2021-02-16 14:59:05 | Complete |
| 5 | 4.3.0 | 2021-02-16 14:59:10 | Complete |
| 6 | 4.4.0 | 2021-02-16 14:59:17 | Complete |
| 7 | 4.4.1 | 2021-02-16 14:59:18 | Complete |
| 8 | 4.4.2 | 2021-02-16 14:59:18 | Complete |
| 9 | 4.5.0 | 2021-02-16 14:59:24 | Complete |
| 10 | 4.4.4 | 2021-02-16 14:59:24 | Complete |
| 11 | 4.5.1 | 2021-02-16 14:59:24 | Complete |
| 12 | 4.5.2 | 2021-02-16 14:59:25 | Complete |
| 13 | 4.5.3 | 2021-02-16 14:59:25 | Complete |
| 14 | 4.6.0 | 2021-02-16 14:59:26 | Complete |
| 15 | 4.6.1 | 2021-02-16 14:59:26 | Complete |
| 16 | 4.7.0 | 2021-02-16 14:59:26 | Complete |
| 17 | 4.7.1 | 2021-02-16 14:59:26 | Complete |
| 18 | 4.8.0 | 2021-02-16 14:59:26 | Complete |
| 19 | 4.8.1 | 2021-02-16 14:59:26 | Complete |
| 20 | 4.9.0 | 2021-02-16 14:59:29 | Complete |
| 21 | 4.9.1.0 | 2021-02-16 14:59:30 | Complete |
| 22 | 4.9.2.0 | 2021-02-16 14:59:30 | Complete |
| 23 | 4.9.3.0 | 2021-02-16 14:59:30 | Complete |
| 24 | 4.10.0.0 | 2021-02-16 14:59:32 | Complete |
| 25 | 4.11.0.0 | 2021-02-16 14:59:33 | Complete |
| 26 | 4.11.1.0 | 2021-02-16 14:59:34 | Complete |
| 27 | 4.11.2.0 | 2021-02-16 14:59:34 | Complete |
| 28 | 4.11.3.0 | 2021-02-16 14:59:34 | Complete |
| 29 | 4.12.0.0 | 2021-02-16 14:59:35 | Complete |
| 30 | 4.13.0.0 | 2021-02-16 14:59:36 | Complete |
| 31 | 4.13.1.0 | 2021-02-16 14:59:36 | Complete |
| 32 | 4.14.0.0 | 2021-02-16 14:59:37 | Complete |
| 33 | 4.15.0.0 | 2021-02-16 14:59:39 | Complete |
| 34 | 4.15.1.0 | 2021-02-16 14:59:39 | Complete |
| 35 | 4.16.0.0 | 2021-02-16 14:59:39 | Complete |
+----+----------+---------------------+----------+
35 rows in set (0.00 sec)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-778594081
travis and manual tests, this is only API and DB so it should do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779727647
@soreana , can you also try running your upgrade coe in isolation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-778121377
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779900936
@DaanHoogland @rhtyd
I have setup a testing environment and added a zone successfully. it looks good.
might it because travis/jenkins use simulator for testing ?
```
mysql> select * from version;
+----+----------+---------------------+----------+
| id | version | updated | step |
+----+----------+---------------------+----------+
| 1 | 4.0.0 | 2021-02-16 14:58:19 | Complete |
| 2 | 4.1.0 | 2021-02-16 14:58:55 | Complete |
| 3 | 4.2.0 | 2021-02-16 14:59:05 | Complete |
| 4 | 4.2.1 | 2021-02-16 14:59:05 | Complete |
| 5 | 4.3.0 | 2021-02-16 14:59:10 | Complete |
| 6 | 4.4.0 | 2021-02-16 14:59:17 | Complete |
| 7 | 4.4.1 | 2021-02-16 14:59:18 | Complete |
| 8 | 4.4.2 | 2021-02-16 14:59:18 | Complete |
| 9 | 4.5.0 | 2021-02-16 14:59:24 | Complete |
| 10 | 4.4.4 | 2021-02-16 14:59:24 | Complete |
| 11 | 4.5.1 | 2021-02-16 14:59:24 | Complete |
| 12 | 4.5.2 | 2021-02-16 14:59:25 | Complete |
| 13 | 4.5.3 | 2021-02-16 14:59:25 | Complete |
| 14 | 4.6.0 | 2021-02-16 14:59:26 | Complete |
| 15 | 4.6.1 | 2021-02-16 14:59:26 | Complete |
| 16 | 4.7.0 | 2021-02-16 14:59:26 | Complete |
| 17 | 4.7.1 | 2021-02-16 14:59:26 | Complete |
| 18 | 4.8.0 | 2021-02-16 14:59:26 | Complete |
| 19 | 4.8.1 | 2021-02-16 14:59:26 | Complete |
| 20 | 4.9.0 | 2021-02-16 14:59:29 | Complete |
| 21 | 4.9.1.0 | 2021-02-16 14:59:30 | Complete |
| 22 | 4.9.2.0 | 2021-02-16 14:59:30 | Complete |
| 23 | 4.9.3.0 | 2021-02-16 14:59:30 | Complete |
| 24 | 4.10.0.0 | 2021-02-16 14:59:32 | Complete |
| 25 | 4.11.0.0 | 2021-02-16 14:59:33 | Complete |
| 26 | 4.11.1.0 | 2021-02-16 14:59:34 | Complete |
| 27 | 4.11.2.0 | 2021-02-16 14:59:34 | Complete |
| 28 | 4.11.3.0 | 2021-02-16 14:59:34 | Complete |
| 29 | 4.12.0.0 | 2021-02-16 14:59:35 | Complete |
| 30 | 4.13.0.0 | 2021-02-16 14:59:36 | Complete |
| 31 | 4.13.1.0 | 2021-02-16 14:59:36 | Complete |
| 32 | 4.14.0.0 | 2021-02-16 14:59:37 | Complete |
| 33 | 4.15.0.0 | 2021-02-16 14:59:39 | Complete |
| 34 | 4.15.1.0 | 2021-02-16 14:59:39 | Complete |
| 35 | 4.16.0.0 | 2021-02-16 14:59:39 | Complete |
+----+----------+---------------------+----------+
35 rows in set (0.00 sec)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-780016084
@weizhouapache this is a "intermediate" upgrade problem. No issue woth the PR. The aledged/suspect code turned out to also exist in schema-41000to41100.sql which was a side catch and a very minor issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779700497
@DaanHoogland this has caused a regression on master, no trillian tests seen. Can you check and revert or address the issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779706357
I think fresh install would fail with Trillian if something's wrong in the db schema @DaanHoogland nevertheless for PRs that bring in too many DB changes - it's important that we kick and validate them with Trillian tests, since Travis based tests uses mvn and not the actual cloudstack-setup-databases scripts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-779798101
I've kicked smoketests on master/4.16 via https://github.com/apache/cloudstack/pull/4577 let's wait and see
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4649: add creation date as a value for domains and accounts.
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4649:
URL: https://github.com/apache/cloudstack/pull/4649#issuecomment-778158346
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2689
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org