Refactor few process of VirtualMachineManagerImpl and improve logs

[GutoVeronezi]

Description

This PR intends to refactor a few process, like extract repeated code to methods, remove unnecessary logic... Of class VirtualMachineManagerImpl and improve logging to facilitate troubleshooting.

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 functioanlity)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

How Has This Been Tested?

It has been tested locally on a test lab.

[DaanHoogland]

clgtm

[blueorangutan]

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 549

[yadvr]

@GutoVeronezi why are you converting them into hand-written json?

[yadvr]

@GutoVeronezi you've submitted many PRs which are the same class of PRs around log improvements and refactorings and changing the resource/vo/object toString() that returns hand-written json and add inconsistency across other VOs/objects/... Why is that? Are the json output from log statements consumed somewhere?

Can you club all such PRs into a single PR with separate/individual commits? They make it difficult to review and test individually and take a lot of time and resources from reviewers and BO/lab.

[GutoVeronezi]

@GutoVeronezi you've submitted many PRs which are the same class of PRs around log improvements and refactorings and changing the resource/vo/object toString() that returns hand-written json and add inconsistency across other VOs/objects/... Why is that? Are the json output from log statements consumed somewhere?

Can you club all such PRs into a single PR with separate/individual commits? They make it difficult to review and test individually and take a lot of time and resources from reviewers and BO/lab.

@rhtyd

Most of toString() implementations uses a simple hyphen (-) to separete values, but what NicProfile[1-2-3-192.168.0.1-...] means? Operators get confused and need to open the code to know what each value means. Moreover, if a string contains a hyphen, it will print as an separator and can confuse much more the operator.
JSON was chosen because it is an easy format to read key-value pairs. NicProfile {"id": 1, "vmId": 2, "reservationId": "3", "iPv4Address": "192.168.0.1", "broadcastUri": "..."} tells much more than a simple -----.

But that is not the focus of the PR. The focus is improve logging of these classes, as they do not supply an appropriate context to make the operation feasible.

About join all this PRs in only one (separated in commits):
Each PR is separted by context. Each one has their own peculiarity. Each peculiarity refactored is separated by commits. Join this all in a single commit will make it even more difficult to review, as there will be a lot of changes without punctual descriptions.

[GabrielBrascher]

Here follow my 2cents.

1 - Number of PRs

@rhtyd raised an interesting point regartding the quantity of PRs.
I agree that there are quite a lot of PRs, and that this can make our life harder to track, test, and review them all. However, I understand that many of them hold their particular contexts.

I think that creating one issue to track these PRs would help on testing and reviewing them all. The idea would be to link any PR that fits this effort on enhancing log messages as well as keeping a track of each PR (merged, under review, draft, etc).

2 - Creating an log message pattern

I have been running 4.16-SNAPSHOT already with a couple of these toString() methods. Personally I am happy to see log messages with a pattern and holding solid information. Maybe we can find better formatations than the JSON like. However, I find them interesting as it is a globally understood pattern for anyone at IT.

The biggest plus in adopting toString is the standardtization on log messages. Nowadays we see all kind of ways of logging resources. Some logs present the name, or the ID, sometimes just the UUID, or combinations of them. It is about time to promote a standard (which would be possible mostly due to toString).

[DaanHoogland]

@rhtyd please let's make changes per PR as little as possible. smaller PRs are usually easier to review. Only when the code is very intertwined should we combine PRs into bigger ones.

[yadvr]

@DaanHoogland I've just shared my preference that is around optimising around reviewers' and lab resources time. One can get the same thing with a single PR with individual small commits that are easier to review (Github allows you to review each commit individually). The benefit of the latter is you save a lot of overhead in people's time in tracking/test, and on smoketest resources.

