Uses namespace labels for default options by krsoninikhil · Pull Request #2626 · containerd/containerd

krsoninikhil · 2018-09-10T18:12:56Z

This is in reference to #2285. I tried to configure default runtime and snapshotter using namespace labels. Please suggest me the right approach if this is not the best way to do it.

Signed-off-by: Nikhil Soni [email protected]

kadisi · 2018-09-11T08:34:08Z

i think , the meaning of issue #2285 maybe not only get specific label from default namespace . we should get the specific label from every namespaces to set defaults for certain items which created in it`s namespace.

cc @stevvooe @crosbymichael PTAL

krsoninikhil · 2018-09-11T16:20:54Z

Thanks @kadisi for taking a look. During client instantiation, we don't have a context and only namespace we can consider, is the one which is passed with ClientOpt, which should be default if nothing is passed. Are there any other possible namespaces during client instantiation?

codecov-io · 2018-09-12T19:20:43Z

Codecov Report

Merging #2626 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2626   +/-   ##
=======================================
  Coverage   43.98%   43.98%           
=======================================
  Files         102      102           
  Lines       10873    10873           
=======================================
  Hits         4782     4782           
  Misses       5358     5358           
  Partials      733      733

Flag	Coverage Δ
#linux	`47.63% <ø> (ø)`	⬆️
#windows	`41.19% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7f24e2...da2ab86. Read the comment docs.

dmcgowan · 2018-09-17T18:34:02Z

