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

proto: initial protobuf for config #185

Closed
wants to merge 8 commits into from

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Sep 14, 2015

To have a less source based, and more consumable example of structures,
this is an initial pass at protobuf structures for the structures from all the current golang source.

There is some exercise needed in platform specific structures.

The Makefile defaults to golang source output, but also has a c target for C source and py target for python as well.

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

@laijs
Copy link
Contributor

laijs commented Sep 15, 2015

Don't put a refactor patch which refactors another patch inside the same pr.

@vbatts
Copy link
Member Author

vbatts commented Sep 15, 2015

@laijs eh?

@laijs
Copy link
Contributor

laijs commented Sep 15, 2015

I found you pr has a style-related patch and an indent patch, they are applied for an earlier patch in the same pr, it is bad, the earlier patch should be updated directly to resolve these problem.

@vbatts
Copy link
Member Author

vbatts commented Sep 15, 2015

Oh I see, I'll flatten the indentation fixes. Not sure why that wasn't
visible earlier. The style change was more for commentary.
As this PR is not against master but for conversation on the whether this
approach with protobuf is sufficient.
On Sep 14, 2015 22:37, "Lai Jiangshan" notifications@github.com wrote:

I found you pr has a style-related patch and an indent patch, they are
applied for an earlier patch in the same pr, it is bad, the earlier patch
should be updated directly to resolve these problem.


Reply to this email directly or view it on GitHub
#185 (comment).

To have a less source based, and more consumable example of structures,
this is an initial pass at protobuf structures for the structures in the
specs golang source.

There is some exercise needed in platform specific structures.

For the sake of example, the Makefile defaults to outputing golang
source, but has a 'c' target (`make c`) for C source, a `make py` and
`make all` target.

This User structure does not map to the cleanliness of the current go
structure, but will allow the definition to be all in one place,
regardless of the host that is doing the compilation.

All field rules to `optional` for now. Per vish and the docs,
https://developers.google.com/protocol-buffers/docs/proto?csw=1#specifying-field-rules
"! Required Is Forever"

There are a couple of concessions, but is pretty much lined up.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
Let the user decide the value.

opencontainers#179 (comment)

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
per opencontainers#179 (comment)

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts
Copy link
Member Author

vbatts commented Sep 15, 2015

@laijs I squashed some and moved the indentation fixes to the initial commit

@philips
Copy link
Contributor

philips commented Sep 24, 2015

The idea is that we use protobuf 3 JSON encodings?

Update: this is what I mean by protobuf encoding: https://developers.google.com/protocol-buffers/docs/proto3?hl=en#json

@vbatts
Copy link
Member Author

vbatts commented Sep 24, 2015

https://github.com/google/protobuf/releases/tag/v3.0.0-beta-1 beta was only 28 days ago...

@vbatts
Copy link
Member Author

vbatts commented Sep 24, 2015

@philips here is the output with this most recent push:

vbatts@valse ~/src/opencontainers/specs/proto (protobuf) $ make 
protoc --go_out=./go config.proto
protoc --go_out=./go runtime_config.proto
go run ./example.go
{
  "spec": {
    "platform": {
      "os": "linux",
      "arch": "x86_64"
    },
    "process": {
      "env": [
        "TERM=linux"
      ],
      "cwd": "/"
    }
  }
}

@mrunalp
Copy link
Contributor

mrunalp commented Sep 24, 2015

@vbatts The json looks good 👍

Now the default make target shows json output from the example.go
source.

Consolidated the protobuf files due to a cyclic import issue.

Cleaned up outputs to source respective outputs directories.

Added a `cpp` target.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@wking
Copy link
Contributor

wking commented Sep 26, 2015

On Thu, Sep 24, 2015 at 10:21:17AM -0700, Brandon Philips wrote:

The idea is that we use protobuf 3 JSON encodings?

I've pushed some work along this lines to vbatts#2, which turns
up some problems with Any support in Go.

I've also pushed some proto2 work fleshing out Process to
vbatts#1 which turns up some problems with the proto2 JSON
serialization.

I'll leave it there for now, but I'm happy to help push this forward
in either direction if folks see a way around those issues.

@wking
Copy link
Contributor

wking commented Sep 27, 2015

On Sat, Sep 26, 2015 at 01:12:07PM -0700, W. Trevor King wrote:

I've pushed some work along this lines to vbatts#2, which
turns up some problems with Any support in Go.

I've also pushed some proto2 work fleshing out Process to
vbatts#1 which turns up some problems with the proto2 JSON
serialization.

