[feature] Add presets #62

Devansh3712 · 2024-06-17T16:31:00Z

Description

Presets allow users to save teuthology-suite command config with a custom name that can be used again.

This PR includes:

Create presets table using SQLModel and setup database migrations using alembic
Add presets service for ORM queries
Add endpoints for /presets route
Add fixtures to conftest.py
Add tests for /preset endpoints
Add limit for creating presets
Create a request body schema for PresetArgs

For testing this PR,

TEUTHOLOGY_API_SQLITE_URI should be added to the .env file using the format sqlite:////<path>
Containerized Setup: Run the start.sh script in teuthology/docs/docker-compose
Non-containerized Setup:
- Create a virtual environment and install the dependencies
- Run alembic upgrade head command to migrate the database

Workflow

The create, update and delete endpoints required a user to be logged in. Users can read any stored preset, but cannot modify or delete them.

Creating a Preset

A POST request to the endpoint /presets/add with the following request body:

{
    "username": "example",
    "name": "test",
    "suite": "teuthology:no-ceph",
    "cmd": "<SuiteArgs format for /suite endpoint>"
}

Returns the created preset data, along with a preset ID which can be used to modify the preset
A user can only store 10 presets at a time. If they want to delete their oldest preset and store the new one, append ?replace=true to the endpoint

Get a Preset

A GET request to the endpoint /presets?username=<github_username>&name=<preset_name>
Returns the preset in the following format if it exists:

{
    "id": 1,
    "username": "example",
    "name": "test",
    "suite": "teuthology:no-ceph",
    "cmd": "<SuiteArgs format for /suite endpoint>"
}

Get All Presets of a User

A GET request to the endpoint /presets/list?username=<github_username>
Returns a list of user's presets

Update a Preset

A PUTrequest to the endpoint /presets/edit/<preset_id> with the parameters that need to updated in the preset in the request body
Returns the updated preset contents

Delete a Preset

A DELETE request to the endpoint /presets/delete/<preset_id>

Pulpito-ng Integration

Most of the teuthology commands have default parameters, only some such as suite and branch need to be changed according to the user's need. On the frontend, we can present default values for the command parameters and users can change them accordingly. Once the parameter values are confirmed, they can be sent as a JSON request body to the presets/add endpoint.

A form can be used for taking the input values, along with a checkbox to set the replace query parameter to true, which will delete the user's oldest preset if their count of stored presets is 10.

Checklist

Changes tested in container setup
Changes tested in non-containerized setup
Includes tests
Documentation updated

Signed-off-by: Devansh Singh <[email protected]>

Create presets service for database operations Signed-off-by: Devansh Singh <[email protected]>

Create endpoints for presets Signed-off-by: Devansh Singh <[email protected]>

Add fixtures to conftest.py for database session and FastAPI TestClient Signed-off-by: Devansh Singh <[email protected]>

Add unittests for /presets endpoint Signed-off-by: Devansh Singh <[email protected]>

Signed-off-by: Devansh Singh <[email protected]>

Add TEUTHOLOGY_API_SQLITE_URI environment variable to tox.ini and unit_tests.yaml Signed-off-by: Devansh Singh <[email protected]>

migrations/README

VallariAg

Looks great, thanks for adding a detailed PR description. It would also be helpful to add the setup instructions (for containerised and containerised) in README.

src/teuthology_api/models/presets.py

gh-actions/start.sh

src/teuthology_api/routes/presets.py

Added a limit on how many presets a user can store at a time (currently 10) Signed-off-by: Devansh Singh <[email protected]>

Added a schema for /add endpoint request body for validating cmd attribute. Updated the cmd column datatype from string to JSON for the Presets model. Signed-off-by: Devansh Singh <[email protected]>

Signed-off-by: Devansh Singh <[email protected]>

VallariAg

It's working great! Few things I noticed while testing the PR:

Username should be case insensitive: if I someone has username JohnDoe, it should return their presets even if username is "johndoe". This is because github usernames are case-insensitive (both vallariag and VallariAg redirect to my profile).
/edit: "cmd" should not be required. We can probably send requests to edit without changing cmd. Example, renaming a preset.
/edit and /add: Because we have "--suite" inside the "cmd", I don't think we should be asking the user for that input again in request body with "suite".

{
  "name": "edit_test",
  "suite": "string",         <- this input can be removed 
   "cmd": { 
        "--dry-run": false,
        "--non-interactive": false, 
        "--suite": "string1"
    }
}

We can store/update suite column in our DB based on what is inside "cmd"'s "--suite". And return an error response if suite is missing.

VallariAg

