Skip to content

Comments

manually revert changes from PR #3902#4075

Merged
jamieklassen merged 2 commits intomasterfrom
fix-topgun-4070
Jun 27, 2019
Merged

manually revert changes from PR #3902#4075
jamieklassen merged 2 commits intomasterfrom
fix-topgun-4070

Conversation

@ddadlani
Copy link
Contributor

@ddadlani ddadlani commented Jun 27, 2019

The changes made in PR #3902 (fixing #3301) were not fully correct. The changes caused unused db.CreatingContainers to be created in the case where two identical get steps were run in quick succession. This broke the topgun build (issue #4070).

Since a manual merge was done after #3902 was merged in to master, we decided to manually revert the changes. This PR removes the container creation and locking logic from pool.FindOrChooseWorkerForContainer and puts the db.CreatingContainer creation back into worker.FindOrCreateContainer.

Fixes #4070

Signed-off-by: Divya Dadlani [email protected]

@ddadlani ddadlani requested review from jamieklassen and vito June 27, 2019 19:50
@jamieklassen jamieklassen merged commit fef0184 into master Jun 27, 2019
@jamieklassen jamieklassen deleted the fix-topgun-4070 branch June 27, 2019 20:34
@aledegano
Copy link
Contributor

@ddadlani @deniseyu do you plan to reintroduce #3902 fixing the issue described above?
I'm asking because I'm working on a PR that fixes #2928 which relies in the locking mechanism.

@marco-m
Copy link
Contributor

marco-m commented Jun 28, 2019

@aledeganopix4d the answer is explained at the bottom of #3301, which @ddadlani reopened after this revert.

@ddadlani
Copy link
Contributor Author

@aledeganopix4d yes we are going to find another solution for the locking mechanism that does not affect other things. We might even go ahead with the scheduling queue described in #3695 since that would guarantee one container placed at a time.

@marco-m
Copy link
Contributor

marco-m commented Jun 28, 2019

@ddadlani could you please inform us as soon as you took a decision on how to proceed? I am asking because this impacts our work related to #2928, for which a PoC is in PR #4076. Thanks :-) If you could have a look at PR #4076 I could beer++ that I owe you (or any beverage you might prefer) :-)

@ddadlani
Copy link
Contributor Author

@marco-m we are discussing it, we'll be sure to let you know what we decide. I'll also be sure to look at the PR before EOD today

@marco-m
Copy link
Contributor

marco-m commented Jun 28, 2019

Thanks Divya!

@jamieklassen jamieklassen added the release/no-impact This is an issue that never affected released versions (i.e. a regression caught prior to shipping). label Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release/no-impact This is an issue that never affected released versions (i.e. a regression caught prior to shipping).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix breaking topgun build caused by extra creating container

5 participants