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

Agent job refactor #371

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Agent job refactor #371

wants to merge 14 commits into from

Conversation

boukeas
Copy link
Contributor

@boukeas boukeas commented Oct 4, 2024

Description

This PR is a refactoring of the main Testflinger agent loop, i.e. the TestflingerAgent.process_jobs method which controls how an agent picks up a job and controls the execution of its individual phases.

A lot of functionality has been abstracted away from the TestflingerAgent.process_jobs method into the TestflingerJob class and the newly introduced JobPhase class and its derived classes.

Note to reviewers

The structural changes are significant and focusing narrowly on the diff in order to review this sort of refactoring would be disorientating and make little sense. Please read and review TestflingerAgent.process_jobs and the job.py module in their entirety.

Resolved issues

Documentation

Web service API changes

Tests

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

This is one spot that's been begging for a refactor for a while now. I really like your idea and where you are taking it, thanks for taking this on!
I know it's still draft and some obvious things. I was actually able to get it running through some phases in some local testing. I just set up some fake commands for it to run and had it attach to a server I was running in docker locally, then submitted a job to that server for it to pick up. I did find one corner case because it was a new agent and didn't have the directory structure in place for (results, logs, run...). There's an os.makedirs() you removed that I think we'll either need to keep, or handle somewhere to ensure that run dir exists before we try to drop the job file into it.
Looking good so far!

agent/testflinger_agent/agent.py Show resolved Hide resolved
agent/testflinger_agent/tests/test_agent.py Outdated Show resolved Hide resolved
agent/testflinger_agent/job.py Outdated Show resolved Hide resolved
agent/testflinger_agent/job.py Outdated Show resolved Hide resolved
@boukeas boukeas requested a review from plars October 28, 2024 15:44
Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

A few minor comments. I did a bit of testing with this and it mostly worked for me except for one thing that was totally my fault (forgot the current server revision deployed is the one from Varun's branch).

The main thing to fix here is documentation and possibly a renaming of "unpack" but see what you think. I'm not tied to a specific word there as long as it's documented.

But there's an opportunity to fix another bug here too. I think it might be an easy opportunistic fix, and related enough to include it here. But if not, then feel free to kick it back and we can file it as a bug and handle it in a separate PR.

Also, please go ahead and take this out of draft mode so that everyone knows it's something you think is ready for review and merging as soon as it's good :)


try:
self.client.transmit_job_outcome(rundir)
self.client.transmit_job_outcome(str(job.params.rundir))
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I forgot that I had the edge revision of serve from Varun's branch deployed when I was testing this today, and predictably hit a 422 error when it tried to transmit the outcome of a job containing attachments. But then I realized the job was never marked complete. This is because that information is conveyed through transmit_job_outcome. The result is that the CLI is just left in a "stuck" state waiting for the job to complete.

Of course, this isn't really from your patch, but it gave me an "aha" moment for this pathological case that could absolutely occur. I wonder if instead of just failing here, if we could go ahead an try to at least do a client.post_job_state() with the state that we intend to transmit (cancelled or complete) if we fall into the exception block here? That would help with things like this in the future.

@@ -22,6 +22,7 @@

class JobState(StrEnum):
WAITING = "waiting"
UNPACK = "unpack"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we call this "unpack" or some variation of "attachments"? I think the latter sounds more awkward but also makes it more obvious what's happening in this phase.

Either way, we should make sure the documentation is updated to reflect this new phase. In particular, in docs/reference/test-phases.rst and perhaps others that may describe results schema

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.

2 participants