Inhibit issues with concurrent execs by hickeng · Pull Request #8098 · vmware/vic

hickeng · 2018-06-26T23:33:32Z

This PR contains two sets of changes:

long term code using updated extraconfig recursion tags
short term code addressing a serialization issue with the "started" field and palliative locking

[skip unit]
[specific ci=1-38-Docker-Exec,6-15-Syslog]
[parallel jobs=4]

Towards #7410

Investigating CI failures:

zjs · 2018-06-27T02:05:04Z

lib/portlayer/exec/base.go

+		// we special case this for now as we _presume_ that it's either a conflict
+		// of reconfigure with guest operation, or a conflict with another guest operation.
+		// These cases do not return a TaskInProgress or concurrent modification.
+		return ConcurrentAccessError{errors.New("invalid state from start guest program")}


Would there be any benefit to logging err or including it in the new error being constructed here? (E.g., to allow us to use log data to validate the presumption described in the comment above if something doesn't seem to be working quite as expected.)

zjs · 2018-06-27T02:08:27Z

lib/tether/config.go

 	IP *net.IPNet `vic:"0.1" scope:"read-only" key:"ip"`

 	// Actual IP address assigned
+	// TODO: should this skip decode? - if it's used to seed DHCP requests for semi-stable addressing then


Should we file a ticket to track this TODO?

Will remove the TODO - with the benefit of time I can recall that the non-persistent keys will never be written from the API side while the VM is powered on. This would only become necessary if we were to change that AND had a reason to update this field from the API side (I cannot think of one).

zjs · 2018-06-27T02:10:28Z

pkg/vsphere/tasks/waiter.go

+}
+
+// IsInvalidStateError is an error certifier function for errors coming back from vsphere. It checks for an InvalidStateFault
+func IsInvalidStateError(err error) bool {


Is there code that should be updated to call this now that it exists? (If so, that should probably be handled in a separate PR.)

There is, for example lib/portlayer/storage/vsphere/toolbox_common.go. Opened #8099 for it.

zjs · 2018-06-27T02:11:32Z

pkg/vsphere/tasks/waiter.go

+// IsInvalidStateError is an error certifier function for errors coming back from vsphere. It checks for an InvalidStateFault
+func IsInvalidStateError(err error) bool {
+	if soap.IsVimFault(err) {
+		_, ok1 := soap.ToVimFault(err).(*types.InvalidState)


In the other similar methods, we also check for the *types.____Fault here. Is there a reason we don't check for types.InvalidStateFault in this case? (If so, that would be good to document in a comment here so that future readers don't assume it's a mistake.)

iirc types.InvalidStateFault does not implement GetMethodFault() method so it won't compile. I confess I've not dug into why this one is not the same as the others. @dougm ?

zjs · 2018-06-27T02:13:09Z

pkg/vsphere/tasks/waiter.go

+		return ok1 || ok2 || soap.ToSoapFault(err).String == "vim.fault.InvalidPowerState" ||
+			soap.ToSoapFault(err).String == "vim.fault.InvalidPowerState"
+	}
+	return false


This whole method seems to be basically repeated many times. Would there be value in defining a helper that takes the four "variable" pieces (two interfaces, two strings) as arguments? (If so, that should probably be handled in a separate PR.)

The entire thing should be in govmomi. I think it would end up being either significantly rewritten to use reflection (viable but much harder to read) or would take four types as the pointers are separate types in this case.

I'd prefer just to move it all to govmomi and make it @dougm's problem ;) Opened #8099 for follow up.

hickeng · 2018-06-27T15:19:42Z

1-09-Docker-Attach

Attach with short input
VCH: VCH-19517-3070
Bridge: VCH-19517-705
Container: vigilant_yonath-6c2994112e23
Command:

docker -H dhcp-wdc-pod2-haas01-76.eng.vmware.com:2376 --tls start -ai 6c2994112e231402e60f218368f19b120b5b5f73721e5d44a04ba46322c9807a < /tmp/tmp.0k7B3JBWly/fifo

Stderr: Unrecognized input header: 83

Personna:

op=351.18 - container create
op=351.19 - container inspect

op=351.20 - container attach
op=351.21 - container start
==> Event Monitor received eventID(339.76) for container(6c2994112e23) - Started
op=351.20 - container attach - end
op=351.21 - container start - end

op=351.22 - exit code (from event monitor)
==> die event for container(6c2994112e23) with exitCode[0]

op=351.23 - container inspect

op=351.24 - container attach
op=351.25 - container start
==> Event Monitor received eventID(339.92) for container(6c2994112e23) - Started
op=351.24 - container attach - end
op=351.26 - exit code (from event monitor)
==> die event for container(6c2994112e23) with exitCode[0]
op=351.25 - container start - end

op=351.27 - container inspect

op=351.28 - container attach
==> Cannot complete operation due to concurrent modification by another operation
op=351.28 - container attach - end
op=351.29 - container start
==> Event Monitor received eventID(339.107) for container(6c2994112e23) - Started
---> We got `Started` instead of `Powered On` because the attach failed, meaning we don't enter into RunBlocking state.

Questions:

why didn't the attach retry?
- IsConcurrentAccessError method was not working correctly
what was the conflicting operations that caused the concurrent access?
- The network unbind triggered by the power off event for the prior iteration - this is a perfectly acceptable occurrence so why isn't the retry handling it - will answer in question above.

Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important. (cherry picked from commit 5271ea7)

There are outstanding issues to address with concurrent exec. This work is palliative rather than an actual fix. Removes checking for "started" in the status string - we reliably see this field not propagating to the property collector despite being logged as set in the tether. This _only_ applies to execs at this time as that is the only path calling task.State (via InspectTask). Adds locking around dispatch of execs, with a timeout, to serialize that initial dispatch path against a single container. If the timeout expires it reverts to current behaviour and relies on concurrent modification and retry. (cherry picked from commit f6c176f)

Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important.

There are outstanding issues to address with concurrent exec. This work is palliative rather than an actual fix. Removes checking for "started" in the status string - we reliably see this field not propagating to the property collector despite being logged as set in the tether. This _only_ applies to execs at this time as that is the only path calling task.State (via InspectTask). Adds locking around dispatch of execs, with a timeout, to serialize that initial dispatch path against a single container. If the timeout expires it reverts to current behaviour and relies on concurrent modification and retry.

…e#8101) Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important. (cherry picked from commit 014952b)

There are outstanding issues to address with concurrent exec. This work is palliative rather than an actual fix. Removes checking for "started" in the status string - we reliably see this field not propagating to the property collector despite being logged as set in the tether. This _only_ applies to execs at this time as that is the only path calling task.State (via InspectTask). Adds locking around dispatch of execs, with a timeout, to serialize that initial dispatch path against a single container. If the timeout expires it reverts to current behaviour and relies on concurrent modification and retry. (cherry picked from commit c99f021)

Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important.

There are outstanding issues to address with concurrent exec. This work is palliative rather than an actual fix. Removes checking for "started" in the status string - we reliably see this field not propagating to the property collector despite being logged as set in the tether. This _only_ applies to execs at this time as that is the only path calling task.State (via InspectTask). Adds locking around dispatch of execs, with a timeout, to serialize that initial dispatch path against a single container. If the timeout expires it reverts to current behaviour and relies on concurrent modification and retry.

…e#8101) Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important. (cherry picked from commit f907974)

There are outstanding issues to address with concurrent exec. This work is palliative rather than an actual fix. Removes checking for "started" in the status string - we reliably see this field not propagating to the property collector despite being logged as set in the tether. This _only_ applies to execs at this time as that is the only path calling task.State (via InspectTask). Adds locking around dispatch of execs, with a timeout, to serialize that initial dispatch path against a single container. If the timeout expires it reverts to current behaviour and relies on concurrent modification and retry. (cherry picked from commit c99f021)

Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important. (cherry picked from commit f907974)

There are outstanding issues to address with concurrent exec. This work is palliative rather than an actual fix. Removes checking for "started" in the status string - we reliably see this field not propagating to the property collector despite being logged as set in the tether. This _only_ applies to execs at this time as that is the only path calling task.State (via InspectTask). Adds locking around dispatch of execs, with a timeout, to serialize that initial dispatch path against a single container. If the timeout expires it reverts to current behaviour and relies on concurrent modification and retry. (cherry picked from commit c99f021)

Makes use of extraconfig update for suppressing decode of fields into existing structures to prevent overwriting of in-memory state updates during a reload. This is necessary because there's no test-and-set guarantees between API and guest side updates with guestinfo. namespacedb would address this at an infrastructure level. Adds mapping of InvalidState that we can receive when multiple guest operations collide to a concurrent modification so that a retry can be attempted by the caller. Handling of guest operations does not trigger TaskInProgress or ConcurrentModification as we'd expected from the infrastructure. Updates the unit tests to use the structure without the suppression of decoding - the differentiation wasn't important previously but now the structure handling is asymmetric depending on whether it's tether or API so the correct pacakge reference is now important.

hickeng requested a review from a team as a code owner June 26, 2018 23:33

vmwclabot added the cla-not-required label Jun 26, 2018

hickeng force-pushed the 7410-minimal branch 2 times, most recently from 9a6e0ba to c554f91 Compare June 27, 2018 01:26

zjs reviewed Jun 27, 2018

View reviewed changes

hickeng mentioned this pull request Jun 27, 2018

Update codebase to use common vSphere error type tests #8099

Open

2 tasks

hickeng force-pushed the 7410-minimal branch 2 times, most recently from 6d40464 to 6950f83 Compare June 27, 2018 04:08

zjs approved these changes Jun 27, 2018

View reviewed changes

hickeng force-pushed the 7410-minimal branch 4 times, most recently from d5c63bd to f6c176f Compare June 27, 2018 18:03

hickeng mentioned this pull request Jun 27, 2018

Cherry-pick a variety of changes in 1.4.1 (concurrent exec) #8101

Merged

hickeng force-pushed the 7410-minimal branch from f6c176f to c99f021 Compare June 27, 2018 23:37

hickeng added 2 commits June 27, 2018 17:35

hickeng force-pushed the 7410-minimal branch from c99f021 to 579a6cd Compare June 28, 2018 00:38

lcastellano approved these changes Jun 28, 2018

View reviewed changes

hickeng merged commit fddf519 into vmware:master Jun 28, 2018

hickeng mentioned this pull request Feb 21, 2019

consistent docker exec failure after docker cp failures. #7878

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inhibit issues with concurrent execs#8098

Inhibit issues with concurrent execs#8098
hickeng merged 2 commits intovmware:masterfrom
hickeng:7410-minimal

hickeng commented Jun 26, 2018 •

edited

Loading

Uh oh!

zjs Jun 27, 2018

Uh oh!

zjs Jun 27, 2018

Uh oh!

hickeng Jun 27, 2018

Uh oh!

zjs Jun 27, 2018

Uh oh!

hickeng Jun 27, 2018

Uh oh!

zjs Jun 27, 2018

Uh oh!

hickeng Jun 27, 2018

Uh oh!

zjs Jun 27, 2018

Uh oh!

hickeng Jun 27, 2018 •

edited

Loading

Uh oh!

hickeng commented Jun 27, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

hickeng commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickeng Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hickeng commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hickeng commented Jun 26, 2018 •

edited

Loading

hickeng Jun 27, 2018 •

edited

Loading

hickeng commented Jun 27, 2018 •

edited

Loading