Skip to content
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.

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

<feature>[qga]: QGA Improvements IPv6 and Expanded OS Support #406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MatheMatrix
Copy link
Member

@MatheMatrix MatheMatrix commented Dec 3, 2023

1.Add IPv6 vm config support
2.Read the IPv6 address from the VM.
3.Support additional guest operating systems, such as Centos6, Ubuntu16, debian, etc. 4.More bugfix

Resolves: ZSTAC-56243

Change-Id: I6868776c277a28fd38ad45eb9014d64a3931b6d0

Summary by CodeRabbit

  • New Features

    • Introduced support for CentOS version 6.x in service restart commands.
    • Added functionality to retrieve network information for Windows 2008 VMs.
    • Enhanced VM hostname setting to support different QGA OS versions.
  • Improvements

    • Implemented a new decoding function to handle byte decoding with multiple fallback encodings.
  • Bug Fixes

    • Updated VM configuration synchronization to include support for EL6 Linux distributions.
    • Refined methods in the VmQga class to utilize the new decoding function for improved data handling.

1.Add IPv6 vm config support
2.Read the IPv6 address from the VM.
3.Support additional guest operating systems, such as Centos6, Ubuntu16, debian, etc.
4.More bugfix

Resolves: ZSTAC-56243

Change-Id: I6868776c277a28fd38ad45eb9014d64a3931b6d0
@MatheMatrix
Copy link
Member Author

MatheMatrix commented Dec 3, 2023

@coderabbitai review

Copy link

coderabbitai bot commented Dec 3, 2023

Walkthrough

The updates across the KvmAgent and ZStackLib modules introduce new functionalities and enhancements tailored for specific operating systems. A new restart command for CentOS 6.x, handling of Windows 2008 network information, and support for additional Linux distributions in VM configuration sync are key additions. The codebase also sees improvements in handling byte decoding with fallbacks and refined methods for retrieving OS information.

Changes

File Path Change Summary
.../qga_zwatch.py Added ZWATCH_RESTART_CMD_EL6 and functions for Windows 2008 network info.
.../vm_config.py Updated VM config sync for EL6 support and enhanced hostname setting logic.
.../utils/qga.py Added decode_with_fallback function and new Linux OS types; updated methods for consistent byte decoding and OS info retrieval.

🐇✨
To the code we hop and leap,
With every line, the changes seep.
For CentOS, Debian, and Windows' sake,
We craft the code, no bug we make. 🌟


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1813a3b and 59d11f0.
Files selected for processing (3)
  • kvmagent/kvmagent/plugins/qga_zwatch.py (4 hunks)
  • kvmagent/kvmagent/plugins/vm_config.py (2 hunks)
  • zstacklib/zstacklib/utils/qga.py (9 hunks)
Additional comments: 16
kvmagent/kvmagent/plugins/qga_zwatch.py (5)
  • 32-33: The addition of ZWATCH_RESTART_CMD_EL6 aligns with the PR objective to support CentOS 6.

  • 321-364: The new functions is_windows_2008, subnet_mask_to_prefix_length, and get_nic_info_for_windows_2008 are consistent with the PR objective to handle Windows 2008 specific network information retrieval.

  • 116-125: The logic in qga_get_vm_nic to check for the existence of the NIC info script and retrieve NIC info based on the OS type is consistent with the PR objective to extend the capability of reading IPv6 addresses from VMs.

  • 148-153: The logic in zwatch_qga_monitor_vm to handle the restart command for CentOS 6.x is consistent with the PR objective to support additional guest operating systems.

  • 316-318: The push_metrics_to_gateway function is not directly related to the PR objectives, but it seems to be part of the overall QGA functionality.

kvmagent/kvmagent/plugins/vm_config.py (3)
  • 142-159:
    The update to the VM_CONFIG_SYNC_OS_VERSION_SUPPORT dictionary correctly adds support for EL6 as outlined in the PR objective and the summary. This aligns with the goal of expanding support for additional guest operating systems.

  • 217-224:
    The logic within set_vm_hostname_by_qga has been enhanced to conditionally use a different command file for EL6, which is consistent with the PR objective to expand support for additional guest operating systems. The function signature remains unchanged, which is inconsistent with the summary that suggests a potential signature change.

  • 145-145:
    The addition of the VM_QGA_SET_HOSTNAME_EL6 global variable is consistent with the PR objective to expand support for additional guest operating systems, specifically mentioning EL6.

zstacklib/zstacklib/utils/qga.py (8)
  • 51-57: The decode_with_fallback function is a good addition for handling multiple encodings. Ensure that this function is used consistently wherever byte decoding is required in the codebase.

  • 98-103: The addition of new Linux OS types (VM_OS_LINUX_DEBIAN, VM_OS_LINUX_FEDORA) is in line with the PR objective. Ensure that these new OS types are integrated into the existing logic where OS types are used.

  • 217-224: The use of decode_with_fallback in guest_exec_bash is a good practice for handling multiple encodings. Ensure that the decoded data is handled properly after this change.

  • 262-269: The use of decode_with_fallback in guest_exec_python is consistent with the changes made in other methods. Ensure that the decoded data is handled properly after this change.

  • 298-305: The use of decode_with_fallback in guest_exec_zs_tools is consistent with the changes made in other methods. Ensure that the decoded data is handled properly after this change.

  • 332-339: The use of decode_with_fallback in guest_exec_wmic is consistent with the changes made in other methods. Ensure that the decoded data is handled properly after this change.

  • 366-373: The use of decode_with_fallback in guest_exec_powershell is consistent with the changes made in other methods. Ensure that the decoded data is handled properly after this change.

  • 489-513: The modifications to guest_get_os_info to handle different cases for retrieving OS information are in line with the PR objective. Ensure that the new logic is robust and that it doesn't introduce any regressions.

Comment on lines 72 to 73
except:
return False
Copy link

Choose a reason for hiding this comment

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

The addition of a broad except clause in is_qga_connected may suppress important errors. Consider logging the exception or using a more specific exception type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants