-
Notifications
You must be signed in to change notification settings - Fork 93
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
Log install preflight errors/warnings in both the CLI and kotsadm pod #4046
Changes from 10 commits
1cdacaa
d72c667
2f05a48
4450d4a
2c2132d
90917d4
bf41bf3
23e9bcb
ea089ab
65bb82a
2b3501b
cadc62a
2924058
c1f3cfe
43bdb66
ca22ad8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,6 @@ import ( | |
"encoding/json" | ||
"fmt" | ||
"io" | ||
"io/ioutil" | ||
"mime/multipart" | ||
"net/http" | ||
"net/url" | ||
|
@@ -33,6 +32,7 @@ import ( | |
"github.com/replicatedhq/kots/pkg/kurl" | ||
"github.com/replicatedhq/kots/pkg/logger" | ||
"github.com/replicatedhq/kots/pkg/metrics" | ||
"github.com/replicatedhq/kots/pkg/print" | ||
"github.com/replicatedhq/kots/pkg/pull" | ||
"github.com/replicatedhq/kots/pkg/replicatedapp" | ||
"github.com/replicatedhq/kots/pkg/store/kotsstore" | ||
|
@@ -47,6 +47,10 @@ import ( | |
"k8s.io/client-go/kubernetes" | ||
) | ||
|
||
var client = &http.Client{ | ||
Timeout: time.Second * 30, | ||
} | ||
|
||
func InstallCmd() *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "install [upstream uri]", | ||
|
@@ -321,7 +325,7 @@ func InstallCmd() *cobra.Command { | |
|
||
log.ActionWithoutSpinner("Extracting airgap bundle") | ||
|
||
airgapRootDir, err := ioutil.TempDir("", "kotsadm-airgap") | ||
airgapRootDir, err := os.MkdirTemp("", "kotsadm-airgap") | ||
if err != nil { | ||
return errors.Wrap(err, "failed to create temp dir") | ||
} | ||
|
@@ -426,6 +430,7 @@ func InstallCmd() *cobra.Command { | |
case err := <-errChan: | ||
if err != nil { | ||
log.Error(err) | ||
// TODO: Why is this a negative exit code? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question... I think this end up resulting in a 255 exit code:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's fix this and set it to 255 |
||
os.Exit(-1) | ||
} | ||
case <-stopCh: | ||
|
@@ -445,10 +450,15 @@ func InstallCmd() *cobra.Command { | |
case storetypes.VersionPendingPreflight: | ||
log.ActionWithSpinner("Waiting for preflight checks to complete") | ||
if err := ValidatePreflightStatus(deployOptions, authSlug, apiEndpoint); err != nil { | ||
log.FinishSpinnerWithError() | ||
return errors.Wrap(err, "failed to validate preflight results") | ||
log.FinishSpinner() | ||
banjoh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
log.Errorf("Preflight checks contain warnings or errors. The application was not deployed") | ||
perr := preflightError{} | ||
if errors.As(err, &perr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any benefit to handling specific error types this way vs. how do it elsewhere in the codebase? For example: https://github.com/replicatedhq/kots/blob/main/pkg/handlers/snapshots.go#L202 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
They both achieve the same result I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if they achieve the same thing, let's just stick to |
||
print.PreflightErrors(log, perr.Results) | ||
os.Exit(1) | ||
} | ||
return err | ||
} | ||
log.FinishSpinner() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should leave this as we need to stop the spinner in the case where preflights pass too. Otherwise we are left with an indefinite spinner:
|
||
case storetypes.VersionPendingConfig: | ||
log.ActionWithoutSpinnerWarning("Additional app configuration is required. Please login to the Admin Console to continue", nil) | ||
} | ||
|
@@ -692,7 +702,7 @@ func getIngressConfig(v *viper.Viper) (*kotsv1beta1.IngressConfig, error) { | |
|
||
ingressConfig := kotsv1beta1.IngressConfig{} | ||
if ingressConfigPath != "" { | ||
content, err := ioutil.ReadFile(ingressConfigPath) | ||
content, err := os.ReadFile(ingressConfigPath) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to read ingress service config file") | ||
} | ||
|
@@ -719,7 +729,7 @@ func getIdentityConfig(v *viper.Viper) (*kotsv1beta1.IdentityConfig, error) { | |
|
||
identityConfig := kotsv1beta1.IdentityConfig{} | ||
if identityConfigPath != "" { | ||
content, err := ioutil.ReadFile(identityConfigPath) | ||
content, err := os.ReadFile(identityConfigPath) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to read identity service config file") | ||
} | ||
|
@@ -910,7 +920,7 @@ func getAutomatedInstallStatus(url string, authSlug string) (*kotsstore.TaskStat | |
return nil, errors.Errorf("unexpected status code %d", resp.StatusCode) | ||
} | ||
|
||
b, err := ioutil.ReadAll(resp.Body) | ||
b, err := io.ReadAll(resp.Body) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to read response body") | ||
} | ||
|
@@ -964,7 +974,7 @@ func getPreflightResponse(url string, authSlug string) (*handlers.GetPreflightRe | |
newReq.Header.Add("Content-Type", "application/json") | ||
newReq.Header.Add("Authorization", authSlug) | ||
|
||
resp, err := http.DefaultClient.Do(newReq) | ||
resp, err := client.Do(newReq) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to execute request") | ||
} | ||
|
@@ -974,7 +984,7 @@ func getPreflightResponse(url string, authSlug string) (*handlers.GetPreflightRe | |
return nil, errors.Errorf("unexpected status code %d", resp.StatusCode) | ||
} | ||
|
||
b, err := ioutil.ReadAll(resp.Body) | ||
b, err := io.ReadAll(resp.Body) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to read response body") | ||
} | ||
|
@@ -1005,6 +1015,17 @@ func checkPreflightsComplete(response *handlers.GetPreflightResultResponse) (boo | |
return true, nil | ||
} | ||
|
||
type preflightError struct { | ||
Msg string | ||
Results []*preflight.UploadPreflightResult | ||
} | ||
|
||
func (e preflightError) Error() string { | ||
return e.Msg | ||
} | ||
|
||
func (e preflightError) Unwrap() error { return fmt.Errorf(e.Msg) } | ||
|
||
func checkPreflightResults(response *handlers.GetPreflightResultResponse) (bool, error) { | ||
if response.PreflightResult.Result == "" { | ||
return false, nil | ||
|
@@ -1027,14 +1048,23 @@ func checkPreflightResults(response *handlers.GetPreflightResultResponse) (bool, | |
} | ||
|
||
if isWarn && isFail { | ||
return false, errors.New("There are preflight check failures and warnings for the application. The app was not deployed.") | ||
return false, preflightError{ | ||
Msg: "There are preflight check failures and warnings for the application. The app was not deployed.", | ||
Results: results.Results, | ||
} | ||
} | ||
|
||
if isWarn { | ||
return false, errors.New("There are preflight check warnings for the application. The app was not deployed.") | ||
return false, preflightError{ | ||
Msg: "There are preflight check warnings for the application. The app was not deployed.", | ||
Results: results.Results, | ||
} | ||
} | ||
if isFail { | ||
return false, errors.New("There are preflight check failures for the application. The app was not deployed.") | ||
return false, preflightError{ | ||
Msg: "There are preflight check failures for the application. The app was not deployed.", | ||
Results: results.Results, | ||
} | ||
} | ||
|
||
return true, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ func setPreflightResult(appID string, sequence int64, preflightResults *types.Pr | |
// execute will execute the preflights using spec in preflightSpec. | ||
// This spec should be rendered, no template functions remaining | ||
func execute(appID string, sequence int64, preflightSpec *troubleshootv1beta2.Preflight, ignorePermissionErrors bool) (*types.PreflightResults, error) { | ||
logger.Debug("executing preflight checks", | ||
logger.Info("executing preflight checks", | ||
zap.String("appID", appID), | ||
zap.Int64("sequence", sequence)) | ||
|
||
|
@@ -65,7 +65,12 @@ func execute(appID string, sequence int64, preflightSpec *troubleshootv1beta2.Pr | |
if err, ok := msg.(error); ok { | ||
logger.Errorf("error while running preflights: %v", err) | ||
} else { | ||
logger.Infof("preflight progress: %v", msg) | ||
switch m := msg.(type) { | ||
case preflight.CollectProgress: | ||
logger.Infof("preflight progress: %s", m.String()) | ||
default: | ||
logger.Infof("preflight progress: %+v", msg) | ||
} | ||
} | ||
|
||
progress, ok := msg.(preflight.CollectProgress) | ||
|
@@ -121,7 +126,7 @@ func execute(appID string, sequence int64, preflightSpec *troubleshootv1beta2.Pr | |
KubernetesRestConfig: restConfig, | ||
} | ||
|
||
logger.Debug("preflight collect phase") | ||
logger.Info("preflight collect phase") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this to info cause they show progress when performing an installation, which is critical functionality IMO to always log |
||
collectResults, err := troubleshootpreflight.Collect(collectOpts, preflightSpec) | ||
if err != nil && !isPermissionsError(err) { | ||
preflightRunError = err | ||
|
@@ -147,7 +152,7 @@ func execute(appID string, sequence int64, preflightSpec *troubleshootv1beta2.Pr | |
} | ||
uploadPreflightResults.Errors = rbacErrors | ||
} else { | ||
logger.Debug("preflight analyze phase") | ||
logger.Info("preflight analyze phase") | ||
analyzeResults := collectResults.Analyze() | ||
|
||
// the typescript api added some flair to this result | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -160,13 +160,26 @@ func Run(appID string, appSlug string, sequence int64, isAirgap bool, archiveDir | |
preflight.Spec.Collectors = collectors | ||
|
||
go func() { | ||
logger.Debug("preflight checks beginning") | ||
logger.Info("preflight checks beginning") | ||
uploadPreflightResults, err := execute(appID, sequence, preflight, ignoreRBAC) | ||
if err != nil { | ||
logger.Error(errors.Wrap(err, "failed to run preflight checks")) | ||
return | ||
} | ||
logger.Debug("preflight checks completed") | ||
|
||
// Log the preflight results if there are any warnings or errors | ||
// The app may not get installed so we need to see this info for debugging | ||
if GetPreflightState(uploadPreflightResults) != "pass" { | ||
logger.Warnf("Preflight checks completed with warnings or errors. The application will not get deployed") | ||
for _, result := range uploadPreflightResults.Results { | ||
if result == nil { | ||
continue | ||
} | ||
logger.Infof("preflight state=%s title=%q message=%q", preflightState(*result), result.Title, result.Message) | ||
} | ||
} else { | ||
logger.Info("preflight checks completed") | ||
} | ||
|
||
go func() { | ||
err := reporting.GetReporter().SubmitAppInfo(appID) // send app and preflight info when preflights finish | ||
|
@@ -216,6 +229,21 @@ func Run(appID string, appSlug string, sequence int64, isAirgap bool, archiveDir | |
return nil | ||
} | ||
|
||
func preflightState(p troubleshootpreflight.UploadPreflightResult) string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The state from |
||
if p.IsFail { | ||
return "fail" | ||
} | ||
|
||
if p.IsWarn { | ||
return "warn" | ||
} | ||
|
||
if p.IsPass { | ||
return "pass" | ||
} | ||
return "unknown" | ||
} | ||
|
||
// maybeDeployFirstVersion will deploy the first version if preflight checks pass | ||
func maybeDeployFirstVersion(appID string, sequence int64, preflightResults *types.PreflightResults) (bool, error) { | ||
if sequence != 0 { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
configtypes "github.com/replicatedhq/kots/pkg/kotsadmconfig/types" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/replicatedhq/kots/pkg/logger" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/replicatedhq/troubleshoot/pkg/preflight" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
func ConfigValidationErrors(log *logger.CLILogger, groupValidationErrors []configtypes.ConfigGroupValidationError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -26,3 +27,26 @@ func ConfigValidationErrors(log *logger.CLILogger, groupValidationErrors []confi | |||||||||||||||||||||||||||||||||||||||||||||||||||
log.FinishSpinnerWithError() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
log.Errorf(sb.String()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
func PreflightErrors(log *logger.CLILogger, results []*preflight.UploadPreflightResult) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not following the existing pattern of printing? Lines 25 to 49 in bc8004b
that would help align the column titles with the values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No particular reason. I'll try to adopt the same approach |
||||||||||||||||||||||||||||||||||||||||||||||||||||
var s strings.Builder | ||||||||||||||||||||||||||||||||||||||||||||||||||||
s.WriteString("\nPreflight check results (state - title - message)") | ||||||||||||||||||||||||||||||||||||||||||||||||||||
for _, result := range results { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
s.WriteString(fmt.Sprintf("\n%s - %q - %q", preflightState(result), result.Title, result.Message)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
log.Info(s.String()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
func preflightState(r *preflight.UploadPreflightResult) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll DRY this out in favour of |
||||||||||||||||||||||||||||||||||||||||||||||||||||
if r.IsFail { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
return "FAIL" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if r.IsWarn { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
return "WARN" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
if r.IsPass { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
return "PASS" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
// We should never get here | ||||||||||||||||||||||||||||||||||||||||||||||||||||
return "UNKNOWN" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, there was no timeout. Just being defensive here.