Skip to content

Improve logs on KVMHostActivityChecker #4706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
DaanHoogland merged 2 commits into from
Apr 13, 2021
Merged

Improve logs on KVMHostActivityChecker #4706

DaanHoogland merged 2 commits into from
Apr 13, 2021

Conversation

@GutoVeronezi
Copy link
Contributor

@GutoVeronezi GutoVeronezi commented Feb 18, 2021

Description

This PR intends to improve logging in the class KVMHostActivityChecker 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.

GabrielBrascher
GabrielBrascher reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@GabrielBrascher GabrielBrascher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall code looks good; most of the changes regard adding logs while keeping the core behavior of the execution flow.

I would like just to bring another option to enhance the logs: what do you think of adding the host's name combined (or not) with the host ID?

In my opinion, this would make it easier to find the problematic host in a blink of an eye, otherwise, there is the need of checking via DB/API to find which host has the respective ID.

@GabrielBrascher GabrielBrascher added this to the 4.16.0.0 milestone Feb 18, 2021
@DaanHoogland
Copy link
Contributor

DaanHoogland commented Feb 18, 2021

pressed enter by accident ^^
code LGTM not tested

@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Feb 18, 2021

Overall code looks good; most of the changes regard adding logs while keeping the core behavior of the execution flow.

I would like just to bring another option to enhance the logs: what do you think of adding the host's name combined (or not) with the host ID?

In my opinion, this would make it easier to find the problematic host in a blink of an eye, otherwise, there is the need of checking via DB/API to find which host has the respective ID.

@GabrielBrascher as you suggested, I added more host information to the log.

GabrielBrascher
GabrielBrascher approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@GabrielBrascher GabrielBrascher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, I did not test it though.

  • +1 on log improvements;
  • the proposal for changing the HostVO toString() format LGTM as well.

@GabrielBrascher GabrielBrascher mentioned this pull request Feb 19, 2021
Merged
7 tasks
@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Apr 7, 2021

@GabrielBrascher @DaanHoogland is there anything else to do?

@blueorangutan
Copy link

blueorangutan commented Apr 9, 2021

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

@GabrielBrascher
Copy link
Member

GabrielBrascher commented Apr 9, 2021

This one looks ready for merge. Travis and Jenkins's tests have passed and the code has 2 LGTMs. Maybe we can spawn @blueorangutan for testing.

@blueorangutan
Copy link

blueorangutan commented Apr 9, 2021

Trillian test result (tid-393)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49324 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4706-t393-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 86 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_deploy_kubernetes_cluster Failure 3610.68 test_kubernetes_clusters.py
test_04_deploy_and_scale_kubernetes_cluster Failure 41.93 test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster Failure 3616.17 test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 49.03 test_kubernetes_clusters.py
test_01_migrate_VM_and_root_volume Error 69.33 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 52.23 test_vm_life_cycle.py

@DaanHoogland DaanHoogland merged commit 911376e into apache:master Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@GabrielBrascher GabrielBrascher GabrielBrascher approved these changes

Assignees

@GutoVeronezi GutoVeronezi

Labels

component:kvm type:enhancement

Projects

None yet

Milestone

4.16.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@GutoVeronezi @DaanHoogland @blueorangutan @GabrielBrascher