I'm not sure about this:

In /presets/add, I tested with payload:

{
  "name": "test_defaults",
  "suite": "suite:test",
  "cmd": {
           "--ceph": "main",
        "--ceph-repo": "https://github.com/ceph/ceph-ci.git",
        "--machine-type": "testnode",
        "--num": "1",
        "--priority": "70",
        "--suite": "teuthology:no-ceph",
        "--suite-branch": "main",
        "--suite-repo": "https://github.com/ceph/ceph-ci.git",
        "--teuthology-branch": "",
        "--verbose": "1",
        "<config_yaml>": ["/teuthology/containerized_node.yaml"],
        "--owner": "example",
        "--sha1": "dda7c8f4d64b3b69a12734212f3b6b3dd1821349"
  }
}

But when I read this presets with GET /presets/, I get response:

{
  "id": 4,
  "name": "test_defaults",
  "cmd": {
    "dry_run": false,
    "non_interactive": false,
    "verbose": 1,
    "help": false,
    "arch": null,
    "ceph": "main",
    "ceph_repo": "https://github.com/ceph/ceph-ci.git",
    "distro": null,
    "distro_version": null,
    "email": null,
    "flavor": "default",
    "kernel": "distro",
    "machine_type": "testnode",
    "newest": "0",
    "rerun_status": false,
    "rerun_statuses": "fail,dead",
    "sha1": "dda7c8f4d64b3b69a12734212f3b6b3dd1821349",
    "sleep_before_teardown": "0",
    "suite": "teuthology:no-ceph",
    "suite_branch": "main",
    "suite_dir": null,
    "suite_relpath": "qa",
    "suite_repo": "https://github.com/ceph/ceph-ci.git",
    "teuthology_branch": "",
    "validate_sha1": "true",
    "wait": false,
    "config_yaml": [
      "/teuthology/containerized_node.yaml"
    ],
    "owner": "example",
    "num": "1",
    "priority": "70",
    "queue_backend": null,
    "rerun": null,
    "seed": "-1",
    "force_priority": false,
    "no_nested_subset": false,
    "job_threshold": "500",
    "archive_upload": null,
    "archive_upload_url": null,
    "throttle": null,
    "filter": null,
    "filter_out": null,
    "filter_all": null,
    "filter_fragments": "false",
    "subset": null,
    "timeout": "43200",
    "rocketchat": null,
    "limit": "0"
  },
  "suite": "suite:test",
  "username": "VallariAg"
}

This added the SuiteArg defaults and saved the presets with that. When user query this preset, they'll probably gonna see all the defaults and get confused by all these extra args.
Which makes me wonder if we don't set these defaults and let teuthology-api call teuthology.suite with only the args user set, would teuthology be able to run as expected? If we DO need to set defaults before calling teuthology, then we probably need to figure out how to use defaults for teuthology.suite but not save them in DB.

Removed the suite attribute from PresetArgs, created a new model for /update with optional preset fields Signed-off-by: Devansh Singh <[email protected]>

VallariAg

This is looking great to me!
Using TestClient and storing JSON is really nice work :)

VallariAg · 2024-09-06T16:34:28Z

.env.dev

@@ -17,3 +17,5 @@ SESSION_SECRET_KEY=my-secret-key
 # Path where all logs for teuthology-suite or teuthology-kill would be collected
 ARCHIVE_DIR=/archive_dir/
 TEUTHOLOGY_PATH=/teuthology
+
+TEUTHOLOGY_API_SQLITE_URI=


I think we can add here sqlite:////teuthology_api/teuthology.db as value . The above two paths are also for container setup. What do you think?

I left it because I wasn't sure what the actual path for the database would be. If we're keeping teuthology_api/teuthology.db as the path, I'll modify the last commit and update the .env.dev

Devansh3712 self-assigned this Jun 17, 2024

Devansh3712 force-pushed the wip-devansh-presets branch from bfb9d1e to 29de503 Compare June 25, 2024 04:25

VallariAg added enhancement Improving existing feature or logic gsoc-24 Related to GSoC 2024 teuthology project labels Jun 27, 2024

VallariAg linked an issue Jun 27, 2024 that may be closed by this pull request

create presets db and endpoints #56

Open

Devansh3712 force-pushed the wip-devansh-presets branch 2 times, most recently from bba2469 to e33dcbb Compare July 3, 2024 16:14

Devansh3712 marked this pull request as ready for review July 3, 2024 16:21

Devansh3712 requested review from kamoltat and VallariAg July 3, 2024 16:22

Devansh3712 force-pushed the wip-devansh-presets branch 2 times, most recently from 1b08872 to 88ee986 Compare July 12, 2024 07:30

Devansh3712 added 7 commits July 31, 2024 21:37

Add presets table and setup alembic for migrations

0745faa

Signed-off-by: Devansh Singh <[email protected]>

Add services/presets.py

edd9da5

Create presets service for database operations Signed-off-by: Devansh Singh <[email protected]>

Add routes/presets.py

35838c7

Create endpoints for presets Signed-off-by: Devansh Singh <[email protected]>

tests/conftest.py: Add fixtures

3d3f4b5

Add fixtures to conftest.py for database session and FastAPI TestClient Signed-off-by: Devansh Singh <[email protected]>

Add tests/test_preset.py

b78886a

Add unittests for /presets endpoint Signed-off-by: Devansh Singh <[email protected]>

gh-actions/start.sh: Add database environment variable

b2a7de8

Signed-off-by: Devansh Singh <[email protected]>

.github/workflows/unit_tests.yaml: Add database env variable

54099fc

Add TEUTHOLOGY_API_SQLITE_URI environment variable to tox.ini and unit_tests.yaml Signed-off-by: Devansh Singh <[email protected]>

Devansh3712 force-pushed the wip-devansh-presets branch from c8d5b61 to 54099fc Compare July 31, 2024 16:13

kshtsk reviewed Aug 5, 2024

View reviewed changes

migrations/README Show resolved Hide resolved

VallariAg requested changes Aug 5, 2024

View reviewed changes

src/teuthology_api/models/presets.py Show resolved Hide resolved

src/teuthology_api/models/presets.py Outdated Show resolved Hide resolved

gh-actions/start.sh Show resolved Hide resolved

VallariAg reviewed Aug 8, 2024

View reviewed changes

src/teuthology_api/routes/presets.py Outdated Show resolved Hide resolved

VallariAg reviewed Aug 8, 2024

View reviewed changes

src/teuthology_api/routes/presets.py Outdated Show resolved Hide resolved

Devansh3712 force-pushed the wip-devansh-presets branch from 1c41db5 to c3506eb Compare August 8, 2024 14:10

Add check for preset limit

bd3b11d

Added a limit on how many presets a user can store at a time (currently 10) Signed-off-by: Devansh Singh <[email protected]>

Devansh3712 force-pushed the wip-devansh-presets branch from c3506eb to bd3b11d Compare August 8, 2024 14:20

Add PresetArgs schema, update Presets cmd column type

955c1dc

Added a schema for /add endpoint request body for validating cmd attribute. Updated the cmd column datatype from string to JSON for the Presets model. Signed-off-by: Devansh Singh <[email protected]>

Devansh3712 force-pushed the wip-devansh-presets branch from f0fa121 to 955c1dc Compare August 13, 2024 14:14

Update documentation

2b70962

Signed-off-by: Devansh Singh <[email protected]>

Add query parameter to search preset by suite

72c88d9

Signed-off-by: Devansh Singh <[email protected]>

VallariAg reviewed Aug 26, 2024

View reviewed changes

Convert username to lowercase, update request models for presets

1b9424f

Removed the suite attribute from PresetArgs, created a new model for /update with optional preset fields Signed-off-by: Devansh Singh <[email protected]>

Devansh3712 force-pushed the wip-devansh-presets branch from 405a537 to 1b9424f Compare September 4, 2024 13:24

VallariAg approved these changes Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Add presets #62

[feature] Add presets #62

Devansh3712 commented Jun 17, 2024 •

edited

Loading

VallariAg left a comment

VallariAg left a comment •

edited

Loading

VallariAg left a comment

VallariAg left a comment

VallariAg Sep 6, 2024

Devansh3712 Sep 6, 2024

[feature] Add presets #62

Are you sure you want to change the base?

[feature] Add presets #62

Conversation

Devansh3712 commented Jun 17, 2024 • edited Loading

Description

Workflow

Creating a Preset

Get a Preset

Get All Presets of a User

Update a Preset

Delete a Preset

Pulpito-ng Integration

Checklist

VallariAg left a comment

Choose a reason for hiding this comment

VallariAg left a comment • edited Loading

Choose a reason for hiding this comment

VallariAg left a comment

Choose a reason for hiding this comment

VallariAg left a comment

Choose a reason for hiding this comment

VallariAg Sep 6, 2024

Choose a reason for hiding this comment

Devansh3712 Sep 6, 2024

Choose a reason for hiding this comment

Devansh3712 commented Jun 17, 2024 •

edited

Loading

VallariAg left a comment •

edited

Loading