UI: allow new keys for VM details by DaanHoogland · Pull Request #7793 · apache/cloudstack

DaanHoogland · 2023-07-31T09:58:29Z

Description

This PR...

~~Fixes: #6937~~
Fixes: #7700

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

harikrishna-patnala · 2023-07-31T10:52:49Z

@DaanHoogland does this mean, we now allow empty values for the settings?

codecov · 2023-07-31T10:53:22Z

Codecov Report

Merging #7793 (44f75db) into 4.18 (a2eb103) will not change coverage.
Report is 1 commits behind head on 4.18.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               4.18    #7793   +/-   ##
=========================================
  Coverage     13.02%   13.02%           
  Complexity     9030     9030           
=========================================
  Files          2720     2720           
  Lines        257001   257001           
  Branches      40083    40083           
=========================================
  Hits          33463    33463           
  Misses       219337   219337           
  Partials       4201     4201

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

weizhouapache · 2023-07-31T11:07:27Z

@DaanHoogland
after some thinking, I think it is not a UI bug, but a missing feature.

actually when I add a vm setting "extraconfig", it failed

Failed to update VM, due to: 'extraconfig' should not be included in details as key

in the EditVM dialog, we need to add a new field "extraconfig"

DaanHoogland · 2023-07-31T12:01:21Z

@DaanHoogland does this mean, we now allow empty values for the settings?

good question. If the API accepts it we do, i guess.

harikrishna-patnala

what I observed with this fix is

We are not allowing empty values even after the fix. The observed fix is, we are now handling this properly in UI without any errors in the debugger console. In my opinion, this is good.
We could now add extra custom details which are not predefined. Not sure if this is the intention of this fix, but this is also good, there is another issue raised to allow custom keys #7700

Specific to the key value "extraconfig" mentioned in the original issue, intentionally we are not allowing it in the backend, not sure about the reason.

cloudstack/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

Lines 2772 to 2774 in 981d748

    
           if (details.containsKey("extraconfig")) { 
        
               throw new InvalidParameterValueException("'extraconfig' should not be included in details as key"); 
        
           }

@DaanHoogland am I correct with these points ? specifically point 1, it is contradicting the subject

DaanHoogland · 2023-08-01T07:03:02Z

what I observed with this fix is

1. We are not allowing empty values even after the fix. The observed fix is, we are now handling this properly in UI without any errors in the debugger console. In my opinion, this is good.

yes

2. We could now add extra custom details which are not predefined. Not sure if this is the intention of this fix, but this is also good, there is another issue raised to allow custom keys [VM Settings validation blocks custom key value pairs #7700](https://github.com/apache/cloudstack/issues/7700)

yes, it is

3. Specific to the key value "extraconfig" mentioned in the original issue, intentionally we are not allowing it in the backend, not sure about the reason.

functionally i am not sure, but the intention was to allow any key. I did not test with this particular string 🤕 .

@DaanHoogland am I correct with these points ? specifically point 1, it is contradicting the subject

I will change the subject.

@weizhouapache @rohityadavcloud @vishesh92 @nvazquez @MejdiB @Arceoavs, what do and do we not want to allow?

nvazquez · 2023-08-01T10:28:36Z

Hi @DaanHoogland @harikrishna-patnala, I think the reason the 'extraconfig' key is not allowed is because it is a parameter on the deployVirtualMachine API. In case the user passes extra config, those are persisted as VM details with the key 'extraconfig' which can cause issues

harikrishna-patnala · 2023-08-01T13:28:33Z

@nvazquez @DaanHoogland

I saw there is an extra parameter for this updateVirtualMachine API which is "extraconfig".

cloudstack/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java

Lines 147 to 148 in 713a236

    
           @Parameter(name = ApiConstants.EXTRA_CONFIG, type = CommandType.STRING, since = "4.12", description = "an optional URL encoded string that can be passed to the virtual machine upon successful deployment", authorized = { RoleType.Admin }, length = 5120) 
        
           private String extraConfig;

From UI, we are using "details" parameters through which extraconfig key is blocked. This is causing the actual issue.

If we use the separate parameter "extraconfig" in the API then the value will be saved based on the global settings "enable.additional.vm.configuration".

So I think what we can do here is (if at all we decide to allow extraconfig from UI) to check if the detail key is "extraconfig" and then send the value in API parameter "extraconfig" (not the "details").

weizhouapache · 2023-08-02T00:35:57Z

So I think what we can do here is (if at all we decide to allow extraconfig from UI) to check if the detail key is "extraconfig" and then send the value in API parameter "extraconfig" (not the "details").

this looks like a good solution. @harikrishna-patnala @DaanHoogland

nvazquez · 2023-08-02T01:12:23Z

The 'extraconfig' parameter was introduced by this feature: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Enable+additional+configuration+metadata+to+virtual+machines. It lacks UI support, so was only possible to set it in the API. I think that it can be a good partial solution as the way the extra config is stored on the vm_details table is using either named keys such as: extraconfig-NAME1, extraconfig-NAME2 or numerical: extraconfig-1, extraconfig-2. So the updateVirtualMachine API may allow editing the first time but not the second time as the details will be split into multiple details with the extraconfig prefix (all this needs to be tested, I had tested it long ago :) )

weizhouapache · 2023-08-02T01:26:51Z

The 'extraconfig' parameter was introduced by this feature: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Enable+additional+configuration+metadata+to+virtual+machines. It lacks UI support, so was only possible to set it in the API. I think that it can be a good partial solution as the way the extra config is stored on the vm_details table is using either named keys such as: extraconfig-NAME1, extraconfig-NAME2 or numerical: extraconfig-1, extraconfig-2. So the updateVirtualMachine API may allow editing the first time but not the second time as the details will be split into multiple details with the extraconfig prefix (all this needs to be tested, I had tested it long ago :) )

thanks @nvazquez for the info

it looks we need to investigate more on #6937 . sorry @DaanHoogland , I thought it is a simple bug fix

harikrishna-patnala · 2023-08-03T02:53:07Z

The 'extraconfig' parameter was introduced by this feature: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Enable+additional+configuration+metadata+to+virtual+machines. It lacks UI support, so was only possible to set it in the API. I think that it can be a good partial solution as the way the extra config is stored on the vm_details table is using either named keys such as: extraconfig-NAME1, extraconfig-NAME2 or numerical: extraconfig-1, extraconfig-2. So the updateVirtualMachine API may allow editing the first time but not the second time as the details will be split into multiple details with the extraconfig prefix (all this needs to be tested, I had tested it long ago :) )

thanks @nvazquez for the info

it looks we need to investigate more on #6937 . sorry @DaanHoogland , I thought it is a simple bug fix

@weizhouapache this PR can still be merged as this is fixing the other issue #7700 to allow custom keys, also this PR handles the errors that are coming in the debugger console. We can handle #6937 separately.

weizhouapache · 2023-08-03T02:55:18Z

ok @harikrishna-patnala ,thanks for pointing it out
this lgtm

weizhouapache

tested ok

harikrishna-patnala

LGTM. I've verified the fix for the issue #7700

weizhouapache · 2023-08-03T02:58:03Z

LGTM. I've verified the fix for the issue #7700

thanks @harikrishna-patnala . merging

* 4.18: UI: allow new keys for VM details (#7793) Refactoring StorPool's smoke tests (#7392) UI: decode userdata in EditVM dialog (#7796) packaging: unalias cp before package upgrade (#7722) make NoopDbUpgrade do a systemvm template check (#7564) UI unit test: fix expected values (#7792)

empty value on new details

44f75db

DaanHoogland added the component:UI label Jul 31, 2023

DaanHoogland added this to the 4.18.1.0 milestone Jul 31, 2023

DaanHoogland requested a review from weizhouapache July 31, 2023 10:14

weizhouapache changed the title ~~empty value on new details~~ UI: empty value on new VM details Jul 31, 2023

harikrishna-patnala reviewed Aug 1, 2023

View reviewed changes

DaanHoogland changed the title ~~UI: empty value on new VM details~~ UI: allow new keys for VM details Aug 1, 2023

weizhouapache approved these changes Aug 3, 2023

View reviewed changes

harikrishna-patnala approved these changes Aug 3, 2023

View reviewed changes

weizhouapache merged commit c86684f into apache:4.18 Aug 3, 2023

weizhouapache mentioned this pull request Aug 3, 2023

VM Settings validation blocks custom key value pairs #7700

Closed

DaanHoogland deleted the addExtraConfig branch August 3, 2023 06:33

	if (details.containsKey("extraconfig")) {
	throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
	}

Uh oh!

Conversation

DaanHoogland commented Jul 31, 2023 • edited by harikrishna-patnala Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

harikrishna-patnala commented Jul 31, 2023

Uh oh!

codecov Bot commented Jul 31, 2023

Codecov Report

Uh oh!

weizhouapache commented Jul 31, 2023

Uh oh!

DaanHoogland commented Jul 31, 2023

Uh oh!

harikrishna-patnala left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvazquez commented Aug 1, 2023

Uh oh!

harikrishna-patnala commented Aug 1, 2023

Uh oh!

weizhouapache commented Aug 2, 2023

Uh oh!

nvazquez commented Aug 2, 2023

Uh oh!

weizhouapache commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harikrishna-patnala commented Aug 3, 2023

Uh oh!

weizhouapache commented Aug 3, 2023

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Aug 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaanHoogland commented Jul 31, 2023 •

edited by harikrishna-patnala

Loading

harikrishna-patnala left a comment •

edited

Loading

DaanHoogland commented Aug 1, 2023 •

edited

Loading

weizhouapache commented Aug 2, 2023 •

edited

Loading