cleanup function will return the code which ssh returns #445
Conversation
gardener-robot
added
needs/review
gardener-robot-ci-1
added
the
reviewed/ok-to-test
|
Hi @dansible ,
BTW the openstack infra itself is not very stable - in your case i saw you were unable to ssh to gctl-os shoot but today i can ssh without error, this is the same when i was doing gardenctl ssh openstack_node - sometimes the ssh failed for unknown reason...i didn't dig out why, retry will success....
|
pkg/cmd/ssh_openstack.go
Outdated
| } | ||
| defer a.cleanUpOpenstack(sshStatus) |
There was a problem hiding this comment.
what's the purpose of having defer as last statement of the function? I would refrain from moving the deferred cleanup call to the bottom.
There was a problem hiding this comment.
during my test, if i move the defer a.cleanUpOpenstack(sshStatus) to above lines of value assignment of sshStatus , it won't get correct values.
Say if i keep defer a.cleanUpOpenstack(sshStatus) to original line, when some error occurs in ssh, sshStatus will be assigned with value 1 (for example) but in defer a.cleanUpOpenstack(sshStatus) the value of sshStatus is still 0
do you have a better idea how i can get sshStatus correctly passed to cleanUpOpenstack meanwhile i can keep the original line for the defer function? thanks!
There was a problem hiding this comment.
if you want to achieve this you need to pass a pointer
There was a problem hiding this comment.
thanks! i will have a try
There was a problem hiding this comment.
Hi @petersutter , i've modified the code using *int to pass values, thanks for your suggestion!
-Neo
neo-liang-sap
force-pushed
the
openstack-ssh-return-with-error-code
branch
from
6204f9b
to
1a544ae
Compare
gardener-robot-ci-2
added
needs/ok-to-test
…ction in ssh_openstack.go
gardener-robot
added
size/s
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-3
removed
the
reviewed/ok-to-test
|
@dansible , per our discussion in planning mtg this week, i implement a function to check whether IP and port are reachable (#448) , i implement this feature in this PR as this function will impact the overall exit status so i append this feature together in this PR. Please review it when you have time, thanks! |
pkg/cmd/utils.go
Outdated
| attemptCnt := 0 | ||
| for attemptCnt < 6 { | ||
| ncCmd := fmt.Sprintf("timeout 10 nc -vtnz %s %s", ip, port) | ||
| cmd := exec.Command("bash", "-c", ncCmd) |
There was a problem hiding this comment.
- Stop calling bash
- Use
exec. CommandContextto pass a context with a timeout to achieve the same behavior instead of callingtimeout - Does this work on windows? I'd rather prefer a native-go solution
gardener-robot-ci-1
added
the
reviewed/ok-to-test
|
thanks @petersutter , i used go native method to check whether the ip and port reachable |
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
| timeout := time.Second * 60 | ||
| conn, err := net.DialTimeout("tcp", net.JoinHostPort(ip, port), timeout) |
There was a problem hiding this comment.
and this works without having to dial/try multiple times within a certain amount of time?
There was a problem hiding this comment.
Yes, in my test it tries within 1 min, as I set in the timeout
gardener-robot
added
reviewed/lgtm
What this PR does / why we need it:
cleanup function will return the code which ssh returns
Which issue(s) this PR fixes:
Fixes #444 and #448
Special notes for your reviewer:
Release note: