-
Notifications
You must be signed in to change notification settings - Fork 24
fix: lme timeout fix optimization #1220
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -73,6 +73,8 @@ func (cmd *ActivateCmd) Validate() error { | |
| log.Trace("Entering Validate method of ActivateCmd") | ||
|
|
||
| // Determine if caller intends local activation (explicit --local or local-only flags) | ||
| cmd.URL = normalizeActivateURL(cmd.URL) | ||
|
|
||
| localIntent := cmd.Local || cmd.hasLocalActivationFlags() | ||
|
|
||
| // Resolve local-vs-remote precedence when both are present. | ||
|
|
@@ -81,7 +83,6 @@ func (cmd *ActivateCmd) Validate() error { | |
| if localIntent && cmd.URL != "" { | ||
| lowerURL := strings.ToLower(cmd.URL) | ||
| if strings.HasPrefix(lowerURL, "http://") || strings.HasPrefix(lowerURL, "https://") { | ||
| log.Warn("Both --url and local activation flags detected; proceeding with local activation via http://") | ||
| // Clear URL so we don't trigger HTTP profile fullflow during local runs (prevents recursion) | ||
| cmd.URL = "" | ||
| } | ||
|
|
@@ -201,6 +202,25 @@ func (cmd *ActivateCmd) hasLocalActivationFlags() bool { | |
| cmd.ProvisioningCert != "" || cmd.ProvisioningCertPwd != "" || cmd.SkipIPRenew | ||
| } | ||
|
|
||
| func normalizeActivateURL(raw string) string { | ||
| value := strings.TrimSpace(raw) | ||
| if value == "" { | ||
| return "" | ||
| } | ||
|
|
||
| u, err := url.Parse(value) | ||
| if err != nil { | ||
| return value | ||
| } | ||
|
|
||
| scheme := strings.ToLower(u.Scheme) | ||
| if (scheme == "http" || scheme == "https" || scheme == "ws" || scheme == "wss") && u.Host == "" { | ||
| return "" | ||
| } | ||
|
|
||
| return value | ||
| } | ||
|
|
||
| // Run executes the activate command based on detected mode | ||
| func (cmd *ActivateCmd) Run(ctx *commands.Context) error { | ||
| log.Tracef("Entering Run method of ActivateCmd. Context: %s", ctx.AuthEndpoint) | ||
|
|
@@ -210,10 +230,9 @@ func (cmd *ActivateCmd) Run(ctx *commands.Context) error { | |
| if err := cmd.EnsureAMTPassword(ctx, cmd); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := cmd.EnsureWSMAN(ctx); err != nil { | ||
| return err | ||
| } | ||
| // Do not pre-create WSMAN for local activation here. | ||
| // LocalActivateCmd sets up its own local WSMAN transport, and doing both | ||
| // can trigger an extra LME/APF initialize cycle. | ||
|
Contributor
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. Move this content as part of the PR description
Contributor
Author
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. done |
||
| } | ||
| // Determine activation mode based on flags | ||
| if cmd.URL != "" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,9 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/amt/general" | ||
| "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/client" | ||
| "github.com/device-management-toolkit/rpc-go/v2/internal/certs" | ||
| "github.com/device-management-toolkit/rpc-go/v2/internal/commands" | ||
|
|
@@ -479,7 +481,7 @@ func (service *LocalActivationService) activateCCM() error { | |
|
|
||
| // Get general settings for digest realm | ||
|
|
||
| generalSettings, err := service.wsman.GetGeneralSettings() | ||
| generalSettings, err := service.getGeneralSettingsWithRetry() | ||
| if err != nil { | ||
| return utils.ActivationFailedGeneralSettings | ||
| } | ||
|
|
@@ -749,7 +751,7 @@ func (service *LocalActivationService) activateACMWithTLS(tlsConfig *tls.Config) | |
|
|
||
| // Get general settings to obtain digest realm for password hashing | ||
|
|
||
| generalSettings, err := service.wsman.GetGeneralSettings() | ||
| generalSettings, err := service.getGeneralSettingsWithRetry() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get AMT general settings: %w", err) | ||
| } | ||
|
|
@@ -969,7 +971,7 @@ func (service *LocalActivationService) activateACMLegacy(tlsConfig *tls.Config) | |
|
|
||
| // Get general settings for digest realm | ||
|
|
||
| generalSettings, err := service.wsman.GetGeneralSettings() | ||
| generalSettings, err := service.getGeneralSettingsWithRetry() | ||
| if err != nil { | ||
| return utils.ActivationFailedGeneralSettings | ||
| } | ||
|
|
@@ -1044,6 +1046,37 @@ func (service *LocalActivationService) handleSetupErrorWithControlModeVerificati | |
| return utils.ActivationFailedControlMode | ||
| } | ||
|
|
||
| // TODO: Move retry logic in wsman pkg | ||
| func (service *LocalActivationService) getGeneralSettingsWithRetry() (general.Response, error) { | ||
|
Contributor
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. Can this retry logic be implemented in the GoWSMAN package. Check this issue: device-management-toolkit/go-wsman-messages#656
Contributor
Author
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. yes, for now we can keep it, todo added |
||
| const maxRetries = 3 | ||
|
|
||
| var lastErr error | ||
|
|
||
| for attempt := 0; attempt <= maxRetries; attempt++ { | ||
| response, err := service.wsman.GetGeneralSettings() | ||
| if err == nil { | ||
| return response, nil | ||
| } | ||
|
|
||
| lastErr = err | ||
| errText := strings.ToLower(err.Error()) | ||
|
|
||
| transientBusy := strings.Contains(errText, "device or resource busy") || | ||
| strings.Contains(errText, "resource busy") || | ||
| strings.Contains(errText, "no such device") || | ||
| strings.Contains(errText, "device unavailable") | ||
| if !transientBusy || attempt == maxRetries { | ||
| break | ||
| } | ||
|
|
||
| delay := time.Duration(attempt+1) * time.Duration(utils.HeciConnectRetryBackoff) * time.Millisecond | ||
| log.WithError(err).Warnf("GetGeneralSettings busy, retrying (%d/%d)", attempt+1, maxRetries) | ||
| time.Sleep(delay) | ||
| } | ||
|
|
||
| return general.Response{}, lastErr | ||
| } | ||
|
|
||
| // Certificate handling methods for ACM activation | ||
|
|
||
| // convertPfxToObject converts a base64 PFX certificate to a CertsAndKeys object | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ import ( | |
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| const lmeAPFChannelDataFlushOverride = 500 * time.Millisecond | ||
|
|
||
| // LMConnection is struct for managing connection to LMS | ||
| type LMEConnection struct { | ||
| Command pthi.Command | ||
|
|
@@ -179,7 +181,7 @@ func (lme *LMEConnection) execute(bin_buf bytes.Buffer) error { | |
| return err | ||
| } | ||
|
|
||
| bin_buf = apf.Process(result, lme.Session) | ||
| bin_buf = lme.processWithLocalTimerOverride(result) | ||
|
Contributor
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. This change could you please explain as part of the PR description what was the behavior before and after
Contributor
Author
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. TODO added |
||
| if bin_buf.Len() == 0 { | ||
| log.Debug("done EXECUTING.........") | ||
|
|
||
|
|
@@ -244,7 +246,7 @@ func (lme *LMEConnection) Listen() { | |
|
|
||
| break | ||
| } else { | ||
| result := apf.Process(result2, lme.Session) | ||
| result := lme.processWithLocalTimerOverride(result2) | ||
| if result.Len() != 0 { | ||
| err2 = lme.execute(result) | ||
| if err2 != nil { | ||
|
|
@@ -257,6 +259,24 @@ func (lme *LMEConnection) Listen() { | |
| } | ||
| } | ||
|
|
||
| // TODO: Optimize/test changes if wsman pkg can handle it | ||
| func (lme *LMEConnection) processWithLocalTimerOverride(message []byte) bytes.Buffer { | ||
| processed := apf.Process(message, lme.Session) | ||
|
|
||
| if len(message) > 0 && message[0] == apf.APF_CHANNEL_DATA && lme.Session.Timer != nil { | ||
| if !lme.Session.Timer.Stop() { | ||
| select { | ||
| case <-lme.Session.Timer.C: | ||
| default: | ||
| } | ||
| } | ||
|
|
||
| lme.Session.Timer.Reset(lmeAPFChannelDataFlushOverride) | ||
| } | ||
|
|
||
| return processed | ||
| } | ||
|
|
||
| // Close closes the LME connection | ||
|
|
||
| func (lme *LMEConnection) Close() error { | ||
|
|
||
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.
The comment describing local-vs-remote precedence for HTTP(S) URLs doesn’t match the implemented behavior: the code clears an HTTP(S)
--urlwhen local intent is detected (local flags win), but the comment says to keep the URL and ignore local flags. Please update the comment (or the logic) so the documented precedence matches whatValidate()actually enforces.