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

feat(provider/aws): Fix lifecycle hooks racing condition #4082

Merged
merged 2 commits into from
Oct 10, 2019

Conversation

costimuraru
Copy link
Contributor

@costimuraru costimuraru commented Oct 8, 2019

Fixes spinnaker/spinnaker#5001

Below you can find some Deploy stage logs, which proves that lifecycle hooks are created before scaling out the server group:

{ 
   "phase":"AWS_DEPLOY",
   "status":"Beginning Amazon deployment."
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Beginning ASG deployment."
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Found ancestor server group, parsing details (name: testaws-stage-v009)"
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Deploying ASG: testaws-stage-v010"
},
{ 
   "phase":"AWS_DEPLOY",
   "status":" > Deploying to subnetIds: subnet-3d7dd13b,subnet-28b24012,subnet-5bc30543"
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Creating lifecycle hooks for: testaws-stage-v010"
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Creating lifecycle hook ({LifecycleHookName: testaws-stage-v010-lifecycle-8b39f839-6c5c-490c-ae3a-58518c1914b5,AutoScalingGroupName: testaws-stage-v010,LifecycleTransition: autoscaling:EC2_INSTANCE_LAUNCHING
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Creating lifecycle hook ({LifecycleHookName: testaws-stage-v010-lifecycle-08d2b7f7-d8a9-47dd-9157-7d86ee3a81d9,AutoScalingGroupName: testaws-stage-v010,LifecycleTransition: autoscaling:EC2_INSTANCE_TERMINATING
},
{ 
   "phase":"AWS_DEPLOY",
   "status":"Setting size of testaws-stage-v010 in aam-npe/us-east-1 to [min=1, max=1, desired=1]"
},
{ 
   "phase":"DEPLOY",
   "status":"Created the following deployments: [us-east-1:testaws-stage-v010]"
},
{ 
   "phase":"DEPLOY",
   "status":"Server Groups: [us-east-1:testaws-stage-v010] created."
},
{ 
   "phase":"ORCHESTRATION",
   "status":"Orchestration completed."
}

Previous logs were something like this (hooks created after scaling out):

...
{
"phase": "AWS_DEPLOY",
"status": "Setting size of recommservice-stage-va6-v286 in aam-npe/us-east-1 to [min=1, max=1, desired=1]"
},
{
"phase": "AWS_DEPLOY",
"status": "Creating lifecycle hook ({LifecycleHookName: recommservice-stage-va6-v286-lifecycle-2de43220-adb3-4151-8d81-8f2eb48f1944,AutoScalingGroupName: recommservice-stage-va6-v286,LifecycleTransition: autoscaling:EC2_INSTANCE_LAUNCHING"
},
{
"phase": "AWS_DEPLOY",
"status": "Creating lifecycle hook ({LifecycleHookName: recommservice-stage-va6-v286-lifecycle-ad1c18e1-c573-4914-bb3f-477946bdc067,AutoScalingGroupName: recommservice-stage-va6-v286,LifecycleTransition: autoscaling:EC2_INSTANCE_TERMINATING"
}

@spinnakerbot
Copy link
Contributor

The following commits need their title changed:

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

@costimuraru
Copy link
Contributor Author

costimuraru commented Oct 8, 2019

@robzienert @ajordens @dreynaud @cfieber please have a look at this small but meaningful bug fix.

This fixes the issue caused by the fact that the auto scale group is scaled out before the lifecycle hooks are attached to the ASG. This opened the door for a race condition, where an instance was sometimes created before the lifecycle hooks were attached.

Copy link
Member

@robzienert robzienert left a comment

Choose a reason for hiding this comment

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

Yup yup yup. One change.

Add test

Signed-off-by: Costi Muraru <costimuraru@gmail.com>

Update clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/deploy/AutoScalingWorker.groovy

Co-Authored-By: Rob Zienert <rob@robzienert.com>
Signed-off-by: Constantin Muraru <cmuraru@adobe.com>
Copy link
Contributor

@dreynaud dreynaud left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix 👌

@costimuraru
Copy link
Contributor Author

Thanks, @dreynaud @robzienert
Let me know if we can merge this.

@robzienert robzienert merged commit 7db3aca into spinnaker:master Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants