Fix mrack import requiring mrack import

[martinhoyer]

Resolves #3716

Added some debug logging as well.

[happz]

https://github.com/teemtee/tmt/issues/3659 is the same issue, just different place.

[martinhoyer]

@coderabbitai full review

[coderabbitai[bot]]

📝 Walkthrough

Walkthrough

The is_ready property of the GuestBeaker class in tmt/steps/provision/mrack.py was updated to ensure that the mrack module and its dependencies are imported before any usage, especially before exception handling involving mrack.errors. Additional debug logging was added for various guest job states, and exception handling was improved.

Changes

File(s)	Change Summary
tmt/steps/provision/mrack.py	Ensured mrack and dependencies are imported before usage in is_ready, added debug logging for job states, improved exception handling for mrack.errors.MrackError.

Sequence Diagram(s)

sequenceDiagram
    participant GuestBeaker
    participant MrackDeps
    participant MrackAPI
    participant Logger

    GuestBeaker->>MrackDeps: import_and_load_mrack_deps(workdir, name, logger)
    GuestBeaker->>MrackAPI: api.inspect()
    MrackAPI-->>GuestBeaker: response
    alt status == "Aborted"
        GuestBeaker->>Logger: debug("Guest job aborted")
        GuestBeaker-->>GuestBeaker: return False
    else status in {"Error, Aborted", "Cancelled"}
        GuestBeaker->>Logger: debug("Guest job in error/cancelled state")
        GuestBeaker-->>GuestBeaker: return False
    else status == "Reserved"
        GuestBeaker->>Logger: debug("Guest job reserved")
        GuestBeaker-->>GuestBeaker: return True
    else
        GuestBeaker->>Logger: debug("Guest job not ready")
        GuestBeaker-->>GuestBeaker: return False
    end
    Note over GuestBeaker: On MrackError exception
    GuestBeaker->>Logger: warning("Failed to inspect guest: {exception}")
    GuestBeaker-->>GuestBeaker: return False

Assessment against linked issues

Objective	Addressed	Explanation
Ensure import_and_load_mrack_deps() is called before using mrack and handling mrack.errors (#3716)	✅

[!TIP]

⚡️ Faster reviews with caching

• CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.

---

📜 Recent review details

Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b91175bf2d17e1f2222e33a8920619306d936e7e and ceae89fb873073ffdbae41fc562c5a206b571654.

📒 Files selected for processing (1)

• tmt/steps/provision/mrack.py (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (50)

• GitHub Check: osh-diff-scan:fedora-rawhide-x86_64
• GitHub Check: testing-farm:centos-stream-9-x86_64:core
• GitHub Check: testing-farm:centos-stream-9-x86_64:core
• GitHub Check: testing-farm:centos-stream-9-x86_64:core
• GitHub Check: testing-farm:fedora-42-x86_64:core
• GitHub Check: testing-farm:fedora-42-x86_64:core
• GitHub Check: testing-farm:fedora-42-x86_64:core
• GitHub Check: osh-diff-scan:fedora-rawhide-x86_64
• GitHub Check: testing-farm:fedora-42-x86_64:internal-wow
• GitHub Check: testing-farm:fedora-42-x86_64:internal-plugins
• GitHub Check: testing-farm:fedora-42-x86_64:provision
• GitHub Check: testing-farm:fedora-42-x86_64:extended-unit-tests
• GitHub Check: testing-farm:centos-stream-9-x86_64:full
• GitHub Check: testing-farm:fedora-42-x86_64:full
• GitHub Check: testing-farm:centos-stream-9-x86_64:core
• GitHub Check: testing-farm:fedora-42-x86_64:core
• GitHub Check: testing-farm:fedora-41-x86_64:full
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: testing-farm:fedora-40-x86_64:full
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:fedora-40-x86_64
• GitHub Check: testing-farm:fedora-rawhide-x86_64:full
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:fedora-40-x86_64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-40-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: testing-farm:fedora-42-x86_64:internal-wow
• GitHub Check: testing-farm:fedora-42-x86_64:internal-plugins
• GitHub Check: testing-farm:fedora-42-x86_64:provision
• GitHub Check: testing-farm:fedora-42-x86_64:extended-unit-tests
• GitHub Check: testing-farm:fedora-40-x86_64:full
• GitHub Check: testing-farm:fedora-42-x86_64:full
• GitHub Check: testing-farm:centos-stream-9-x86_64:full
• GitHub Check: testing-farm:fedora-41-x86_64:full
• GitHub Check: testing-farm:fedora-rawhide-x86_64:full
• GitHub Check: testing-farm:fedora-42-x86_64:core
• GitHub Check: testing-farm:centos-stream-9-x86_64:core
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:fedora-40-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-rawhide-x86_64

🔇 Additional comments (5)

tmt/steps/provision/mrack.py (5)

1247-1252: Excellent fix for the proper module import sequence.

The code now correctly ensures that mrack and its dependencies are loaded before attempting to use them, particularly before accessing mrack.errors in the exception handling block. This addresses the issue mentioned in the PR description about "mrack import requiring mrack import."

---

1259-1260: Good addition of detailed debug logging.

This debug statement provides valuable information about the job status when it's in the "Aborted" state, which will be helpful for troubleshooting.

---

1265-1266: Consistent debug logging for error states.

Adding debug logging for additional failure states maintains consistency and improves observability of the guest job lifecycle.

---

1270-1273: Comprehensive status logging.

The additional comment and debug statement provide clear context for when a job isn't in the "Reserved" state yet, completing the logging picture for all possible states.

---

1276-1277: Improved exception handling with detailed logging.

Capturing the exception details in the warning message is appropriate and will be helpful for debugging issues with the Beaker job inspection.

To answer martinhoyer's question: Yes, using a warning level makes sense here. It's appropriate for operational issues that aren't critical failures but still require attention, striking the right balance between informational logging and error reporting.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• 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/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[psss]

#3659 is the same issue, just different place.

So this is something which still needs to be covered?

[happz]

#3659 is the same issue, just different place.

So this is something which still needs to be covered?

Rather examined, whether the fix proposed in this PR applies to #3659 as well. I think it does, but since @martinhoyer was knee-deep in fixing this mrack issue, he could be in a better position to find out.

[martinhoyer]

#3659 is the same issue, just different place.

So this is something which still needs to be covered?

Right, I knew I forgot something. @happz @psss I've tried to handle it holistically and it eventually spiraled out of control, where I created _compat/mrack.py, added mrack to banned api and it was just so many changed lines with type checkers going insane. Reverted back to this https://github.com/teemtee/tmt/pull/3718/commits/f2913fa7528825f5a4c681478a884b28ab60fd02, but I'm not sure if it make sense to go this way.

edit: I guess it's one of those things where it's better to set it on fire and start again, but don't mind me, I'm just tired :)

[happz]

#3659 is the same issue, just different place.

So this is something which still needs to be covered?

Right, I knew I forgot something. @happz @psss I've tried to handle it holistically and it eventually spiraled out of control, where I created _compat/mrack.py, added mrack to banned api and it was just so many changed lines with type checkers going insane. Reverted back to this f2913fa, but I'm not sure if it make sense to go this way.

edit: I guess it's one of those things where it's better to set it on fire and start again, but don't mind me, I'm just tired :)

Yes, I believe there is another way. I tried with some lazy imports, and I failed. I think there is a chance for some smart solution, but I think we somehow lost the need for a big, generic approach: there used to be more optional imports, today we're down to mrack and testcloud. Which means, it's probably not necessary to spend more time here and now on the grand solution I'd like to see, and improving the situation is good enough. Which I think your patch does, so, LGTM.

[martinhoyer]

closing in favor of https://github.com/teemtee/tmt/pull/3754