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

Update runtime-spec package #140

Merged
merged 1 commit into from
Jul 26, 2016
Merged

Conversation

haiyanmeng
Copy link
Contributor

The PR vendors the latest version of the runtime-spec package, and updates generate.go.

Signed-off-by: Haiyan Meng <hmeng@redhat.com>
@haiyanmeng
Copy link
Contributor Author

@mrunalp , @wking , PTAL.

@wking
Copy link
Contributor

wking commented Jul 21, 2016

On Thu, Jul 21, 2016 at 01:57:50PM -0700, hmeng-19 wrote:

The PR vendors the latest version of the runtime-spec package…

I'd rather only do this on runtime-spec releases; see #108.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 21, 2016

@wking We can start tagging releases to runc releases but I don't see why we can't pull in intermediate code to make progress here.

@wking
Copy link
Contributor

wking commented Jul 21, 2016

On Thu, Jul 21, 2016 at 03:53:05PM -0700, Mrunal Patel wrote:

@wking We can start tagging releases to runc releases but I don't
see why we can't pull in intermediate code to make progress here.

There are two axes for progress:

a. Keeping up with runtime-spec as it evolves, and
b. Improved coverage for a particular runtime-spec release.

Historically, ocitools has focused on (a) and neglected (b). As a
runtime-implementation maintainer in the run-up to runtime-spec 1.0,
I'd like to see more work on (b).

#108 asks for some kind of plan to move in both directions, and floats
per-release/series branches for working on (b) and and ‘master’ for
working on (a). Otherwise, you're testing runtimes against a moving
target, and ccon at least, complains when you ask it to run an
ociVersion it doesn't understand.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 21, 2016

@wking I think 1.0 is good point for improving b. Till then there is going to be some churn in the spec and we'll have to pick up updates to make progress here.

@wking
Copy link
Contributor

wking commented Jul 21, 2016

On Thu, Jul 21, 2016 at 04:20:44PM -0700, Mrunal Patel wrote:

@wking I think 1.0 is good point for improving b. Till then there is
going to be some churn in the spec and we'll have to pick up updates
to make progress here.

That means you'll hit 1.0 without (ever?) having validated a runtime's
ociVersion parsing. I see no reason that we can't start using
whatever approach we have planned for 1.0 now ;).

@mrunalp
Copy link
Contributor

mrunalp commented Jul 21, 2016

We could tag 1.0.0.rc1 and then allow intermediate commits.

@wking
Copy link
Contributor

wking commented Jul 22, 2016

On Thu, Jul 21, 2016 at 04:54:14PM -0700, Mrunal Patel wrote:

We could tag 1.0.0.rc1 and then allow intermediate commits.

The current ocitools validation does not cover runtime-spec 1.0.0-rc1
(e.g. there's nothing about Solaris). So I don't think tag-and-forget
is viable; I think you need branch and extend. Ideally runtime-spec
releases come frequently enough that we don't need to many ocitools
changes that require post-release spec work.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 22, 2016

@wking There is always going to be some lag to support all features in the spec. Question is whether it is worth branching off for 1.0.0.rc1?

@wking
Copy link
Contributor

wking commented Jul 22, 2016

On Fri, Jul 22, 2016 at 09:53:01AM -0700, Mrunal Patel wrote:

Question is whether it is worth branching off for 1.0.0.rc1?

I think it's worth having an ocitools branch/tag that is as complete
as possible on whatever the most recent runtime-spec tag is. What
features are we trying to address that need this bump to an unreleased
runtime-spec? Is it just #112? If we don't have anything specific in
mind, why bump?

@mrunalp
Copy link
Contributor

mrunalp commented Jul 22, 2016

Yes, we can address #112. I think master should keep tracking the master of the runtime-spec so we don't have to wait for a release. I don't mind us creating a branch/tag for 1.0.0.rc1 if that means that we can keep progressing on master.

@wking
Copy link
Contributor

wking commented Jul 22, 2016

On Fri, Jul 22, 2016 at 10:08:58AM -0700, Mrunal Patel wrote:

Yes, we can address #112. I think master should keep tracking the
master of the runtime-spec so we don't have to wait for a release. I
don't mind us creating a branch/tag for 1.0.0.rc1 if that means that
we can keep progressing on master.

I'd recommend a branch, because I don't think the current ocitools
completely covers runtime-spec 1.0.0-rc1. Then new PRs have to decide
“do I require post-1.0.0-rc1 features” to figure out whether they
should target ocitools' 1.0.0-rc1 branch or master. You can merge the
1.0.0-rc1 branch into master as you see fit, and once runtime-spec
cuts a new release we can merge the 1.0.0-rc1 branch's tip, delete the
branch, and create a new branch tracking support for runtime-spec's
next release.

So the 1.0.0-rc1 branch would (as long as it existed) always have the
most complete tests possible for the 1.0.0-rc1 spec, and master would
have that and additional work based on unreleased runtime-spec work.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 22, 2016

I have pushed a branch v1.0.0.rc1.

@wking
Copy link
Contributor

wking commented Jul 22, 2016

On Fri, Jul 22, 2016 at 11:08:25AM -0700, Mrunal Patel wrote:

I have pushed a branch v1.0.0.rc1.

What to do about open PRs targetting master which don't need
post-v1.0.0-rc1 features? (All of them except #112?). I recommend
merging them into v1.0.0.rc1 from the command line when they're ready,
and then merging v1.0.0.rc1 into master to close the PR.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 22, 2016

I don't think we need to push all the open PRs to v1.0.0.rc1. They could go into master. Can you identify any PRs that are needed in that branch? If really needed they could be ported to that branch as cherry picks.

@wking
Copy link
Contributor

wking commented Jul 22, 2016

On Fri, Jul 22, 2016 at 11:38:43AM -0700, Mrunal Patel wrote:

I don't think we need to push all the open PRs to v1.0.0.rc1. They
could go into master. Can you identify any PRs that are needed in
that branch?

I'd like the v1.0.0-rc1 branch to validate cgroups (#93) and mount
order (#88) if those get to a landable state before runtime-spec cuts
its next release. And if we leave the pointer-receiver change 1 out
of v1.0.0-rc1 we'd be setting ourselves up for merge conflicts as work
continues on the branches in parallel. I think the safest approach is
to have as little as possible in master that's not also in the
v1.0.0-rc1 branch.

If really needed they could be ported to that branch as cherry picks.

If you'd rather push the big green button to land them in master, we
can also file additional PRs to land them in v1.0.0-rc1. That would be:

o---o----a---o master
| / -+--o feature
\ ---b---o v1.0.0-rc1

With (a) being the initial PR and (b) being the v1.0.0-rc1 follow-up
PR. That covers all the current PRs, but it's not going to work if
you have a new PR targeting master that builds on top of
post-1.0.0-rc1 branch-off master work. So I recommend all new PRs
build off and target v1.0.0-rc1 unless they need a post-1.0.0-rc1
feature.

And to give my preferred merge-to-v1.0.0-rc1-first approach a graph,
it would be:

    o   feature-1
   / \

o---o---a---c master
/
o---b v1.0.0-rc1/
`---o feature-2

where feature-1 is post-v.1.0.0-rc1-specific and lands in master in
(a), feature-2 is v1.0.0-rc1-compliant and lands in v1.0.0-rc1 in (b),
and (c) pulls recent v1.0.0-rc1 updates into master.

@haiyanmeng
Copy link
Contributor Author

@wking , @mrunalp , lots of discussions are going on this PR.
Is there anything I can do to speed up this PR?

@wking
Copy link
Contributor

wking commented Jul 25, 2016

On Mon, Jul 25, 2016 at 01:35:08PM -0700, hmeng-19 wrote:

Is there anything I can do to speed up this PR?

There's a v1.0.0-rc1 branch now 1, and regardless of what we do with
the other open PRs 2, this one certainly belongs in master. So I'm
fine with the Godeps bump, although I'd like to see #112 land in some
form instead of your single & injection.

@mrunalp
Copy link
Contributor

mrunalp commented Jul 26, 2016

LGTM

@wking
Copy link
Contributor

wking commented Jul 30, 2016

In case it helps, I've setup an alternate history where master used the “1.0.0-rc1-compatible changes are based on the v1.0.0.rc1 branch” workflow. It makes it easy to see what's unique to post-1.0.0-rc1:

$ git log --graph --oneline --decorate v1.0.0.rc1-full..master-2
* 8ba4c45 (HEAD, wking/master-2, master-2) Merge branch 'v1.0.0.rc1' into master
* 769a064 Merge branch 'v1.0.0.rc1' into master
* 3598f61 Merge branch 'v1.0.0.rc1' into master
* c43c0ee Merge branch 'v1.0.0.rc1' into master
* 6d40d14 Merge branch 'v1.0.0.rc1' into master
* f0ecc98 Merge pull request #140 from hmeng-19/update_vendor
* 6e7b418 (origin/pr/140) Update runtime-spec package

So just this PR at the moment.

master-2 maintenance is mechanical merging except for:

Comparing with our actual master:

$ git log --graph --oneline --decorate v1.0.0.rc1-full..master
*   032bc62 (origin/master, master) Merge pull request #170 from caniszczyk/add-travisci-badge
|\  
| * d0ad8ca (origin/pr/170) Add Travis CI badge to README
|/  
*   8738f2f Merge pull request #168 from Mashimiao/generate-remove-unnecessary-init-spec
|\  
| * 1371b52 (origin/pr/168) generate: remove unnecessary spec initialization
* |   083fb79 Merge pull request #157 from Mashimiao/generate-fix-cap_last_cap
|\ \  
| * | f7df9c7 (origin/pr/157) generate: fix capability.List() for cap_last_cap not exist
| |/  
* |   7db5d52 Merge pull request #158 from mrunalp/remove_else
|\ \  
| |/  
|/|   
| * 198ffb6 (origin/pr/158) Remove unnecessary else
* | 008f8f7 Merge pull request #154 from Mashimiao/man-add-tty-and-tfix
|/  
*   3c2125c Merge pull request #155 from Mashimiao/generate-fix-tmpfs-adding
|\  
| * 8396ff4 (origin/pr/155) generate: fix tmpfs adding based on manpage
* |   bc8aadb Merge pull request #138 from hmeng-19/fix_privileged_caplist
|\ \  
| |/  
|/|   
| * bf3f0d4 (origin/pr/138) Check CAP_LAST_CAP while setting privileged
|/  
*   3334d03 Merge pull request #144 from hmeng-19/init_spec_fields
|\  
| * 60df8fc (origin/pr/144) Check pointer fields of g.spec
|/  
* f0ecc98 Merge pull request #140 from hmeng-19/update_vendor
* 6e7b418 (origin/pr/140) Update runtime-spec package

If you use --first-parent that becomes more manageable:

$ git log --graph --oneline --decorate --first-parent v1.0.0.rc1-full..master
* 032bc62 (origin/master, master) Merge pull request #170 from caniszczyk/add-travisci-badge
* 8738f2f Merge pull request #168 from Mashimiao/generate-remove-unnecessary-init-spec
* 083fb79 Merge pull request #157 from Mashimiao/generate-fix-cap_last_cap
* 7db5d52 Merge pull request #158 from mrunalp/remove_else
* 008f8f7 Merge pull request #154 from Mashimiao/man-add-tty-and-tfix
* 3c2125c Merge pull request #155 from Mashimiao/generate-fix-tmpfs-adding
* bc8aadb Merge pull request #138 from hmeng-19/fix_privileged_caplist
* 3334d03 Merge pull request #144 from hmeng-19/init_spec_fields
* f0ecc98 Merge pull request #140 from hmeng-19/update_vendor

but the current master approach still requires maintainer activity to merge per-feature, vs. the master-2 approach which allows commits like 769a064 that bring in a bunch of v1.0.0-rc1-compatible changes in a single go.

For folks who want a full graph of both master and v1.0.0.rc1 tips with both approaches, here's the master-2 approach since the commit on which this PR is based:

$ git log --graph --topo-order --oneline --decorate origin/pr/140^^..master-2
*   8ba4c45 (HEAD, wking/master-2, master-2) Merge branch 'v1.0.0.rc1' into master
|\  
| *   af72292 (v1.0.0.rc1-full) Merge remote-tracking branch 'origin/pr/162' into v1.0.0.rc1-full
| |\  
| | * 562e912 (wking/man-add-tty-and-tfix, origin/pr/162, origin/pr/154, man-add-tty-and-tfix) man: add tty option and tfix
* | |   769a064 Merge branch 'v1.0.0.rc1' into master
|\ \ \  
| |/ /  
| * |   c186f4f (wking/v1.0.0.rc1, origin/v1.0.0.rc1, v1.0.0.rc1) Merge pull request #173 from wking/add-travis-ci-badge
| |\ \  
| | * | 11f1b12 (origin/pr/173) Add Travis CI badge to README
| * | |   8f9f38f Merge pull request #172 from wking/generate-remove-unnecessary-init-spec
| |\ \ \  
| | * | | 928bf89 (origin/pr/172) generate: remove unnecessary spec initialization
| | |/ /  
| * | |   3a58f65 Merge pull request #171 from wking/generate-fix-cap_last_cap
| |\ \ \  
| | |/ /  
| |/| |   
| | * | e5bcb77 (origin/pr/171) generate: fix capability.List() for cap_last_cap not exist
| |/ /  
| * |   28ddd80 Merge pull request #164 from wking/privileged-cap-list
| |\ \  
| | * | 5295830 (origin/pr/164) Check CAP_LAST_CAP while setting privileged
| * | |   5067a2e Merge pull request #165 from wking/generate-fix-tmpfs-adding
| |\ \ \  
| | * | | 3968a06 (origin/pr/165) generate: fix tmpfs adding based on manpage
| | |/ /  
* | | |   3598f61 Merge branch 'v1.0.0.rc1' into master
|\ \ \ \  
| |/ / /  
| * | |   30e2ea2 Merge pull request #163 from wking/check-pointer-fields
| |\ \ \  
| | * | | bbe46e2 (origin/pr/163) Check pointer fields of g.spec
| |/ / /  
* | | |   c43c0ee Merge branch 'v1.0.0.rc1' into master
|\ \ \ \  
| |/ / /  
| * | |   6acca9e Merge pull request #153 from wking/v1.0.0.rc1-godep
| |\ \ \  
| | |/ /  
| |/| |   
| | * | 1862ec6 (origin/pr/153) Godeps: Roll runtime-spec back to v1.0.0-rc1
| |/ /  
* | |   6d40d14 Merge branch 'v1.0.0.rc1' into master
|\ \ \  
| |/ /  
| * |   71fd99c Merge pull request #151 from wking/move-default-cap-to-validate
| |\ \  
| * \ \   0d42f0c Merge pull request #150 from wking/pointer-receivers
| |\ \ \  
| * \ \ \   ee4d82c Merge pull request #149 from wking/generate-output-remove-unuseful-default-value
| |\ \ \ \  
| * \ \ \ \   d07fade Merge pull request #148 from wking/makefile-clean-man-page
| |\ \ \ \ \  
* | \ \ \ \ \   f0ecc98 Merge pull request #140 from hmeng-19/update_vendor
|\ \ \ \ \ \ \  
| |_|_|_|_|_|/  
|/| | | | | |   
| * | | | | | 6e7b418 (origin/pr/140) Update runtime-spec package
* | | | | | |   564b30e Merge pull request #147 from Mashimiao/move-default-cap-to-validate
|\ \ \ \ \ \ \  
| | |_|_|_|_|/  
| |/| | | | |   
| * | | | | | 5f07d9d (origin/pr/151, origin/pr/147) move defaultCaps to validate
| | |/ / / /  
| |/| | | |   
* | | | | |   2928edb Merge pull request #146 from Mashimiao/generate-output-remove-unusefule-default-value
|\ \ \ \ \ \  
| | |_|_|/ /  
| |/| | | |   
| * | | | | fb5f605 (origin/pr/149, origin/pr/146) generate: remove unuseful default value for output
| |/ / / /  
* | | | |   307be40 Merge pull request #143 from hmeng-19/pointer_methods
|\ \ \ \ \  
| | |_|_|/  
| |/| | |   
| * | | | 35ab0f1 (origin/pr/150, origin/pr/143) Use Generator pointers as methods receivers
| |/ / /  
* | | |   8fd80d4 Merge pull request #145 from Mashimiao/makefile-clean-man-page
|\ \ \ \  
| |/ / /  
|/| | /   
| | |/    
| |/|     
| * | c2c3fed (origin/pr/148, origin/pr/145) Makefile: clean up compiled man page
|/ /  
* |   55645ff Merge pull request #141 from wking/test-runtime-no-generate-pushd
|\ \  
| * | 447d089 (origin/pr/141) test_runtime.sh: Drop pushd/popd from around 'generate'
|/ /  
* |   cc7b4cb (wking/master) Merge pull request #139 from hmeng-19/remove_extra_else
|\ \  
| |/  
|/|   
| * 4a8e72d (origin/pr/139) remove extra else
|/  
* 3c4fc86 Merge pull request #137 from hmeng-19/expand_libgen
* c92ad7a (origin/pr/137) Add resource set funcs into generate library

And the graph of all the commits since this PR's base in both master and v1.0.0.rc1-full (the current v1.0.0.rc1 + #162) is:

$ git log --graph --topo-order --oneline --decorate origin/pr/140^^..master origin/pr/140^^..v1.0.0.rc1-full
*   af72292 (v1.0.0.rc1-full) Merge remote-tracking branch 'origin/pr/162' into v1.0.0.rc1-full
|\  
* \   c186f4f (wking/v1.0.0.rc1, origin/v1.0.0.rc1, v1.0.0.rc1) Merge pull request #173 from wking/add-travis-ci-badge
|\ \  
| * | 11f1b12 (origin/pr/173) Add Travis CI badge to README
* | |   8f9f38f Merge pull request #172 from wking/generate-remove-unnecessary-init-spec
|\ \ \  
| * | | 928bf89 (origin/pr/172) generate: remove unnecessary spec initialization
| |/ /  
* | |   3a58f65 Merge pull request #171 from wking/generate-fix-cap_last_cap
|\ \ \  
| |/ /  
|/| |   
| * | e5bcb77 (origin/pr/171) generate: fix capability.List() for cap_last_cap not exist
|/ /  
* |   28ddd80 Merge pull request #164 from wking/privileged-cap-list
|\ \  
| * | 5295830 (origin/pr/164) Check CAP_LAST_CAP while setting privileged
* | |   5067a2e Merge pull request #165 from wking/generate-fix-tmpfs-adding
|\ \ \  
| * | | 3968a06 (origin/pr/165) generate: fix tmpfs adding based on manpage
| |/ /  
* | |   30e2ea2 Merge pull request #163 from wking/check-pointer-fields
|\ \ \  
| * | | bbe46e2 (origin/pr/163) Check pointer fields of g.spec
|/ / /  
* | |   6acca9e Merge pull request #153 from wking/v1.0.0.rc1-godep
|\ \ \  
| |/ /  
|/| |   
| * | 1862ec6 (origin/pr/153) Godeps: Roll runtime-spec back to v1.0.0-rc1
|/ /  
* |   71fd99c Merge pull request #151 from wking/move-default-cap-to-validate
|\ \  
* \ \   0d42f0c Merge pull request #150 from wking/pointer-receivers
|\ \ \  
* \ \ \   ee4d82c Merge pull request #149 from wking/generate-output-remove-unuseful-default-value
|\ \ \ \  
* \ \ \ \   d07fade Merge pull request #148 from wking/makefile-clean-man-page
|\ \ \ \ \  
| | | | | | *   032bc62 (origin/master, master) Merge pull request #170 from caniszczyk/add-travisci-badge
| | | | | | |\  
| | | | | | | * d0ad8ca (origin/pr/170) Add Travis CI badge to README
| | | | | | |/  
| | | | | | *   8738f2f Merge pull request #168 from Mashimiao/generate-remove-unnecessary-init-spec
| | | | | | |\  
| | | | | | | * 1371b52 (origin/pr/168) generate: remove unnecessary spec initialization
| | | | | | * |   083fb79 Merge pull request #157 from Mashimiao/generate-fix-cap_last_cap
| | | | | | |\ \  
| | | | | | | * | f7df9c7 (origin/pr/157) generate: fix capability.List() for cap_last_cap not exist
| | | | | | | |/  
| | | | | | * |   7db5d52 Merge pull request #158 from mrunalp/remove_else
| | | | | | |\ \  
| | | | | | | |/  
| | | | | | |/|   
| | | | | | | * 198ffb6 (origin/pr/158) Remove unnecessary else
| | | | | | * |   008f8f7 Merge pull request #154 from Mashimiao/man-add-tty-and-tfix
| | | | | | |\ \  
| | | | | | |/ /  
| | | | | |/| /   
| | | | | | |/    
| | | | | * | 562e912 (wking/man-add-tty-and-tfix, origin/pr/162, origin/pr/154, man-add-tty-and-tfix) man: add tty option and tfix
| | | | | | *   3c2125c Merge pull request #155 from Mashimiao/generate-fix-tmpfs-adding
| | | | | | |\  
| | | | | | | * 8396ff4 (origin/pr/155) generate: fix tmpfs adding based on manpage
| | | | | | * |   bc8aadb Merge pull request #138 from hmeng-19/fix_privileged_caplist
| | | | | | |\ \  
| | | | | | | |/  
| | | | | | |/|   
| | | | | | | * bf3f0d4 (origin/pr/138) Check CAP_LAST_CAP while setting privileged
| | | | | | |/  
| | | | | | *   3334d03 Merge pull request #144 from hmeng-19/init_spec_fields
| | | | | | |\  
| | | | | | | * 60df8fc (origin/pr/144) Check pointer fields of g.spec
| | | | | | |/  
| | | | | | *   f0ecc98 Merge pull request #140 from hmeng-19/update_vendor
| | | | | | |\  
| | | | | |/ /  
| | | | | | * 6e7b418 (origin/pr/140) Update runtime-spec package
| | | | | * |   564b30e Merge pull request #147 from Mashimiao/move-default-cap-to-validate
| | | | | |\ \  
| | | | | |/ /  
| | | | |/| |   
| | | | * | | 5f07d9d (origin/pr/151, origin/pr/147) move defaultCaps to validate
| |_|_|/ / /  
|/| | | | |   
| | | | * |   2928edb Merge pull request #146 from Mashimiao/generate-output-remove-unusefule-default-value
| | | | |\ \  
| | | |_|/ /  
| | |/| | |   
| | * | | | fb5f605 (origin/pr/149, origin/pr/146) generate: remove unuseful default value for output
| |/ / / /  
|/| | | |   
| | | * |   307be40 Merge pull request #143 from hmeng-19/pointer_methods
| | | |\ \  
| | | |/ /  
| | |/| |   
| | * | | 35ab0f1 (origin/pr/150, origin/pr/143) Use Generator pointers as methods receivers
| |/ / /  
|/| | |   
| | * |   8fd80d4 Merge pull request #145 from Mashimiao/makefile-clean-man-page
| | |\ \  
| |/ / /  
|/| / /   
| |/ /    
| * | c2c3fed (origin/pr/148, origin/pr/145) Makefile: clean up compiled man page
|/ /  
* |   55645ff Merge pull request #141 from wking/test-runtime-no-generate-pushd
|\ \  
| * | 447d089 (origin/pr/141) test_runtime.sh: Drop pushd/popd from around 'generate'
|/ /  
* |   cc7b4cb (wking/master) Merge pull request #139 from hmeng-19/remove_extra_else
|\ \  
| |/  
|/|   
| * 4a8e72d (origin/pr/139) remove extra else
|/  
* 3c4fc86 Merge pull request #137 from hmeng-19/expand_libgen
* c92ad7a (origin/pr/137) Add resource set funcs into generate library

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.

3 participants