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

ISSUE-779 - check for agent status before deregistration #795

Closed
wants to merge 1 commit into from

Conversation

TomasKohout
Copy link

Changes proposed in this PR:
Potential fix for ISSUE-779

How I've tested this PR:
Deployed to our DEV cluster and ungracefully killed a node.

system-consul/consul-consul-connect-injector-webhook-deployment-755d55bdzb245[sidecar-injector]: 2021-10-19T12:29:11.955Z	INFO	controller.endpoints	retrieved	{"name": "csi-rbdplugin-metrics", "ns": "system-storage"}
system-consul/consul-consul-connect-injector-webhook-deployment-755d55bdzb245[sidecar-injector]: 2021-10-19T12:29:11.959Z	INFO	controller.endpoints	**Consul agent is not ready, skipping sync**	{"consul-agent": "consul-consul-8kw6s"}

How I expect reviewers to test this PR:

On cluster with consul agent and connected pods, ungracefully restart one of a node and see if new consul connected pods are able to start.

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 19, 2021

CLA assistant check
All committers have signed the CLA.

@david-yu
Copy link
Contributor

Hi @TomasKohout thank you for the PR! We saw the issue and it looks like we may be able to repro based on the description you provided in the PR.

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

This looks great! thanks for working on a fix for it!

I think it'd be good to add a test for this, perhaps a case in the TestReconcileDeleteEndpoint test that checks that if the consul client pod is not ready, the service is not deregistered. Otherwise, this is good to merge.

@@ -521,7 +508,6 @@ func TestProcessUpstreams(t *testing.T) {
AllowK8sNamespacesSet: mapset.NewSetWith("*"),
DenyK8sNamespacesSet: mapset.NewSetWith(),
EnableConsulNamespaces: tt.consulNamespacesEnabled,
EnableConsulPartitions: tt.consulPartitionsEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks like it's not intentional. I think we want to keep this and the the corresponding test case param above.

@ishustava
Copy link
Contributor

Thanks so much for the contribution @TomasKohout !!

I'm going to close this PR as I've included your change in #991

@ishustava ishustava closed this Jan 26, 2022
@TomasKohout
Copy link
Author

Thank you so much! 🙂 I'm sorry that I couldn't get this done in a reasonable amount of time though. Shame on me. 😬😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants