/juju

Bug 1690456: validate directory provided by --metadata-url during bootstrap #7354

Merged
merged 1 commit into from May 23, 2017

Conversation

Reviewers

@jameinel jameinel

@wallyworld wallyworld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@hmlanigan @wallyworld @jujubot @jameinel
@hmlanigan
Member

hmlanigan commented May 17, 2017 •
Edited 1 time

If the directory ends in 'tools' or images', assume
that only the respective value will be used. If a high level
directory is listed, check for tools and images in it. If no
tools are listed, use tools from streams.canonical.com.

Description of change

Attempting to anticipate what users intend with bootstrap --metadata-source.

  • If the directory doesn't exist, fail quickly so the user can retry.
  • If the directory does exist, look for tools and images directories inside.
    • If tools does exist, replace the default tool location.
    • If tools doesn't exist, leave the default tools as simple streams.canonical.com....
  • If the directory ends in 'images' assume that only image metadata will be found
  • If the directory ends in 'tools' assume that only tools metadata will be found

The contents of tools and images are validated further on.

QA steps

Bootstrap openstack with arm64 nova compute nodes from a host machine with an a
different processor (arm64), don't use a nightly build to test as tools for nightly are not
on streams.canonical.com. - to verify bug fix.

Unit tests will be updated to verify the listed possibilities.

Documentation changes

N/A

Bug reference

https://bugs.launchpad.net/juju/+bug/1690456
@wallyworld

wallyworld requested changes May 17, 2017
View changes

Please also expand the PR description to explain the issue

environs/bootstrap/bootstrap.go
- tools.DefaultBaseURL = metadataDir
+ if _, err := os.Stat(metadataDir); err != nil {
+ if !os.IsNotExist(err) {
+ return nil, errors.Annotate(err, "cannot access metadata directory")
@wallyworld

wallyworld May 17, 2017

Owner

"... access simplestreams metadata ..."

@hmlanigan

hmlanigan May 18, 2017

Member

done

environs/bootstrap/bootstrap.go
+ if !os.IsNotExist(err) {
+ return nil, errors.Annotate(err, "cannot access metadata directory")
+ }
+ logger.Infof("metadata source: %s, not found", metadataDir)
@wallyworld

wallyworld May 17, 2017

Owner

We should return a not found error here so it bubbles up and aborts bootstrap
return errors.NotFoundf("simplestreams metadata source directory %v", metadataDir)

@hmlanigan

hmlanigan May 18, 2017

Member

done

environs/bootstrap/bootstrap.go
+ if !os.IsNotExist(err) {
+ return nil, errors.Annotate(err, "cannot access tools metadata")
+ }
+ logger.Infof("default tools metadata source: %s", tools.DefaultBaseURL)
@wallyworld

wallyworld May 17, 2017

Owner

no need to log at info, just debug

@hmlanigan

hmlanigan May 18, 2017

Member

done

environs/bootstrap/bootstrap.go
+ }
+ logger.Infof("default tools metadata source: %s", tools.DefaultBaseURL)
+ } else {
+ logger.Infof("setting default tools metadata source: %s", toolsMetadataDir)
@wallyworld

wallyworld May 17, 2017

Owner

ditto about debug

@hmlanigan

hmlanigan May 18, 2017

Member

done

environs/bootstrap/bootstrap.go
+ logger.Infof("default tools metadata source: %s", tools.DefaultBaseURL)
+ } else {
+ logger.Infof("setting default tools metadata source: %s", toolsMetadataDir)
+ tools.DefaultBaseURL = metadataDir
@wallyworld

wallyworld May 17, 2017

Owner

we only want to do this if metadataDir ends with /tools
so the logic for this block should be:

toolsMetadataDir = metadataDir
if not toolsMetadataDir endswith /tools then
toolsMetadataDir += /tools
if toolsMetadataDir exists then
tools.DefaultBaseURL = toolsMetadataDir

@hmlanigan

hmlanigan May 18, 2017

Member

done

environs/bootstrap/bootstrap.go
+ } else {
+ logger.Infof("setting default tools metadata source: %s", toolsMetadataDir)
+ tools.DefaultBaseURL = metadataDir
+ }
imageMetadataDir := filepath.Join(metadataDir, storage.BaseImagesPath)
@wallyworld

wallyworld May 17, 2017

Owner

we want the same logic as for tools above.
ie
the user can specify --metadata-source as one of:

somedir
somedir/tools
somedir/images

and the logic should Do The Right Thing

@hmlanigan

hmlanigan May 18, 2017

Member

done

@hmlanigan
Member

hmlanigan commented May 18, 2017

working on tests now.

@jameinel jameinel changed the title from Bug 1690456: validate directory provided by --metadata-url during to Bug 1690456: validate directory provided by --metadata-url during bootstrap May 20, 2017

@jameinel

jameinel requested changes May 20, 2017
View changes

I tweaked your review request comment so that it formats correctly as Markdown.

I do think a better fix would be to not overload one CLI arg to mean 2 possible things. In config we have 'image-metadata-url' and 'agent-metadata-url' as 2 different config options that can be set, we probably shouldn't have a single CLI arg that means both of them.
(we probably have to keep it for compatibility, but I think we can add more for users to be clearer about what they are doing.)

cmd/juju/commands/bootstrap_test.go
)
c.Check(s.tw.Log(), jc.LogMatches, []jc.SimpleMessage{
- {loggo.ERROR, "failed to bootstrap model: cannot package bootstrap agent binary: no agent binaries for you"},
+ {loggo.ERROR, "failed to bootstrap model: no matching tools available"},
@jameinel

jameinel May 20, 2017

Owner

'agent binaries' is the preferred name to 'matching tools'.
Mark especially doesn't like the word 'tools' and we should avoid using it in messages to users.

@wallyworld

wallyworld May 22, 2017

Owner

There's still legacy error messages floating around - agree it would be good to fix those as a driveby. We should also support looking for "agents" as well as "tools" as a sub dir under metadatasource dir.

@hmlanigan

hmlanigan May 22, 2017

Member

The wording update in this specific case will be addressed in a follow-on PR, as will
looking for an 'agents' directory.

environs/bootstrap/bootstrap.go
-// for tools syncing, and adds an image metadata source after verifying
-// the contents.
+// setPrivateMetadataSources verifys the metadataDir and uses it to
+// set the default tools metadata source for tools syncing, and adds
@jameinel

jameinel May 20, 2017

Owner

s/tools/agent binaries/

I realize we're currently looking for a directory ending in 'tools' but telling people 'we find agent binaries in a directory named tools' is still clearer what we are actually looking for. 'tools' could be anything (such as helper commands, eg juju-metadata, etc).

@hmlanigan

hmlanigan May 22, 2017

Member

updated the log messages to use agent binary instead of tools

environs/bootstrap/bootstrap.go
+ } else {
+ if ending == storage.BaseToolsPath {
+ tools.DefaultBaseURL = filepath.Dir(metadataDir)
+ logger.Debugf("setting default tools metadata source: %s", tools.DefaultBaseURL)
@jameinel

jameinel May 20, 2017

Owner

agent binaries

especially given the config option would be "agent-metadata-url"

@wallyworld
Owner

wallyworld commented May 22, 2017

@jameinel the reason for one directory is that the user can choose to put there whatever necessary metadata is needed. They may want to provide image metadata, agent metadata, or both. They do this by using sub directories "images" or "tools" <- yes, "tools" is outdated. We should add support for "agents" as well. Regardless, the --metadata-source dir is meant to be the top level dir which contains sub dirs.

The issue addressed here is partly a bug (Juju wasn't checking the validity of the dir supplied), but also a usability issue - people have been specifying a full path to "sourcedir/images" instead of "sourcedir". So part of the change here is to support DWIM.
@hmlanigan
Member

hmlanigan commented May 23, 2017

!!testme!!
@wallyworld

wallyworld approved these changes May 23, 2017
View changes

Thank you. Might be worth re-doing the manual QA just to be sure given the changes done

environs/bootstrap/bootstrap.go
-// setPrivateMetadataSources sets the default tools metadata source
-// for tools syncing, and adds an image metadata source after verifying
-// the contents.
+// setPrivateMetadataSources verifys the metadataDir and uses it to
@wallyworld

wallyworld May 23, 2017

Owner

verifies the specified metadataDir exists

@hmlanigan

hmlanigan May 23, 2017

Member

done

environs/bootstrap/bootstrap.go
-// the contents.
+// setPrivateMetadataSources verifys the metadataDir and uses it to
+// set the default agent binaries metadata source for agent binary
+// syncing, and adds an image metadata source after verifying the
@wallyworld

wallyworld May 23, 2017

Owner

s/syncing//

@hmlanigan

hmlanigan May 23, 2017

Member

done

environs/bootstrap/bootstrap.go
+// set the default agent binaries metadata source for agent binary
+// syncing, and adds an image metadata source after verifying the
+// contents. If the directory ends in tools, only the default tools
+// metadata source will be set. Same for images.
@wallyworld

wallyworld May 23, 2017

Owner

remove extra space

@hmlanigan

hmlanigan May 23, 2017

Member

done

environs/bootstrap/bootstrap.go
+ if !os.IsNotExist(err) {
+ return nil, errors.Annotate(err, "cannot access agent binary metadata")
+ }
+ logger.Debugf("default agent binary metadata source: %s", tools.DefaultBaseURL)
@wallyworld

wallyworld May 23, 2017

Owner

can we reword to "no agent directory found, using default agent binary....."

@hmlanigan

hmlanigan May 23, 2017

Member

done

environs/bootstrap/bootstrap.go
+ if ending == storage.BaseToolsPath {
+ tools.DefaultBaseURL = filepath.Dir(metadataDir)
+ logger.Debugf("setting default agent binary metadata source: %s", tools.DefaultBaseURL)
+ return nil, nil
@wallyworld

wallyworld May 23, 2017

Owner

can we add a comment explaining why it's ok to exit early here

@hmlanigan

hmlanigan May 23, 2017

Member

done

environs/bootstrap/bootstrap_test.go
+// juju bootstrap --metadata-source <dir>
+// juju bootstrap --metadata-source <dir>/images
+// where <dir>/tools doesn't exist
+func (s *bootstrapSuite) TestBootstrapMetadataImagesNoTools(c *gc.C) {
@wallyworld

wallyworld May 23, 2017

Owner

this should either be 2 tests, or a single test inside a for loop
metadataDir, _ := createImageMetadata(c)
for _, suffix := range []string{"", "images"} {
sourceDir := filepath.Join(metadataDir, suffix)
do test stuff...
}

@hmlanigan

hmlanigan May 23, 2017

Member

done

environs/bootstrap/bootstrap_test.go
+// juju bootstrap --metadata-source <dir>
+// juju bootstrap --metadata-source <dir>/tools
+// where <dir>/images doesn't exist
+func (s *bootstrapSuite) TestBootstrapMetadataToolsNoImages(c *gc.C) {
@wallyworld

wallyworld May 23, 2017

Owner

ditto

@hmlanigan

hmlanigan May 23, 2017

Member

done

@hmlanigan
Bug 1690456: validate directory provided by --metadata-url during 
bootstrap.  If the directory ends in 'tools' or images', assume
that only the respective value will be used.  If a high level
directory is listed, check for tools and images directories in it.
If no tools are listed, use tools from simplestreams.canonical.com.
048c0fc
@hmlanigan
Member

hmlanigan commented May 23, 2017

manual QA has been done on the last changes.
@hmlanigan
Member

hmlanigan commented May 23, 2017

$$merge$$
@jujubot
Contributor

jujubot commented May 23, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented May 23, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10957
@hmlanigan
Member

hmlanigan commented May 23, 2017

$$merge$$
@jujubot
Contributor

jujubot commented May 23, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 074ca56 into juju:develop May 23, 2017
1 check passed

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@hmlanigan hmlanigan deleted the hmlanigan:bug-1690456 branch May 23, 2017

@hmlanigan hmlanigan referenced this pull request May 24, 2017

Merged

Update user facing messages to reference 'agent binaries' instead of 'tools' #7380

jujubot added a commit that referenced this pull request May 24, 2017

@jujubot
Merge pull request #7380 from hmlanigan/agent-binary 
Update user facing messages to reference 'agent binaries' instead of 'tools'



## Please provide the following details to expedite Pull Request review:

----

## Description of change

This is a follow on to #7354.  It was mentioned in general
to use 'agent binaries' or the like instead of 'tools'.  Updating user facing output accordingly.

## QA steps

1. Verify visually during juju bootstrap --debug and in logs
2. Ran make check in juju/juju

## Documentation changes

Unknown, it may affect some documentation examples?

## Bug reference
04bf2ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment