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

Rename version to ociVersion according to runtime.md #633

Merged
merged 1 commit into from
Dec 6, 2016

Conversation

WeiZhang555
Copy link
Contributor

@WeiZhang555 WeiZhang555 commented Nov 28, 2016

* `Root` is path to the container's rootfs which can be useful information to hook, and the official sample runtime implementation--runc--has already added this field as payload to hook, we should add this to the spec.
  • Status seems like unnecessary field, because State is payload to
    hook and hook is executed in special time point, e.g.
  1. Prestart is a list of hooks to be run before the container process is
    executed, indicating a status of created.
  2. Poststart is a list of hooks to be run immediately after the container
    process is started, indicating a status of running.
  3. Poststop is a list of hooks to be run after the container process exits,
    indicating a status of stopped.
    So the Status field is redundant.

According to definition of state, we should rename
version field in state.go to ociVersion

Signed-off-by: Zhang Wei zhangwei555@huawei.com

@WeiZhang555 WeiZhang555 changed the title Add root field in State Add Root and remove Status in State Nov 28, 2016
@crosbymichael
Copy link
Member

I think status is included because this state object can be used as the output for runtime state <id>

@WeiZhang555
Copy link
Contributor Author

WeiZhang555 commented Nov 29, 2016

Oh, my... I'm too confused now, it seems that we have many conflicts between specs and runc.

runc containerState: https://github.com/opencontainers/runc/blob/master/list.go#L24-L41
runc hookState: https://github.com/opencontainers/runc/blob/master/libcontainer/configs/config.go#L246-L252

specs description about container state:
https://github.com/opencontainers/runtime-spec/blob/master/runtime.md#state
specs structure about state:
https://github.com/opencontainers/runtime-spec/blob/master/specs-go/state.go

In my understanding, they should be the same, but currently they conflict with each other.

conflicting fields include:

  • "version" vs "ociVersion"
  • missing "root" field in specs, and also "root" vs "rootfs"
  • missing "status" in hooks
  • extra "created" field in runc state
  • "bundlePath" vs "bundle"

I'll try to make them synchronous, but the breaking of backward compatibility seems inevitable 😢

According to definition of [state](runtime.md#State), we should rename
`version` field in `state.go` to `ociVersion`

Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
@WeiZhang555
Copy link
Contributor Author

Update:

only update ociVersion field because state.go didn't follow runtime.md

Others:

  • missing "root" field in specs, and also "root" vs "rootfs": root is redundant, as we already have bundle, rootfs can be retrieved from bundle/config.md
  • missing "status" in hooks: modify runc
  • extra "created" field in runc state: keep it only in runc, not add it in spec. Unless all persons agree that it's truly necessary, but this can be discussed seperately.
  • "bundlePath" vs "bundle": runc should follow the spec

Please take a look. @opencontainers/runc-maintainers @opencontainers/runtime-spec-maintainers

@WeiZhang555 WeiZhang555 changed the title Add Root and remove Status in State Rename version to ociVersion according to runtime.md Nov 29, 2016
@wking
Copy link
Contributor

wking commented Dec 1, 2016 via email

@hqhq
Copy link
Contributor

hqhq commented Dec 6, 2016

LGTM

Approved with PullApprove

1 similar comment
@vbatts
Copy link
Member

vbatts commented Dec 6, 2016

LGTM

Approved with PullApprove

@vbatts vbatts merged commit d12da90 into opencontainers:master Dec 6, 2016
@WeiZhang555 WeiZhang555 deleted the syncup-hook-state branch December 7, 2016 04:02
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 7, 2016
Through d12da90 (Merge pull request opencontainers#633 from
WeiZhang555/syncup-hook-state, 2016-12-06).

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 7, 2016
Through d12da90 (Merge pull request opencontainers#633 from
WeiZhang555/syncup-hook-state, 2016-12-06).

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 7, 2016
Through d12da90 (Merge pull request opencontainers#633 from
WeiZhang555/syncup-hook-state, 2016-12-06).

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 7, 2016
Through d12da90 (Merge pull request opencontainers#633 from
WeiZhang555/syncup-hook-state, 2016-12-06).

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 7, 2016
Through d12da90 (Merge pull request opencontainers#633 from
WeiZhang555/syncup-hook-state, 2016-12-06).

Signed-off-by: W. Trevor King <wking@tremily.us>
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.

5 participants