I'll leave it there for now, but I'm happy to help push this forward
in either direction if folks see a way around those issues.

One way around the Any+Go issue is to use C++ instead of Go ;), so
I've gone that way with vbatts#3. Differences vs. the current
Markdown-defined spec are the process.user @type (used by Any) 1 and
the runtime.json differences I talk through in
wking/opencontainer-runtime-spec@5f422ee6ae.

@vbatts
Copy link
Member Author

vbatts commented Sep 28, 2015

one approach that could work, regardless of protobuf, but could facilitate this kind of multi-platform accommodation, would be to have the canonical configuration be config.json and platform specific be held in config-$keyword.json. That keyworded filename could be a referenced from the config.json, such that tooling could implement repeatable patterns, but something like the spec would not need to define up front config-{linux,windows,etc}.json.

@conqerAtapple
Copy link

How about concept of overlaid configurations?

-jojy

On Sep 28, 2015, at 7:51 AM, Vincent Batts notifications@github.com wrote:

one approach that could work, regardless of protobuf, but could facilitate this kind of multi-platform accommodation, would be to have the canonical configuration be config.json and platform specific be held in config-$keyword.json. That keyworded filename could be a referenced from the config.json, such that tooling could implement repeatable patterns, but something like the spec would not need to define up front config-{linux,windows,etc}.json.


Reply to this email directly or view it on GitHub.

@wking
Copy link
Contributor

wking commented Sep 28, 2015

On Mon, Sep 28, 2015 at 08:30:23AM -0700, Jojy George Varghese wrote:

How about concept of overlaid configurations?

Previous discussion along these lines in #73, #74, and #76. My
current feeling on this front is that this spec shouldn't talk about
that sort of thing, which we can always punt to an external
preprocessor 1.

@wking
Copy link
Contributor

wking commented Sep 28, 2015

On Mon, Sep 28, 2015 at 07:51:04AM -0700, Vincent Batts wrote:

one approach that could work, regardless of protobuf, but could
facilitate this kind of multi-platform accommodation, would be to
have the canonical configuration be config.json and platform
specific be held in config-$keyword.json

I don't think that works for multi-platform bundles. For example,
process isn't in the platform-specific Linux section, but process.user
is Linux specific. If you process (platform-specific user, likely a
platform specific command), platform, root (likely pointing at a
platform-specific directory), and mounts (likely pointing at
platform-specific target paths) into the platform-specific section,
the only things left in config.json are version and hostname. That's
not much of a win for sharing a cross-platform config ;).

If you're just suggesting it as a way to work around the platform.user
typing, I think protobuf already gives us that.

@conqerAtapple
Copy link

Thanks for the history Trevor. If there has to be a way to "include" other configs to form a configuration "union", shouldnt there be a spec so that its uniform and unambiguous?

-jojy

On Sep 28, 2015, at 9:15 AM, W. Trevor King notifications@github.com wrote:

On Mon, Sep 28, 2015 at 08:30:23AM -0700, Jojy George Varghese wrote:

How about concept of overlaid configurations?

Previous discussion along these lines in #73, #74, and #76. My
current feeling on this front is that this spec shouldn't talk about
that sort of thing, which we can always punt to an external
preprocessor 1.


Reply to this email directly or view it on GitHub.

@wking
Copy link
Contributor

wking commented Sep 28, 2015

On Mon, Sep 28, 2015 at 09:48:43AM -0700, Jojy George Varghese wrote:

If there has to be a way to "include" other configs to form a
configuration "union" shouldnt there be a spec so that its uniform
and unambiguous?

I think this is getting pretty off-topic for “what do we think about
this protobuf approach” ;). Maybe move this config-generation
discussion to #73 or a mailing list thread?

@vbatts
Copy link
Member Author

vbatts commented Sep 28, 2015

@conqer I tried this, but when importing, it added to the golang source an import ".", so the resulting source was a cyclic dependency. Would need a more sophisticated structure. Also, would still have the XXX_* extension issue for marshalling structures.

@bufdev
Copy link

bufdev commented Oct 24, 2015

Just as an interested observer, I'd highly recommend going with proto3 syntax, the beta was only 28 days ago but it's pretty highly stable and used in production (and gRPC is really good).

Some things that might help:

go.pedge.io/google-protobuf - takes care of $(which protoc)/../include/google/protobuf files
go.pedge.io/googleapis - takes care of https://github.com/google/googleapis
go get go.pedge.io/tools/protoc-all (https://github.com/peter-edge/go-tools/tree/master/protoc-all) - I'm going to redo this code in a bit, but this is basically how I do any compilation of protos at this point, both for work and personal projects. Takes care of all import paths (I just wrote go.pedge.io/googleapis so I'll add that in soon), and grpc, grpc-gateway, and my own personal go.pedge.io/protolog.

Let me know if I can be of help.

* a more field out config structure
* display using the jsonpb library
* display the binary proto message

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts
Copy link
Member Author

vbatts commented Dec 11, 2015

@peter-edge thanks for the pointers.

@bufdev
Copy link

bufdev commented Dec 11, 2015

I did this recently too: https://github.com/peter-edge/go-protoeasy

@vbatts
Copy link
Member Author

vbatts commented Dec 14, 2015

Further updates on this protobuf. One thing that I am not entirely certain about, is the ability to have variation in the fields of a message.

For example, if there is the ability to introduce new fields in a message type, such that did not conflict with prior versions of the message type. Would decoders of the prior version of the message type decode only the fields known it its version?

Hopefully that question is clear enough.

@bufdev
Copy link

bufdev commented Dec 14, 2015

Yes, that's basically half the point of protobuf, and why a lot of people use it :) Let me see if I can find some docs to explain better.

@bufdev
Copy link

bufdev commented Dec 14, 2015

Actually https://developers.google.com/protocol-buffers/docs/overview explains it a little at the bottom in the "A bit of history" section. Most protocol buffer language implementations have the notion of unrecognized fields, that is fields that given the definition of a message it has, do not have matching tag numbers.

When you edit a protocol buffer message, you only ever add fields, and if you do want to delete fields, the general pattern is you always use an increasing field number for new fields, to avoid ever reusing old field numbers.

@bufdev
Copy link

bufdev commented Dec 14, 2015

Two other things:

  1. I would highly recommend moving to proto3 instead of proto2, especially since you are using golang. Oh I just saw I already mentioned this.
  2. The makefile could be reduced to protoeasy --cpp --python --go --go-import-path github.com/opencontainers/specs/proto proto, other than the C files (need to check that out) https://go.pedge.io/protoeasy

@vbatts
Copy link
Member Author

vbatts commented Dec 16, 2015

I'm increasingly of the mind that going the route of protobuf is too much a hammer-looking-for-a-nail. I think this needs a step back, to solve the actual problem of implementation independent reference and schema.

@mrunalp
Copy link
Contributor

mrunalp commented Dec 16, 2015

I agree.

Sent from my iPhone

On Dec 16, 2015, at 8:53 AM, Vincent Batts notifications@github.com wrote:

I'm increasingly of the mind that going the route of protobuf is too much a hammer-looking-for-a-nail. I think this needs a step back, to solve the actual problem of implementation independent reference and schema.


Reply to this email directly or view it on GitHub.

@duglin
Copy link
Contributor

duglin commented Dec 16, 2015

I tend to agree. Going away from other, simple, choices like json, yml (or heck even xml) feels really odd to me.

@bufdev
Copy link

bufdev commented Dec 16, 2015

Just the minority opinion here:

Protobufs make dev life way nicer once you figure it out. Really the auto-generation of what amount to DTO objects is enough for a huge win - once you start using them, a lot of code that would otherwise not be easy to document or would be inconsistent is suddenly just generated. Compiling etc is very easy once you understand it.

@LK4D4
Copy link
Contributor

LK4D4 commented Dec 16, 2015

@peter-edge I agree that code-generation is a huge plus. I tried myself and schema much more convenient than json, apart from human-readability of the ready spec itself.

@bufdev
Copy link

bufdev commented Dec 16, 2015

Protobufs have a lot of other wins too:

@bufdev
Copy link

bufdev commented Dec 16, 2015

Another instructive example: here's the docker volume API as a protobuf service, generated to grpc and grpc-gateway golang code: https://github.com/peter-edge/go-dockervolume/blob/master/dockervolume.proto

@wking wking mentioned this pull request Jan 13, 2016
@vbatts
Copy link
Member Author

vbatts commented Jan 13, 2016

Closing this. The specification of the configuration file is a json schema question. Even #276 moves in this direction, by having the golang source being a reference to achieve the desired structures.
We have discussed and investigated json-schema, and perhaps may have future developments related to that. But the protobuf investigation was using protobuf primarily for code generation, and this is not a guarantee that can be entirely relied on.

See also #306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants