Ensure that start() in StartAndAttach() is locked

[mheon]

StartAndAttach() runs start() in a goroutine, which can allow it to fire after the caller returns - and thus, after the defer to unlock the container lock has fired.

The start() call must occur while the container is locked, or else state inconsistencies may occur.

Fixes #3114

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[giuseppe]

thanks, tested locally and fixes the issue we've seen.

LGTM

[cevich]

running this through #3115

[cevich]

initial poking looks good. Next I'll find my bigger stick...

[cevich]

woops

[cevich]

LGTM (do not fully understand actual code changes though)

[mheon]

While I'm confident this did fix the issue in question, it didn't fix our CI timeouts

[TomSweeneyRedHat]

LGTM, assuming happy tests

[vrothberg]

Does not look like this should be merged.

[cevich]

@vrothberg this option is needed with -nodes 3 to make ginkgo log output from each node. Otherwise, if any node hangs for any reason, it's incredibly difficult to debug which test caused the problem.

[vrothberg]

Alright, that sounds important. The commit message mentions not to merge it, so we might need to change the message to avoid confusion.

[mheon]

Initially we just wanted it for debug, so I added the quick "HACK" message. Changed now.

[vrothberg]

Nit: Can we use a Mutex instead of a WaitGroup? The WaitGroup implies that we are waiting for multiple tasks to finish which does not seem to be the case.

[mheon]

On looking into this more: I'd prefer to stick with a WG - it's a lot more clear what's going on than with a mutex (Why am I unlocking this mutex inside of attachContainerSocket()? What locked it in the first place?). The control flow here is already complicated enough (took four hours to zero in on this bug) and I'd prefer not to make it any more so.

[cevich]

@mheon This will cause the ginkgo -debug logs to be collected inline with the CI task:

diff --git a/.cirrus.yml b/.cirrus.yml
index 51488996..95c33219 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -292,12 +292,14 @@ testing_task:
     setup_environment_script: '$SCRIPT_BASE/setup_environment.sh |& ${TIMESTAMP}'
     unit_test_script: '$SCRIPT_BASE/unit_test.sh |& ${TIMESTAMP}'
     integration_test_script: '$SCRIPT_BASE/integration_test.sh |& ${TIMESTAMP}'
+    ginkgo_node_logs_script: 'cat $SCRIPT_BASE/test/e2e/ginkgo-node-*.log || echo "Ginkgo node logs not found"'
     audit_log_script: 'cat /var/log/audit/audit.log || cat /var/log/kern.log'
     journalctl_b_script: 'journalctl -b'
 
     on_failure:
         failed_master_script: '$CIRRUS_WORKING_DIR/$SCRIPT_BASE/notice_master_failure.sh'
         # Job has already failed, don't fail again and miss collecting data
+        failed_ginkgo_node_logs_script: 'cat $SCRIPT_BASE/test/e2e/ginkgo-node-*.log || echo "Ginkgo node logs not found"'
         failed_audit_log_script: 'cat /var/log/audit/audit.log || cat /var/log/kern.log || echo "Uh oh, cat audit.log failed"'
         failed_journalctl_b_script: 'journalctl -b || echo "Uh oh, journalctl -b failed"'
 

[mheon]

Updated to include @cevich changes - Ginkgo is now in debug mode, with cirrus collecting debug logs, to aid in chasing flakes.

I'll hit the mutex comment from @vrothberg later today

[cevich]

Oops, that's a bad path: should be $CIRRUS_WORKING_DIR/test/e2e/ginkgo-node-*.log

[cevich]

Here too. Sorry 😞

[cevich]

In fact, we might want to consider adding -debug here as well.

[mheon]

@baude The cp tests aren't failing - I'm seeing a lot more failures from the rootless tests everywhere.

I'd say we merge this and #3091 and that should put CI right.

[baude]

/lgtm