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

Probe Envoy pods to determine when a ClusterIngress is actually deployed #4734

Merged
merged 2 commits into from Jul 29, 2019

Conversation

@JRBANCEL
Copy link
Contributor

JRBANCEL commented Jul 12, 2019

Fixes #3312 & #1582

Proposed Changes

See the design doc for details.

TODO

  • Working implementation
  • Fix existing tests
  • Add tests

Next

  • Remove the special handling of 404s in E2E tests (in a subsequent PR)
Copy link

knative-prow-robot left a comment

@JRBANCEL: 0 warnings.

In response to this:

Fixes #3312 & #1582

Proposed Changes

See the design doc for details.

TODO

  • Working implementation
  • Fix existing tests
  • Add tests
  • Remove the special handling of 404s in E2E tests

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.

@knative-metrics-robot

This comment has been minimized.

Copy link

knative-metrics-robot commented Jul 12, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/networking/v1alpha1/ingress_lifecycle.go 91.7% 73.3% -18.3
pkg/network/prober/prober.go 97.7% 91.3% -6.4
pkg/reconciler/ingress/ingress.go Do not exist 0.0%
pkg/reconciler/ingress/resources/virtual_service.go 97.6% 95.9% -1.7
pkg/reconciler/ingress/status.go Do not exist 0.0%
Copy link
Contributor

vagababov left a comment

Mostly superficial items.

