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

translate: Stub in a translation framework #54

Closed
wants to merge 1 commit into from

Conversation

wking
Copy link
Contributor

@wking wking commented May 3, 2016

Add tooling to translate higher-level configs into the basic OCI config. On IRC, @julz floated a linux.namespaces[].fromContainer as a higher-level version of
linux.namespaces[].path for emulating exec (opencontainers/runtime-spec#391). That makes sense to me, with a UI like:

$ ocitools generate --template=high-level-config.json --translate=fromContainer --runtime=runc

This commit still needs:

I'm comfortable working through the former, but would appreciate pointers on the latter from anyone who is more Go-literate than I.

More generally, I'm curious to know what folks think of this approach. I'd rather make those before grinding through the implementation details ;).

@liangchenye
Copy link
Member

I tried 'reflect' to convert an interface to a struct before. But later I choose json.Marshal/Unmarshal, it feels not good but make the implementation easy and right.

buf, _ := json.Marshal(interfaceA)
json.Unmarshal(buf, &structA)

@liangchenye
Copy link
Member

The purpose is converting between different specs, like between rkt and oci, right?
From the commit, seems we can only convert from OCI to another (fromContainer in this) format.

How about maintain a list like this:

     [{"from": "Linux:namespace:fromContainer",  "to": "Linux:namespace:path"},
      {"from":  "Platform:FakeOS",  "to": "Platform:OS"},
     ]

Otherwise it feels so special to see these certain field names: 'linux', 'namespace', 'path'.

wking added a commit to wking/ocitools-v2 that referenced this pull request May 3, 2016
Add tooling to translate higher-level configs into the basic OCI
config.  On IRC, Julz floated a linux.namespaces[].fromContainer as a
higher-level version of linux.namespaces[].path for emulating exec
[1].  That makes sense to me, and Mrunal is open to something like
[2]:

  $ ocitools generate --template=high-level-config.json --translate=fromContainer --runtime=runc

The interface{} -> rspec.Spec conversion happens through a JSON
serialization trick suggested by 梁辰晔 (Liang Chenye) [3] after the
translations, because before the translations the template may contain
JSON that's not represented in the OCI Spec structure
(e.g. fromContainer in namespace entries).

This commit still needs:

* The state JSON lookup and path logic from [4].
* Adjustments to setupNamespaces to avoid clobbering the template.

[1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-27.log.html#t2016-04-27T20:32:09
[2]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-28.log.html#t2016-04-28T16:12:48
[3]: opencontainers#54 (comment)
[4]: opencontainers/runtime-spec#391

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented May 3, 2016

On Tue, May 03, 2016 at 04:18:46AM -0700, 梁辰晔 (Liang Chenye) wrote:

… But later I choose json.Marshal/Unmarshal…

Going through JSON to setup strictSpec works for me. Done in 215cee9
0463c2a.

Now that that's working, testing turned up setupNamespaces clobbering
the template's namespace entries, so I'll need to figure that out too
(likely by making the clobbering opt-in).

@wking
Copy link
Contributor Author

wking commented May 3, 2016

On Tue, May 03, 2016 at 04:48:19AM -0700, 梁辰晔 (Liang Chenye) wrote:

The purpose is converting between different specs, like between rkt
and oci, right?

It's currently * → OCI. The fromNamespace translator is (OCI +
fromNamespace extension 1 → OCI). But you could make a rkt → OCI
translator using this approach as well.

Otherwise it feels so special to see these certain field names:
'linux', 'namespace', 'path'.

I'm not sure where you're going with your from / to list. Can you
give more detail? And docs for the fromContainer translation will be
good. I'll update ocitools-generate(1) tonight.

wking added a commit to wking/ocitools-v2 that referenced this pull request May 3, 2016
Add tooling to translate higher-level configs into the basic OCI
config.  On IRC, Julz floated a linux.namespaces[].fromContainer as a
higher-level version of linux.namespaces[].path for emulating exec
[1].  That makes sense to me, and Mrunal is open to something like
[2]:

  $ ocitools generate --template=high-level-config.json --translate=fromContainer --runtime=runc

The interface{} -> rspec.Spec conversion happens through a JSON
serialization trick suggested by 梁辰晔 (Liang Chenye) [3] after the
translations, because before the translations the template may contain
JSON that's not represented in the OCI Spec structure
(e.g. fromContainer in namespace entries).

This commit still needs:

* The state JSON lookup and path logic from [4].
* Adjustments to setupNamespaces to avoid clobbering the template.

[1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-27.log.html#t2016-04-27T20:32:09
[2]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-28.log.html#t2016-04-28T16:12:48
[3]: opencontainers#54 (comment)
[4]: opencontainers/runtime-spec#391

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented May 3, 2016 via email

wking added a commit to wking/ocitools-v2 that referenced this pull request May 3, 2016
Add tooling to translate higher-level configs into the basic OCI
config.  On IRC, Julz floated a linux.namespaces[].fromContainer as a
higher-level version of linux.namespaces[].path for emulating exec
[1].  That makes sense to me, and Mrunal is open to something like
[2]:

  $ ocitools generate --template=high-level-config.json --translate=fromContainer --runtime=runc

The interface{} -> rspec.Spec conversion happens through a JSON
serialization trick suggested by 梁辰晔 (Liang Chenye) [3] after the
translations, because before the translations the template may contain
JSON that's not represented in the OCI Spec structure
(e.g. fromContainer in namespace entries).

This commit still needs (marked by FIXMEs):

* Lookup /proc in /proc/self/mounts (recursive?).
* Compare /proc with state JSON namespace [4].
* Adjustments to setupNamespaces to avoid clobbering the template.
* Better handling for user-namespace injection.
* A way to handle nested definition lists in man pages.  I expect we
  should drop go-md2man in favor of a more established man-page format
  (e.g. AsciiDoc).

[1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-27.log.html#t2016-04-27T20:32:09
[2]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-28.log.html#t2016-04-28T16:12:48
[3]: opencontainers#54 (comment)
[4]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/ujtABQoCmgk
     Subject: Linux: Adding the PID namespace inode to state JSON
     Date: Wed, 30 Dec 2015 21:34:57 -0800
     Message-ID: <[email protected]>

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented May 3, 2016

I just pushed c565214dae2098, which:

  • Adds the --runtime config option.
  • Passes *cli.Context to Translate functions (so fromContainer
    can get --runtime, and future translators can get similar
    configuration).
  • Factor out namespaceFromContainer to its own function since
    FromContainer was getting long.
  • Call the runtime to get the state, and lookup the
    appropriate (usually, see below) namespace path in /proc.
  • Document --translate and --runtime in ocitools-generate(1).

Still todo (marked by FIXMEs):

  • Lookup /proc in /proc/self/mounts. I don't like hard-coding
    /proc, but this is a chicken/egg issue, so we may not be able
    to do anything about that.

  • Compare /proc with the state JSON namespace 1. We currently
    just assume that /proc was mounted by a process in the PID
    namespace in which the state JSON's "pid" entry is valid, but
    that's a brittle assumption.

  • Adjust setupNamespaces to avoid clobbering the template.

  • Shift user-namespace injection into setupNamespaces to avoid
    duplicates.

  • Markup for nested definition lists in the man page. I expect
    we should drop go-md2man in favor of a more established
    man-page format (e.g. AsciiDoc).

    Subject: Linux: Adding the PID namespace inode to state JSON
    Date: Wed, 30 Dec 2015 21:34:57 -0800
    Message-ID: [email protected]

@liangchenye
Copy link
Member

@wking I was thinking of having a framework for translation, most of your codes could be reuse if someone want to implement another extension, like rkt to oci. I'll try to find out a case and explain it in code.

@wking
Copy link
Contributor Author

wking commented May 4, 2016

A rkt -> OCI translator should fit cleanly into what I have now. You'd only need to rethink things if you wanted to cover OCI -> rkt, and other translation chains that don't end in OCI.

@wking
Copy link
Contributor Author

wking commented May 7, 2016

On Tue, May 03, 2016 at 03:47:16PM -0700, W. Trevor King wrote:

  • Adjust setupNamespaces to avoid clobbering the template.
  • Shift user-namespace injection into setupNamespaces to avoid
    duplicates.

I've pushed fixes for the namespace-setup issues in #60. I'll rebase
this branch on top of that once it lands.

wking added a commit to wking/ocitools-v2 that referenced this pull request May 13, 2016
Add tooling to translate higher-level configs into the basic OCI
config.  On IRC, Julz floated a linux.namespaces[].fromContainer as a
higher-level version of linux.namespaces[].path for emulating exec
[1].  That makes sense to me, and Mrunal is open to something like
[2]:

  $ ocitools generate --template=high-level-config.json --translate=fromContainer --runtime=runc

The interface{} -> rspec.Spec conversion happens through a JSON
serialization trick suggested by 梁辰晔 (Liang Chenye) [3] after the
translations, because before the translations the template may contain
JSON that's not represented in the OCI Spec structure
(e.g. fromContainer in namespace entries).

This commit still needs (marked by FIXMEs):

* Lookup /proc in /proc/self/mounts (recursive?).
* Compare /proc with state JSON namespace [4].
* A way to handle nested definition lists in man pages.  I expect we
  should drop go-md2man in favor of a more established man-page format
  (e.g. AsciiDoc).

[1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-27.log.html#t2016-04-27T20:32:09
[2]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-28.log.html#t2016-04-28T16:12:48
[3]: opencontainers#54 (comment)
[4]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/ujtABQoCmgk
     Subject: Linux: Adding the PID namespace inode to state JSON
     Date: Wed, 30 Dec 2015 21:34:57 -0800
     Message-ID: <[email protected]>

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented May 13, 2016

On Sat, May 07, 2016 at 12:01:18AM -0700, W. Trevor King wrote:

I've pushed fixes for the namespace-setup issues in #60. I'll
rebase this branch on top of that once it lands.

Rebased with dae2098f7f25b3. Still a few TODOs in the commit
message, but I'm fine punting on everything except docs. Do we have a
way to handle nested definition lists? Should I create a separate
section for available translations (instead of listing them under
--translate)?

Add tooling to translate higher-level configs into the basic OCI
config.  On IRC, Julz floated a linux.namespaces[].fromContainer as a
higher-level version of linux.namespaces[].path for emulating exec
[1].  That makes sense to me, and Mrunal is open to something like
[2]:

  $ ocitools generate --template=high-level-config.json --translate=fromContainer --runtime=runc

The interface{} -> rspec.Spec conversion happens through a JSON
serialization trick suggested by 梁辰晔 (Liang Chenye) [3] after the
translations, because before the translations the template may contain
JSON that's not represented in the OCI Spec structure
(e.g. fromContainer in namespace entries).

The /proc lookup could still be improved:

* Lookup the mount path (currently hard-coded to /proc) in
  /proc/self/mounts.  This would extend support to users who had
  mounted proc at a different location, but it's hard to find
  self/mounts unless you already know where proc is mounted ;).
* Compare /proc with the state JSON's implied PID namespace [4].  This
  would extend support to users in a different PID namespace than the
  one in which the state JSON's 'pid' entry applied.

But those are likely to be corner cases (and until something like [4]
lands, the latter is not possible).  So I'm punting them into the
future, where they can be fixed as we discover methods to fix them.

[1]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-27.log.html#t2016-04-27T20:32:09
[2]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2016-04-28.log.html#t2016-04-28T16:12:48
[3]: opencontainers#54 (comment)
[4]: https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/ujtABQoCmgk
     Subject: Linux: Adding the PID namespace inode to state JSON
     Date: Wed, 30 Dec 2015 21:34:57 -0800
     Message-ID: <[email protected]>

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented May 14, 2016

On Fri, May 13, 2016 at 04:58:56PM -0700, W. Trevor King wrote:

Still a few TODOs in the commit message, but I'm fine punting on
everything except docs.

Moved the “available translations” docs to a subsection in f7f25b3
10fe0e9 and adjusted the commit message to explain why I'm ok punting
on the two proc issues. I think this is ready to land now.

@wking wking changed the title WIP: translate: Stub in a translation framework translate: Stub in a translation framework May 14, 2016
@wking
Copy link
Contributor Author

wking commented May 14, 2016

On Tue, May 03, 2016 at 03:47:16PM -0700, W. Trevor King wrote:

  • Lookup /proc in /proc/self/mounts. I don't like hard-coding
    /proc, but this is a chicken/egg issue, so we may not be able
    to do anything about that.

We may be able to do this with ‘mount -l -t proc’, but mount(8) says
that the listing mode is for backwards compatibility only and suggests
findmnt. findmnt(8) talks explicitly about reading from
/proc/self/mountinfo, and stracing both ‘mount -l -t proc’ and
‘findmnt -t proc’ turns up open("/proc/…") only in the findmnt case.
I haven't dug deeper today to figure out how ‘mount -l …’ is getting
its information, but that may be a possible route if we don't want to
hard-code /proc.

@wking
Copy link
Contributor Author

wking commented May 15, 2016

On Sat, May 14, 2016 at 03:15:43PM -0700, W. Trevor King wrote:

I haven't dug deeper today to figure out how ‘mount -l …’ is getting
its information…

I'm not sure why strace only shows entries like:

open(0x7ff987f091ba, …)

for the ‘mount -l …’ call, but it appears to be using a hard-coded
/proc/self/mountinfo as well:

$ LIBMOUNT_DEBUG=tab mount -l -t proc
31039: libmount: INIT: library debug mask: 0x0022
31039: libmount: INIT: library version: 2.26.0
31039: libmount: INIT: feature: assert
31039: libmount: INIT: feature: debug
31039: libmount: TAB: [0x1097310]: alloc
31039: libmount: TAB: [0x1097310]: mtab parse: #1 read mountinfo
31039: libmount: TAB: [0x1097310]: /proc/self/mountinfo: start parsing [entries=0, filter=not]

So I'm currently back to having no ideas about detecting alternative
proc mountpoints. But I'm still ok with this PR landing with a
hard-coded /proc 1.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 13, 2016

I am okay adding this but will wait for rebase after #131

@wking
Copy link
Contributor Author

wking commented Jul 19, 2016

On Tue, Jul 12, 2016 at 05:12:08PM -0700, Mrunal Patel wrote:

I am okay adding this but will wait for rebase after #131

I think I'm going to need some guidance about how much to change after
#131. One of the changes I made in the first part of
generateCommand was using an interface{} instead of an *rspec.Spec
to make space for the out-of-spec fields like fromContainer 1.
However, post-#131 a lot of the methods are attached to a Generator
type with opaque-data 2. I can:

a. Update Generator to hold an interface{} but that makes manipulating
the final spec structure awkward.
b. Update Generator to hold both an interface{} and an *rspec.Spec,
but that isn't very DRY.
c. I can pull New, NewFromFile, etc. off of Generator, have them
return interface{}s, and then add a NewFromInterface that returns a
Generator.

None of those sound perfect to me. I'm leaning towards (c) at the
moment, but wanted to get feedback from maintainers before wading in.

@Mashimiao
Copy link

I think currently this doesn't make too much sense. I prefer to reject this one.
How do you think about it, @opencontainers/runtime-tools-maintainers ?

@liangchenye
Copy link
Member

@Mashimiao I just quickly reviewed this thread. I did not see the user scenario here. +1 for close this.

@Mashimiao
Copy link

closing it

@Mashimiao Mashimiao closed this Jul 3, 2017
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.

4 participants