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

Validations for TaskToCompute.docker_images #822

Open
cameel opened this issue Sep 13, 2018 · 1 comment
Open

Validations for TaskToCompute.docker_images #822

cameel opened this issue Sep 13, 2018 · 1 comment
Labels
feature icebox sync with deployment There is a deployment change that has to be merged first verification use case Additional Verification use case

Comments

@cameel
Copy link
Contributor

cameel commented Sep 13, 2018

Overview

Currently Concent completely ignores TaskToCompute.docker_images. We only support one image right now but we should at least be checking whether the requested image matches the one we have. If it does not, we should refuse service in the additional verification use case because we won't be able to render the result.

We should also more thoroughly validate the format of the field itself.

In the future we will support multiple images in verifier but this is out of scope of this issue.

Format of docker_images

You can see an example in this typical sample of TaskToCompute.

docker_images is a list of references to Docker images stored in the local registry. If multiple images are specified, either one can be used to run the task. The list is in the order of preference - the first available image is used. See how DockerTaskThread in Golem 0.17.1 selects the image.

The entries on the list are converted to DockerImage instances using its build() method. According to that code they can be either:

  • Dicts with fields named repository, image_id, tag. Extra fields are not accepted.
  • Tuples containing no more than three elements
    • If zero elements are present, all values are None
    • If one is present, it's repository, others are None
    • If two are present, first is repository, second is tag, image_id is None
    • If three are present, first is repository, second is image_id, third is tag
  • Instances of DockerImage, which should never be the case when data comes from JSON.

In all cases if tag is None, it's internally converted to "latest".

repository and tag are used only if image_id is not present.

Validations

Our validations for TaskToCompute should ensure that docker_images follows the format described above. If possible, these validations should be added in golem-messages rather than only in Concent.

It's not stated in the class definition but I think all the fields should be required to be either None or strings with some reasonable length limit.

There are also combinations that don't make sense and should not be supported:

  • All fields being None
  • Only tag not being None.

Additional validation use case

In the additional validation use case in addition to checking the structure, we should validate the content. If docker_images does not list any image available in Concent, ServiceRefused should be returned.

The only image supported by Concent right now is golemfactory/blender:1.4 but don't hard-code it. Instead define a new settings called AVAILABLE_VERIFIER_IMAGES that lists available images and compare with that. In deployment we will ensure that this setting contains the image we actually have in verifier.

AVAILABLE_VERIFIER_IMAGES
  • This setting should not have a default in base.py.
  • In development you can hard-code 'golemfactory/blender:1.4' for convenience. This will have to be updated each time we update the image but updates are infrequent and it will cause an easy to spot error anyway.
  • The setting should have the same exact format as docker_images. Add system checks to enforce it.
@cameel cameel added feature not in pivotal Not yet added as a task in any story in Pivotal verification use case Additional Verification use case sync with deployment There is a deployment change that has to be merged first labels Sep 13, 2018
@PaweuB
Copy link

PaweuB commented Sep 14, 2018

@PaweuB PaweuB removed the not in pivotal Not yet added as a task in any story in Pivotal label Sep 14, 2018
@PaweuB PaweuB added the icebox label Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature icebox sync with deployment There is a deployment change that has to be merged first verification use case Additional Verification use case
Projects
None yet
Development

No branches or pull requests

2 participants