+	}
+	namespaces := c.NamespaceService()
+	ctx := context.Background()
+	if labels, err := namespaces.Labels(ctx, defaultns); err == nil {


This should probably only be looked up when a defaultns is provided and no defaultruntime is. The given option should always be preferred and I don't think we should default the ns here.

Also I think NewWithConn should probably have matching logic

krsoninikhil · 2018-09-18T10:53:50Z

Hi @dmcgowan, thanks for reviewing. I've made these changes, can you please again have a look?

krsoninikhil · 2018-09-24T19:24:25Z

Hi @dmcgowan, just wanted to ping you again. Can you please have another look on this?

/cc @crosbymichael

crosbymichael · 2018-09-26T14:49:16Z

Ya, sorry about the delay. We are in the middle of two releases right now that we are trying to get out the door so review times are a little long right now.

stevvooe · 2018-10-02T17:10:02Z

One problem here is that the labels need to be namespaced. This has runtime, but it should be something like containerd.io/defaults/runtime. This will allow different clients to not step on each others feet.

krsoninikhil · 2018-10-02T20:19:12Z

Hey @stevvooe, I have changed the labels to namespaced version. Didn't realize that's why your example labels in issue are namespaced. Can you please take another look?

krsoninikhil · 2018-10-24T17:39:27Z

Hi @stevvooe @crosbymichael, just a ping. Please let me know if this requires any more changes.

ehazlett · 2019-01-29T15:06:22Z

@krsoninikhil sorry for the delay! can you rebase this please? thanks!

Signed-off-by: Nikhil Soni <[email protected]>

options. Implements same approach of setting defaults for `NewWithConn`. Signed-off-by: Nikhil Soni <[email protected]>

krsoninikhil · 2019-01-30T18:12:26Z

Hi @ehazlett, I've rebased it with master, please let me know if any other changes are required.

Signed-off-by: Nikhil Soni <[email protected]>

krsoninikhil · 2019-02-01T18:54:08Z

@AkihiroSuda @ehazlett I've added the documentation also, please have a look if it can be improved.

krsoninikhil · 2019-02-06T18:55:16Z

@AkihiroSuda @ehazlett, just wanted to follow up on this, if any changes are required.

fuweid · 2019-02-11T02:58:58Z

+		namespaceService := client.NamespaceService()
+		if ns, err := namespaces.NamespaceRequired(ctx); err == nil {
+			if labels, err := namespaceService.Labels(ctx, ns); err == nil {
+				if snapshotLabel, ok := labels["containerd.io/defaults/snapshotter"]; ok {


how about the const for the label here?

You mean to use defaults package for the label names? That sounds better, should I move both label names to https://github.com/containerd/containerd/blob/master/defaults/defaults.go?

it is ok to move it to defaults

Labels that are used for configuring defaults are moved to defaults package Signed-off-by: Nikhil Soni <[email protected]>

fuweid

LGTM

krsoninikhil · 2019-03-31T11:15:02Z

Hi @ehazlett, not sure why is this not getting merged. Are there any more changes required?

ehazlett · 2019-04-02T16:36:33Z

LGTM

dmcgowan · 2019-04-02T18:46:26Z

LGTM

krsoninikhil force-pushed the defaults3 branch 2 times, most recently from bbf8134 to a6c0be1 Compare September 10, 2018 19:03

krsoninikhil force-pushed the defaults3 branch from a6c0be1 to 76163bd Compare September 12, 2018 19:02

dmcgowan reviewed Sep 17, 2018

View reviewed changes

krsoninikhil force-pushed the defaults3 branch 2 times, most recently from 377595b to 3ac507a Compare September 18, 2018 10:33

krsoninikhil force-pushed the defaults3 branch from 3ac507a to d6af8d6 Compare October 2, 2018 20:10

krsoninikhil force-pushed the defaults3 branch 2 times, most recently from b6faa62 to 2e858a9 Compare October 3, 2018 07:41

krsoninikhil force-pushed the defaults3 branch 2 times, most recently from ffecdab to d9f9d68 Compare January 29, 2019 21:24

krsoninikhil added 2 commits January 30, 2019 23:21

Take default runtime and snapshotter from namespace labels

59432aa

Signed-off-by: Nikhil Soni <[email protected]>

Use labels only when default namespace is provided and prefer given

3432398

options. Implements same approach of setting defaults for `NewWithConn`. Signed-off-by: Nikhil Soni <[email protected]>

krsoninikhil force-pushed the defaults3 branch from d9f9d68 to 3432398 Compare January 30, 2019 17:54

AkihiroSuda reviewed Jan 31, 2019

View reviewed changes

Comment thread client.go

Add documentation for using namespace labels for configuring defaults.

da2ab86

Signed-off-by: Nikhil Soni <[email protected]>

AkihiroSuda approved these changes Feb 6, 2019

View reviewed changes

fuweid reviewed Feb 11, 2019

View reviewed changes

Use defaults package for listing namespace labels

6a21728

Labels that are used for configuring defaults are moved to defaults package Signed-off-by: Nikhil Soni <[email protected]>

krsoninikhil force-pushed the defaults3 branch from 7e42f0f to 6a21728 Compare February 11, 2019 19:57

fuweid reviewed Feb 12, 2019

View reviewed changes

dmcgowan merged commit 2f60e38 into containerd:master Apr 2, 2019

Conversation

krsoninikhil commented Sep 10, 2018

Uh oh!

kadisi commented Sep 11, 2018

Uh oh!

krsoninikhil commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmcgowan Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

krsoninikhil commented Sep 18, 2018

Uh oh!

krsoninikhil commented Sep 24, 2018

Uh oh!

crosbymichael commented Sep 26, 2018

Uh oh!

stevvooe commented Oct 2, 2018

Uh oh!

krsoninikhil commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krsoninikhil commented Oct 24, 2018

Uh oh!

ehazlett commented Jan 29, 2019

Uh oh!

krsoninikhil commented Jan 30, 2019

Uh oh!

Uh oh!

krsoninikhil commented Feb 1, 2019

Uh oh!

krsoninikhil commented Feb 6, 2019

Uh oh!

fuweid Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

krsoninikhil Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

fuweid Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

krsoninikhil commented Mar 31, 2019

Uh oh!

ehazlett commented Apr 2, 2019

Uh oh!

dmcgowan commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

krsoninikhil commented Sep 11, 2018 •

edited

Loading

codecov-io commented Sep 12, 2018 •

edited

Loading

krsoninikhil commented Oct 2, 2018 •

edited

Loading