pkg/apis/networking/v1alpha1/ingress_lifecycle.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/resources/virtual_service.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
@@ -140,6 +176,37 @@ func makeVirtualServiceSpec(ia v1alpha1.IngressAccessor, gateways map[v1alpha1.I
return &spec
}

func makeProbeRoute(host string) v1alpha3.HTTPRoute {

This comment has been minimized.

Copy link
@markusthoemmes

markusthoemmes Jul 15, 2019

Contributor

Why do we need to add a dummy route? Couldn't we also use our probe? It'll either return activator or queue-proxy but we'd know it works without side-effects I think? Is this to keep the layering strict?

This comment has been minimized.

Copy link
@JRBANCEL

JRBANCEL Jul 15, 2019

Author Contributor

Why do we need to add a dummy route?

It is an invalid Spec without it.

Couldn't we also use our probe? It'll either return activator or queue-proxy but we'd know it works without side-effects I think? Is this to keep the layering strict?

We could. It is simply because it is simpler, has lower latency and more reliable.

This comment has been minimized.

Copy link
@markusthoemmes

markusthoemmes Jul 15, 2019

Contributor

Is it guaranteed that the fake route is propageted on the same paths as the actual route? As in: Can we guarantee the actual route is ready and propagated to envoys if we probe the fake route?

This comment has been minimized.

Copy link
@JRBANCEL

JRBANCEL Jul 15, 2019

Author Contributor

We can double check the Istio implementation.
I am assuming that yes, since both routes are bound to the same Istio Gateway, they will be both applied to the same Envoy pods.

An interesting related question is how does Istio apply the config changes?
If it is atomic, good. If not, it is possible that the probe route is applied before the actual route.

@JRBANCEL JRBANCEL force-pushed the JRBANCEL:cluster-ingress-status branch from e3906a2 to 11432e2 Jul 15, 2019
@knative-prow-robot knative-prow-robot added size/XL and removed size/L labels Jul 16, 2019
@JRBANCEL JRBANCEL force-pushed the JRBANCEL:cluster-ingress-status branch from 82db439 to 9b4c24d Jul 16, 2019
Copy link

knative-prow-robot left a comment

@JRBANCEL: 1 warning.

In response to this:

/lint
/test pull-knative-serving-go-coverage

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.

pkg/reconciler/ingress/status.go Show resolved Hide resolved
Copy link

knative-prow-robot left a comment

@JRBANCEL: 1 warning.

In response to this:

/lint
/test pull-knative-serving-go-coverage

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.

pkg/reconciler/ingress/status.go Outdated Show resolved Hide resolved
Copy link

knative-prow-robot left a comment

@JRBANCEL: 0 warnings.

In response to this:

/lint
/test pull-knative-serving-go-coverage

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.

@JRBANCEL JRBANCEL changed the title [WIP] Probe Envoy pods to determine when a ClusterIngress is actually deployed Probe Envoy pods to determine when a ClusterIngress is actually deployed Jul 16, 2019
@JRBANCEL

This comment has been minimized.

Copy link
Contributor Author

JRBANCEL commented Jul 17, 2019

/test pull-knative-serving-integration-tests

Copy link
Contributor

markusthoemmes left a comment

Should we remove the retries on 404s to assert that this really works as intended?

See

if resp.StatusCode == http.StatusNotFound {
return false, nil
}
and the respective file in v1beta1.

if you choose to do that: Please leave the helper there even though it doesn't do anything currently. We can use it as a convenient hook until we have baked the networking flakiness some more and are sure we can take the extra wheels off.

@JRBANCEL

This comment has been minimized.

Copy link
Contributor Author

JRBANCEL commented Jul 17, 2019

Should we remove the retries on 404s to assert that this really works as intended?

Yes, it is planned. See last item in the TODO.
I'll most likely do it in another PR because it is a bit more complex than just removing the retries for 404.

@JRBANCEL JRBANCEL force-pushed the JRBANCEL:cluster-ingress-status branch from 5467969 to be182f5 Jul 25, 2019
@knative-prow-robot knative-prow-robot added size/XXL and removed size/XL labels Jul 25, 2019
@mattmoor

This comment has been minimized.

Copy link
Member

mattmoor commented Jul 28, 2019

Needs a rebase.

Copy link
Member

mattmoor left a comment

Seems like this still has a bunch of debugging stuff in it?

pkg/apis/serving/v1alpha1/service_lifecycle_test.go Outdated Show resolved Hide resolved
pkg/apis/serving/v1beta1/service_defaults_test.go Outdated Show resolved Hide resolved
resyncIngressOnVirtualServiceReady := func(vs *v1alpha3.VirtualService) {
// Reconcile when a VirtualService becomes ready
impl.EnqueueLabelOfClusterScopedResource(networking.ClusterIngressLabelKey)(vs)
}

This comment has been minimized.

Copy link
@mattmoor

mattmoor Jul 28, 2019

Member

Could this just be:

resyncIngressOnVirtualServiceReady := impl.EnqueueLabelOfClusterScopedResource(networking.ClusterIngressLabelKey)

This comment has been minimized.

Copy link
@JRBANCEL

JRBANCEL Jul 29, 2019

Author Contributor

Unfortunately, EnqueueLabelOfClusterScopedResource is func(obj interface{}), resyncIngressOnVirtualServiceReady expects a func(*v1alpha3.VirtualService)

@JRBANCEL JRBANCEL force-pushed the JRBANCEL:cluster-ingress-status branch from be182f5 to 85386cc Jul 29, 2019
@knative-prow-robot knative-prow-robot added size/XL and removed size/XXL labels Jul 29, 2019
@JRBANCEL JRBANCEL force-pushed the JRBANCEL:cluster-ingress-status branch from 85386cc to e8b03d7 Jul 29, 2019
@JRBANCEL

This comment has been minimized.

Copy link
Contributor Author

JRBANCEL commented Jul 29, 2019

@tcnghia, @mattmoor it is ready to be approved/lgtm 🚀

/test pull-knative-serving-istio-1.1-mesh
/test pull-knative-serving-istio-1.1-no-mesh
/test pull-knative-serving-istio-1.2-mesh
/test pull-knative-serving-istio-1.2-no-mesh

Copy link
Contributor

tcnghia left a comment

/approve

There is one left over from merge, but otherwise /lgtm

pkg/reconciler/ingress/resources/virtual_service.go Outdated Show resolved Hide resolved
@JRBANCEL

This comment has been minimized.

Copy link
Contributor Author

JRBANCEL commented Jul 29, 2019

/test pull-knative-serving-istio-1.1-mesh
/test pull-knative-serving-istio-1.1-no-mesh
/test pull-knative-serving-istio-1.2-mesh
/test pull-knative-serving-istio-1.2-no-mesh

@mattmoor

This comment has been minimized.

Copy link
Member

mattmoor commented Jul 29, 2019

/lgtm
/approve

Let's get it baking.

@knative-prow-robot

This comment has been minimized.

Copy link

knative-prow-robot commented Jul 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JRBANCEL, mattmoor, tcnghia

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:

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

@JRBANCEL

This comment has been minimized.

Copy link
Contributor Author

JRBANCEL commented Jul 29, 2019

/hold cancel 🚀

@tcnghia

This comment has been minimized.

Copy link
Contributor

tcnghia commented Jul 29, 2019

/hold cancel

@knative-prow-robot knative-prow-robot merged commit cbe9951 into knative:master Jul 29, 2019
12 checks passed
12 checks passed
cla/google All necessary CLAs are signed
pull-knative-serving-build-tests Job succeeded.
Details
pull-knative-serving-go-coverage Job succeeded.
Details
pull-knative-serving-integration-tests Job succeeded.
Details
pull-knative-serving-istio-1.1-mesh Job succeeded.
Details
pull-knative-serving-istio-1.1-no-mesh Job succeeded.
Details
pull-knative-serving-istio-1.2-mesh Job succeeded.
Details
pull-knative-serving-istio-1.2-no-mesh Job succeeded.
Details
pull-knative-serving-smoke-tests Job succeeded.
Details
pull-knative-serving-unit-tests Job succeeded.
Details
pull-knative-serving-upgrade-tests Job succeeded.
Details
tide In merge pool.
Details
@vagababov

This comment has been minimized.

Copy link
Contributor

vagababov commented Jul 29, 2019

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.