Changed the Deployment strategy to 'Recreate' so multiple external-dns pods don't conflict with each other.

[mac-chaffee]

Description

Before the new record format rolls out, it would probably be a good idea to ensure only one external-dns pod is running at that time to avoid any risk of conflicts.

Checklist

• [n/a] Unit tests updated
• End user documentation updated

[k8s-ci-robot]

Welcome @mac-chaffee!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[stevehipwell]

@mac-chaffee this change would be detrimental to the chart, the recreate strategy isn't really for this purpose and the split brain behaviour is controlled by the leader election already. I'd be happy to support having a configurable strategy but I'm 100% against defaulting to recreate.

Also the notes on the change you mentioned say that both types will be supported so I'm not sure why we even need to consider a change here?

If you want to continue with this PR I'll do a more in-depth review on the changes?

[mac-chaffee]

Oh external-dns has leader election already? I thought it didn't yet: #164 #2430

[stevehipwell]

Oh external-dns has leader election already? I thought it didn't yet: #164 #2430

No you're right, I was thinking about another project.

[mac-chaffee]

Rebased, should be good to go now, preferably soon so I don't have to update the release note date ;)

[stevehipwell]

@mac-chaffee I don't think that changing the default chart deployment strategy is necessarily a good idea; do you have an example of what you think could happen and what the actual impact would be? I'm happy for the strategy to be configurable, although IMHO implementing #2430 would be a better solution.

[mac-chaffee]

I think the reason why #2430 exists at all (instead of just increasing replicas to 2) kinda hints at the reason: We can't know all of the possible race conditions that could appear when running two external-dns pods at the same time. So it's best to just prevent that all together via Recreate or leader election.

I agree doing leader election would be better, but until that's implemented, I think we're subjecting users to unnecessary risk whenever they do upgrades. Regardless, I think the only downside of this change is that there might be an extra few seconds between syncs after an upgrade since we have to wait for the new pod to start. Do you see a bigger downside?

[stevehipwell]

@mac-chaffee I've just checked the YAML in the examples and it looks like the current chart behaviour isn't consistent with this and your PR will align the behaviour. Just to point out that another downside of recreate is that there is no guarantee that there will be a running pod at the end if there aren't enough resources (rollback wont even fix this) or if the new configuration is invalid.

@Raffo could you enable the workflow?

/approve

[stevehipwell]

@mac-chaffee could you please remove the changes to Chart.yaml from this PR as multiple PRs will be combined for the release (#2916).

[stevehipwell]

/remove-approve

[stevehipwell]

@mac-chaffee could you please remove the changes to Chart.yaml from this PR as multiple PRs will be combined for the release (#2916).

@mac-chaffee I'd like to get this merged and into the upcoming release, could you take a look a this?

[mac-chaffee]

Sorry, work has been busy!

[stevehipwell]

/lgtm

[Raffo]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mac-chaffee, Raffo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• charts/OWNERS [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment