elb_target_group: only drain all elb targets if targets is an empty list and modify_targets is true #39715
Conversation
ansibot
added
aws
bug
|
Not 100% convinced this is the best fix. If the targets aren't changing at all, then they shouldn't all be drained. I'd like to see (either here or in the associated issue) a minimum test case, rather than changing default behaviour - I've never hit this issue, and I'd like to understand why. |
ansibot
removed
the
needs_triage
|
In this case, modify_targets is really purge_targets. It will only have an effect if you explicitly set I agree that the default behaviour should be not to purge targets, and also think modify_targets should be renamed to purge_targets (with a backward compatible modify_targets alias) |
|
Based on what ive seen, that's not quite what it's doing. Behaviorally, 'purge targets' (we'll go with that name for now, for clarity sake in behavior) is purging all targets in the group, even when 0 targets are being supplied. If it is supposed to only take effect when a target is supplied, then that may be the ultimate cause of the bug, and should be investigated further.. I'll post a concrete example of my playbook later, as its on the work machine, but it definitely does not have any targets associated with the play (just with the target group) |
|
Ok, so reading through the code more thoroughly: Based on the second link above, we see that it literally behaves as 'unless you pass the parameter, it will wipe out all targets'.... Why would someone think to pass modify_targets: false when they aren't passing targets in the first place and modify_targets is an optional parameter |
Reverted to default behavior Updating naming convention based on input from team Added slightly stronger documentation (suggested note) Added alias for purge_targets to preserve current syntax
| @@ -119,7 +120,7 @@ | |||
| version_added: 2.5 | |||
| targets: | |||
| description: | |||
| - A list of targets to assign to the target group. This parameter defaults to an empty list. Unless you set the 'modify_targets' parameter then | |||
| - A list of targets to assign to the target group. This parameter defaults to an empty list. Unless you set the 'purge_targets' parameter then | |||
There was a problem hiding this comment.
s/This parameter defaults to an empty list./This parameter defaults to None.
The line below should be edited too since to drain all targets it must be set to an empty list to use in conjunction with purge_targets.
There was a problem hiding this comment.
It still does not default to an empty list, it defaults to None. It will appear null in the invocation.
| if module.params.get("modify_targets"): | ||
| if module.params.get("targets"): | ||
| if module.params.get("purge_targets"): | ||
| if module.params.get("targets") == []: |
There was a problem hiding this comment.
I think if module.params.get("targets"): is right here. It indicates that the module needs to compare the current targets with the targets the user provided and remove any that the user hasn't provided. If the comparison within this if is wrong and the wrong targets get removed then that should be fixed.
If I provide
...
purge_targets: true
targets:
- Id: i-01234567
Port: 80
- Id: i-98765432
Port: 80
and then in a second playbook:
...
purge_targets: true
targets:
- Id: i-01234567
Port: 80
I would expect target i-98765432 to be purged after the second playbook. But with this patch this if would now be false, so the path on line 538 would be used and all targets would be purged regardless of the target I want to keep.
I was thinking this line should be changed: https://github.com/ansible/ansible/pull/39715/files#diff-c008972f0ccccd4a4d9b8c1de95c4d08L538 to elif module.params.get("targets") == [] or elif module.params.get("targets") is not None
|
Removed the empty list check, but im hesitant to change the else: as it would change the current default behavior (fixing my own issue, but changing wimnat's expected behavior currently). In this case, 'core' can make the final decision on how to handle it and I can change quickly. I already tested the various methods for that line and have it saved as a comment locally. |
|
The test |
ansibot
added
ci_verified
| - A list of targets to assign to the target group. This parameter defaults to an empty list. Unless you set the 'modify_targets' parameter then | ||
| all existing targets will be removed from the group. The list should be an Id and a Port parameter. See the Examples for detail. | ||
| - A list of targets to assign to the target group. The list should be an Id and a Port parameter. | ||
| Defaults to 'None.' See the Examples for detail. |
There was a problem hiding this comment.
No quotes for None, makes it unclear if it's python None/yaml null or a string.
There was a problem hiding this comment.
Feel free to add here as well something along the lines of prior to 2.6 all targets were drained if this option was None and I(purge_targets) was True. As of 2.6, to drain all targets this option must be set to an empty list to use in conjunction with I(purge_targets).
| @@ -535,7 +537,7 @@ def create_or_update_target_group(connection, module): | |||
| status_achieved, registered_instances = wait_for_status(connection, module, tg['TargetGroupArn'], instances_to_remove, 'unused') | |||
| if not status_achieved: | |||
| module.fail_json(msg='Error waiting for target deregistration - please check the AWS console') | |||
| else: | |||
| elif module.get.parames("targets") is None: | |||
There was a problem hiding this comment.
This should be elif module.get.parames("targets") is not None: or elif module.get.parames("targets") == []:
|
shippable is complaining because: |
ansibot
removed
the
ci_verified
|
The test |
ansibot
added
community_review
s-hertel
changed the title
|
@wimnat Hi, do you mind reviewing this? |
| @@ -477,7 +481,7 @@ def create_or_update_target_group(connection, module): | |||
| module.fail_json_aws(e, msg="Couldn't update target group") | |||
|
|
|||
| # Do we need to modify targets? | |||
| if module.params.get("modify_targets"): | |||
| if module.params.get("purge_targets"): | |||
There was a problem hiding this comment.
This change will mean that new targets will not get registered if purge_targets is False, which is not what would be expected.
We could really use a test suite to catch this kind of thing.
There was a problem hiding this comment.
Yeah, maybe purge_targets is not the right name for this. @willthames Do you have a problem with this fix if the option name remains 'modify_targets'?
There was a problem hiding this comment.
This seems like a problem with overloading what purge_ means. The code does what I'd expect, but not what I'd expect a purge_targets option to do. @Daemoen, what do you think?
ansibot
added
needs_revision
|
Just a little background on this... When I initially wrote the module for my use case there was no I would create a target group and ELB and then an ECS service. The ECS service was responsible for putting targets in to the target group. I would never have When I ran a new deploy of my application, the target group module would remove all the targets that ECS had put in there before then updating the ECS service and then ECS would put the targets back. This was obviously bad as there would be a period where all targets were removed before ECS put targets in so I added the modify_targets flag so that all that was happening was the module was checking the target group existed and not worrying about targets. It was just a quick fix - maybe if i'd thought about it some more i would of just said if As for purge_targets defaulting to yes, i always disagree with most people when it comes to this. I see ansible as a tool to reach desired state - IMO it should always purge unless said otherwise IF the parameter is passed. E.g. with tags - if you've passed a set of tags then you shouldn't have to worry about some other tags be present unless you explicitly say 'purge_tags=no'. Same for targets. I think this is a good change to take in to account whether the Hope that makes sense. Anyway, with this change things should still work for me as I expect so LGTM |
| - A list of targets to assign to the target group. The list should be an Id and a Port parameter. | ||
| Defaults to None. See the Examples for detail. | ||
| Prior to 2.6, all targets were drained if targets was None and I(purge_targets) was True. As of 2.6, | ||
| to drain all targets, this option must be set to an empty list to use in conjustion with I(purge_targets). |
ansibot
added
affects_2.6
ansibot
added
the
needs_repo
ansibot
added
support:core
ansibot
added
needs_maintainer
ansibot
removed
the
needs_maintainer
ansibot
added
the
needs_rebase
|
@Daemoen Your branch does not contain a shippable.yml file. Please rebase your branch to trigger running of current tests. |
ansibot
added
the
needs_shippable
|
Bump - this is causing unexpected behavior (ECS services throw 503 errors instead of doing rolling deployments). If there is any way I can help push this PR along I would be happy to, just let me know. |
|
Wow, not sure what happened here. I thought we had closed out and resolved this weeks ago, just noticed it hanging around at the top of my notifications. I can take a look at it later today. |
|
@Daemoen Hi, are you able to look at this still? I notice your source branch has been deleted, so you may need to create a fresh PR & branch. |
|
Assuming not, so closing. |
… targets
SUMMARY
The current default of 'True' is dangerous because you would not expect to change targets unless you want to change targets. It is safer to change this to false and prevent unexpected default assumptions.This was the first approach. Rather than changing a default, now check if targets is an empty list. Now if targets and modify_targets options are omitted from the play all targets are not drained.ISSUE TYPE
COMPONENT NAME
elb_target_group.py
ANSIBLE VERSION
2.5+
ADDITIONAL INFORMATION
Related to #39709