cloudutils: fix warning, error during kvm agent installation #11318
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11318 +/- ##
=========================================
Coverage 16.17% 16.17%
- Complexity 13297 13299 +2
=========================================
Files 5656 5656
Lines 498331 498349 +18
Branches 60476 60480 +4
=========================================
+ Hits 80591 80597 +6
- Misses 408767 408779 +12
Partials 8973 8973
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
should this be targeted towards 4.20? |
boring-cyborg
bot
added
component:agent
component:api
component:build
component:integration
labels
+1 @shwstppr can you maybe rebase it with 4.20 ? |
|
@weizhouapache will do |
Fixes apache#10379 Signed-off-by: Abhishek Kumar <[email protected]>
shwstppr
force-pushed
the
fix-kvm-agent-install-warnings
branch
from
cb7d2ff to
b809e89
Compare
boring-cyborg
bot
added
component:templates
component:UI
systemvm-template-change
labels
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
thanks @shwstppr |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 14903 |
|
@blueorangutan test ubuntu24 kvm-ubuntu24 keepEnv |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ubuntu24 mgmt + kvm-ubuntu24) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-14246) |
|
@blueorangutan test ubuntu24 kvm-ubuntu24 |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ubuntu24 mgmt + kvm-ubuntu24) has been kicked to run smoke tests |
|
there is only a warning now @shwstppr |
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15426 |
|
@blueorangutan test ubuntu24 kvm-ubuntu24 |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ubuntu24 mgmt + kvm-ubuntu24) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14644)
|
|
[SF] Trillian Build Failed (tid-14670) |
|
[SF] Trillian Build Failed (tid-14672) |
|
[SF] Trillian Build Failed (tid-14674) |
|
looks good 4.21.0.0 with this PR |
There was a problem hiding this comment.
Pull Request Overview
Fixes parsing and regex issues in cloudutils that led to warnings/errors during KVM agent installation by tightening output parsing and improving regex usage.
- Make getLines return actual lines rather than a single string.
- Harden default route parsing and add validation for expected output shape.
- Improve regex usage in network and config parsing (raw strings, tighter patterns).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| python/lib/cloudutils/utilities.py | Change getLines to return a list of lines rather than a single string. |
| python/lib/cloudutils/networkConfig.py | Add stricter parsing/validation for default route output; refine regex for inet/Bcast/Mask extraction. |
| python/lib/cloudutils/configFileOps.py | Switch to raw string regex patterns for matching config lines. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| output = cmd.getStdout().strip() | ||
| result = output.split(" ") | ||
| if len(result) < 2: | ||
| logging.debug("Output for the default route incomplete: %s. It should have been '<GATEWAY> <DEVICE>'" % output) | ||
| raise CloudRuntimeException("Output for the default route incomplete") | ||
| gateway = result[0] | ||
| dev = result[1] | ||
|
|
There was a problem hiding this comment.
Splitting by a literal space can yield empty tokens when the output contains multiple spaces or tabs (common in shell output), causing dev to become an empty string despite len(result) >= 2. Use whitespace splitting to normalize tokens, e.g., parts = output.split(), then gateway = parts[0], dev = parts[1], or use a regex like m = re.match(r'(\S+)\s+(\S+)', output).
| result = output.split(" ") | ||
| if len(result) < 2: | ||
| logging.debug("Output for the default route incomplete: %s. It should have been '<GATEWAY> <DEVICE>'" % output) | ||
| raise CloudRuntimeException("Output for the default route incomplete") |
There was a problem hiding this comment.
The raised exception lacks context for troubleshooting. Consider including the actual output and the expected format in the exception message, e.g., raise CloudRuntimeException("Output for the default route incomplete: '%s' (expected ' ')" % output).
| raise CloudRuntimeException("Output for the default route incomplete") | |
| raise CloudRuntimeException("Output for the default route incomplete: '%s' (expected '<GATEWAY> <DEVICE>')" % output) |
|
|
||
| def getLines(self): | ||
| return self.stdout.decode('utf-8').strip('\n') | ||
| return self.stdout.decode('utf-8').strip('\n').split('\n') |
There was a problem hiding this comment.
Use splitlines() to robustly handle different newline conventions and avoid edge cases with trailing newlines: return self.stdout.decode('utf-8').splitlines().
| return self.stdout.decode('utf-8').strip('\n').split('\n') | |
| return self.stdout.decode('utf-8').splitlines() |
| matchString = r"^\ *" + entry.name + ".*" | ||
| elif entry.separator == " ": | ||
| matchString = "^\ *" + entry.name + "\ *" + entry.value | ||
| matchString = r"^\ *" + entry.name + r"\ *" + entry.value | ||
| else: | ||
| if entry.separator == "=": | ||
| matchString = "^\ *" + entry.name + "\ *=\ *" + entry.value | ||
| matchString = r"^\ *" + entry.name + r"\ *=\ *" + entry.value | ||
| else: | ||
| matchString = "^\ *" + entry.name + "\ *" + entry.value | ||
| matchString = r"^\ *" + entry.name + r"\ *" + entry.value |
There was a problem hiding this comment.
These regexes should use \s* (or \s+) for whitespace and escape dynamic components to prevent unintended regex metacharacter interpretation. Example: name = re.escape(entry.name); value = re.escape(entry.value); then build patterns with r'^\s*' + name + r'\s*=\s*' + value (or r'^\s*' + name + r'\s+' + value). Also, r'\ *' is unusual; prefer clear whitespace classes.
| macAddr = line.split("HWaddr ")[1].strip(" ") | ||
| elif line.find("inet ") != -1: | ||
| m = re.search("addr:(.*)\ *Bcast:(.*)\ *Mask:(.*)", line) | ||
| m = re.search(r"addr:([^\s]+)\s*Bcast:([^\s]+)\s*Mask:([^\s]+)", line) |
There was a problem hiding this comment.
this does not work on either ubuntu or rocky/ol
the line looks like
inet 10.0.34.xxx netmask 255.255.240.0 broadcast 10.0.47.255
we can fix it later
…11318) * cloudutils: fix warning, error during kvm agent installation Fixes apache#10379 Signed-off-by: Abhishek Kumar <[email protected]> * fix Signed-off-by: Abhishek Kumar <[email protected]> * Update utilities.py --------- Signed-off-by: Abhishek Kumar <[email protected]>
5 participants
Description
Fixes #10379
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?