Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: guard cast of launchtemplate with type check #254

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

backjo
Copy link
Collaborator

@backjo backjo commented Feb 12, 2021

Closes #253

Signed-off-by: Jonah Back [email protected]

@backjo backjo requested review from a team as code owners February 12, 2021 00:20
@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #254 (5a4377a) into master (010bc48) will decrease coverage by 0.06%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   87.82%   87.76%   -0.07%     
==========================================
  Files          17       17              
  Lines        2251     2255       +4     
==========================================
+ Hits         1977     1979       +2     
- Misses        171      173       +2     
  Partials      103      103              
Impacted Files Coverage Δ
controllers/provisioners/eks/update.go 82.03% <71.42%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 010bc48...5a4377a. Read the comment docs.


status.SetActiveLaunchTemplateName(configName)
status.SetLatestTemplateVersion(latestVersionString)
switch scalingConfigType := (*scalingConfig).(type) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in this case spec.IsLaunchTemplate() is true because spec has changed to LaunchTemplate, but scalingConfig is not yet created? what does that mean for the rolling upgrade?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more that the scalingConfig exists, but it's of type *scaling.LaunchConfig.

Just tested the rolling upgrade out - there's one small issue in that two CRs get submitted (with the default concurrency policy). One like rollup-vault-20210208201848-0, and another like rollup-vault-20210208201848-1. This is probably due to our defaulting the LaunchTemplate version to '0' if it's not set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably just default it to 1 in the case that scalingConfig is of type *scaling.LaunchConfig

Copy link
Collaborator

@eytan-avisror eytan-avisror Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when we go from LaunchTemplate back to LaunchConfig?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question - I haven't tried that case :) - though I don't think this PR would change it, since this is only running if type==LaunchTemplate. Let me check real quick though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LC -> LT works fine

@eytan-avisror eytan-avisror merged commit 6773818 into keikoproj:master Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade from LaunchConfig -> LaunchTemplate causes Segfault
2 participants