Skip to content

leases: support resource management#3304

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
fuweid:me-update-lease
Jun 3, 2019
Merged

leases: support resource management#3304
crosbymichael merged 1 commit intocontainerd:masterfrom
fuweid:me-update-lease

Conversation

@fuweid
Copy link
Copy Markdown
Member

@fuweid fuweid commented May 26, 2019

Add three methods for lease service so that the client can use it to
manage the resource by lease, not just gc.root label. With the following
methods, it is easy for client to maintain their own cache system.

 - AddResource(context.Context, Lease, Resource) error
 - RemoveResource(context.Context, Lease, Resource) error
 - ListResources(context.Context, Lease) ([]Resource, error)

And the resource is to be

type Resource {
  ID   string
  Type string
}

For the snapshots, the Type field will be formatted by
snapshots/%{type}, like snapshots/overlayfs.

fix: #3295

Signed-off-by: Wei Fu [email protected]

@fuweid fuweid force-pushed the me-update-lease branch from 6ca4907 to 3ce6a1f Compare May 26, 2019 16:10
@fuweid fuweid requested a review from dmcgowan May 26, 2019 16:10
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 26, 2019

Build succeeded.

@fuweid fuweid force-pushed the me-update-lease branch from 3ce6a1f to 8ed9377 Compare May 26, 2019 16:34
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 26, 2019

Build succeeded.

@fuweid fuweid force-pushed the me-update-lease branch from 8ed9377 to 101df28 Compare May 26, 2019 17:03
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 26, 2019

Build succeeded.

@fuweid fuweid force-pushed the me-update-lease branch from 101df28 to f75e87e Compare May 27, 2019 01:16
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented May 27, 2019

The failed case is here :

--- FAIL: TestClientTTRPC_Close (0.00s)
    client_ttrpc_test.go:70: assertion failed: rpc error: code = InvalidArgument desc = event envelope has invalid namespace: namespace "" must match ^[A-Za-z][A-Za-z0-9]+(:?[-]+[A-Za-z][A-Za-z0-9]+)*(?:[.](?:[A-Za-z][A-Za-z0-9]+(:?[-]+[A-Za-z][A-Za-z0-9]+)*))*$: invalid argument (err *status.statusError) != ttrpc: closed (ttrpc.ErrClosed *errors.fundamental)

I think we should check close status before we start to handle call or message

# vendor/github.com/containerd/ttrpc/client.go
for {
                select {
                case call := <-calls:
                        if err := c.send(call.ctx, streamID, messageTypeRequest, call.req); err != nil {
                                call.errs <- err
                                continue
                        }

                        waiters[streamID] = call
                        streamID += 2 // enforce odd client initiated request ids
                case msg := <-incoming:
                        call, ok := waiters[msg.StreamID]
                        if !ok {
                                logrus.Errorf("ttrpc: received message for unknown channel %v", msg.StreamID)
                                continue
                        }

                        call.errs <- c.recv(call.resp, msg)
                        delete(waiters, msg.StreamID)
                case <-shutdown:
                        if shutdownErr != nil {
                                shutdownErr = filterCloseErr(shutdownErr)
                        } else {
                                shutdownErr = ErrClosed
                        }

                        shutdownErr = errors.Wrapf(shutdownErr, "ttrpc: client shutting down")

                        c.err = shutdownErr
                        for _, waiter := range waiters {
                                waiter.errs <- shutdownErr
                        }
                        c.Close()
                        return
                case <-c.closed:
                        if c.err == nil {
                                c.err = ErrClosed
                        }
                        // broadcast the shutdown error to the remaining waiters.
                        for _, waiter := range waiters {
                                waiter.errs <- c.err
                        }
                        return
 }

@fuweid fuweid force-pushed the me-update-lease branch from f75e87e to a5aebe9 Compare May 27, 2019 11:54
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 27, 2019

Build succeeded.

@fuweid fuweid force-pushed the me-update-lease branch from a5aebe9 to 3064332 Compare May 27, 2019 14:59
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3304 into master will increase coverage by 0.17%.
The diff coverage is 63.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3304      +/-   ##
==========================================
+ Coverage    44.6%   44.78%   +0.17%     
==========================================
  Files         112      112              
  Lines       12180    12299     +119     
==========================================
+ Hits         5433     5508      +75     
- Misses       5913     5943      +30     
- Partials      834      848      +14
Flag Coverage Δ
#linux 48.69% <68.75%> (+0.2%) ⬆️
#windows 40.14% <63.02%> (+0.26%) ⬆️
Impacted Files Coverage Δ
metadata/leases.go 51.25% <63.02%> (+6.97%) ⬆️

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 0e7a3c9...3064332. Read the comment docs.

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 27, 2019

Codecov Report

Merging #3304 into master will increase coverage by 0.17%.
The diff coverage is 63.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3304      +/-   ##
==========================================
+ Coverage    44.6%   44.78%   +0.17%     
==========================================
  Files         112      112              
  Lines       12180    12299     +119     
==========================================
+ Hits         5433     5508      +75     
- Misses       5913     5943      +30     
- Partials      834      848      +14
Flag Coverage Δ
#linux 48.69% <68.75%> (+0.2%) ⬆️
#windows 40.14% <63.02%> (+0.26%) ⬆️
Impacted Files Coverage Δ
metadata/leases.go 51.25% <63.02%> (+6.97%) ⬆️

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 0e7a3c9...8a388d6. Read the comment docs.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 27, 2019

Build succeeded.

Comment thread metadata/leases.go Outdated
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this would be a good place to use ErrNotImplemented to distinguish between a bad request and something that could be used in the future to check for compatibility. Just a thought.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is reasonable to me. Updated. PTAL

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I am ok deferring the error code discussion

Add three methods for lease service so that the client can use it to
manage the resource by lease, not just gc.root label. With the following
methods, it is easy for client to maintain their own cache system.

```
 - AddResource(context.Context, Lease, Resource) error
 - RemoveResource(context.Context, Lease, Resource) error
 - ListResources(context.Context, Lease) ([]Resource, error)
```

And the resource is to be

```golang
type Resource {
  ID   string
  Type string
}
```

For the snapshots, the Type field will be formatted by
snapshots/%{type}, like snapshots/overlayfs.

fix: containerd#3295

Signed-off-by: Wei Fu <[email protected]>
@fuweid fuweid force-pushed the me-update-lease branch from 3064332 to 8a388d6 Compare May 29, 2019 03:00
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented May 29, 2019

Build succeeded.

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

ping @tonistiigi , do you want to review this?

@tonistiigi
Copy link
Copy Markdown
Member

@crosbymichael Design LGTM, for buildkit ns support would also need a #3307 follow-up

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit d4e7efb into containerd:master Jun 3, 2019
@fuweid fuweid deleted the me-update-lease branch June 4, 2019 01:25
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.

proposal: update lease manager to support explicitly managing resources

5 participants