@GutoVeronezi what you've raised is a valid issue, however, there was no discussion around a new standard/consistent format for people to follow for new code. For example and as a suggestion, instead of NicProfile {"id": 1, "vmId": 2, "reservationId": "3", "iPv4Address": "192.168.0.1", "broadcastUri": "..."}, something like NicProfile [id=1, vmId=2, reservationId=3, iPv4Address=192.168.0.1, ...] may read better, and developers don't need to work with handwritten json. This is merely an example - I'm not asking you to change the string format.

Here something to think about:

• You can present the enhancement cleanups under a standard initiative, start discussion on dev@, i.e. what should be a new standard on improving logs, what new developers, new feature/changes should follow. For example, Gabriel's suggestion on a standard logging pattern is great feedback.
• A central utility/method that can help switch between formats of objects (json, csv, ...), if that is even feasible? Do a poc with your proposal.
• Should we log internal DB IDs in logs? Is the cleanup PRs is replacing old bad logging style in a new way?
• I've seen customers/users who pass logs through an aggregation system such as Splunk where they may have rules to make sense of things. Many old dinosaurs in the community like me are used to a pattern/style of reading logs and code i.e. have we considered what could/would break for existing users and developers, or their approach as their eyes are set looking for something they'll not adjust (written with humour - this point can be completely ignored we don't maintain backward compatibility for logs)

On the issue of context, frankly, any PR that is more than a 100 lines of changes is not going to get a thorough review for each and every line by 2xreviewers, it's simply not feasible at least for me and people I know. Instead, we use smoketests and other means (manual QA, design/architecture review, consistency/pattern of changes, ...) to validate such PRs. I would club all such PRs around the same initiative which in this particular case is around logging/refactoring improvements. Splitting the work around the same initiative in 10 different PRs camouflaging under different contexts will just make the process a lot slower and you're essentially in some way telling the community (of dinosaurs :D) that you want them to work hard in their "free" time and you essentially don't care of the overhead it causes them on reviewing/tracking/testing/...

[yadvr]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 269

[blueorangutan]

Trillian test result (tid-2053)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40902 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4966-t2053-xenserver-71.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[blueorangutan]

Trillian test result (tid-2055)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41000 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4966-t2055-vmware-65u2.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[blueorangutan]

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

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	Error	0.63	test_primary_storage.py
test_01_primary_storage_nfs	Error	0.12	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	Error	0.21	test_primary_storage.py
ContextSuite context=TestKubernetesCluster>:teardown	Error	77.00	test_kubernetes_clusters.py
test_02_list_snapshots_with_removed_data_store	Error	1.19	test_snapshots.py
test_01_secure_vm_migration	Error	178.17	test_vm_life_cycle.py
test_02_unsecure_vm_migration	Error	278.34	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	Error	149.01	test_vm_life_cycle.py
test_08_migrate_vm	Error	44.78	test_vm_life_cycle.py
test_hostha_enable_ha_when_host_in_maintenance	Error	303.83	test_hostha_kvm.py

[nvazquez]

Thanks for reviewing @rhtyd I see your points - opening for discussion @GutoVeronezi @sureshanaparti @rhtyd @GabrielBrascher I think we all agree this PR will need some extensive testing for regressions on VM lifecycle operations. A good sign is that smoke tests are returning good results. Given that we are close to cutting the RC, do you think we can complete all the required testing in time for 4.16?

[yadvr]

@nvazquez General comment - closer to an RC you want to exclude any non-essential PRs you aren't confident on wrt stability and potential regressions. We can move such PRs to merging right after cutting the RC, and move them to the next major or minor release which should give enough time for community to test the changes until the next major/minor release.

[nvazquez]

@rhtyd makes sense, thanks. If no objections let's move this PR to the next milestone to prevent unexpected regressions close to the RC cut date

[DaanHoogland]

@nvazquez @rhtyd does it make sense to move this to the milestone 4.16.1.0? it is not a new feature and was built for 4.16.

[GutoVeronezi]

Restart jenkins...

[GutoVeronezi]

Restart jenkins...

[DaanHoogland]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1757

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

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

Test	Result	Time (s)	Test File