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

feat: check image checksum #739

Merged
merged 22 commits into from
Feb 8, 2024
Merged

feat: check image checksum #739

merged 22 commits into from
Feb 8, 2024

Conversation

SdgJlbl
Copy link
Contributor

@SdgJlbl SdgJlbl commented Sep 22, 2023 •
edited by ThibaultFy
Loading

Description

Reviewable commit by commit

Companion PR

Fixes FL-1151, FL-1149, FL-1161

TODO

  • check possible impact on frontend
  • Failed builds are not propagating to compute task status (to check in orchestrator)

How has this been tested?

Tested on a local deployment with Titanic (modified to run on 2 orgs)

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@SdgJlbl SdgJlbl requested a review from a team as a code owner September 22, 2023 17:36
oleobal
oleobal previously approved these changes Oct 2, 2023
View reviewed changes
Copy link
Collaborator

@oleobal oleobal left a comment

Choose a reason for hiding this comment

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

I might be lacking in context, so feel free to wait for further input, but this seems good

@SdgJlbl SdgJlbl force-pushed the feat/check-image-checksum branch 2 times, most recently from ee08679 to 85a097d Compare October 2, 2023 10:08
@linear
Copy link

linear bot commented Oct 2, 2023

FL-1161 Check permissions when downloading docker image on a distant backend

Context

cf

substra-backend/backend/api/views/function.py

Line 217 in 78685ff

# TODO refactor the code duplication with api.views.utils.PermissionMixin.download_file

FL-1149 Checking Docker images checksums after download

We bypass the check in

substra-backend/backend/substrapp/clients/organization.py

Line 177 in b0442ba

def get(
because we don't share the checksum with all backends (through a gRPC event)

FL-1151 Add the image to the Function model?

Instead of having a separate table for it 🤔

@SdgJlbl SdgJlbl marked this pull request as draft October 2, 2023 12:28
@thbcmlowk
Copy link
Contributor

Can we not merge the PR until the orchestrator part is done (if not)?

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 12, 2023
@SdgJlbl SdgJlbl force-pushed the feat/check-image-checksum branch 2 times, most recently from 113b1fe to f8bbd29 Compare October 17, 2023 16:11
This was referenced Oct 18, 2023
Merged
Merged
Base automatically changed from poc-decoupled-builder to main October 25, 2023 13:11
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Oct 27, 2023
@SdgJlbl SdgJlbl changed the title Feat: check image checksum feat: check image checksum Oct 27, 2023
@SdgJlbl SdgJlbl marked this pull request as ready for review December 14, 2023 12:46
@ThibaultFy
Copy link
Member

/e2e --tests sdk,substrafl --refs orchestrator=feat/add-function-image,substra-backend=feat/check-image-checksum,substra=feat/rename-function-to-archive,substrafl=feat/rename-function-to-archive

@Owlfred
Copy link

Owlfred commented Dec 21, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

What a surprise... 🙄

@ThibaultFy
Copy link
Member

/e2e --tests sdk,substrafl --refs orchestrator=feat/add-function-image,substra-backend=feat/check-image-checksum,substra=feat/rename-function-to-archive,substrafl=feat/rename-function-to-archive

@Owlfred
Copy link

Owlfred commented Feb 8, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

That was easy.

guilhem-barthes
guilhem-barthes approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

Thanks a lot 🏁

SdgJlbl and others added 22 commits February 8, 2024 16:17
@SdgJlbl @ThibaultFy
chore: update protobuf
71769f3 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
chore: rename function.function to archive
e59b11b 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
feat: send function image checksum to orchestrator
aeca68c 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
chore: restore checksum check for image
32e4b51 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
chore: rename API Function function_ fields to archive_
597dda3 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
chore: add function image to ApiFunction
946f129 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
chore: refactor function image download
c46cc29 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
chore: update protobuf for UpdateFunctionParam
f7bf490 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
fix: update properly APIFunction object with built image
469cba1 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
fix: workaround for updating function image URI
b44426b 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
fix: workaround for function name
0c8b3cd 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
revert fix
481ef6c 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
WIP fix Failure report handling when errors during remote function do…
07bec36 
…wnload

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
fix: rebase migrations
27470c2 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl @ThibaultFy
fix: linter
7f30b8a 
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@ThibaultFy
chore: make proto orc
cd93b41 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
test: rename in tests
0738a25 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: update from thibault
b089e39 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: guilhem review
20dded4 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: changelog
efcd12d 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: use save image task output to update orc function
188d056 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: mypy + comment
502503e 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
ThibaultFy added a commit to Substra/orchestrator that referenced this pull request Feb 8, 2024
@SdgJlbl @ThibaultFy
feat: add function image (#288)
e466541 
## Description
Add the docker image URI + checksum to the orchestrator

Reviewable commit by commit

Companion PR Substra/substra-backend#739

Fixes FL-1149

## How has this been tested?

<!-- Please describe the tests that you ran to verify your changes.  -->

## Checklist

- [ ] [changelog](../CHANGELOG.md) was updated with notable changes
- [ ] documentation was updated

---------

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: SdgJlbl <SdgJlbl@users.noreply.github.com>
Co-authored-by: ThibaultFy <thibault.fouqueray@gmail.com>
Co-authored-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy ThibaultFy merged commit d3fa377 into main Feb 8, 2024
7 checks passed
@ThibaultFy ThibaultFy deleted the feat/check-image-checksum branch February 8, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThibaultFy ThibaultFy ThibaultFy left review comments

@guilhem-barthes guilhem-barthes guilhem-barthes approved these changes

@oleobal oleobal oleobal left review comments

@thbcmlowk thbcmlowk Awaiting requested review from thbcmlowk

Assignees
No one assigned
Labels
api compute-engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SdgJlbl @thbcmlowk @ThibaultFy @Owlfred @oleobal @guilhem-barthes