use LoadBalancer type service for e2e service test to patch ingress status #119454

pacoxu · 2023-07-20T03:05:23Z

What type of PR is this?

/kind failing-test
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #119452

Special notes for your reviewer:

The failure is after #118895.
/cc @RyanAoh

This test is flake not failing every time.

Does this PR introduce a user-facing change?

None

k8s-ci-robot · 2023-07-20T03:05:32Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

pacoxu · 2023-07-20T03:19:21Z

/priority important-soon

RyanAoh · 2023-07-20T10:53:38Z

/lgtm

k8s-ci-robot · 2023-07-20T10:53:45Z

LGTM label has been added.

Git tree hash: 317b55dc51fd032fd40c8255374daa7c29d57ba0

pacoxu · 2023-07-20T16:21:05Z

/assign @MrHohn @robscott

robscott · 2023-07-20T16:27:19Z

@RyanAoh It seems like there must be some kind of mistake here. If I'm understanding this PR correctly, it appears that IPMode was added as a required field without a default value, which would be a backwards incompatible change. Did you mean to add the new field as optional?

/cc @aojea @thockin

RyanAoh · 2023-07-21T03:33:11Z

IPMode is an optional field. Looks like we should allow the load-balancer status updated with only ingress.ip specified, and the ingress.ipMode will default to "VIP" eventually.
The fllowing validation may need to be removed：

kubernetes/pkg/apis/core/validation/validation.go

Lines 7066 to 7069 in 4457f85

    
           if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil { 
        
           	if len(ingress.IP) > 0 { 
        
           		allErrs = append(allErrs, field.Required(idxPath.Child("ipMode"), "must be specified when `ip` is set")) 
        
           	}

Any advice? @thockin

pacoxu · 2023-07-21T03:36:59Z

The following validation may need to be removed.

remove the validation
default to "VIP"

Either can solve the failure without the e2e test change.

k8s-ci-robot · 2023-07-31T21:26:33Z

LGTM label has been added.

Git tree hash: 6c480982b7afb633749f9fce9a31047050ba7ae0

thockin · 2023-07-31T21:27:22Z

/retest

pacoxu · 2023-08-01T03:09:08Z

+1 for Warn and then discard the value. It seems to be a more API-consistent change.

aojea · 2023-08-01T16:20:32Z

/approve

heh, this is why everybody loves Service 🙃

what an interesting failure

k8s-ci-robot · 2023-08-01T16:20:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, pacoxu, thockin

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:

~~test/e2e/network/OWNERS~~ [aojea,thockin]

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

aojea · 2023-08-01T16:25:32Z

/hold cancel
/milestone next-candidate

only impacts an alpha feature gate

pacoxu · 2023-08-16T03:35:32Z

/retest

pacoxu · 2023-08-16T13:26:26Z

/checkcla
/easycla
/retest

thockin

I think the timeout at L3518 is now too short for an LB service.

We should either call GetServiceLoadBalancerPropagationTimeout or call e2eservice.WaitForServiceDeletedWithFinalizer or just bump this timeout or something.

Do we really need to test patch here? If not, it would be simpler to remove this section - it is exercised in the loadbalancer tests already.

@aojea

pacoxu · 2023-08-17T13:19:12Z

/retest

pacoxu · 2023-08-17T13:19:25Z

/retest-required

aojea · 2023-08-17T16:18:43Z

/hold

we need to understand the consequnces of using a LoadBalancer

test/e2e/network/service.go

thockin · 2023-08-18T23:01:44Z

test/e2e/network/service.go

@@ -3357,6 +3357,7 @@ var _ = common.SIGDescribe("Services", func() {
 					Port:       int32(80),
 					TargetPort: intstr.FromInt(80),
 				}},
+				LoadBalancerClass: utilpointer.String("test.com/test"),


super super nit: example.com should be used for things like this.

Updated to "example.com/internal-vip. Is this OK?

thockin

I'm going to approve it, but not LGTM. Change the domain name and then LGTM is easy.

thockin · 2023-08-19T01:26:30Z

/lgtm

k8s-ci-robot · 2023-08-19T01:26:36Z

LGTM label has been added.

Git tree hash: 769a3ecc3d8d347c1c3cd431c9921c4a5bda2e6d

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 20, 2023

k8s-ci-robot requested review from aojea and freehan July 20, 2023 03:06

k8s-ci-robot added area/test sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 20, 2023

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 20, 2023

k8s-ci-robot assigned RyanAoh Jul 20, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2023

k8s-ci-robot assigned MrHohn and robscott Jul 20, 2023

k8s-ci-robot requested a review from thockin July 20, 2023 16:27

robscott mentioned this pull request Jul 20, 2023

Make Kubernetes aware of the LoadBalancer behaviour #118895

Merged

pacoxu changed the title ~~add IPMode for ingress in e2e service~~ Fix IPMode have to be specified for ingress in e2e service if IP is set Jul 21, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2023

k8s-ci-robot added this to the next-candidate milestone Aug 1, 2023

pacoxu mentioned this pull request Aug 7, 2023

Gate: disallow .status.loadBalancer on non-LB svc #119789

Merged

thockin reviewed Aug 16, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2023

aojea reviewed Aug 17, 2023

View reviewed changes

test/e2e/network/service.go Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2023

k8s-ci-robot requested a review from thockin August 18, 2023 01:03

thockin reviewed Aug 18, 2023

View reviewed changes

thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2023

add a fake load balancer class to fix e2e test failure

30ff937

pacoxu force-pushed the fix-ip-mode branch from d4a866e to 30ff937 Compare August 19, 2023 00:50

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2023

k8s-ci-robot merged commit c2bddad into kubernetes:master Aug 19, 2023
15 of 16 checks passed

k8s-ci-robot modified the milestones: next-candidate, v1.29 Aug 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use LoadBalancer type service for e2e service test to patch ingress status #119454

use LoadBalancer type service for e2e service test to patch ingress status #119454

pacoxu commented Jul 20, 2023 •

edited

k8s-ci-robot commented Jul 20, 2023

pacoxu commented Jul 20, 2023

RyanAoh commented Jul 20, 2023

k8s-ci-robot commented Jul 20, 2023

pacoxu commented Jul 20, 2023

robscott commented Jul 20, 2023

RyanAoh commented Jul 21, 2023

pacoxu commented Jul 21, 2023

k8s-ci-robot commented Jul 31, 2023

thockin commented Jul 31, 2023

pacoxu commented Aug 1, 2023

aojea commented Aug 1, 2023

k8s-ci-robot commented Aug 1, 2023

aojea commented Aug 1, 2023

pacoxu commented Aug 16, 2023

pacoxu commented Aug 16, 2023

thockin left a comment

pacoxu commented Aug 17, 2023

pacoxu commented Aug 17, 2023

aojea commented Aug 17, 2023

thockin Aug 18, 2023

pacoxu Aug 19, 2023

thockin left a comment

thockin commented Aug 19, 2023

k8s-ci-robot commented Aug 19, 2023

use LoadBalancer type service for e2e service test to patch ingress status #119454

use LoadBalancer type service for e2e service test to patch ingress status #119454

Conversation

pacoxu commented Jul 20, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jul 20, 2023

pacoxu commented Jul 20, 2023

RyanAoh commented Jul 20, 2023

k8s-ci-robot commented Jul 20, 2023

pacoxu commented Jul 20, 2023

robscott commented Jul 20, 2023

RyanAoh commented Jul 21, 2023

pacoxu commented Jul 21, 2023

k8s-ci-robot commented Jul 31, 2023

thockin commented Jul 31, 2023

pacoxu commented Aug 1, 2023

aojea commented Aug 1, 2023

k8s-ci-robot commented Aug 1, 2023

aojea commented Aug 1, 2023

pacoxu commented Aug 16, 2023

pacoxu commented Aug 16, 2023

thockin left a comment

Choose a reason for hiding this comment

pacoxu commented Aug 17, 2023

pacoxu commented Aug 17, 2023

aojea commented Aug 17, 2023

thockin Aug 18, 2023

Choose a reason for hiding this comment

pacoxu Aug 19, 2023

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

thockin commented Aug 19, 2023

k8s-ci-robot commented Aug 19, 2023

pacoxu commented Jul 20, 2023 •

edited