320 automated ami upgrades + upgrade locking #327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
- Coverage 55.87% 51.08% -4.79%
==========================================
Files 27 33 +6
Lines 4041 4504 +463
==========================================
+ Hits 2258 2301 +43
- Misses 1640 2062 +422
+ Partials 143 141 -2
Continue to review full report at Codecov.
|
|
scenarios tested:
|
|
Thanks for the PR @preflightsiren ! I think it would have less implications if we only avoid upgrades (e.g. a condition check before triggering upgrades) vs. a reconcile state. Otherwise we need to consider all other states and transitions between them from Locked state. |
|
Hey mate, thanks for taking a look. From observation each change to the IG sets the state back to I would want a In its current form it only locks updates and upgrades - although currently implemented it's not a great experience with creates/deletes. Next steps are to address the create/delete flows for locked IGs, and implement the imageID lookup. |
|
Just looking through https://docs.aws.amazon.com/systems-manager/latest/userguide/parameter-store-public-parameters-eks.html The ssm GetParameter response contains What do you think? |
|
success with multiple OS's |
|
Updating a locked instance group: |
|
@preflightsiren thanks, I am OOO until mid next week - will review as soon as I can. Suggest in the meanwhile you consider adding a functional test? Or at least make sure they are passing considering this is a fairly large change. |
|
Good feedback; haven't had a chance yet, but I'll add functional tests, and I'll look at providing a ssm parameter path override annotation as well |
|
woo, got the BDD tests running, all passed: |
preflightsiren
force-pushed
the
preflightsiren-latest-ami-update
branch
from
a69fc23
to
a9cde7a
Compare
|
Added BDD tests for locking the group |
now includes tests for locking and latest. |
| return false | ||
| } | ||
|
|
||
| func HasAnnotationWithValue(annotations map[string]string, key, value string) bool { |
There was a problem hiding this comment.
I renamed this method as this tests the value of the annotation, not just that it exists - happy for suggestions for method names :)
| annotations := instanceGroup.GetAnnotations() | ||
|
|
||
| var OSFamily string | ||
| if kubeprovider.HasAnnotation(annotations, OsFamilyAnnotation) { |
There was a problem hiding this comment.
previous version, now HasAnnotationWithValue caused some unexpected logic problems; I've added a new method to just detect the presence of a key in the annotation
preflightsiren
changed the title
preflightsiren
force-pushed
the
preflightsiren-latest-ami-update
branch
2 times, most recently
from
a5a0b85
to
a78d23b
Compare
|
@eytan-avisror @backjo this is ready for final review; I want to finish this off and get it ready to merge before improving/expanding it. My thoughts for next steps are:
|
|
Do you prefer to have commits squashed before merge? so I can give it a nice commit message - or would you prefer to do it once merging the PR? |
|
Not sure if we have a policy for the project - I definitely prefer squashed commits before merge personally :) |
| @@ -1173,3 +1173,40 @@ func (ctx *EksInstanceGroupContext) GetDesiredMixedInstancesPolicy(name string) | |||
|
|
|||
| return policy | |||
| } | |||
|
|
|||
| func FilterSupportedArch(architectures []string) string { | |||
There was a problem hiding this comment.
Is FilterSupportedArch only meant to check if a slice of architectures is in slice of SupportedArchitectures ?
Could we just use common.StringSliceContains, or a different generic function?
In the below code it seems we return the first architecture that we find a match for, is there a condition where there are multiple supported architectures?
There was a problem hiding this comment.
FilterSupportedArch returns the first match between the SupportedArchitectures
from my investigation of the aws api most instance types either return
"arch": [
"i386",
"x86_64"
]
or
"arch": [
"arm64"
]
since i386 isn't a supported arch, common.StringSliceContains would fail. Happy to use a different generic if one exists. If ever amazon provides an instance with multiple supported architectures, we would either need to explain the preference order, or provide some way of preferencing one.
|
This is looking pretty good to me, @backjo do you have any additional comments? |
Just the doc updates for SSM permissions - that's all that's left on my end |
|
Cheers, I have that done locally, just trying to bump up the codecov - should have this finalised shortly. |
preflightsiren
force-pushed
the
preflightsiren-latest-ami-update
branch
from
a78d23b
to
de62999
Compare
|
I've taken another look at the codecov, I don't get where the -5% comes from or where to expand our test coverage. If someone could help point it out I'll happily add in the testing functions. |
Automated AMI management InstanceGroups can now set the image configuration to "latest". This will result the ami value being retrieved from a ssm parameter (https://docs.aws.amazon.com/eks/latest/userguide/retrieve-ami-id.html). This will ensure that nodes within an InstanceGroup are kept up-to-date and is especially useful in development clusters. Automated AMI management supports retrieving amazon amis for amazon linux 2, bottlerocket and windows nodes. This can be configured using the annotation `instancemgr.keikoproj.io/os-family`. Upgrade locking InstanceGroups can now set an annotation `instancemgr.keikoproj.io/lock-upgrades="true"` which will prevent the InstanceGroup from entering the InitUpgrade state. This is useful for controlling when the nodes of an InstanceGroup can be upgraded, pairing well with the automated AMI management feature. Resolves: 320 Signed-off-by: Sebastian Cole <[email protected]>
preflightsiren
force-pushed
the
preflightsiren-latest-ami-update
branch
from
de62999
to
21d9792
Compare
|
@preflightsiren BDD has been failing with: I think you forgot to check in the YAML files for BDD, can you take a look? |
|
I'll check tonight. |
|
@eytan-avisror can you try running the BDD tests with this patch #334 , I'm unable to run the tests at the moment to verify myself, but I'm confident this is the thing missing. |
Fixes #320