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.

Sign up for GitHub

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

Jump to bottom

Adding build canceling (only works with compatible celery brokers redis and rabbitmq) #531

Merged
merged 31 commits into from
Dec 18, 2023
Merged

Adding build canceling (only works with compatible celery brokers redis and rabbitmq) #531

merged 31 commits into from
Dec 18, 2023

Conversation

costrouc
Copy link
Member

@costrouc costrouc commented Aug 8, 2023 •
edited
Loading

Closes #306

Build canceling exposed at PUT /api/v1/build/<build-id>/cancel/ and added a new permissions build::cancel which is available by default for admin and developer roles.

@costrouc costrouc changed the title Adding build canceling (only works with compatible celery brokers redis and rabbitmq Adding build canceling (only works with compatible celery brokers redis and rabbitmq) Aug 8, 2023
@costrouc

This comment was marked as resolved.

Sign in to view
@trallard

This comment was marked as outdated.

Sign in to view
@costrouc

This comment was marked as outdated.

Sign in to view
@trallard

This comment was marked as outdated.

Sign in to view
@trallard trallard added this to the 🚀 JATIC - Q1 milestone Aug 15, 2023
@costrouc

This comment was marked as outdated.

Sign in to view
@anirrudh

This comment was marked as outdated.

Sign in to view
@anirrudh

This comment was marked as resolved.

Sign in to view
@costrouc
Copy link
Member Author

There is a some logic in marking a build as canceled here

conda-store/conda-store-server/conda_store_server/build.py

Line 88 in b6be123

datetime.datetime.utcnow() - datetime.timedelta(seconds=5)
that might be important context @anirrudh . It requires that a build has been BUILDING for at least 5 seconds before it would be marked as failed.

So we should wait 5 seconds in the test before canceling the task. I added the 5 seconds as a safe guard from race conditions though I'm not really sure that check is needed.

costrouc

This comment was marked as outdated.

Sign in to view

@costrouc
Copy link
Member Author

LGTM. Thanks @anirrudh !

pierrotsmnrd
pierrotsmnrd previously requested changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@pierrotsmnrd pierrotsmnrd left a comment

Choose a reason for hiding this comment

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

At least one requested change to properly cancel "docker" tasks. The rest is improvements / suggestions.

conda-store-server/conda_store_server/server/views/api.py Outdated Show resolved Hide resolved
conda-store-server/conda_store_server/server/views/api.py Outdated Show resolved Hide resolved
@costrouc costrouc mentioned this pull request Aug 24, 2023
Closed
@pierrotsmnrd pierrotsmnrd mentioned this pull request Aug 25, 2023
Closed
@costrouc costrouc removed their assignment Aug 30, 2023
@anirrudh anirrudh removed their assignment Sep 18, 2023
@costrouc costrouc mentioned this pull request Oct 31, 2023
Closed
anirrudh and others added 24 commits December 18, 2023 17:37
@anirrudh @nkaretnikov
use proper schema object
8224827
@anirrudh @nkaretnikov
ensure build status is marked as failed
1de3b48
@anirrudh @nkaretnikov
typo fix
740eddf
@anirrudh @nkaretnikov
add time delays to ensure statuses are updated to most recent state
e7b187c
@anirrudh @nkaretnikov
increase timeout
f522779
@anirrudh @nkaretnikov
add while loop to periodically ping the API and see if building
ef5c7d4
@anirrudh @nkaretnikov
modify tests to catch edge cases
36912a0
@anirrudh @nkaretnikov
playing with time, again
14a5568
@anirrudh @nkaretnikov
set timeout to 3 mins
7067180
@nkaretnikov
Use conda_store.get_db instead of dependencies.get_db
a079ff8
@nkaretnikov
Reduce the delay to fix a flaky test
9f21660
@nkaretnikov
Reduce another delay that's likely too long
60cf561
@nkaretnikov
Check the full message text
78bce82
@nkaretnikov
Handle exception when getting a build
52699b3 
```
requests.exceptions.ConnectionError: ('Connection aborted.',
  RemoteDisconnected('Remote end closed connection without response'))
```
@nkaretnikov
Add test_api_cancel_build_unauth
a29efb0
@nkaretnikov
Add an assert
892c056
@nkaretnikov
Formatting fixes
b1d7191
@nkaretnikov
Add raise_for_status
198eb42
@nkaretnikov
Fix the task id
2cb8c7b
@nkaretnikov
Add a comment
f37c92f
@nkaretnikov
Fix a flaky test
68dd0f1
@nkaretnikov
Split a long string
347a9aa
@nkaretnikov
Make the test less flaky
405036b
@nkaretnikov
Add a comment about countdown=5
97b9940
@nkaretnikov nkaretnikov dismissed pierrotsmnrd’s stale review December 18, 2023 17:12

Approved by Tania after that

@nkaretnikov nkaretnikov merged commit f7884a2 into main Dec 18, 2023
18 checks passed
@nkaretnikov nkaretnikov deleted the feat-cancel-build branch December 18, 2023 17:14
@nkaretnikov nkaretnikov mentioned this pull request Dec 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkaretnikov nkaretnikov nkaretnikov left review comments

@anirrudh anirrudh anirrudh left review comments

@pierrotsmnrd pierrotsmnrd pierrotsmnrd left review comments

@trallard trallard trallard approved these changes

@dcmcand dcmcand Awaiting requested review from dcmcand

Assignees

@nkaretnikov nkaretnikov

Labels
area: api area: user experience 👩🏻‍💻 Items impacting the end-user experience
Projects
conda-store 🐍
Status: Done 💪🏾
Milestone
🚀 JATIC - Q1
Development

Successfully merging this pull request may close these issues.

Ability to cancel a build.
6 participants
@costrouc @trallard @anirrudh @nkaretnikov @pierrotsmnrd @pavithraes