Update refactor and Compare versions for validating update #56
Conversation
|
Keeping It draft until dependent PR in cron-updater branch is merged. |
srgothi92
force-pushed
the
cron-updater
branch
5 times, most recently
from
06f240d to
f920185
Compare
srgothi92
force-pushed
the
update-refactor
branch
from
e13de2d to
59dc47e
Compare
srgothi92
force-pushed
the
update-refactor
branch
from
59dc47e to
ec07b3d
Compare
srgothi92
force-pushed
the
update-refactor
branch
from
ec07b3d to
467cf93
Compare
updater/aws.go
Outdated
| // updateInstance starts an update process on an instance, and changes instance | ||
| // state from DRAINING to ACTIVE in case of success or failure. |
There was a problem hiding this comment.
I think it'd be a bit easier to reason about if we pulled the activateInstance bits out of this function and back into the loop in main.go. In all cases we want to reactivate the instance; we don't want to leave it in DRAINING indefinitely. What we can do in main.go is, after reactivating, decide whether or not to continue in the loop based on whether instances are updating successfully or not. And that way we can distinguish between an error in doing the update and an error in reactivating.
Something like this:
err = u.updateInstance(i)
activateErr := u.activateInstance(i)
if err != nil {
// return? continue?
}
if activateErr != nil {
// probably return here
}An alternative would be to have the reactivation happen in a defer inside updateInstance (now that it's extracted to a function this becomes a reasonable option, but that would mean that the caller of updateInstance would lose the ability to determine the difference between an update failure and a reactivation failure.
There was a problem hiding this comment.
defer would have been nice, but it was hard to defer something that can result in an error and making sure all the errors are properly logged. I followed your first suggestion with slight change in handling error. Let me know if it looks okay.
srgothi92
force-pushed
the
update-refactor
branch
from
467cf93 to
3817f66
Compare
updater/aws.go
Outdated
| updatedVersion := output.ActivePartition.Image.Version | ||
| if updatedVersion == inst.bottlerocketVersion { | ||
| log.Printf("Container instance %q did not update, its current "+ | ||
| "version %s and updated version %s are same", inst.containerInstanceID, inst.bottlerocketVersion, updatedVersion) |
There was a problem hiding this comment.
nit: missing "the" in the print statement: "version %s and updated version %s are the same"
srgothi92
force-pushed
the
update-refactor
branch
from
3817f66 to
ece94f7
Compare
updater/main.go
Outdated
| } | ||
| updateState, err := isUpdateAvailable(updateResult) | ||
| if err != nil { | ||
| log.Printf("Unable to determine update result. Manual verification of %#q required", i) | ||
| if updateErr != nil { |
There was a problem hiding this comment.
nit: I'd either do these three as a series of if/else if statements or as cases in a switch. Each conditional ends up being mutually exclusive with all the others.
There was a problem hiding this comment.
changed to if/else if blocks
Previously, there were separate methods for getting active version and UpdateState. However in all the cases, we need both the fields together. So, this change adds a method to parse the raw bytes of command output and extract all the fields required in a struct
Previously version before update was not stored, so it was hard to verify if update happened or not. This change stores the version and compares it with updated version to report approiate logs.
srgothi92
force-pushed
the
update-refactor
branch
from
ece94f7 to
b6eb22b
Compare
Issue number:
#48 , Partial #40
Description of changes:
This PR includes 3 commits:
Refactor logs for do update and verify update step
Refactor parsing check command output
Testing done:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.