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

Add experiment requirements #450

Merged
merged 11 commits into from
Sep 9, 2019
Merged

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Sep 5, 2019

Resolves #430.

This adds a requirement field to Experiment, which specifies what requirements (if any) an agent must meet to run that experiment. Requirements are not structured, they are simply strings which are compared for equality. Agents advertise what requirements they meet via the request body when getting their configuration from the /config endpoint. Only experiments whose requirements are a subset of an agent's capabilities can be assigned to that agent. By default, experiments created via the CLI have no requirements--they can be run on any machine--and ones created via webhook (e.g. from craterbot) have the linuxrequirement--they must be run on a Linux machine.

For now, an experiment can have zero or one requirements. In the future, we may want to allow experiments to have an arbitrary number of requirements, and only agents which match all of them will be selected to run that experiment. This would require an additional table however.

For a crater run to be scheduled on both a Linux and Windows agent, you'll need to invoke craterbot twice, once with the linux requirement and once with the windows requirement. Perhaps later we could add some sugar for this?

r? @pietroalbini

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Left a few things to improve but otherwise the changes in this PR look great! In addition to the comments I left it would be nice if the latest agent capabilities were recorded in the agents table and displayed in the agents page of the UI.

docs/agent-http-api.md Outdated Show resolved Hide resolved
src/agent/api.rs Outdated Show resolved Hide resolved
src/experiments.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
@pietroalbini pietroalbini self-assigned this Sep 6, 2019
@ecstatic-morse
Copy link
Contributor Author

If we want to persist agent capabilities to the database, maybe the agent should post its capabilities at start-up when doing GET /config. Do you foresee capabilities changing frequently enough that we need to repost them on every call to /next-experiment?

@pietroalbini
Copy link
Member

If we want to persist agent capabilities to the database, maybe the agent should post its capabilities at start-up when doing GET /config. Do you foresee capabilities changing frequently enough that we need to repost them on every call to /next-experiment?

Yeah sure, you can post them on GET /config.

@ecstatic-morse ecstatic-morse force-pushed the capabilities branch 2 times, most recently from 2bf618f to fd7e526 Compare September 6, 2019 22:33
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 6, 2019

Okay, I add a normalized agent_capabilities table, and use it in a subquery when fetching the next experiment. This required a pretty major rewrite of the PR, so you should take another look.

With the exception of including agent capabilities in their HTML status page, all of your previous concerns should be resolved. Capabilities is now a proper data structure (albeit one that has deref coercion to enable extend and into_iter), and both the define-ex and edit-ex commands allow setting the requirement of an experiment.

I changed GET /config to POST /config, since now behavior depends on the request body. I'm not sure how zealotic you are about REST semantics 😄. I left in the route for GET /config, but it always uses a default set of capabilities (["linux"]).

Also, I'm not quite sure how to handle requirements for Assignee::CLI. Right now, fetching the next experiment for a CLI will ignore experiment requirements. I left a FIXME in src/experiments.rs to this effect.

These capabilities will then be checked against the requirements of
queued experiments to determine which experiment is returned by
`/next-experiment`.
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

All the current code looks good! Once the capabilities are added to the HTML page I think we're ready to merge this!

@pietroalbini
Copy link
Member

I'm not sure how zealotic you are about REST semantics.

Not that much :D

Also, I'm not quite sure how to handle requirements for Assignee::CLI. Right now, fetching the next experiment for a CLI will ignore experiment requirements. I left a FIXME in src/experiments.rs to this effect.

Having the CLI ignoring requirements is fine, as requirements don't really make sense in a CLI environment.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 8, 2019

@pietroalbini Capabilities are now rendered to the status page.

I based Agent::with_capabilities in the second to last commit on Agent::with_experiment (immediately above the newly added function). Maybe there's a better way to do lazy initialization here?

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Looks great! Other than the minor nit this is ready to be merged.

templates/ui/agents.html Outdated Show resolved Hide resolved
@pietroalbini
Copy link
Member

Thanks ❤️

@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2019

📌 Commit d54cd7f has been approved by pietroalbini

@bors
Copy link
Contributor

bors commented Sep 9, 2019

⌛ Testing commit d54cd7f with merge 1ed788c...

bors added a commit that referenced this pull request Sep 9, 2019
Add experiment requirements

Resolves #430.

This adds a `requirement` field to `Experiment`, which specifies what requirements (if any) an agent must meet to run that experiment.  Requirements are not structured, they are simply strings which are compared for equality. Agents advertise what requirements they meet via the request body when getting their configuration from the `/config` endpoint. Only experiments whose requirements are a subset of an agent's capabilities can be assigned to that agent. By default, experiments created via the CLI have no requirements--they can be run on any machine--and ones created via webhook (e.g. from `craterbot`) have the `linux`requirement--they must be run on a Linux machine.

For now, an experiment can have zero or one requirements. In the future, we may want to allow experiments to have an arbitrary number of requirements, and only agents which match all of them will be selected to run that experiment. This would require an additional table however.

For a crater run to be scheduled on both a Linux and Windows agent, you'll need to invoke `craterbot` twice, once with the `linux` requirement and once with the `windows` requirement. Perhaps later we could add some sugar for this?

r? @pietroalbini
@bors
Copy link
Contributor

bors commented Sep 9, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: pietroalbini
Pushing 1ed788c to master...

@bors bors merged commit d54cd7f into rust-lang:master Sep 9, 2019
@bors bors mentioned this pull request Sep 9, 2019
@ecstatic-morse ecstatic-morse deleted the capabilities branch September 10, 2019 17:40
bors added a commit that referenced this pull request Sep 11, 2019
…troalbini

Render experiment requirements in UI

Extends #450 to display experiment requirements in the web UI for both the queue (`/`) and the page for each experiment (`/ex/ex-name`).
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.

Add experiment requirements and agent capabilities
3 participants