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

Make canary-only configuration for skipper-ingress possible #7117

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

RomanZavodskikh
Copy link
Contributor

@RomanZavodskikh RomanZavodskikh commented Mar 4, 2024

When "skipper_ingress_canary_enabled" and "skipper_ingress_test_single_pod" are both "true"

@RomanZavodskikh RomanZavodskikh added the minor Minor changes, e.g. low risk config updates, changes that do not introduce a new API call. label Mar 4, 2024
@szuecs
Copy link
Member

szuecs commented Mar 4, 2024

@RomanZavodskikh that looks super complex. Why not something like a custom bool config item?
Another way could be also if max_replica set to 0 then delete hpa, but I guess a bool would be simple and cover what we need.

@RomanZavodskikh RomanZavodskikh force-pushed the skipper/have-only-canary branch 3 times, most recently from 0a30063 to a0ce77f Compare March 4, 2024 16:43
@RomanZavodskikh
Copy link
Contributor Author

Yes, I tried this idea, and I agree the configuration becomes much simpler. However, what if "skipper_ingress_canary_enabled" is set to "exclusive" in some important production cluster? Such accidential configuration will make all traffic go through single skipper-ingress pod.
I have considered such a scenario and added additional safety net.

@szuecs
Copy link
Member

szuecs commented Mar 4, 2024

@RomanZavodskikh you can also damage it by setting max replica to 1...

@RomanZavodskikh RomanZavodskikh force-pushed the skipper/have-only-canary branch 2 times, most recently from 071cb19 to 0a30063 Compare March 4, 2024 16:46
@RomanZavodskikh
Copy link
Contributor Author

Okay, I see, we cannot take so much responsibility here. Reverted to the most simple option.

@RomanZavodskikh RomanZavodskikh force-pushed the skipper/have-only-canary branch 2 times, most recently from 5ec2ad2 to 0a30063 Compare March 4, 2024 16:49
@szuecs
Copy link
Member

szuecs commented Mar 4, 2024

Looks much better now!

@AlexanderYastrebov
Copy link
Member

I think you can add replicas: 0 to the deployment conditionally - HPA does not scale deployment that has zero replicas

@AlexanderYastrebov
Copy link
Member

I also do not like that we change boolean (note _enabled suffix) config into multivalue but alternative is another config item which might be even worse.

@RomanZavodskikh RomanZavodskikh force-pushed the skipper/have-only-canary branch 5 times, most recently from 1c37c1e to 21009ca Compare March 4, 2024 19:58
@RomanZavodskikh RomanZavodskikh force-pushed the skipper/have-only-canary branch from 21009ca to 69606de Compare March 5, 2024 09:28
@RomanZavodskikh
Copy link
Contributor Author

👍

1 similar comment
@szuecs
Copy link
Member

szuecs commented Mar 5, 2024

👍

@szuecs szuecs merged commit 744d80f into dev Mar 5, 2024
10 checks passed
@szuecs szuecs deleted the skipper/have-only-canary branch March 5, 2024 11:44
@@ -39,6 +39,8 @@ metadata:
spec:
{{ if index . "replicas" }}
replicas: {{ .replicas }}
{{ else if eq .Cluster.ConfigItems.skipper_ingress_test_single_pod "true" }}
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think it would be cleaner to do

{{ if eq .Cluster.ConfigItems.skipper_ingress_test_single_pod "true" }}
{{ template "skipper-ingress" dict
  "name" "skipper-ingress"
  "internal_version" $internal_version
  "replicas": 0
  "Cluster" .Cluster
  "Values" .Values
}}
{{ else }}
{{ template "skipper-ingress" dict
  "name" "skipper-ingress"
  "internal_version" $internal_version
  "Cluster" .Cluster
  "Values" .Values
}}
{{ end }}

Copy link
Member

@AlexanderYastrebov AlexanderYastrebov Mar 5, 2024

Choose a reason for hiding this comment

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

Also skipper_ingress_test_single_pod implies test and single, but it is single canary pod which in the future might become several pods.
Maybe it should be just skipper_ingress_replicas that is empty by default:

{{ if ne .Cluster.ConfigItems.skipper_ingress_replicas "" }}
{{ template "skipper-ingress" dict
  "name" "skipper-ingress"
  "internal_version" $internal_version
  "replicas": .Cluster.ConfigItems.skipper_ingress_replicas
  "Cluster" .Cluster
  "Values" .Values
}}
{{ else }}
{{ template "skipper-ingress" dict
  "name" "skipper-ingress"
  "internal_version" $internal_version
  "Cluster" .Cluster
  "Values" .Values
}}
{{ end }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/alpha merged/beta merged/stable minor Minor changes, e.g. low risk config updates, changes that do not introduce a new API call.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants