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

allocrunner: provide factory function so we can build mock ARs #17161

Merged
merged 1 commit into from
May 12, 2023

Conversation

tgross
Copy link
Member

@tgross tgross commented May 11, 2023

Tools like the nomad-nodesim are unable to implement a minimal implementation of an allocrunner so that we can test the client communication without having to lug around the entire allocrunner/taskrunner code base. The allocrunner was implemented with an interface specifically for this purpose, but there were circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory function type. Provide a minimal test that exercises the new function so that consumers have some idea of what the minimum implementation required is.

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

Step 2: group drivers & allocrunner plugins
Step 3: profit???

client/allocrunner/testing.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client_interface_test.go Outdated Show resolved Hide resolved
@schmichael
Copy link
Member

Should we backport this to ease future backports that touch this code? It seems very safe. 🤔

@tgross
Copy link
Member Author

tgross commented May 11, 2023

I've moved this to draft as with the new changes I've got a version that compiles but probably doesn't pass tests yet. Will pick it up in the morning.

@tgross
Copy link
Member Author

tgross commented May 12, 2023

@schmichael @shoenig I've reworked based on @schmichael's feedback by moving the allocrunner.Config into client/config.AllocRunnerConfig, which we can use without creating an import cycle. This suggests long-term that pushing some more of the core types for the client into client/structs might be nice at some point in terms of untangling all of this, but this gives us an incremental improvement and a new interface for testing client.Client behavior.

@tgross
Copy link
Member Author

tgross commented May 12, 2023

Should we backport this to ease future backports that touch this code? It seems very safe

Will do.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgross tgross added backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line backport/1.5.x backport to 1.5.x release line labels May 12, 2023
@tgross tgross merged commit 88323ba into main May 12, 2023
@tgross tgross deleted the mock-allocrunner branch May 12, 2023 17:29
tgross added a commit that referenced this pull request May 12, 2023
Tools like `nomad-nodesim` are unable to implement a minimal implementation of
an allocrunner so that we can test the client communication without having to
lug around the entire allocrunner/taskrunner code base. The allocrunner was
implemented with an interface specifically for this purpose, but there were
circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory
function type. Provide a minimal test that exercises the new function so that
consumers have some idea of what the minimum implementation required is.
tgross added a commit that referenced this pull request May 12, 2023
Tools like `nomad-nodesim` are unable to implement a minimal implementation of
an allocrunner so that we can test the client communication without having to
lug around the entire allocrunner/taskrunner code base. The allocrunner was
implemented with an interface specifically for this purpose, but there were
circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory
function type. Provide a minimal test that exercises the new function so that
consumers have some idea of what the minimum implementation required is.
tgross added a commit that referenced this pull request May 12, 2023
Tools like `nomad-nodesim` are unable to implement a minimal implementation of
an allocrunner so that we can test the client communication without having to
lug around the entire allocrunner/taskrunner code base. The allocrunner was
implemented with an interface specifically for this purpose, but there were
circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory
function type. Provide a minimal test that exercises the new function so that
consumers have some idea of what the minimum implementation required is.
tgross added a commit that referenced this pull request May 12, 2023
… (#17174)

Tools like `nomad-nodesim` are unable to implement a minimal implementation of
an allocrunner so that we can test the client communication without having to
lug around the entire allocrunner/taskrunner code base. The allocrunner was
implemented with an interface specifically for this purpose, but there were
circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory
function type. Provide a minimal test that exercises the new function so that
consumers have some idea of what the minimum implementation required is.

Co-authored-by: Tim Gross <[email protected]>
tgross added a commit that referenced this pull request May 12, 2023
… (#17173)

Tools like `nomad-nodesim` are unable to implement a minimal implementation of
an allocrunner so that we can test the client communication without having to
lug around the entire allocrunner/taskrunner code base. The allocrunner was
implemented with an interface specifically for this purpose, but there were
circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory
function type. Provide a minimal test that exercises the new function so that
consumers have some idea of what the minimum implementation required is.

Co-authored-by: Tim Gross <[email protected]>
tgross added a commit that referenced this pull request May 12, 2023
… (#17172)

Tools like `nomad-nodesim` are unable to implement a minimal implementation of
an allocrunner so that we can test the client communication without having to
lug around the entire allocrunner/taskrunner code base. The allocrunner was
implemented with an interface specifically for this purpose, but there were
circular imports that made it challenging to use in practice.

Move the AllocRunner interface into an inner package and provide a factory
function type. Provide a minimal test that exercises the new function so that
consumers have some idea of what the minimum implementation required is.

Co-authored-by: Tim Gross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line backport/1.5.x backport to 1.5.x release line theme/testing Test related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants