cloudstack icon indicating copy to clipboard operation
cloudstack copied to clipboard

Increase size of column 'value' at table 'account_details'

Open GabrielBrascher opened this issue 3 years ago • 39 comments

Description

This PR increases the column value at table account_details from 255 chars to 4096, matching with the value allowed in the API command for updating the configuration of accounts.

When the value length is bigger than 255, the following log is presented right after the updateConfiguration API call:

2022-03-09 17:50:24,627 ERROR [c.c.a.ApiServer] (qtp30578394-234766:ctx-cad18b45 ctx-32e954dd) (logid:0948e203) unhandled exception executing api command: [Ljava.lang.String;@117c6ba7
com.cloud.utils.exception.CloudRuntimeException: DB Exception on: com.mysql.cj.jdbc.ClientPreparedStatement: INSERT INTO account_details (account_details.account_id, account_details.name, account_details.value) VALUES (123, _binary'api.allowed.source.cidr.list', _binary'<huge binary>')
	at com.cloud.utils.db.GenericDaoBase.persist(GenericDaoBase.java:1450)
	at jdk.internal.reflect.GeneratedMethodAccessor168.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	....
	....
	....
Caused by: com.mysql.cj.jdbc.exceptions.MysqlDataTruncation: Data truncation: Data too long for column 'value' at row 1
	at com.mysql.cj.jdbc.exceptions.SQLExceptionsMapping.translateException(SQLExceptionsMapping.java:104)
	at com.mysql.cj.jdbc.ClientPreparedStatement.executeInternal(ClientPreparedStatement.java:953)
	at com.mysql.cj.jdbc.ClientPreparedStatement.executeUpdateInternal(ClientPreparedStatement.java:1092)
	... 83 more

Types of changes

  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] New feature (non-breaking change which adds functionality)
  • [X] 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
  • [ ] Minor

Bug Severity

  • [ ] BLOCKER
  • [ ] Critical
  • [ ] Major
  • [X] Minor
  • [ ] Trivial

Screenshots (if appropriate):

How Has This Been Tested?

  • Update configurations for an account
  • Assert that configuration is properly updated (which was not the case before the change)

GabrielBrascher avatar Mar 09 '22 17:03 GabrielBrascher

@blueorangutan package

GabrielBrascher avatar Mar 09 '22 17:03 GabrielBrascher

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan avatar Mar 09 '22 17:03 blueorangutan

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 2817

blueorangutan avatar Mar 09 '22 18:03 blueorangutan

@blueorangutan test

nvazquez avatar Mar 10 '22 02:03 nvazquez

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan avatar Mar 10 '22 02:03 blueorangutan

Trillian test result (tid-3552) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 31419 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6080-t3552-kvm-centos7.zip Smoke tests completed. 92 look OK, 0 have errors Only failed tests results shown below:

Test Result Time (s) Test File

blueorangutan avatar Mar 10 '22 12:03 blueorangutan

Before merging, I am afraid that we might need to also increase the size of the value in the response object, otherwise, the API response will not contain the whole value stored in DB. Placing it as a draft in the meantime to avoid it getting merged.

GabrielBrascher avatar Mar 10 '22 12:03 GabrielBrascher

Thanks @GabrielBrascher I was spinning an env for testing it - will post my review after that

nvazquez avatar Mar 10 '22 12:03 nvazquez

Thanks, @nvazquez! I will do some changes and run some manual tests.

GabrielBrascher avatar Mar 10 '22 13:03 GabrielBrascher

@GabrielBrascher I've tested through the API and UI:

  • API works as expected, passed value parameters with length > 255 and the response was properly retrieved.
  • UI seems to still be truncating the value at 255 characters, can you check if this can be extended?

Example: input length = 650 characters:

(localcloud) 🐱 > update configuration accountid=deb22344-0869-49a2-ac1f-116cb858ee03 name='api.allowed.source.cidr.list' value=12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
{
  "configuration": {
    "category": "Advanced",
    "description": "Comma separated list of IPv4/IPv6 CIDRs from which API calls can be performed. Can be set on Global and Account levels.",
    "isdynamic": true,
    "name": "api.allowed.source.cidr.list",
    "scope": "account",
    "value": "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890"
  }
}
Screen Shot 2022-03-10 at 10 13 45

nvazquez avatar Mar 10 '22 13:03 nvazquez

@nvazquez I am seeing the following:

  • Update command and response looks OK:
(poc) 🐱 > update configuration accountid=064a02be-caf8-46c7-8264-92c5a9d782b2 name=api.allowed.source.cidr.list value=12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
{
  "configuration": {
    "category": "Advanced",
    "description": "Comma separated list of IPv4/IPv6 CIDRs from which API calls can be performed. Can be set on Global and Account levels.",
    "isdynamic": true,
    "name": "api.allowed.source.cidr.list",
    "scope": "account",
    "value": "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890"
  }
}
  • But list command returns a fraction:
(poc) 🐱 > list configurations name=api.allowed.source.cidr.list accountid=064a02be-caf8-46c7-8264-92c5a9d782b2 pagesize=600 page=1
{
  "configuration": [
    {
      "category": "Advanced",
      "description": "Comma separated list of IPv4/IPv6 CIDRs from which API calls can be performed. Can be set on Global and Account levels.",
      "isdynamic": true,
      "name": "api.allowed.source.cidr.list",
      "scope": "account",
      "value": "123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345"
    }
  ],
  "count": 1
}

Updated value with length of 651

"value": "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890"

listed value with length of 256

"value": "123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345"

GabrielBrascher avatar Mar 10 '22 14:03 GabrielBrascher

Yes @GabrielBrascher seeing the same - the only remark from your comment is the lengths are 650 and 255. Extending the response field should solve it, worth checking if the UI does truncate or simply displays the received data for configurations

nvazquez avatar Mar 10 '22 14:03 nvazquez

Hi @GabrielBrascher please advise when this one is ready for testing again

nvazquez avatar Mar 15 '22 14:03 nvazquez

@GabrielBrascher can you address the comments and fix the issue in your testing ?

weizhouapache avatar Apr 04 '22 07:04 weizhouapache

Hi @${author}, your pull request has merge conflicts. Can you fix the conflicts and sync your branch with the base branch?

github-actions[bot] avatar Apr 07 '22 05:04 github-actions[bot]

Sorry for the silence on my part here @weizhouapache @nvazquez. I will try to have it sorted this week, but I am afraid that I will need to keep it on hold/Draft due to more pressing demands on another task.

GabrielBrascher avatar Apr 20 '22 13:04 GabrielBrascher

Found UI changes, kicking a new UI QA build @blueorangutan ui

acs-robot avatar Aug 05 '22 12:08 acs-robot

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan avatar Aug 05 '22 12:08 blueorangutan

UI build: :heavy_multiplication_x: (SL-JID-2085)

blueorangutan avatar Aug 05 '22 12:08 blueorangutan

@blueorangutan package

shwstppr avatar Aug 05 '22 12:08 shwstppr

@shwstppr a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan avatar Aug 05 '22 12:08 blueorangutan

Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 3921

blueorangutan avatar Aug 08 '22 04:08 blueorangutan

@blueorangutan package

shwstppr avatar Aug 09 '22 05:08 shwstppr

@shwstppr a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan avatar Aug 09 '22 05:08 blueorangutan

@BartJM I asked you to move the schema change to 4.17.1 upgrade path as milestone is set to 4.17.1 but the PR is targeted for main, do you want it to go only into main? If you want it in 4.17.1 can you please rebase and target it to 4.17 branch. Else you will need to again move schema changes into 41710to41800 upgrade path. Sorry for not mentioning it earlier. cc @GabrielBrascher

shwstppr avatar Aug 09 '22 05:08 shwstppr

Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 3942

blueorangutan avatar Aug 09 '22 06:08 blueorangutan

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

github-actions[bot] avatar Aug 09 '22 10:08 github-actions[bot]

Found UI changes, kicking a new UI QA build @blueorangutan ui

acs-robot avatar Aug 09 '22 11:08 acs-robot

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan avatar Aug 09 '22 12:08 blueorangutan

UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6080 (SL-JID-2113)

blueorangutan avatar Aug 09 '22 12:08 blueorangutan