From 4b8a386ad6c07ef77588f05b7e7529d7f059f91e Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Tue, 18 Sep 2018 16:50:42 -0700 Subject: [PATCH 1/4] openshift-install: decrease default log verbosity Users generally won't be interested in anything but WARN and above during normal operation. --- cmd/openshift-install/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/openshift-install/main.go b/cmd/openshift-install/main.go index 07e32e1575e..bb1006a71ba 100644 --- a/cmd/openshift-install/main.go +++ b/cmd/openshift-install/main.go @@ -15,7 +15,7 @@ var ( ignitionConfigsCommand = kingpin.Command("ignition-configs", "Generate the Ignition Config assets") dirFlag = kingpin.Flag("dir", "assets directory").Default(".").String() - logLevel = kingpin.Flag("log-level", "log level (e.g. \"debug\")").Default("info").Enum("debug", "info", "warn", "error", "fatal", "panic") + logLevel = kingpin.Flag("log-level", "log level (e.g. \"debug\")").Default("warn").Enum("debug", "info", "warn", "error", "fatal", "panic") ) func main() { From f0281007ddab64c447d123c96f543afcb14e6ec2 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Tue, 18 Sep 2018 16:55:08 -0700 Subject: [PATCH 2/4] asset/*: read user prompts from same line It's a little more common to provide a response to a prompt on the same line. --- pkg/asset/installconfig/platform.go | 6 +++--- pkg/asset/installconfig/stock.go | 10 +++++----- pkg/asset/userprovided.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/asset/installconfig/platform.go b/pkg/asset/installconfig/platform.go index 053149cf4ce..dd1f1463d0d 100644 --- a/pkg/asset/installconfig/platform.go +++ b/pkg/asset/installconfig/platform.go @@ -17,7 +17,7 @@ const ( var ( validPlatforms = []string{AWSPlatformType, LibvirtPlatformType} - platformPrompt = fmt.Sprintf("Platform (%s):", strings.Join(validPlatforms, ", ")) + platformPrompt = fmt.Sprintf("Platform (%s)", strings.Join(validPlatforms, ", ")) ) // Platform is an asset that queries the user for the platform on which to install @@ -70,7 +70,7 @@ func (a *Platform) queryUserForPlatform() string { func (a *Platform) awsPlatform() (*asset.State, error) { return assetStateForStringContents( AWSPlatformType, - asset.QueryUser(a.InputReader, "Region:"), + asset.QueryUser(a.InputReader, "Region"), ), nil } @@ -78,7 +78,7 @@ func (a *Platform) libvirtPlatform() (*asset.State, error) { return assetStateForStringContents( LibvirtPlatformType, // TODO(yifan): Set the default URI. - asset.QueryUser(a.InputReader, "URI:"), + asset.QueryUser(a.InputReader, "URI"), ), nil } diff --git a/pkg/asset/installconfig/stock.go b/pkg/asset/installconfig/stock.go index 0e33a9125b3..b9293b3f8ae 100644 --- a/pkg/asset/installconfig/stock.go +++ b/pkg/asset/installconfig/stock.go @@ -51,26 +51,26 @@ func (s *StockImpl) EstablishStock(directory string, inputReader *bufio.Reader) } s.clusterID = &clusterID{} s.emailAddress = &asset.UserProvided{ - Prompt: "Email Address:", + Prompt: "Email Address", InputReader: inputReader, } s.password = &asset.UserProvided{ - Prompt: "Password:", + Prompt: "Password", InputReader: inputReader, } s.sshKey = &sshPublicKey{ inputReader: inputReader, } s.baseDomain = &asset.UserProvided{ - Prompt: "Base Domain:", + Prompt: "Base Domain", InputReader: inputReader, } s.clusterName = &asset.UserProvided{ - Prompt: "Cluster Name:", + Prompt: "Cluster Name", InputReader: inputReader, } s.pullSecret = &asset.UserProvided{ - Prompt: "Pull Secret:", + Prompt: "Pull Secret", InputReader: inputReader, } s.platform = &Platform{InputReader: inputReader} diff --git a/pkg/asset/userprovided.go b/pkg/asset/userprovided.go index 77c8d9e6e72..fb05df5a608 100644 --- a/pkg/asset/userprovided.go +++ b/pkg/asset/userprovided.go @@ -32,7 +32,7 @@ func (a *UserProvided) Generate(map[Asset]*State) (*State, error) { // QueryUser queries the user for input. func QueryUser(inputReader *bufio.Reader, prompt string) string { for { - fmt.Println(prompt) + fmt.Printf("%s: ", prompt) input, err := inputReader.ReadString('\n') if err != nil && err != io.EOF { fmt.Println("Could not understand response. Please retry.") From a23d3e609d226605092b3e2097e69557758efcb8 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Tue, 18 Sep 2018 16:54:34 -0700 Subject: [PATCH 3/4] asset/*: add names to all assets This will help with logging in the near future. --- pkg/asset/asset.go | 3 +++ pkg/asset/ignition/bootstrap.go | 5 +++++ pkg/asset/ignition/master.go | 5 +++++ pkg/asset/ignition/testasset_test.go | 4 ++++ pkg/asset/ignition/worker.go | 5 +++++ pkg/asset/installconfig/clusterid.go | 5 +++++ pkg/asset/installconfig/installconfig.go | 5 +++++ pkg/asset/installconfig/installconfig_test.go | 4 ++++ pkg/asset/installconfig/platform.go | 5 +++++ pkg/asset/installconfig/ssh.go | 5 +++++ pkg/asset/kubeconfig/kubeconfig.go | 5 +++++ pkg/asset/kubeconfig/kubeconfig_test.go | 4 ++++ pkg/asset/store_test.go | 4 ++++ pkg/asset/tls/certkey.go | 5 +++++ pkg/asset/tls/certkey_test.go | 4 ++++ pkg/asset/tls/keypair.go | 5 +++++ pkg/asset/tls/root.go | 5 +++++ pkg/asset/userprovided.go | 5 +++++ 18 files changed, 83 insertions(+) diff --git a/pkg/asset/asset.go b/pkg/asset/asset.go index 4e6c8883243..ec6884912fb 100644 --- a/pkg/asset/asset.go +++ b/pkg/asset/asset.go @@ -12,6 +12,9 @@ type Asset interface { // Generate generates this asset given the states of its dependent assets. Generate(map[Asset]*State) (*State, error) + + // Name returns the human-friendly name of the asset. + Name() string } // GetDataByFilename searches the file in the asset.State.Contents, and returns its data. diff --git a/pkg/asset/ignition/bootstrap.go b/pkg/asset/ignition/bootstrap.go index db80e5d4c7f..a1d87d24980 100644 --- a/pkg/asset/ignition/bootstrap.go +++ b/pkg/asset/ignition/bootstrap.go @@ -166,6 +166,11 @@ func (a *bootstrap) Generate(dependencies map[asset.Asset]*asset.State) (*asset. }, nil } +// Name returns the human-friendly name of the asset. +func (a *bootstrap) Name() string { + return "Bootstrap Ignition Config" +} + // getTemplateData returns the data to use to execute bootstrap templates. func (a *bootstrap) getTemplateData(installConfig *types.InstallConfig) (*bootstrapTemplateData, error) { clusterDNSIP, err := installconfig.ClusterDNSIP(installConfig) diff --git a/pkg/asset/ignition/master.go b/pkg/asset/ignition/master.go index 345b108ddaf..967b837dd3c 100644 --- a/pkg/asset/ignition/master.go +++ b/pkg/asset/ignition/master.go @@ -56,3 +56,8 @@ func (a *master) Generate(dependencies map[asset.Asset]*asset.State) (*asset.Sta return state, nil } + +// Name returns the human-friendly name of the asset. +func (a *master) Name() string { + return "Master Ignition Config(s)" +} diff --git a/pkg/asset/ignition/testasset_test.go b/pkg/asset/ignition/testasset_test.go index 27d8adb6e8c..b5189b9f25f 100644 --- a/pkg/asset/ignition/testasset_test.go +++ b/pkg/asset/ignition/testasset_test.go @@ -16,6 +16,10 @@ func (a *testAsset) Generate(map[asset.Asset]*asset.State) (*asset.State, error) return nil, nil } +func (a *testAsset) Name() string { + return "Test Asset" +} + func stateWithContentsData(contentsData ...string) *asset.State { state := &asset.State{ Contents: make([]asset.Content, len(contentsData)), diff --git a/pkg/asset/ignition/worker.go b/pkg/asset/ignition/worker.go index bb922cda23a..a23d10c16d4 100644 --- a/pkg/asset/ignition/worker.go +++ b/pkg/asset/ignition/worker.go @@ -52,3 +52,8 @@ func (a *worker) Generate(dependencies map[asset.Asset]*asset.State) (*asset.Sta }}, }, nil } + +// Name returns the human-friendly name of the asset. +func (a *worker) Name() string { + return "Worker Ignition Config" +} diff --git a/pkg/asset/installconfig/clusterid.go b/pkg/asset/installconfig/clusterid.go index 1fbbebde41e..5adebe26a27 100644 --- a/pkg/asset/installconfig/clusterid.go +++ b/pkg/asset/installconfig/clusterid.go @@ -23,3 +23,8 @@ func (a *clusterID) Generate(map[asset.Asset]*asset.State) (*asset.State, error) }, }, nil } + +// Name returns the human-friendly name of the asset. +func (a *clusterID) Name() string { + return "Cluster ID" +} diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index 858a7abecba..c7f8875297b 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -117,6 +117,11 @@ func (a *installConfig) Generate(dependencies map[asset.Asset]*asset.State) (*as }, nil } +// Name returns the human-friendly name of the asset. +func (a installConfig) Name() string { + return "Install Config" +} + // GetInstallConfig returns the *types.InstallConfig from the parent asset map. func GetInstallConfig(installConfig asset.Asset, parents map[asset.Asset]*asset.State) (*types.InstallConfig, error) { var cfg types.InstallConfig diff --git a/pkg/asset/installconfig/installconfig_test.go b/pkg/asset/installconfig/installconfig_test.go index e45607f410a..c21d9f129ad 100644 --- a/pkg/asset/installconfig/installconfig_test.go +++ b/pkg/asset/installconfig/installconfig_test.go @@ -27,6 +27,10 @@ func (a *testAsset) Generate(map[asset.Asset]*asset.State) (*asset.State, error) return nil, nil } +func (a *testAsset) Name() string { + return "Test Asset" +} + func TestInstallConfigDependencies(t *testing.T) { stock := &StockImpl{ clusterID: &testAsset{name: "test-cluster-id"}, diff --git a/pkg/asset/installconfig/platform.go b/pkg/asset/installconfig/platform.go index dd1f1463d0d..eeef04121bc 100644 --- a/pkg/asset/installconfig/platform.go +++ b/pkg/asset/installconfig/platform.go @@ -54,6 +54,11 @@ func (a *Platform) Generate(map[asset.Asset]*asset.State) (*asset.State, error) } } +// Name returns the human-friendly name of the asset. +func (a *Platform) Name() string { + return "Platform" +} + func (a *Platform) queryUserForPlatform() string { for { input := asset.QueryUser(a.InputReader, platformPrompt) diff --git a/pkg/asset/installconfig/ssh.go b/pkg/asset/installconfig/ssh.go index 1861c637e1a..fdfd3d07603 100644 --- a/pkg/asset/installconfig/ssh.go +++ b/pkg/asset/installconfig/ssh.go @@ -112,3 +112,8 @@ func (a *sshPublicKey) Generate(map[asset.Asset]*asset.State) (state *asset.Stat }, }, nil } + +// Name returns the human-friendly name of the asset. +func (a *sshPublicKey) Name() string { + return "SSH Key" +} diff --git a/pkg/asset/kubeconfig/kubeconfig.go b/pkg/asset/kubeconfig/kubeconfig.go index 0ae622c65c8..9ab30341656 100644 --- a/pkg/asset/kubeconfig/kubeconfig.go +++ b/pkg/asset/kubeconfig/kubeconfig.go @@ -114,3 +114,8 @@ func (k *Kubeconfig) Generate(parents map[asset.Asset]*asset.State) (*asset.Stat }, }, nil } + +// Name returns the human-friendly name of the asset. +func (k *Kubeconfig) Name() string { + return "Kubeconfig" +} diff --git a/pkg/asset/kubeconfig/kubeconfig_test.go b/pkg/asset/kubeconfig/kubeconfig_test.go index a6d72cdc19a..1c79268d400 100644 --- a/pkg/asset/kubeconfig/kubeconfig_test.go +++ b/pkg/asset/kubeconfig/kubeconfig_test.go @@ -23,6 +23,10 @@ func (f fakeAsset) Generate(map[asset.Asset]*asset.State) (*asset.State, error) return nil, nil } +func (f fakeAsset) Name() string { + return "Fake Asset" +} + func TestKubeconfigGenerate(t *testing.T) { testDir, err := ioutil.TempDir(os.TempDir(), "kubeconfig_test") if err != nil { diff --git a/pkg/asset/store_test.go b/pkg/asset/store_test.go index 2546c2d90f8..839f9bc4d37 100644 --- a/pkg/asset/store_test.go +++ b/pkg/asset/store_test.go @@ -29,6 +29,10 @@ func (a *testAsset) Generate(map[Asset]*State) (*State, error) { return nil, nil } +func (a *testAsset) Name() string { + return "Test Asset" +} + func newTestAsset(gl *generationLog, name string) *testAsset { return &testAsset{ name: name, diff --git a/pkg/asset/tls/certkey.go b/pkg/asset/tls/certkey.go index 4a1aa78fc06..71358c01342 100644 --- a/pkg/asset/tls/certkey.go +++ b/pkg/asset/tls/certkey.go @@ -136,6 +136,11 @@ func (c *CertKey) Generate(parents map[asset.Asset]*asset.State) (*asset.State, }, nil } +// Name returns the human-friendly name of the asset. +func (c *CertKey) Name() string { + return fmt.Sprintf("Certificate (%s)", c.Subject.CommonName) +} + func parseCAFromAssetState(ca *asset.State) (*rsa.PrivateKey, *x509.Certificate, error) { var key *rsa.PrivateKey var cert *x509.Certificate diff --git a/pkg/asset/tls/certkey_test.go b/pkg/asset/tls/certkey_test.go index d2fb16790cd..ec893440abc 100644 --- a/pkg/asset/tls/certkey_test.go +++ b/pkg/asset/tls/certkey_test.go @@ -32,6 +32,10 @@ func (f fakeInstallConfig) Generate(map[asset.Asset]*asset.State) (*asset.State, }, nil } +func (f fakeInstallConfig) Name() string { + return "Fake Install Config" +} + func TestCertKeyGenerate(t *testing.T) { testDir, err := ioutil.TempDir(os.TempDir(), "certkey_test") if err != nil { diff --git a/pkg/asset/tls/keypair.go b/pkg/asset/tls/keypair.go index 21b51b5040b..a254fa55ffe 100644 --- a/pkg/asset/tls/keypair.go +++ b/pkg/asset/tls/keypair.go @@ -46,3 +46,8 @@ func (k *KeyPair) Generate(map[asset.Asset]*asset.State) (*asset.State, error) { }, }, nil } + +// Name returns the human-friendly name of the asset. +func (k *KeyPair) Name() string { + return fmt.Sprintf("Key Pair (%s)", k.PubKeyFileName) +} diff --git a/pkg/asset/tls/root.go b/pkg/asset/tls/root.go index eaa9e8d42c3..fe91ee2a98c 100644 --- a/pkg/asset/tls/root.go +++ b/pkg/asset/tls/root.go @@ -48,3 +48,8 @@ func (c *RootCA) Generate(parents map[asset.Asset]*asset.State) (*asset.State, e }, }, nil } + +// Name returns the human-friendly name of the asset. +func (c *RootCA) Name() string { + return "Root CA" +} diff --git a/pkg/asset/userprovided.go b/pkg/asset/userprovided.go index fb05df5a608..fabd9513d6b 100644 --- a/pkg/asset/userprovided.go +++ b/pkg/asset/userprovided.go @@ -29,6 +29,11 @@ func (a *UserProvided) Generate(map[Asset]*State) (*State, error) { }, nil } +// Name returns the human-friendly name of the asset. +func (a UserProvided) Name() string { + return a.Prompt +} + // QueryUser queries the user for input. func QueryUser(inputReader *bufio.Reader, prompt string) string { for { From 5236a967cad5a87823edccaadbd1771d2b51ec73 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Tue, 18 Sep 2018 16:55:20 -0700 Subject: [PATCH 4/4] asset/store: log asset creation This makes it clear when and why a particular asset was created so it should help with debugging. The output of this looks as follows: INFO[0000] Fetching Bootstrap Ignition Config... DEBU[0000] Generating dependencies of Bootstrap Ignition Config... INFO[0000] Fetching Install Config... DEBU[0000] Generating dependencies of Install Config... INFO[0000] Fetching Cluster ID... DEBU[0000] Generating Cluster ID... INFO[0000] Fetching Email Address... DEBU[0000] Generating Email Address... --- pkg/asset/BUILD.bazel | 1 + pkg/asset/store.go | 24 ++++++++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/asset/BUILD.bazel b/pkg/asset/BUILD.bazel index 0365f70ddee..427ea835cc6 100644 --- a/pkg/asset/BUILD.bazel +++ b/pkg/asset/BUILD.bazel @@ -11,6 +11,7 @@ go_library( ], importpath = "github.com/openshift/installer/pkg/asset", visibility = ["//visibility:public"], + deps = ["//vendor/github.com/Sirupsen/logrus:go_default_library"], ) go_test( diff --git a/pkg/asset/store.go b/pkg/asset/store.go index 28583ab2885..0964b1d084a 100644 --- a/pkg/asset/store.go +++ b/pkg/asset/store.go @@ -1,5 +1,9 @@ package asset +import ( + "github.com/Sirupsen/logrus" +) + // Store is a store for the states of assets. type Store interface { // Fetch retrieves the state of the given asset, generating it and its @@ -15,19 +19,31 @@ type StoreImpl struct { // Fetch retrieves the state of the given asset, generating it and its // dependencies if necessary. func (s *StoreImpl) Fetch(asset Asset) (*State, error) { + return s.fetch(asset, "") +} + +func (s *StoreImpl) fetch(asset Asset, indent string) (*State, error) { + logrus.Infof("%sFetching %s...", indent, asset.Name()) state, ok := s.assets[asset] if ok { + logrus.Debugf("%sFound %s...", indent, asset.Name()) return state, nil } - dependies := asset.Dependencies() - dependenciesStates := make(map[Asset]*State, len(dependies)) - for _, d := range dependies { - ds, err := s.Fetch(d) + + dependencies := asset.Dependencies() + dependenciesStates := make(map[Asset]*State, len(dependencies)) + if len(dependencies) > 0 { + logrus.Debugf("%sGenerating dependencies of %s...", indent, asset.Name()) + } + for _, d := range dependencies { + ds, err := s.fetch(d, indent+" ") if err != nil { return nil, err } dependenciesStates[d] = ds } + + logrus.Debugf("%sGenerating %s...", indent, asset.Name()) state, err := asset.Generate(dependenciesStates) if err != nil { return nil, err