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

[stable/consul] Pass HttpPort to consul#3375

Merged
k8s-ci-robot merged 3 commits intohelm:masterfrom
jonstacks:consul-http-port
Jan 20, 2018
Merged

[stable/consul] Pass HttpPort to consul#3375
k8s-ci-robot merged 3 commits intohelm:masterfrom
jonstacks:consul-http-port

Conversation

@jonstacks
Copy link
Copy Markdown
Contributor

Fixes #1614.

We were not passing the HTTP port through to consul. This fixes it so that consul serves is HTTP interface on the user supplied port.

@k8s-ci-robot k8s-ci-robot added 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 18, 2018
@lachie83 lachie83 self-assigned this Jan 18, 2018
@lachie83
Copy link
Copy Markdown
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 18, 2018
@lachie83
Copy link
Copy Markdown
Contributor

/ltgm

@jonstacks
Copy link
Copy Markdown
Contributor Author

jonstacks commented Jan 18, 2018

@lachie83,

From what I can tell, only 2/3 pods in the stateful set were up when the tests ran. I'm wondering if maybe we shouldn't update the helm install command with a --wait flag? Do you think re-running the test might fix it?

Relevant lines from the test:

W0118 23:40:37.065] ++ kubectl get pods --namespace pr-3375-4762 -o 'jsonpath={.items[*].status.containerStatuses[?(@.ready!=true)].name}'
I0118 23:40:37.166] INFO: All pods entered the Running state
I0118 23:40:37.413] INFO: All containers are ready
W0118 23:40:37.513] + UNREADY_CONTAINERS=
W0118 23:40:37.514] + '[' -n '' ']'
W0118 23:40:37.514] + echo 'INFO: All containers are ready'
W0118 23:40:37.514] + exit 0
W0118 23:40:37.514] + kubectl get pods --namespace pr-3375-4762
I0118 23:40:37.722] NAME                   READY     STATUS    RESTARTS   AGE
I0118 23:40:37.723] consul-4762-consul-0   1/1       Running   0          28s
I0118 23:40:37.723] consul-4762-consul-1   0/1       Pending   0          0s
W0118 23:40:37.823] + kubectl get svc --namespace pr-3375-4762
I0118 23:40:38.011] NAME                    CLUSTER-IP     EXTERNAL-IP   PORT(S)                                                                            AGE
I0118 23:40:38.011] consul-4762-consul      None           <none>        8500/TCP,8400/TCP,8301/TCP,8301/UDP,8302/TCP,8302/UDP,8300/TCP,8600/TCP,8600/UDP   29s
I0118 23:40:38.011] consul-4762-consul-ui   10.3.253.253   <nodes>       8500:32432/TCP                                                                     29s
W0118 23:40:38.112] + kubectl get deployments --namespace pr-3375-4762
W0118 23:40:38.304] No resources found.
W0118 23:40:38.307] + kubectl get endpoints --namespace pr-3375-4762
I0118 23:40:38.604] NAME                    ENDPOINTS                                                  AGE
I0118 23:40:38.604] consul-4762-consul      10.0.7.99:8600,10.0.7.99:8400,10.0.7.99:8302 + 6 more...   29s
I0118 23:40:38.604] consul-4762-consul-ui   10.0.7.99:8500                                             29s
W0118 23:40:38.705] + helm test consul-4762

@jonstacks
Copy link
Copy Markdown
Contributor Author

/retest

@jonstacks
Copy link
Copy Markdown
Contributor Author

Had to update part of the tests to get this to pass. Not sure if this is the best place to do it or if we should make modifications to the /test/verify-release.sh which checks for all pods being available.

@lachie83
Copy link
Copy Markdown
Contributor

@jonstacks --wait has had issue between releases in the past. How about we make the helm test script handle the cluster coming up for some predetermined time -- https://github.com/kubernetes/charts/blob/master/stable/consul/templates/test-config.yaml

@jonstacks
Copy link
Copy Markdown
Contributor Author

@lachie83, Sounds good. I'll Remove the wait and update the consul test.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 19, 2018
@jonstacks
Copy link
Copy Markdown
Contributor Author

@lachie83,

I think this should be good to go now.

@lachie83
Copy link
Copy Markdown
Contributor

/approve

@lachie83
Copy link
Copy Markdown
Contributor

Thanks!

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2018
@jonstacks
Copy link
Copy Markdown
Contributor Author

@lachie83, I'm thinking your /lgtm from earlier didn't register.

@lachie83
Copy link
Copy Markdown
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2018
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonstacks, lachie83

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

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 9cea3ca into helm:master Jan 20, 2018
@jonstacks jonstacks deleted the consul-http-port branch January 20, 2018 05:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HttpPort parameter doesn't works

3 participants