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

Wait for caches to sync when the Ingress Controller starts #1457

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

pleshakov
Copy link
Contributor

Proposed changes

When the Ingress Controller is ready, its readiness probe succeeds, and it starts accepting client traffic.

The IC becomes ready when it drains the work queue at the start - this means the IC generated the configuration for all Ingress and other relevant resources in the cluster.

However, because the IC didn't wait for the caches to sync before starting processing the work queue, the work queue could contain only a fraction of the resources in the cluster. As a result, the IC would drain the queue quickly and mistakenly become ready. This is a bug, as not every relevant resource was processed.

Now, before starting the work queue, the IC waits for the caches to sync. This fixes the problem

Changes:

  • Use shared informer factories for most of the controllers (informers), instead of creating them manually.
  • For controllers that need special parameters, continue creating them manually
  • Wait for the caches of all controllers to sync before starting processing the work queue

@pleshakov pleshakov added the bug An issue reporting a potential bug label Mar 13, 2021
Copy link
Contributor

@soneillf5 soneillf5 left a comment

Choose a reason for hiding this comment

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

Approved subject to tests passing.

@pleshakov pleshakov force-pushed the fix-readiness-check-2 branch 3 times, most recently from db092f0 to 663176e Compare March 15, 2021 21:37
vs_crd_name = get_name_from_yaml(f"{DEPLOYMENTS}/common/crds-v1beta1/k8s.nginx.org_virtualservers.yaml")
vsr_crd_name = get_name_from_yaml(f"{DEPLOYMENTS}/common/crds-v1beta1/k8s.nginx.org_virtualserverroutes.yaml")
pol_crd_name = get_name_from_yaml(f"{DEPLOYMENTS}/common/crds-v1beta1/k8s.nginx.org_policies.yaml")
ts_crd_name = get_name_from_yaml(f"{DEPLOYMENTS}/common/crds-v1beta1/k8s.nginx.org_transportservers.yaml")
Copy link
Contributor

@vepatel vepatel Mar 16, 2021

Choose a reason for hiding this comment

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

do we need ts crd in AP or is it mandatory now to have all crds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whenever -enable-custom-resources is set to true, it is necessary to register all custom resources CRDs. otherwise, the informers will not sync, and the IC will not become ready

Because of the changes in 70e6b8c, the Ingress Controller will not
become ready until all informers are synced.

An informer for a CR will not sync until the corresponding CRD
exists in the cluster.

This commit ensures that all required CRDs are created prior to
deploying the IC, so that CR informers can sync.
@pleshakov pleshakov merged commit 78d3c5f into master Mar 16, 2021
@pleshakov pleshakov deleted the fix-readiness-check-2 branch March 16, 2021 18:33
@pleshakov pleshakov changed the title Wait for caches to sync when IC starts Wait for caches to sync when the Ingress Controller starts Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants