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

Derive the label selector value from the target matchLabels #685

Merged
merged 15 commits into from
Sep 17, 2020

Conversation

splkforrest
Copy link
Contributor

Hi Friends 😄 ,

This PR changes how the label selector value is derived. The way it is today, the label selector is derived from the service name defined in the Canary CR. This PR changes it to be derived from the value as it is already defined in the target Deployment's matchLabels.

This addresses a specific use case where the following occurs:

  1. A pre-existing Deployment has labels that cannot be changed due to observability requirements in an organization
  2. A pre-existing Service name cannot be changed due to downstream dependencies
  3. The Deployment matchLabels are already defined and Deployment selectors are immutable
  4. The Service name does not match the Deployment label value

The above points lead to a scenario where Flagger will create the service-canary with a selector based on the Service name defined in the Canary CR and thus not match the label/label-value on the original/canary Deployment. Initialization will pass and the primary Deployment will work fine because it is a newly created Deployment, but the canary Deployment will fail to receive traffic because of the mismatch in service selector and deployment label/label-value.

This situation can be worked around by deleting the Deployment (without deleting the associated Pods) and creating a new Deployment with updated matchLabels (using a new label and label-value). This works for a PoC, but, this doesn't seem to be a scalable workaround if we adopt Flagger in our organization with dozens and dozens of services. A lot of our services have significant traffic and it only takes one mistake to bring down the Pods with the Deployment.

The majority of the code changes here are tests. The actual change is really in getSelectorLabels and the associated interface changes. I would greatly appreciate any/all feedback. 😄 And thank you so much for this project! We are looking at adopting it more widely in our org. 👍

@splkforrest
Copy link
Contributor Author

Hey @stefanprodan, does my latest commit address your feedback? Any other feedback before this can be merged? Thanks!

@stefanprodan
Copy link
Member

@splkforrest what's the impact to current users?

@splkforrest
Copy link
Contributor Author

Good question. My understanding is that for existing users, there should be no impact. The change will use the labelSelector value on the target Deployment/DaemonSet instead of the service name (or targetRef name) when creating the selector on the services and labelSelector values on the primary Deployment/DaemonSet. Everything else works the same way that it does today. In the case of existing users, the labelSelector value and service name should already match, since that is all that is currently allowed, so there is no affect to those users. In the case of new users, they can either match their labelSelector value to their service name or not.
That's my understanding. Please let me know if you see any problems or if I have misunderstood some implementation detail of Flagger.

@stefanprodan stefanprodan changed the title Add label value Derive the label selector value from the target matchLabels Sep 17, 2020
Copy link
Member

@stefanprodan stefanprodan 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 Forrest 🏅

@stefanprodan stefanprodan merged commit 6d65a2c into fluxcd:master Sep 17, 2020
@splkforrest splkforrest deleted the add-label-value branch September 17, 2020 15:33
@splkforrest
Copy link
Contributor Author

Awesome! Thanks so much @stefanprodan! Love the project and this will allow us to test out a lot of our other services. 👍

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.

2 participants