fix for pod condition type too long #1253

sandeepbhojwani · 2020-05-06T22:02:32Z

existing pod condition type has the format "target-health.alb.ingress.k8s.aws/<INGRESS_NAME><SERVICE_NAME><SERVICE_PORT>". This can exceed the max. allowed length if the <INGRESS_NAME><SERVICE_NAME><SERVICE_PORT> part exceeds 63 characters. To work around this problem, added support for static pod condition type string. The string is "target-health.alb.ingress.k8s.aws/load-balancer-tg-ready"
both old and new format are supported. Only one should be used at a time though.

Testing done

created the required objects in a cluster and verified old format still works
created the required objects in a cluster verified new format works as well
verified a deployment transition from old format to new format

resolves #1217

k8s-ci-robot · 2020-05-06T22:02:40Z

Hi @sandeepbhojwani. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta-bot · 2020-05-07T00:23:25Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

sandeepbhojwani · 2020-05-07T02:26:10Z

/assign @M00nF1sh

shaoxt · 2020-05-11T18:53:52Z

@M00nF1sh @devkid @nirnanaaa This fix is critical for using pod readiness gate. Can you have a review on this?

shaoxt · 2020-05-11T19:04:55Z

To use static string as condition Type also make it easier to avoid the following issue

#1241

M00nF1sh · 2020-05-14T01:33:24Z

Hi, thanks for contributing.
This will only work correctly if a pod is been used in a single TargetGroup. (otherwise, the gate will be unblocked if pod is only registered in one targetGroup but not others).

I think better solution would be

use a single pod readinessGate, but the controller tracks all the targetGroups pod should belong to(will be easy in v2 but hard to implement in this branch TBH), and unblocks when all targetGroup are healthy.
use multiple pod readinessGate, but hash the <INGRESS_NAME><SERVICE_NAME><SERVICE_PORT> part if it becomes too long

I'm happy to merge this change in, and we can extend the functionality in the future, just want to ensure you aware of the restriction above.

/ok-to-test

alfredkrohmer · 2020-05-15T19:20:43Z

I agree with @M00nF1sh. I would suggest to name the condition type something like target-health.alb.ingress.k8s.aws/load-balancer-any-tg-ready (or at least have any somewhere in the string) to stress that this will make the pod ready if it gets healthy in any target group. This way we could introduce target-health.alb.ingress.k8s.aws/load-balancer-all-tgs-ready later on.

sandeepbhojwani · 2020-05-15T19:44:42Z

@devkid agree with your idea. I will change the name to target-health.alb.ingress.k8s.aws/load-balancer-any-tg-ready

alfredkrohmer · 2020-05-16T22:23:39Z

internal/ingress/backend/endpoint.go

@@ -146,7 +146,8 @@ func (resolver *endpointResolver) resolveIP(ingress *extensions.Ingress, backend
 	}

 	conditionType := api.PodConditionType(fmt.Sprintf("target-health.alb.ingress.k8s.aws/%s_%s_%s", ingress.Name, backend.ServiceName, backend.ServicePort.String()))
-
+	// new condition type uses a static string
+	staticConditionType := api.PodConditionType("target-health.alb.ingress.k8s.aws/load-balancer-any-tg-ready")


Can you please make this a global const like in targethealth.go?

internal/alb/tg/targethealth.go

M00nF1sh · 2020-06-05T12:41:21Z

i adjust this PR to make this two readinessGate to work together :D
/ok-to-test

- existing pod condition type has the format "target-health.alb.ingress.k8s.aws/<INGRESS_NAME>_<SERVICE_NAME>_<SERVICE_PORT>". This can exceed the max. allowed length if the <INGRESS_NAME>_<SERVICE_NAME>_<SERVICE_PORT> part exceeds 63 characters. To work around this problem, added support for static pod condition type string. The string is "target-health.alb.ingress.k8s.aws/load-balancer-tg-ready" - both old and new format are supported. Only one should be used at a time though. Testing done - verified old format still works - verified new format works as well - verified a deployment transition from old format to new format

M00nF1sh · 2020-06-05T21:55:15Z

/approve

k8s-ci-robot · 2020-06-05T21:55:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, sandeepbhojwani

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:

~~OWNERS~~ [M00nF1sh]

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

fix for pod condition type too long

k8s-ci-robot requested review from alejandrox1 and bigkraig May 6, 2020 22:02

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 6, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 6, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 7, 2020

k8s-ci-robot assigned M00nF1sh May 7, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 14, 2020

alfredkrohmer suggested changes May 16, 2020

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 5, 2020

Sandeep Bhojwani and others added 6 commits June 5, 2020 11:04

fixed lint issues

e8b1c58

changed the static string based on review comments

0d7ed6f

adjust PR so both readinessGate can work together

84cdc53

fix style check

0f4e3f4

rebase master and fix style issue

e7a011a

M00nF1sh force-pushed the static branch from 8b95c00 to e7a011a Compare June 5, 2020 21:30

M00nF1sh added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 5, 2020

k8s-ci-robot merged commit 6cd8324 into kubernetes-sigs:master Jun 5, 2020

sandeepbhojwani deleted the static branch June 8, 2020 18:13

alebedev87 pushed a commit to alebedev87/aws-load-balancer-controller that referenced this pull request Oct 26, 2023

Merge pull request kubernetes-sigs#1253 from sandeepbhojwani/static

ee1e07e

fix for pod condition type too long

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix for pod condition type too long #1253

fix for pod condition type too long #1253

Uh oh!

sandeepbhojwani commented May 6, 2020

Uh oh!

k8s-ci-robot commented May 6, 2020

Uh oh!

fejta-bot commented May 7, 2020

Uh oh!

sandeepbhojwani commented May 7, 2020

Uh oh!

shaoxt commented May 11, 2020 •

edited

Loading

Uh oh!

shaoxt commented May 11, 2020

Uh oh!

M00nF1sh commented May 14, 2020

Uh oh!

alfredkrohmer commented May 15, 2020

Uh oh!

sandeepbhojwani commented May 15, 2020

Uh oh!

alfredkrohmer May 16, 2020

Uh oh!

sandeepbhojwani May 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

M00nF1sh commented Jun 5, 2020

Uh oh!

M00nF1sh commented Jun 5, 2020

Uh oh!

k8s-ci-robot commented Jun 5, 2020

Uh oh!

Uh oh!

fix for pod condition type too long #1253

fix for pod condition type too long #1253

Uh oh!

Conversation

sandeepbhojwani commented May 6, 2020

Uh oh!

k8s-ci-robot commented May 6, 2020

Uh oh!

fejta-bot commented May 7, 2020

Uh oh!

sandeepbhojwani commented May 7, 2020

Uh oh!

shaoxt commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaoxt commented May 11, 2020

Uh oh!

M00nF1sh commented May 14, 2020

Uh oh!

alfredkrohmer commented May 15, 2020

Uh oh!

sandeepbhojwani commented May 15, 2020

Uh oh!

alfredkrohmer May 16, 2020

Choose a reason for hiding this comment

Uh oh!

sandeepbhojwani May 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

M00nF1sh commented Jun 5, 2020

Uh oh!

M00nF1sh commented Jun 5, 2020

Uh oh!

k8s-ci-robot commented Jun 5, 2020

Uh oh!

Uh oh!

shaoxt commented May 11, 2020 •

edited

Loading