Wait for ovs-vswitchd PID before calling ovs-appctl

[tnqn]

Otherwise the call may fail and crash the process.

Besides, it cleans up the OVS run files on exit to avoid stale PID
from being used.

Signed-off-by: Quan Tian qtian@vmware.com

Fixes #2694

[codecov-commenter]

Codecov Report

Merging #2695 (f010674) into main (dded211) will increase coverage by 6.17%.
The diff coverage is 43.75%.

@@            Coverage Diff             @@
##             main    #2695      +/-   ##
==========================================
+ Coverage   59.50%   65.67%   +6.17%     
==========================================
  Files         285      285              
  Lines       23020    26380    +3360     
==========================================
+ Hits        13697    17326    +3629     
+ Misses       7865     7437     -428     
- Partials     1458     1617     +159     

Flag	Coverage Δ
e2e-tests	57.18% <43.75%> (?)
kind-e2e-tests	48.40% <46.66%> (+1.65%)	⬆️
unit-tests	41.00% <0.00%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/ovs/ovsctl/interface.go	0.00% <0.00%> (ø)
pkg/ovs/ovsctl/ovsctl_others.go	57.14% <50.00%> (+7.14%)	⬆️
pkg/controller/egress/ipallocator/allocator.go	65.00% <0.00%> (-15.42%)	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	77.64% <0.00%> (-13.79%)	⬇️
pkg/legacyapis/core/v1alpha2/register.go	69.23% <0.00%> (-10.77%)	⬇️
pkg/apis/stats/register.go	71.42% <0.00%> (-10.39%)	⬇️
pkg/legacyapis/stats/register.go	71.42% <0.00%> (-10.39%)	⬇️
pkg/ovs/openflow/ofctrl_meter.go	33.84% <0.00%> (-10.16%)	⬇️
pkg/legacyapis/security/v1alpha1/register.go	73.33% <0.00%> (-10.00%)	⬇️
.../registry/networkpolicy/clustergroupmember/rest.go	78.26% <0.00%> (-9.98%)	⬇️
... and 271 more

[tnqn]

/test-all

[jianjuns]

What code at agent startup runs appctl? Why it crashes? Should we just retry there instead?

[antoninbas]

LGTM

[tnqn]

What code at agent startup runs appctl? Why it crashes? Should we just retry there instead?

It's a step that validates whether OVS supports NAT. The linked issue #2694 has the detail of the problem.
We can retry there but it seems not more readable because the higher layer function don't know details of the error and may look like retrying blindly. Then people may start adding retry for other steps too. Since this is kind of expected and recoverable, I feel hiding it from caller and retrying directly should be simpler.

[tnqn]

/test-all

[tnqn]

@antoninbas I found I never got the expected "Waited for" log when verifying the change and it still crashed sometimes, which was because I was checking the condition func's return value err instead of readErr.
I have fixed it and verified it, PTAL.