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: support additional services for blue-green. fixes #451 #2603

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

d3adb5
Copy link

@d3adb5 d3adb5 commented Feb 18, 2023

Add support for additional preview and active services for the BlueGreen
strategy.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

This is my first contribution to Argo Rollouts, and I feel like #451 isn't
really closed if this doesn't encompass the Canary strategy. I want to work on
it, but first I wanted to write E2E tests for this feature. I'm rather new to
Go, so I'm not sure if I'm doing this right. Still combing through the available
tests to see if I can find a good example to follow.

Support for multiple services is added as a couple new optional fields called
additionalPreviewServices and additionalActiveServices. When populated with
service names, these lists will cause the services in them to be updated as
though they were the preview service or the active service.

Help with this would be much appreciated.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2023

Go Published Test Results

1 984 tests   1 984 ✔️  2m 37s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit 0e94be5.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2023

E2E Tests Published Test Results

    4 files      4 suites   3h 34m 19s ⏱️
  96 tests   80 ✔️   5 💤 11
400 runs  364 ✔️ 20 💤 16

For more details on these failures, see this check.

Results for commit 0e94be5.

♻️ This comment has been updated with latest results.

@meeech
Copy link
Contributor

meeech commented Feb 20, 2023

Would it make sense to add activeServices/previewServices, and just make them mutually exclusive?

@d3adb5
Copy link
Author

d3adb5 commented Feb 20, 2023

Would it make sense to add activeServices/previewServices, and just make them mutually exclusive?

If I understand your suggestion correctly, I don't think so. My original use case was I had two Services for a specific service: one attached to a Network Load Balancer, redirecting UDP traffic, and a simple ClusterIP referenced by Ingress. There were two active and two preview Services.

There are more use cases reported in #451. The problem with just using a list is it's not explicit which Service might be the "origin of the truth" in the case of recovering from a crash. Not that that behavior is really made explicit here.

@meeech
Copy link
Contributor

meeech commented Feb 20, 2023

@d3adb5 Sorry, i realize re-reading it that I wasn't clear there. I didn't mean to make them mutually exclusive to each other (i know you need active and preview).
i meant to make the plural and singular (eg: activeService and activeServices) mutually exclusive.
Which I now realize is similar to the suggestion @huikang #451 (comment) made on #451 - though i don't know if they meant to suggest making them mutually exclusive (basically, can only use 1 - the singular, or the plural) with the plural being preferred going forward - since it works with 1 or more services.

As far as source of truth, i would make the first in the list the SoT, and document that.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity.

@d3adb5
Copy link
Author

d3adb5 commented May 23, 2023

I realize it's been a while since I touched this PR. Life's been busy, now there are even merge conflicts. I'll get to it sooner or later.

As far as source of truth, i would make the first in the list the SoT, and document that.

My goal with the additional* parameters was to avoid having to document which one will be the SoT. My hopes were reading the spec would make it immediately clear which one would be used in case of recovery and which ones would become secondary updates, rather than let the user guess which one would be the SoT and have to read the documentation to confirm their assumptions.

@d3adb5 d3adb5 force-pushed the feat/multiple-svc branch from 471e40d to 0e94be5 Compare May 23, 2023 07:07
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
11.4% 11.4% Duplication

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch coverage: 39.39% and project coverage change: -0.07 ⚠️

Comparison is base (6ac1533) 81.68% compared to head (0e94be5) 81.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2603      +/-   ##
==========================================
- Coverage   81.68%   81.61%   -0.07%     
==========================================
  Files         133      133              
  Lines       20178    20211      +33     
==========================================
+ Hits        16483    16496      +13     
- Misses       2843     2857      +14     
- Partials      852      858       +6     
Impacted Files Coverage Δ
rollout/bluegreen.go 65.94% <21.42%> (-2.36%) ⬇️
rollout/service.go 76.82% <52.63%> (-2.15%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 90 days with no activity.

@fdfzcq
Copy link

fdfzcq commented Nov 14, 2024

Hello @d3adb5 any updates on this? :) We are in need of the same feature and wonder if we can help contribute and push for the feature.

@d3adb5
Copy link
Author

d3adb5 commented Nov 15, 2024

Hey @fdfzcq, I haven't had the time to look back at this, and since I've moved on from the project where the use case came up it took a hit in my list of priorities. Last I remember I was a little confused about how to add E2E tests for this and still wondering if I'm going about the type change the right way.

I have a rough week or two ahead of me at work right now, but afterwards I'll be able to commit myself to this. If you want, you can always take over this implementation. Want to explain your use case here or elsewhere so I get a clearer picture of what you guys need?

@yannickepstein
Copy link

@d3adb5 Our use case is kind of similar to the one you have. We want to use Argo Rollouts in a situation where we route traffic to the same deployment through multiple service resources. We need multiple service resources, because each service defines a different protocol (UDP and TCP) for the same L4 pass-through LB (GKE docs).
Multiple protocols on a single service resource is not supported if we want GKE to manage a corresponding L4 pass-through LB for us, so that's why we're hoping that Argo Rollouts might support multiple service resources instead.

@eduardOrthopy
Copy link

So, it has been a while since there were changes made here. @d3adb5 if you are still on this, please let me/us know if/how I/we can support.

This is a feature that I would really love to see in Argo, and that would solve a big pain point for me.

If you cannot find time to contribute to this any more, that is fair. Life moves on and so do we. Just let us know, so that the rest of the community can take over. And thanks for creating this PR in the first place.

@d3adb5 d3adb5 force-pushed the feat/multiple-svc branch from 0e94be5 to 01b48af Compare February 7, 2025 18:55
@d3adb5
Copy link
Author

d3adb5 commented Feb 7, 2025

Sorry for the delay getting back to everyone. @eduardOrthopy, it would help me quite a bit to understand how to properly run and write E2E tests for this. For some reason E2E tests with a kind or k3d cluster on my machine are always failing intermittently. I'm new to Go, too, so any feedback on what I have here would be well appreciated.

I rebased on master and solved conflicts, ran make codegen again, and will be looking into this during my spare time. Please add as much feedback as you like here! If anybody wants to push to my branch, I'm fine with that too. 🤔

Copy link
Contributor

github-actions bot commented Feb 7, 2025

Published E2E Test Results

  4 files    4 suites   3h 9m 4s ⏱️
113 tests 101 ✅  7 💤 5 ❌
460 runs  425 ✅ 28 💤 7 ❌

For more details on these failures, see this check.

Results for commit 4cd5ad0.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Feb 7, 2025

Published Unit Test Results

2 296 tests   2 296 ✅  2m 59s ⏱️
  128 suites      0 💤
    1 files        0 ❌

Results for commit 4cd5ad0.

♻️ This comment has been updated with latest results.

Add support for additional preview and active services for the BlueGreen
strategy.

Signed-off-by: d3adb5 <me@d3adb5.net>
@d3adb5 d3adb5 force-pushed the feat/multiple-svc branch from 01b48af to 4cd5ad0 Compare February 9, 2025 07:33
Copy link

sonarqubecloud bot commented Feb 9, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants