Skip to content
This repository was archived by the owner on Feb 22, 2022. It is now read-only.

[stable/redis] Correct master host env value#19843

Closed
WyriHaximus wants to merge 1 commit intohelm:masterfrom
WyriHaximus-labs:stable/redis-REDIS_MASTER_HOST-fix
Closed

[stable/redis] Correct master host env value#19843
WyriHaximus wants to merge 1 commit intohelm:masterfrom
WyriHaximus-labs:stable/redis-REDIS_MASTER_HOST-fix

Conversation

@WyriHaximus
Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

The REDIS_MASTER_HOST env variable currently passed to slaves currently points to a non-existing service namely redis-master-0.redis-headless.default.svc.local. Which is a mix of pod names and services that is never going to work. So this PR changes that to redis-master.default.svc.local targeting the redis master service.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [stable/mychartname])

Signed-off-by: Cees-Jan Kiewiet <ceesjank@gmail.com>
@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 3, 2020
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: WyriHaximus
To complete the pull request process, please assign desaintmartin
You can assign the PR to them by writing /assign @desaintmartin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details 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

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

Hi @WyriHaximus. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 3, 2020
@WyriHaximus
Copy link
Copy Markdown
Contributor Author

/assign @desaintmartin

@juan131
Copy link
Copy Markdown
Collaborator

juan131 commented Jan 3, 2020

Hi @WyriHaximus

That's a resolvable hostname since we're using a "headless" service to add those entries in the DNS configuration. In other words, it creates a hostname that can be used to target a specific pod.

Here you have an example:

$ helm install stable/redis --name redis
$ kubectl get svc
NAME             TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)    AGE
...
redis-headless   ClusterIP   None            <none>        6379/TCP   3m36s
redis-master     ClusterIP   10.96.143.224   <none>        6379/TCP   3m36s
redis-slave      ClusterIP   10.96.113.236   <none>        6379/TCP   3m36s
$ kg endpoints
NAME             ENDPOINTS                                         AGE
...
redis-headless   172.17.0.5:6379,172.17.0.6:6379,172.17.0.7:6379   3m41s
redis-master     172.17.0.5:6379                                   3m41s
redis-slave      172.17.0.6:6379,172.17.0.7:6379                   3m41s

As you can see there are 3 services (one for master pods, one for slave pods, and there's a headless service).

We can check that we can resolve every hostname:

$ kubectl run --restart=Never -it busybox --image=busybox -- sh
/ # nslookup redis-master.default.svc.cluster.local
Server:		10.96.0.10
Address:	10.96.0.10:53

Name:	redis-master.default.svc.cluster.local
Address: 10.96.143.224
...
/ # nslookup redis-master-0.redis-headless.default.svc.cluster.local
Server:		10.96.0.10
Address:	10.96.0.10:53

Name:	redis-master-0.redis-headless.default.svc.cluster.local
Address: 172.17.0.5

As you can see, redis-master.default.svc.cluster.local resolves to the master service IP: 10.96.143.224; while redis-master-0.redis-headless.default.svc.cluster.local resolves to the master pod IP: 172.17.0.5

@juan131
Copy link
Copy Markdown
Collaborator

juan131 commented Jan 3, 2020

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2020
@WyriHaximus
Copy link
Copy Markdown
Contributor Author

Hey @juan131,

Thank you for your detailed response. When I started doing test runs on how to set up my personal kubernetes cluster this was one of the things I ran into using minikube. At the time installing it with helm install redis stable/redis --values https://raw.githubusercontent.com/helm/charts/master/stable/redis/values-production.yaml would yield the first error of this comment: #17245 (comment)

Fast forward to yesterday I did some light reading on headless services and the idea excites me. But I assumed you could only do headless.NAMESPACE.svc.cluster.local not POD.headless.NAMESPACE.svc.cluster.local. Hence this PR.

Now today after you commented I started doing some experiments. That helm install works on minikube kudo's for that 👍 . But it doesn't on digitalocean's hosted kubernetes, which I'm using for my cluster. Did a test on a fresh cluster to be sure. But the nslookup redis-master-0.redis-headless.default.svc.cluster.local inside busybox doesn't return anything. So I'm going to look into why it's failing there specifically.

@WyriHaximus
Copy link
Copy Markdown
Contributor Author

Did some more digging, it's for sure not the issue I thought that had to be fixed in this PR. The IP resolves but it can't connect for some reason. Will keep digging on why this is happening

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[stable/redis] Slave fails to connect to master with connection timeout

5 participants