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.

Sign up for GitHub

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

Mark LoadBalancerReady False when VirtualService failed to be reconciled #6048

Merged
merged 3 commits into from
Nov 26, 2019
Merged

Mark LoadBalancerReady False when VirtualService failed to be reconciled #6048

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1102ec
Mark LoadBalancerReady false when VirtualService failed to be reconciled
nak3 Nov 16, 2019
433605f
Fix go lint warning
nak3 Nov 16, 2019
069a51b
Fix comment
nak3 Nov 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/apis/networking/v1alpha1/ingress_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ var ingressCondSet = apis.NewLivingConditionSet(
IngressConditionLoadBalancerReady,
)

var VirtualServiceNotReconciledReason = "ReconcileVirtualServiceFailed"
nak3 marked this conversation as resolved.
Show resolved Hide resolved

// GetGroupVersionKind returns SchemeGroupVersion of an Ingress
func (i *Ingress) GetGroupVersionKind() schema.GroupVersionKind {
return SchemeGroupVersion.WithKind("Ingress")
Expand Down Expand Up @@ -73,11 +75,16 @@ func (is *IngressStatus) MarkLoadBalancerReady(lbs []LoadBalancerIngressStatus,

// MarkLoadBalancerPending marks the "IngressConditionLoadBalancerReady" condition to unknown to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// MarkLoadBalancerPending marks the "IngressConditionLoadBalancerReady" condition to unknown to
// MarkLoadBalancerNotReady marks the "IngressConditionLoadBalancerReady" condition to unknown to

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to make status as NotReady or Unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review.

Do we need to make status as NotReady or Unknown?

You mean that it is strange as MarkLoadBalancerNotReady() marks Unknown, correct? I thought so first, but as per #5076 (comment),

MarkFooNotReady -> Ready=Unknown

is the most prevalent for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean that it is strange as MarkLoadBalancerNotReady() marks Unknown, correct? I thought so first, but as per #5076 (comment),

MarkFooNotReady -> Ready=Unknown

is the most prevalent for now.

Yes felt strange.

Thank you i dint see #5076 (comment)

// reflect that the load balancer is not ready yet.
func (is *IngressStatus) MarkLoadBalancerPending() {
func (is *IngressStatus) MarkLoadBalancerNotReady() {
ingressCondSet.Manage(is).MarkUnknown(IngressConditionLoadBalancerReady, "Uninitialized",
"Waiting for VirtualService to be ready")
}

// MarkLoadBalancerFailed marks the "IngressConditionLoadBalancerReady" condition to false.
func (is *IngressStatus) MarkLoadBalancerFailed(reason, message string) {
ingressCondSet.Manage(is).MarkFalse(IngressConditionLoadBalancerReady, reason, message)
}

// MarkIngressNotReady marks the "IngressConditionReady" condition to unknown.
func (is *IngressStatus) MarkIngressNotReady(reason, message string) {
ingressCondSet.Manage(is).MarkUnknown(IngressConditionReady, reason, message)
Expand Down
6 changes: 5 additions & 1 deletion pkg/apis/networking/v1alpha1/ingress_lifecycle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,14 @@ func TestIngressTypicalFlow(t *testing.T) {
apitestv1.CheckConditionOngoing(r.duck(), IngressConditionReady, t)

// Then ingress is pending.
r.MarkLoadBalancerPending()
r.MarkLoadBalancerNotReady()
apitestv1.CheckConditionOngoing(r.duck(), IngressConditionLoadBalancerReady, t)
apitestv1.CheckConditionOngoing(r.duck(), IngressConditionReady, t)

r.MarkLoadBalancerFailed("some reason", "some message")
apitestv1.CheckConditionFailed(r.duck(), IngressConditionLoadBalancerReady, t)
apitestv1.CheckConditionFailed(r.duck(), IngressConditionLoadBalancerReady, t)

// Then ingress has address.
r.MarkLoadBalancerReady(
[]LoadBalancerIngressStatus{{DomainInternal: "gateway.default.svc"}},
Expand Down
3 changes: 2 additions & 1 deletion pkg/reconciler/ingress/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ia *v1alpha1.Ingress)
logger.Infof("Creating/Updating VirtualServices")
ia.Status.ObservedGeneration = ia.GetGeneration()
if err := r.reconcileVirtualServices(ctx, ia, vses); err != nil {
ia.Status.MarkLoadBalancerFailed(v1alpha1.VirtualServiceNotReconciledReason, err.Error())
return err
}

Expand Down Expand Up @@ -218,7 +219,7 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ia *v1alpha1.Ingress)
privateLbs := getLBStatus(privateGatewayServiceURLFromContext(ctx))
ia.Status.MarkLoadBalancerReady(lbs, publicLbs, privateLbs)
} else {
ia.Status.MarkLoadBalancerPending()
ia.Status.MarkLoadBalancerNotReady()
}

// TODO(zhiminx): Mark Route status to indicate that Gateway is configured.
Expand Down
9 changes: 6 additions & 3 deletions pkg/reconciler/ingress/ingress_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,14 +278,17 @@ func TestReconcile(t *testing.T) {
v1alpha1.IngressStatus{
Status: duckv1.Status{
Conditions: duckv1.Conditions{{
Type: v1alpha1.IngressConditionLoadBalancerReady,
Status: corev1.ConditionTrue,
Type: v1alpha1.IngressConditionLoadBalancerReady,
Reason: v1alpha1.VirtualServiceNotReconciledReason,
Severity: apis.ConditionSeverityError,
Message: "failed to update VirtualService: inducing failure for update virtualservices",
Status: corev1.ConditionFalse,
}, {
Type: v1alpha1.IngressConditionNetworkConfigured,
Status: corev1.ConditionTrue,
}, {
Type: v1alpha1.IngressConditionReady,
Status: corev1.ConditionUnknown,
Status: corev1.ConditionFalse,
Severity: apis.ConditionSeverityError,
Reason: notReconciledReason,
Message: notReconciledMessage,
Expand Down