Skip to content

sync container metadata before operations#1595

Merged
estesp merged 1 commit intocontainerd:masterfrom
crosbymichael:container-lock
Oct 5, 2017
Merged

sync container metadata before operations#1595
estesp merged 1 commit intocontainerd:masterfrom
crosbymichael:container-lock

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Oct 4, 2017 •
edited
Loading

This makes sure the client is always in sync with the server before
performing any type of operations on the container metadata.

Signed-off-by: Michael Crosby [email protected]

@Random-Liu
Copy link
Copy Markdown
Member

Random-Liu commented Oct 4, 2017

@crosbymichael In most cases, we only need to read the information in c.c. Can we make this RWMutex so that read-only operations could perform concurrently?

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Oct 4, 2017

@Random-Liu SGTM, i'll update

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Oct 4, 2017

Updated with an RW mutex

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 4, 2017 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1595   +/-   ##
=======================================
  Coverage   46.29%   46.29%           
=======================================
  Files          24       24           
  Lines        3378     3378           
=======================================
  Hits         1564     1564           
  Misses       1456     1456           
  Partials      358      358

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 acba0f5...fa9e9bd. Read the comment docs.

Random-Liu
Random-Liu reviewed Oct 4, 2017
View reviewed changes
Comment thread container.go Outdated
Copy link
Copy Markdown
Member

@Random-Liu Random-Liu Oct 4, 2017

Choose a reason for hiding this comment

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

Seems like a write lock is needed. Although I don't think we need to update container here. :)

Copy link
Copy Markdown
Member Author

@crosbymichael crosbymichael Oct 4, 2017

Choose a reason for hiding this comment

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

done

Comment thread container.go Outdated
Copy link
Copy Markdown
Member

@Random-Liu Random-Liu Oct 4, 2017

Choose a reason for hiding this comment

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

RLock seems good enough.

Copy link
Copy Markdown
Member Author

@crosbymichael crosbymichael Oct 4, 2017

Choose a reason for hiding this comment

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

i don't want this to be called twice since you can only have 1 task per container

mlaventure
mlaventure reviewed Oct 4, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

Labels, Spec and Extensions can be updated by the user. In the case of the Labels we do retrieve the container again before returning them. For consistency, we should probably do the same for Spec and Extensions.

Otherwise, we could add a Refresh method and just returned the currently store values for them.

The first choice is more resilient to change by different clients though.

Comment thread container.go Outdated
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure Oct 4, 2017

Choose a reason for hiding this comment

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

We should probably update the container here, same as labels below since those can be changed.

Copy link
Copy Markdown
Member Author

@crosbymichael crosbymichael Oct 4, 2017

Choose a reason for hiding this comment

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

we will have to update these to return an error, do you think that is ok?

Copy link
Copy Markdown
Contributor

@mlaventure mlaventure Oct 4, 2017

Choose a reason for hiding this comment

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

It's what we have for Labels. And probably what we should have for Specs since it can be updated too. Would make it consistent

Comment thread container.go Outdated
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure Oct 4, 2017

Choose a reason for hiding this comment

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

I don't see this id being used anywhere

@crosbymichael
Fetch current container info before operations
fa9e9bd 
This makes sure the client is always in sync with the server before
performing any type of operations on the container metadata.

Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael crosbymichael changed the title Improve client Container locking sync container metadata before operations Oct 4, 2017
@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Oct 4, 2017

Ok, I updated this and remove the lock all together and fetch the current data from the server before performing or returning anything. This makes sure that the client is never stale and does not store the container metadata locally.

mlaventure
mlaventure approved these changes Oct 4, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

@Random-Liu
Copy link
Copy Markdown
Member

Random-Liu commented Oct 4, 2017

This looks even better. Thanks

@Random-Liu Random-Liu mentioned this pull request Oct 4, 2017
Closed
@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Oct 5, 2017

LGTM

estesp
estesp approved these changes Oct 5, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 72a3a01 into containerd:master Oct 5, 2017
dmcgowan pushed a commit to dmcgowan/containerd that referenced this pull request Oct 6, 2020
@estesp
Merge pull request containerd#1595 from dmcgowan/fix-unix-lint
bc08a19 
Fix unix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Random-Liu Random-Liu Random-Liu left review comments

@estesp estesp estesp approved these changes

@mlaventure mlaventure mlaventure approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@crosbymichael @Random-Liu @codecov-io @stevvooe @estesp @mlaventure