Conversation
|
Blocked on #4443 |
518614c to
4bde734
Compare
4bde734 to
51f8a18
Compare
TL;DR: guest fact discovery now runs as a single remote command, whose output is then parsed into values of individual facts. * Each fact attribute is now defined with a descriptor (see [1]). It keeps the proper type annotation, but we can attach more information. * Each probe is now a shell script snippet, and is attached to its owning fact. * Snippets are collected, joined, and the resulting script is executed. `GuestFacts` then processes the output, delivers discovered values to the right attributes. * No Python code in probes, custom Python code to handle decoding of the script output fully supported (and used, see package manager facts). For the tmt code, nothing changes, `GuestFacts` API remains the same. [1] https://docs.python.org/3/howto/descriptor.html
44acca0 to
aa53188
Compare
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
| is_image_mode = flag_guest_fact( | ||
| """ | ||
| if type bootc 1>&2; then | ||
| if [ "$(sudo bootc status --format yaml | grep image:)" = "image: null" ]; then | ||
| echo 'false' | ||
| else | ||
| echo 'true' | ||
| fi | ||
| else | ||
| echo 'false' | ||
| fi | ||
| """ | ||
| ) |
There was a problem hiding this comment.
The is_image_mode fact has a logic error that will incorrectly return true when the bootc status command fails. If sudo bootc status fails (due to permission issues, bootc errors, etc.), the command substitution returns an empty string, making the comparison [ "" = "image: null" ] evaluate to false, which causes the else branch to execute and echo 'true'. This is incorrect - a failure should result in 'false' or 'unknown'.
# Fix: Check command success explicitly
is_image_mode = flag_guest_fact(
"""
if type bootc 1>&2; then
image=$(sudo bootc status --format yaml 2>/dev/null | grep '^image:' | cut -d' ' -f2)
if [ -n "$image" ] && [ "$image" != "null" ]; then
echo 'true'
else
echo 'false'
fi
else
echo 'false'
fi
"""
)| is_image_mode = flag_guest_fact( | |
| """ | |
| if type bootc 1>&2; then | |
| if [ "$(sudo bootc status --format yaml | grep image:)" = "image: null" ]; then | |
| echo 'false' | |
| else | |
| echo 'true' | |
| fi | |
| else | |
| echo 'false' | |
| fi | |
| """ | |
| ) | |
| is_image_mode = flag_guest_fact( | |
| """ | |
| if type bootc 1>&2; then | |
| image=$(sudo bootc status --format yaml 2>/dev/null | grep '^image:' | cut -d' ' -f2) | |
| if [ -n "$image" ] && [ "$image" != "null" ]; then | |
| echo 'true' | |
| else | |
| echo 'false' | |
| fi | |
| else | |
| echo 'false' | |
| fi | |
| """ | |
| ) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| for line in output.splitlines(): | ||
| line = line.strip() | ||
|
|
||
| if output is None or output.stdout is None: | ||
| return None | ||
| if line.startswith('>>> '): | ||
| current_fact = line[4:] | ||
|
|
||
| return output.stdout.strip() == 'root' | ||
| elif line.startswith('<<<'): | ||
| if current_fact is None: | ||
| raise GeneralError( | ||
| "Malformed fact probe output: closing marker '<<<' without opening marker" | ||
| ) | ||
|
|
||
| def _query_can_sudo(self, guest: 'Guest') -> Optional[bool]: | ||
| try: | ||
| guest.execute(Command("sudo", "-n", "true"), silent=True) | ||
| except tmt.utils.RunError: | ||
| # Failed non-interactive sudo, so we can't sudo | ||
| return False | ||
| # Otherwise we may use sudo | ||
| return True | ||
| facts[current_fact] = self._facts()[current_fact].extract( | ||
| '\n'.join(current_fact_content) | ||
| ) | ||
|
|
||
| def _query_sudo_prefix(self, guest: 'Guest') -> Optional[str]: | ||
| # Note: we cannot reuse `is_superuser` or `can_sudo` fact so we just recall the query | ||
| # functions for now | ||
| if self._query_is_superuser(guest): | ||
| # Root user does not need sudo | ||
| return "" | ||
| if self._query_can_sudo(guest): | ||
| return "sudo" | ||
| return "" | ||
|
|
||
| def _query_is_ostree(self, guest: 'Guest') -> Optional[bool]: | ||
| # https://github.com/vrothberg/chkconfig/commit/538dc7edf0da387169d83599fe0774ea080b4a37#diff-562b9b19cb1cd12a7343ce5c739745ebc8f363a195276ca58e926f22927238a5R1334 | ||
| output = self._execute( | ||
| guest, | ||
| ShellScript( | ||
| """ | ||
| ( [ -e /run/ostree-booted ] || [ -L /ostree ] ) && echo yes || echo no | ||
| """ | ||
| ).to_shell_command(), | ||
| ) | ||
| current_fact = None | ||
| current_fact_content = [] | ||
|
|
||
| if output is None or output.stdout is None: | ||
| return None | ||
| else: | ||
| current_fact_content.append(line) | ||
|
|
||
| return output.stdout.strip() == 'yes' | ||
| return facts |
There was a problem hiding this comment.
Missing validation for unclosed fact markers. If the output ends while current_fact is still set (opened with >>> but never closed with <<<), the method silently loses that fact's data without raising an error. This could happen if a probe script fails mid-execution or produces malformed output.
# Add after the for loop, before return:
if current_fact is not None:
raise GeneralError(
f"Malformed fact probe output: fact '{current_fact}' was opened but never closed"
)| for line in output.splitlines(): | |
| line = line.strip() | |
| if output is None or output.stdout is None: | |
| return None | |
| if line.startswith('>>> '): | |
| current_fact = line[4:] | |
| return output.stdout.strip() == 'root' | |
| elif line.startswith('<<<'): | |
| if current_fact is None: | |
| raise GeneralError( | |
| "Malformed fact probe output: closing marker '<<<' without opening marker" | |
| ) | |
| def _query_can_sudo(self, guest: 'Guest') -> Optional[bool]: | |
| try: | |
| guest.execute(Command("sudo", "-n", "true"), silent=True) | |
| except tmt.utils.RunError: | |
| # Failed non-interactive sudo, so we can't sudo | |
| return False | |
| # Otherwise we may use sudo | |
| return True | |
| facts[current_fact] = self._facts()[current_fact].extract( | |
| '\n'.join(current_fact_content) | |
| ) | |
| def _query_sudo_prefix(self, guest: 'Guest') -> Optional[str]: | |
| # Note: we cannot reuse `is_superuser` or `can_sudo` fact so we just recall the query | |
| # functions for now | |
| if self._query_is_superuser(guest): | |
| # Root user does not need sudo | |
| return "" | |
| if self._query_can_sudo(guest): | |
| return "sudo" | |
| return "" | |
| def _query_is_ostree(self, guest: 'Guest') -> Optional[bool]: | |
| # https://github.com/vrothberg/chkconfig/commit/538dc7edf0da387169d83599fe0774ea080b4a37#diff-562b9b19cb1cd12a7343ce5c739745ebc8f363a195276ca58e926f22927238a5R1334 | |
| output = self._execute( | |
| guest, | |
| ShellScript( | |
| """ | |
| ( [ -e /run/ostree-booted ] || [ -L /ostree ] ) && echo yes || echo no | |
| """ | |
| ).to_shell_command(), | |
| ) | |
| current_fact = None | |
| current_fact_content = [] | |
| if output is None or output.stdout is None: | |
| return None | |
| else: | |
| current_fact_content.append(line) | |
| return output.stdout.strip() == 'yes' | |
| return facts | |
| for line in output.splitlines(): | |
| line = line.strip() | |
| if line.startswith('>>> '): | |
| current_fact = line[4:] | |
| elif line.startswith('<<<'): | |
| if current_fact is None: | |
| raise GeneralError( | |
| "Malformed fact probe output: closing marker '<<<' without opening marker" | |
| ) | |
| facts[current_fact] = self._facts()[current_fact].extract( | |
| '\n'.join(current_fact_content) | |
| ) | |
| current_fact = None | |
| current_fact_content = [] | |
| else: | |
| current_fact_content.append(line) | |
| if current_fact is not None: | |
| raise GeneralError( | |
| f"Malformed fact probe output: fact '{current_fact}' was opened but never closed" | |
| ) | |
| return facts | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
TL;DR: guest fact discovery now runs as a single remote command, whose
output is then parsed into values of individual facts.
keeps the proper type annotation, but we can attach more information.
owning fact.
GuestFactsthen processes the output, delivers discovered values tothe right attributes.
script output fully supported (and used, see package manager facts).
For the tmt code, nothing changes,
GuestFactsAPI remains the same.[1] https://docs.python.org/3/howto/descriptor.html
Pull Request Checklist