Conversation
|
/AzurePipelines run [GITHUB]-trident-pr |
|
Azure Pipelines successfully started running 1 pipeline(s). |
frhuelsz
changed the title
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR refactors the libvirt-based VM console monitoring code by extracting it into a reusable utilities package. The change eliminates code duplication and improves maintainability by consolidating the console grabbing logic that was previously embedded in tools/storm/e2e/scenario/monitoring.go.
Changes:
- Extracted libvirt console monitoring utilities into
tools/storm/utils/libvirtutilspackage - Created reusable
Connect()function for establishing libvirt connections - Created reusable
WaitForVmSerialLogLoginLibvirt()function for monitoring VM console output - Updated
direct_streaming.goto use the new libvirt console utilities instead of file-based serial log monitoring - Updated
monitoring.goto use the centralized console monitoring function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/storm/utils/libvirtutils/open.go | New file providing Connect() function to establish libvirt connections to qemu:///system |
| tools/storm/utils/libvirtutils/console.go | New file containing WaitForVmSerialLogLoginLibvirt() and readerLoop() functions extracted from monitoring.go for console monitoring with login detection |
| tools/storm/helpers/direct_streaming.go | Updated to use new libvirt console utilities instead of file-based WaitForLoginMessageInSerialLog; creates temp file for console output |
| tools/storm/e2e/scenario/monitoring.go | Removed duplicated waitForVmSerialLogLoginLibvirt() and readerLoop() functions, now calls libvirtutils.WaitForVmSerialLogLoginLibvirt() |
| return fmt.Errorf("failed to remove existing VM serial log file: %w", removeErr) | ||
| } | ||
| } | ||
| defer tc.ArtifactBroker().PublishLogFile("vm-serial.log", vmSerialLog) |
There was a problem hiding this comment.
There are conflicting deferred calls to publish "vm-serial.log": this line publishes vmSerialLog (the VM's serial log file path from libvirt domain XML), while lines 122-124 publish logFile.Name() (a newly created temp file). Since the new libvirt console approach writes to a new temp file instead of using the existing VM serial log file, this defer statement should be removed. The VM serial log file at vmSerialLog path is no longer being used or written to in the new implementation.
| defer tc.ArtifactBroker().PublishLogFile("vm-serial.log", vmSerialLog) |
| // Get the VM serial log file path | ||
| vmSerialLog, err := h.findVmSerialLogFile() | ||
| if err != nil { | ||
| tc.FailFromError(err) | ||
| return err | ||
| } | ||
| // For local runs, if serial log already exists, delete it. | ||
| if _, err := os.Stat(vmSerialLog); err == nil { | ||
| logrus.Infof("VM serial log file (%s) already exists, delete it.", vmSerialLog) | ||
| if removeErr := os.Remove(vmSerialLog); removeErr != nil { | ||
| return fmt.Errorf("failed to remove existing VM serial log file: %w", removeErr) | ||
| } | ||
| } |
There was a problem hiding this comment.
The vmSerialLog file path detection, deletion logic (lines 68-79), and the findVmSerialLogFile() function are no longer needed since the new libvirt console approach creates its own temp file for logging (line 117). This code should be removed as it serves no purpose in the new implementation and may cause confusion.
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
🔍 Description
Reuse the libvirt-connection-based vm console grabber.