Removed os.Exec'ing code #2961
Removed os.Exec'ing code #2961
Conversation
| // Connect to the service manager | ||
| m, err := mgr.Connect() | ||
| if err != nil { | ||
| return fmt.Errorf("could not connect to service manager: %v", err) |
| for status.State != svc.Stopped { | ||
| time.Sleep(500 * time.Millisecond) | ||
| status, err = service.Query() | ||
| if err != nil { | ||
| return fmt.Errorf("could not query service status: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This should consult a context.Context for cancelation so that it will not run indefinitely.
| @@ -0,0 +1,46 @@ | |||
| //go:build windows | |||
| // +build windows | |||
There was a problem hiding this comment.
This module requires at least Go 1.18 to build, so we no longer need to add // +build windows. The //go:build pragma is the only one needed.
| key registry.Key | ||
| } | ||
|
|
||
| func (r *WindowsRegistry) OpenKey(k registry.Key, path string, access uint32) (RegistryKey, error) { |
There was a problem hiding this comment.
k is unused here--I don't think that's intentional. Granted, we're probably always going to use HKLM, but who knows? If we're making a generic wrapper, I'd like to be able to, say, edit some keys in HKCU.
There was a problem hiding this comment.
as someone with no understanding of win reg, you could be making these up and I would have no idea 😂
There was a problem hiding this comment.
@rbtr this is cursed knowledge from working summers at my high school playing student sysadmin :-P The Norton Ghosts still haunt me.
| value, valType, err := k.key.GetStringValue(name) | ||
| return value, valType, err |
There was a problem hiding this comment.
This simplifies to
| value, valType, err := k.key.GetStringValue(name) | |
| return value, valType, err | |
| return k.key.GetStringValue(name) |
| func (k *MockRegistryKey) Close() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Would it be incorrect to not close a key? It might be worth adding some bookkeeping in here to make sure it's getting called if so.
| // OpenKey opens a mock registry key. | ||
| func (r *MockRegistry) OpenKey(k registry.Key, path string, access uint32) (RegistryKey, error) { | ||
| // Directly check if the key exists in the mock registry by its path | ||
| if key, exists := r.Keys[path]; exists { |
There was a problem hiding this comment.
nit: Traditionally, the Go community writes the second parameter here as ok.
| if key, exists := r.Keys[path]; exists { | |
| if key, ok := r.Keys[path]; ok { |
| // mockExecClient.SetPowershellCommandResponder(func(_ string) (string, error) { | ||
| // return "False", nil | ||
| // }) |
There was a problem hiding this comment.
Feel free to remove these. Dead code can be found and resurrected with git if necessary.
rbtr
added
cns
|
while i'm thinking about it we'll probably want to backport this to 1.4/1.5 since those are still a substantial install base |
|
This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days |
Reason for Change:
There was a CRI recently where it appears that when CNS is shelling out to PowerShell, spooling up the whole PowerShell env is quite heavy; thus, we are OOMing the CNS pod. There could be other contributing factors that haven't been identified yet as well.
Issue Fixed:
This pr gets rid of os.Exec'ing a shell (creating a whole new instance of PowerShell) each time and instead replacing the code with calling into windows registry.
Requirements:
Notes:
Work Item: https://msazure.visualstudio.com/One/_workitems/edit/29254102