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 <abhishek.mrt22@gmail.com>
|
@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.
@shwstppr changes LGTM ,to allow template creation from volume.
Do you know any reason to keep some keys (eg. kvm.vnc.address) with value null? Ideally, value in the details table shouldn't accept null.
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 |
rohityadavcloud
requested review from
Pearl1594,
sureshanaparti and
DaanHoogland
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 |
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