fleetctl: {load|unload|start|stop} and get unit code cleanups #1433
Conversation
| @@ -482,6 +482,36 @@ func getChecker() *ssh.HostKeyChecker { | |||
| return ssh.NewHostKeyChecker(keyFile) | |||
| } | |||
|
|
|||
| func getUnit(file string) (*unit.UnitFile, error) { | |||
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
from
c181aad
to
de05a8d
Compare
|
PR updated, thanks |
| // returned then try up to that value | ||
| func getBlockAttempts() int { | ||
| // By default we wait forever | ||
| var attempts int = -1 |
There was a problem hiding this comment.
Where do you do anything with this being set to -1?
There was a problem hiding this comment.
Yes I set it to negative value to not set it to zero, this way I can return immediately here endocode@75b6088#diff-197bb752bf0f52567eb6b0023ea65e40R782
And lalso to allow us to continue with the previous logic inside "waitForUnitStates()" 0 or any negative value. I didn't touch "waitForUnitStates()" since it's used in some other parts of the code, so I only converted new calls keeping the same semantics from a user experience. Yes the problem is we have two values inside the code blockattempts and noblock that interfere which other that's why I think we should convert this into one value that allows us to be more consistent in each part of the code. Perhaps later other parts can be converted too, but now I only changed fleetctl commands.
There was a problem hiding this comment.
I was just a bit confused about the inconsistency between BlockAttempts=0 on the command line referring to waiting forever, but getBlockAttempts() returning -1 to indicate the same.
There was a problem hiding this comment.
i.e. implicitly relying on https://github.com/coreos/fleet/pull/1433/files#diff-197bb752bf0f52567eb6b0023ea65e40R861 catching both cases. Maybe instead we could standardise on blockattempts=-1 == non blocking and change the check aat https://github.com/coreos/fleet/pull/1433/files#diff-197bb752bf0f52567eb6b0023ea65e40R817
There was a problem hiding this comment.
Yes exactly relying on that one: https://github.com/coreos/fleet/pull/1433/files#diff-197bb752bf0f52567eb6b0023ea65e40R861 to catch both of them.
For your comment "Maybe instead we could standardise on blockattempts=-1 == non blocking and change the check aat https://github.com/coreos/fleet/pull/1433/files#diff-197bb752bf0f52567eb6b0023ea65e40R817" yes I don't have a strong opinion here, I'm just used to negative values meaning retry or block forever for polling functions. Ok will do that change then, thanks!
|
The functional tests pass. Thanks! |
| if uni == nil { | ||
| return nil, fmt.Errorf("error extracting information from unit name %s", name) | ||
| } else if !uni.IsInstance() { | ||
| return nil, fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", name) |
There was a problem hiding this comment.
this error (and that on line 559) are actually bleeding into parent scope - this function is only checking for templates but the error is referencing filesystem looks (which just implicitly happen to have happened and failed before we're called in this scope). Need to clean this up somehow, not sure off the top of my head
There was a problem hiding this comment.
Hmm, ok so I got you a bit on this one, but yeh not sure what to do, certainly removing errors was not an option for me, so ?!
There was a problem hiding this comment.
It's less than ideal, but you could create some typed errors like errNoTemplateFoundInRegistry , and return them from this function, then do a switch on the error from the outer scope, and assemble the error message there as appropriate.
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
from
75dcaa0
to
e8a80b5
Compare
| func TestGetBlockAttempts(t *testing.T) { | ||
| oldNoBlock := sharedFlags.NoBlock | ||
| oldBlockAttempts := sharedFlags.BlockAttempts | ||
| sharedFlags.NoBlock = true |
There was a problem hiding this comment.
Please turn this into a test table.
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
from
d74b89e
to
cab3099
Compare
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
2 times, most recently
from
2b5edff
to
a3703b3
Compare
|
@jonboulle Investigating why semaphore fails, all comments were handled for this one and all functional tests pass here.... but semaphore fails for another error I guess... |
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
2 times, most recently
from
e74ccba
to
2b3951d
Compare
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
from
2b3951d
to
9581bb6
Compare
* tryWaitForUnitStates() tries to wait for units to reach the desired state. * getBlockAttempts() gets the correct value of how many attempts to try before giving up on an operation. These helpers will be used to make the code more consistent and clean. We do not intended to change any behaviour here.
… getBlockAttempts()
…ileFromTemplate()
Improve code comments about getUnitFileFromTemplate() and kill some other useless code comments.
tixxdz
force-pushed
the
tixxdz/fleet-code-cleaning-v1+1
branch
from
9581bb6
to
ae4b987
Compare
fleetctl: {load|unload|start|stop} and get unit code cleanups
This patches set aims to make the code more consistent and robust. It does not change the current behaviour just improves the code, consolidate it, more debug logs and code comments.
The main goal is to let stop, start, load and unload to share the same code base and probably avoid any corner case or bug that may related to one of these commands.