worker: add platforms support #461
Conversation
|
Can we use labels? |
solver/pb/ops.proto
Outdated
| @@ -122,7 +133,7 @@ message OpMetadata { | |||
| bool ignore_cache = 1; | |||
| // Description can be used for keeping any text fields that builder doesn't parse | |||
| map<string, string> description = 2; | |||
| WorkerConstraint worker_constraint = 3; | |||
| // WorkerConstraint worker_constraint = 3; | |||
There was a problem hiding this comment.
Is this deliberately left commented out?
There was a problem hiding this comment.
Yes. I'll add a better comment.
api/services/control/control.pb.go
Outdated
| Labels map[string]string `protobuf:"bytes,2,rep,name=Labels" json:"Labels,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` | ||
| ID string `protobuf:"bytes,1,opt,name=ID,proto3" json:"ID,omitempty"` | ||
| Labels map[string]string `protobuf:"bytes,2,rep,name=Labels" json:"Labels,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` | ||
| Platforms []*pb.Platform `protobuf:"bytes,3,rep,name=platforms" json:"platforms,omitempty"` |
There was a problem hiding this comment.
Should probably be []pb.Platform, similar to client's WorkerInfo.
cmd/buildkitd/main_oci_worker.go
Outdated
| if len(platformsStr) != 0 { | ||
| platforms, err := parsePlatforms(platformsStr) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "invalid platforms") |
|
Overall looks good |
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
tonistiigi
force-pushed
the
worker-platform
branch
from
3867b26
to
a245ec5
Compare
| ) | ||
|
|
||
| type Worker interface { | ||
| // ID needs to be unique in the cluster | ||
| ID() string | ||
| Labels() map[string]string | ||
| Platforms() []specs.Platform |
There was a problem hiding this comment.
What should we do with?
38 LabelOS = labelPrefix + "os" // GOOS
39 LabelArch = labelPrefix + "arch" // GOARCH
There was a problem hiding this comment.
Currently, they could be still used for constraints with filters but as they are just GOOS/GOARCH they may not be that useful. We can reevaluate if this is more complete. The Platform field is more like a "target platform" for vertex and less like a value based constraint.
There was a problem hiding this comment.
Maybe better to remove these labels for now?
There was a problem hiding this comment.
LGTM after removal of LabelOS and LabelArch consts
Hmm, I'm pretty sure I left answer for that yesterday but can't see it anymore. @AkihiroSuda Did you see the answer? (maybe in e-mail notification) |
Unfortunately no (I'm not using email notification) |
|
Ok, I'll try again. Maybe I just failed to press send.
First problem is that it should be an array. Containerd supports multiple platforms in this value and BuildKit should do it was well. In practical build cases these would be either emulators or different container compatibility versions for windows. If you look at #462, #463 then LLB now always defines the target platform so there is no ambiguity on LLB possibly meaning two different things based on worker configuration. In LLB, target platform is separated from regular constrains filters because it needs to be used for determining the image to pull/export. This happens using the image-spec platform field so it makes sense to use the same type here as well to make in consistent. Also, the target platform isn't neccessarily a constraint for any vertex, eg. a Dockerfile |
Superceded by platforms array. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
tonistiigi
force-pushed
the
worker-platform
branch
from
40f8708
to
85e9810
Compare
Define supported platforms for workers and automatically detect the defaults.
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com