server: filter null details during volume to template creation - #4794
Conversation
Fixes apache#4628 mysql> describe user_vm_details; +---------+-----------------+------+-----+---------+----------------+ | Field | Type | Null | Key | Default | Extra | +---------+-----------------+------+-----+---------+----------------+ | id | bigint unsigned | NO | PRI | NULL | auto_increment | | vm_id | bigint unsigned | NO | MUL | NULL | | | name | varchar(255) | NO | | NULL | | | value | varchar(5120) | YES | | NULL | | | display | tinyint(1) | NO | | 1 | | +---------+-----------------+------+-----+---------+----------------+ 5 rows in set (0.00 sec) mysql> describe vm_template_details; +-------------+-----------------+------+-----+---------+----------------+ | Field | Type | Null | Key | Default | Extra | +-------------+-----------------+------+-----+---------+----------------+ | id | bigint unsigned | NO | PRI | NULL | auto_increment | | template_id | bigint unsigned | NO | MUL | NULL | | | name | varchar(255) | NO | | NULL | | | value | varchar(1024) | NO | | NULL | | | display | tinyint(1) | NO | | 1 | | +-------------+-----------------+------+-----+---------+----------------+ 5 rows in set (0.00 sec) While cloud.user_vm_details allows null values to be added for a detail, cloud.vm_template_details doesn't allow null values. This change filters vm details with null values while creating template from a volume. Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S] |
|
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 89 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests [S] |
|
clgtm, but; while this solves the problem that null values are allowed on volume details and not on template values, this problem is not the root of the issue. Why this discrepancy? I see no reason for the difference and I think we need to address that and not work around it. |
|
[S] Trillian test result (tid-93)
|
| .stream() | ||
| .filter(map -> map.getValue() != null) | ||
| .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); | ||
| details.putAll(vmDetails); |
There was a problem hiding this comment.
exactly, see my general comment above. #4794 (comment)
|
@blueorangutan package |
|
@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S] |
1 similar comment
|
@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S] |
|
Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 181 |
|
Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 184 |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
as stated in #4794 (comment), clgtm but i'm going to 👎 this anyway. both templates and volumes must have null either allowed or both must have null not allowed.
@DaanHoogland do you suggest db changes to bring consistency in vm and template details? |
yes, and first investigate which or the two is the way to go. |
|
I would still accept the PR for defensive changes, i.e. don't return/use null values/details. |
I could live with that if we make sure the alignment is done, @rhtyd |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 313 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-345)
|
|
@sureshanaparti @DaanHoogland are you lgtm on merging this? |
yes, code LGTM @shwstppr can you create an improvement ticket to bring consistency for value in the *_details tables (that it shouldn't accept null), and tag it here. |
|
@sureshanaparti Created #4897 |
6 participants
Description
Fixes #4628
While cloud.user_vm_details allows null values to be added for a detail, cloud.vm_template_details doesn't allow null values.
This change filters VM details with null values while creating template from a volume.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Tested on a KVM env,
VM details (Details such as
kvm.vnc.addresscan have NULL value in some scenarios):Details with NULL value not shown in the API response
Template creation from VM ROOT volume:
Before changes - error:
After changes - no error