cctl upgrade machine #82
Conversation
cmd/machine.go
Outdated
|
|
||
| currentComponentVersions := getCurrentComponentVersions() | ||
|
|
||
| if cmp.Equal(oldMachineSpec.ComponentVersions, currentComponentVersions) { |
There was a problem hiding this comment.
Just a small comment, would it make sense to have a isUpgradeRequired sort of a method which takes old and current versions
There was a problem hiding this comment.
That's a better way to put it than the current implementation, thanks Puneet!
cmd/machine.go
Outdated
| upgradeRequired, upgrade := isUpgradeRequired(oldMachineSpec.ComponentVersions, currentComponentVersions) | ||
|
|
||
| if !upgradeRequired { | ||
| fmt.Printf("Machine is update to date\n") |
There was a problem hiding this comment.
nit: typo, I believe you meant "Machine is up to date"?
There was a problem hiding this comment.
Thanks for catching this!
cmd/machine.go
Outdated
| // and create a new one with the same specs as the old one | ||
| createMachine(ip, oldProvisionedMachine.Spec.SSHConfig.Port, oldProvisionedMachine.Spec.VIPNetworkInterface, | ||
| role, oldProvisionedMachine.Spec.SSHConfig.PublicKeys) | ||
| return |
There was a problem hiding this comment.
Add a note that the specified machine upgraded successfully?
cmd/machine.go
Outdated
| if err := state.PullFromAPIs(); err != nil { | ||
| log.Fatalf("Unable to sync on-disk state: %v", err) | ||
| } | ||
| return |
There was a problem hiding this comment.
Same here: Add a note that the specified machine upgraded successfully?
|
@dlipovetsky I've updated the diff to use log instead of fmt |
cmd/machine.go
Outdated
| deleteMachine(ip) | ||
| role := string(oldMachineSpec.Roles[0]) | ||
| // and create a new one with the same specs as the old one | ||
| createMachine(ip, oldProvisionedMachine.Spec.SSHConfig.Port, oldProvisionedMachine.Spec.VIPNetworkInterface, |
There was a problem hiding this comment.
Let's suppose the user runs cctl machine upgrade, and deleteMachine succeeds, but createMachine fails. That means the machine has been removed from the on-disk state, and cctl machine create must be run.
@puneetguptanitj If you the observation has merit, can you please create an issue?
There was a problem hiding this comment.
I think in this case we can make this less bothersome by checking if createMachine succeeded and if not we can log the details that we use to create the machine.
I'm wondering if a retry would make sense.
There was a problem hiding this comment.
I think we may have to address this separately. Filed #93.
|
|
||
| upgradeRequired, upgrade := isUpgradeRequired(oldMachineSpec.ComponentVersions, currentComponentVersions) | ||
|
|
||
| if !upgradeRequired { |
There was a problem hiding this comment.
Not sure this name makes sense. If upgradeRequired is true, but only the nodeadm/etcdadm versions have changed, then an upgrade is not required--only the state file needs to be updated.
Given that one of the conditional blocks (on L678 and L692) will execute, I think upgradeRequired might not be necessary. The lines on L673-674 could run after the two conditional blocks above.
There was a problem hiding this comment.
I guess in this context I consider any change to be an upgrade even if only a state file change is required. But I see your point though. This could be better named.
|
@vannrt Thanks for changing to |
If any of the components except for nodeadm were updated, trigger an upgrade. A nodeadm version change does not require an actuator call. The ssh-provider actuator will take care of ensuring that the correct version is invoked.
…ere is only an etcdadm update
This reverts commit ba610ae.
vannrt
force-pushed
the
private/vteves/cctl-upgrade-machine
branch
from
87e9bca to
fdf51ab
Compare
| upgrade.KeepalivedVersion || | ||
| upgrade.EtcdVersion { | ||
| // delete machine | ||
| deleteMachine(ip, false, false) |
There was a problem hiding this comment.
@dlipovetsky What you do think about this? deleteMachine here is called with force=False and skipDrainDelete=False.
There was a problem hiding this comment.
Is this with respect to resolving the merge conflict? If so, LGTM.
There was a problem hiding this comment.
Ok sounds good. I was going to add if setting them to False was a good choice but I saw that the default for both options is False so I think we're good. Thanks!
If any of the components except for nodeadm/etcadm were updated, trigger an
upgrade.
A nodeadm/etcdadm version change does not require an actuator call. The
ssh-provider actuator will take care of ensuring that the correct
version is invoked.