Refactor OCPControllerManager to conform to the internal service manager #260
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
husky-parul
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
pkg/util/net.go
Outdated
| status := 0 | ||
| err := wait.Poll(5*time.Second, 120*time.Second, func() (bool, error) { | ||
| timeout := 3 * time.Second | ||
| _, err := tcpnet.DialTimeout("tcp", tcpnet.JoinHostPort("127.0.0.1", "8445"), timeout) |
pkg/util/net.go
Outdated
| _, err := tcpnet.DialTimeout("tcp", tcpnet.JoinHostPort("127.0.0.1", "8445"), timeout) | ||
|
|
||
| if err == nil { | ||
| status = 200 |
There was a problem hiding this comment.
200 is HTTP status. If this is for TCP port probe, returning true/false is good to go
pkg/util/net.go
Outdated
| func RetryTCPConnection(host string, port string) int { | ||
| status := 0 | ||
| err := wait.Poll(5*time.Second, 120*time.Second, func() (bool, error) { | ||
| timeout := 3 * time.Second |
There was a problem hiding this comment.
can you check what's the default timeout in net/http? 3 seconds could be too short
There was a problem hiding this comment.
The default timeout is no timeout so it will keep trying infinitely.
golang/go#24138
Increasing it to 30 * time.Second, I see it is the recommended timeout -> https://pkg.go.dev/net/http
|
/approve |
openshift-ci
bot
added
ok-to-test
|
/retest |
hack/all-in-one/README.md
Outdated
| @@ -68,17 +68,17 @@ kubectl get pods -A | |||
| ### Access the cluster on the host | |||
| #### Linux | |||
| ```bash | |||
| export KUBECONFIG=$(podman volume inspect ushift-vol --format "{{.Mountpoint}}")/microshift/resources/kubeadmin/kubeconfig | |||
| export KUBECONFIG=$(podman volume inspect microshift-data --format "{{.Mountpoint}}")/microshift/resources/kubeadmin/kubeconfig | |||
There was a problem hiding this comment.
I will. Can't get the tests to pass though. 🤔
There was a problem hiding this comment.
can you revert these changes?
Will the changes to volume name will go in a different PR or we don't need this change at all? All other places use microshift-data as the volume name instead of ushift-vol
pkg/util/net.go
Outdated
| _, err := tcpnet.DialTimeout("tcp", tcpnet.JoinHostPort(host, port), timeout) | ||
|
|
||
| if err == nil { | ||
| status = false |
| return false, nil | ||
| }) | ||
| if err != nil && err == wait.ErrWaitTimeout { | ||
| logrus.Warningf("Endpoint is not returning any status code") |
There was a problem hiding this comment.
if timeout, this should return false to tell the caller the connection is not ready
…ger openshift#239 Signed-off-by: Parul <[email protected]>
Signed-off-by: Parul <[email protected]>
husky-parul
force-pushed
the
ocp-cm
branch
2 times, most recently
from
78207fd to
d66ecb7
Compare
|
/retest |
Signed-off-by: Parul <[email protected]>
|
/test e2e-openshift-conformance-sig-instrumentation |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rootfs 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 |
|
@oglok @mangelajo please take a final review |
Refactor OCPControllerManager to conform to the internal service manager #239
Signed-off-by: Parul [email protected]
Which issue(s) this PR addresses:
Closes#239