Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7.8.x patch2 #3017

Merged
merged 4 commits into from
Mar 21, 2019
Merged

7.8.x patch2 #3017

merged 4 commits into from
Mar 21, 2019

Conversation

matthewrmshin
Copy link
Contributor

Back port #2945 #2976 #2988.

sadielbartholomew and others added 4 commits March 19, 2019 14:12
Users are experiencing issues on one of the platforms we deloy Cylc
where remote submission fails intermittently with a file not found error
complaining that a temporary file cannot be found. (The file is used for
piping service files via STDIN (to the SSH) command.)

In this change, we pass the temporary file handle directly instead of
passing the name of the temporary file to the process pool. This appears
to fix the issue. (Note: Before cylc#2590, we were unable to pass the file
handle because the context needs to be serialised for the
multiprocessing.Pool. This is no longer a requirement in our own
subprocess pool, so it is now safer to pass the handle.)

Add basic unit tests for running command with STDIN in subprocess pool.
Rename SuiteProcPool to SubProcPool.
@matthewrmshin matthewrmshin added this to the next-release milestone Mar 19, 2019
@matthewrmshin matthewrmshin self-assigned this Mar 19, 2019
@hjoliver
Copy link
Member

(@cylc/core - can we agree to do duplicate PRs at the time, in future, to avoid the risk of forgetting to do a combined back-port PR like this... or do you have a foolproof way keeping track of these @matthewrmshin )

@kinow
Copy link
Member

kinow commented Mar 20, 2019

Roger that 👍

@sadielbartholomew
Copy link
Collaborator

can we agree to do duplicate PRs at the time, in future, to avoid the risk of forgetting to do a combined back-port PR like this... or do you have a foolproof way keeping track of these @matthewrmshin

Sorry to jump in with a comment directed to Matt, but I am thinking that, despite best intentions given how busy we all are, it is easy to foresee occasions where we might forget to put up the duplicate PR. So setting up some sort of reminder, at least if there are no better ideas, would be sensible.

To do this we could use a webhook to trigger off the event of someone pushing to the master branch, emailing them (or adding an automated comment to the PR, but that would become rather spammy) to ask them to consider whether the PR in question should be back ported. Since both the PR author & reviewing merger strictly "push" to master, there will be two people notified, so that should catch most instances requiring the back port even if someone overlooked their email.

This suggestion may be overkill, but it is a possible solution.

Another would be to nominate a "back port rep" so to speak, being someone to keep an eye on all merges to master & ensure everything necessary is registered to be, and eventually gets, back ported. If there are no volunteers we can always assign the responsibility & re-allocate every so often so we all take a turn. Matt seems to have kindly took on this job so far, so it seems courteous to him if we pass on the baton?

@hjoliver
Copy link
Member

I should have said first, thanks to Matt for doing this so far, given that he never officially volunteered for the job.

I'm not too keen on the commit-hook idea as the vast majority of commits to master will soon be for cylc-8 and should not require back-porting. But a few will!

Maybe all reviewers should just keep this in mind, and request a back-port PR if one is necessary and hasn't been done yet - and don't approve the original PR until both are up?

The back-port one is generally pretty easy and could even be done by someone else (because most of these should just be small bug fixes).

@hjoliver
Copy link
Member

Or at the very least, put up an issue for the back-port and assign it to the next-release milestone. Then we can't forget it. This could be done by the PR author or a reviewer.

@matthewrmshin
Copy link
Contributor Author

The current arrangement is OK - and it helps me keep track of the release. It is a manual process that involves me interpreting the logs and the PRs - and making some judgement on what should be back ported. Not the best process, but it is not something that takes long to do.

@matthewrmshin
Copy link
Contributor Author

(And if we agree that PRs that should be back ported would have separate duplicated back port PRs, then we should not have more of this.)

Please aim to get this merged soon - given that the original changes were already merged to master - we should aim to only pick out any glaring Python 2 compat issues on this back port.

@hjoliver
Copy link
Member

(And if we agree that PRs that should be back ported would have separate duplicated back port PRs, then we should not have more of this.)

I certainly agree we should have this (as you've no doubt noticed, above :-).

Mulling over the options again, my preference is to make it part of the PR review working process. For every PR, the reviewer should think: does this change need back porting to 7.8.x. If it does, request that either a back-port PR is done or a back-port issue raised to next-release, before giving approval. Then we can't forget. Bingo 💥

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and passes all tests.

@sadielbartholomew sadielbartholomew merged commit 2ecb7be into cylc:7.8.x Mar 21, 2019
@matthewrmshin matthewrmshin deleted the 7.8.x-patch2 branch June 3, 2019 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants