feat: Add resource conditions to L4 LB Service status #922
Conversation
k8s-ci-robot
added
the
needs-triage
|
This issue is currently awaiting triage. If the repository mantainers determine this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
k8s-ci-robot
added
size/XL
|
/retest |
1 similar comment
|
/retest |
08volt
force-pushed
the
lb-conditions
branch
from
c4d8d20 to
58f1ce2
Compare
k8s-ci-robot
added
size/L
08volt
force-pushed
the
lb-conditions
branch
from
58f1ce2 to
bd658f9
Compare
|
/retest |
08volt
force-pushed
the
lb-conditions
branch
from
bd658f9 to
534d85c
Compare
|
/assign @mmamczur |
08volt
force-pushed
the
lb-conditions
branch
from
534d85c to
a646ecc
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 08volt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/XL
| } | ||
|
|
||
| func (g *Cloud) ensureTargetPoolAndHealthCheck(tpExists, tpNeedsRecreation bool, svc *v1.Service, loadBalancerName, clusterID, ipAddressToUse string, hosts []*gceInstance, hcToCreate, hcToDelete *compute.HttpHealthCheck) error { | ||
| func (g *Cloud) ensureTargetPoolAndHealthCheck(tpExists, tpNeedsRecreation bool, svc *v1.Service, loadBalancerName, clusterID, ipAddressToUse string, hosts []*gceInstance, hcToCreate, hcToDelete *compute.HttpHealthCheck, conditions *[]metav1.Condition) error { |
There was a problem hiding this comment.
why is this arg a pointer to slice? do we have to add nil checks?
There was a problem hiding this comment.
-
passing only the slice is not 100% safe ( later there are "append" operations and if the lenght exceeds the pre-allocated size it would repalce the original array )
-
nil check is present directly in the function SetStatusCondition that I am using to update this slice
There was a problem hiding this comment.
passing only the slice is not 100% safe ( later there are "append" operations and if the lenght exceeds the pre-allocated size it would repalce the original array )
But isn't the slice already a reference type?
There was a problem hiding this comment.
Yes but it has the reference to the allocated memory + the size of the allocated memory
If the allocated size is not enough for the append it replace the allocated memory and size. In this case after the function you don't have the reference to the new allocated memory but still the old one without the new elements.
08volt
force-pushed
the
lb-conditions
branch
from
a646ecc to
71c98d0
Compare
- Refactor ensureInternalLoadBalancer to return ServiceStatus and update error handling - Changed the return type of ensureInternalLoadBalancer from (*v1.LoadBalancerStatus, error) to (*v1.ServiceStatus, error). - Introduced expectedConditions to manage service status conditions effectively. - Added patch to update conditions in the service
08volt
force-pushed
the
lb-conditions
branch
from
71c98d0 to
d1141d3
Compare
|
This seems like an abuse of the Conditions from a semantics perspective? Would like to understand more if you have any comments from API review about this usage. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
Hi @thockin , could you please confirm this change was discussed with you? |
|
@danwinship already proposed this feature in kubernetes/enhancements#4631 but it requires people to drive it, so this can be a good opportunity to contribute this feature to SIG network |
Thanks @aojea. I didn't know about this proposal, but it sounds a bit different. This feature would allow listing the resource names used in GCP, while the proposal refers to more generic Conditions that allow for a better understanding of the load balancer provisioning/serving status. |
|
Well, you said:
And that sounds like part of what KEP-4631 was trying to do: add conditions to allow the backend to explicitly report when it's having problems allocating a load balancer. |
Exactly, that's not the same thing
|
7 participants
Objective
Improves the observability of L4 Load Balancer provisioning by propagating GCP resources in Service.status.conditions.
Context & Motivation
Currently, the Service status for Load Balancers primarily reflects the ingress IP but lacks visibility into the state of dependent cloud resources.
This change introduces a mechanism to report the successful allocation of specific GCP resources, enabling better introspection.
Design & Implementation
Feature Gating:
Introduced a new controller-manager flag --enable-l4lb-svc-conditions (default: false) to gate this behavior, ensuring backward compatibility and safe rollout.
Added enableLBServiceConditions to the gce.Cloud struct configuration.
API Changes:
Refactored ensureInternalLoadBalancer to return *v1.ServiceStatus instead of *v1.LoadBalancerStatus. This allows the function to propagate both the Ingress IP and the new Conditions.
Defined standard condition types in gce_conditions.go:
State Management (Server-Side Apply):
Implemented patchServiceStatusConditions using Server-Side Apply (SSA) to safely update the status.conditions field without overwriting other status fields managed by different controllers.
Added ConditionsEqual helper to prevent unnecessary API calls if the state has not converged or changed.
The feature is disabled by default via the feature flag.
Verification
Unit Tests: Updated gce_loadbalancer_internal_test.go and gce_loadbalancer_external_test.go to verify that the correct conditions are generated for various provisioning scenarios (success, partial failure).