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

Clarify documentation for Set{Task,Exec}Label #113

Merged
merged 1 commit into from Jul 27, 2020

Conversation

gitstashpop
Copy link
Contributor

In Go, there are no guarantees which goroutine runs in which thread.
It is therefore important to wrap calls to Set{Task,Exec}Label with
runtime.LockOSThread() and runtime.UnlockOSThread() to ensure reliable
behavior of goroutine labeling.

@rhatdan
Copy link
Collaborator

rhatdan commented Jul 22, 2020

There are more calls then just these that need your comment.

Also you need to sign your PR.
git commit -a --amend -s
git push --force

@rhatdan
Copy link
Collaborator

rhatdan commented Jul 22, 2020

SetFSCreateLabel(label string)
SetExecLabel
SetTaskLabel
SetSocketLabel
SetKeyLabel

@rhatdan
Copy link
Collaborator

rhatdan commented Jul 24, 2020

LGTM
@kolyshkin @mrunalp @thaJeztah @crosbymichael PTAL

Approved with PullApprove

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, but left one suggestion

Comment on lines 66 to 71
// SetFSCreateLabel tells the kernel the label to create all file system objects
// created by this task. Set label="" to return to the default label. Calls to SetFSCreateLabel
// should be wrapped in runtime.LockOSThread()/runtime.UnlockOSThread() until file system
// objects created by this task are finished to guarantee another goroutine does not migrate
// to the current thread before execution is complete.
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker (as it was already a bit confusing), but the first line is a bit hard to grasp. Perhaps something like;

Suggested change
// SetFSCreateLabel tells the kernel the label to create all file system objects
// created by this task. Set label="" to return to the default label. Calls to SetFSCreateLabel
// should be wrapped in runtime.LockOSThread()/runtime.UnlockOSThread() until file system
// objects created by this task are finished to guarantee another goroutine does not migrate
// to the current thread before execution is complete.
// SetFSCreateLabel tells the kernel what label to use for all file system objects created by this task.
// Set label to an empty string to return to the default label. Calls to SetFSCreateLabel
// should be wrapped in runtime.LockOSThread()/runtime.UnlockOSThread() until file system
// objects created by this task are finished to guarantee another goroutine does not migrate
// to the current thread before execution is complete.

(suggestions welcome! /cc @rhatdan)

Copy link
Member

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Collaborator

rhatdan commented Jul 27, 2020

@yulicrunchy Please take @thaJeztah input and then we can merge.

In Go, there are no guarantees which goroutine runs in which thread.
It is important to wrap calls to Set{Task,Exec,FsCreate,Socket,Key}Label
with runtime.LockOSThread() and runtime.UnlockOSThread() to ensure reliable
behavior of goroutine labeling.

Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

thaJeztah commented Jul 27, 2020

LGTM

Approved with PullApprove

@thaJeztah
Copy link
Member

@rhatdan @kolyshkin PTAL

@rhatdan rhatdan merged commit e5226d7 into opencontainers:master Jul 27, 2020
@rhatdan
Copy link
Collaborator

rhatdan commented Jul 27, 2020

Thanks @yulicrunchy. This is an issue that often trips people up, including me.

@gitstashpop gitstashpop deleted the clarify-settasklabel branch July 27, 2020 17:48
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.

None yet

4 participants