Browse code
Use GetByName to check for collision before create any context in plugin creation
This fix is a follow up to the comment:
https://github.com/docker/docker/pull/28717#discussion_r90040589
Currently, the collision checking is done at the last step `Add()` of
plugin creation. However, at this stage the context such as plugin
directories have already been creation. In case of name collision,
rollback is needed which could be expensive.
This fix performs the check at the beginning of CreateFromContext using
GetByName. In this way, collision fails fast and no context creation
or rollback is needed.
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
(cherry picked from commit 52405a9b5896fd1c3ea6d8b1ca1c70e5979e3271)
Yong Tang authored on 2016/11/30 05:55:41Showing 2 changed files
| ... | ... |
@@ -321,15 +321,29 @@ func (pm *Manager) Set(name string, args []string) error {
|
| 321 | 321 |
// CreateFromContext creates a plugin from the given pluginDir which contains |
| 322 | 322 |
// both the rootfs and the config.json and a repoName with optional tag. |
| 323 | 323 |
func (pm *Manager) CreateFromContext(ctx context.Context, tarCtx io.Reader, options *types.PluginCreateOptions) error {
|
| 324 |
+ repoName := options.RepoName |
|
| 325 |
+ ref, err := distribution.GetRef(repoName) |
|
| 326 |
+ if err != nil {
|
|
| 327 |
+ return err |
|
| 328 |
+ } |
|
| 329 |
+ |
|
| 330 |
+ name := ref.Name() |
|
| 331 |
+ tag := distribution.GetTag(ref) |
|
| 324 | 332 |
pluginID := stringid.GenerateNonCryptoID() |
| 325 | 333 |
|
| 334 |
+ p := v2.NewPlugin(name, pluginID, pm.runRoot, pm.libRoot, tag) |
|
| 335 |
+ |
|
| 336 |
+ if v, _ := pm.pluginStore.GetByName(p.Name()); v != nil {
|
|
| 337 |
+ return fmt.Errorf("plugin %q already exists", p.Name())
|
|
| 338 |
+ } |
|
| 339 |
+ |
|
| 326 | 340 |
pluginDir := filepath.Join(pm.libRoot, pluginID) |
| 327 | 341 |
if err := os.MkdirAll(pluginDir, 0755); err != nil {
|
| 328 | 342 |
return err |
| 329 | 343 |
} |
| 330 | 344 |
|
| 331 | 345 |
// In case an error happens, remove the created directory. |
| 332 |
- if err := pm.createFromContext(ctx, pluginID, pluginDir, tarCtx, options); err != nil {
|
|
| 346 |
+ if err := pm.createFromContext(ctx, tarCtx, pluginDir, repoName, p); err != nil {
|
|
| 333 | 347 |
if err := os.RemoveAll(pluginDir); err != nil {
|
| 334 | 348 |
logrus.Warnf("unable to remove %q from failed plugin creation: %v", pluginDir, err)
|
| 335 | 349 |
} |
| ... | ... |
@@ -339,20 +353,11 @@ func (pm *Manager) CreateFromContext(ctx context.Context, tarCtx io.Reader, opti |
| 339 | 339 |
return nil |
| 340 | 340 |
} |
| 341 | 341 |
|
| 342 |
-func (pm *Manager) createFromContext(ctx context.Context, pluginID, pluginDir string, tarCtx io.Reader, options *types.PluginCreateOptions) error {
|
|
| 342 |
+func (pm *Manager) createFromContext(ctx context.Context, tarCtx io.Reader, pluginDir, repoName string, p *v2.Plugin) error {
|
|
| 343 | 343 |
if err := chrootarchive.Untar(tarCtx, pluginDir, nil); err != nil {
|
| 344 | 344 |
return err |
| 345 | 345 |
} |
| 346 | 346 |
|
| 347 |
- repoName := options.RepoName |
|
| 348 |
- ref, err := distribution.GetRef(repoName) |
|
| 349 |
- if err != nil {
|
|
| 350 |
- return err |
|
| 351 |
- } |
|
| 352 |
- name := ref.Name() |
|
| 353 |
- tag := distribution.GetTag(ref) |
|
| 354 |
- |
|
| 355 |
- p := v2.NewPlugin(name, pluginID, pm.runRoot, pm.libRoot, tag) |
|
| 356 | 347 |
if err := p.InitPlugin(); err != nil {
|
| 357 | 348 |
return err |
| 358 | 349 |
} |
| ... | ... |
@@ -113,6 +113,13 @@ func (ps *Store) Add(p *v2.Plugin) error {
|
| 113 | 113 |
if v, exist := ps.plugins[p.GetID()]; exist {
|
| 114 | 114 |
return fmt.Errorf("plugin %q has the same ID %s as %q", p.Name(), p.GetID(), v.Name())
|
| 115 | 115 |
} |
| 116 |
+ // Since both Pull() and CreateFromContext() calls GetByName() before any plugin |
|
| 117 |
+ // to search for collision (to fail fast), it is unlikely the following check |
|
| 118 |
+ // will return an error. |
|
| 119 |
+ // However, in case two CreateFromContext() are called at the same time, |
|
| 120 |
+ // there is still a remote possibility that a collision might happen. |
|
| 121 |
+ // For that reason we still perform the collision check below as it is protected |
|
| 122 |
+ // by ps.Lock() and ps.Unlock() above. |
|
| 116 | 123 |
if _, exist := ps.nameToID[p.Name()]; exist {
|
| 117 | 124 |
return fmt.Errorf("plugin %q already exists", p.Name())
|
| 118 | 125 |
} |