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

Fix requester pays access by updating the google-cloud-nio library to the latest release #7700

Merged
merged 8 commits into from
Mar 9, 2022

Conversation

droazen
Copy link
Collaborator

@droazen droazen commented Feb 25, 2022

Fixes #6179

* com.google.cloud:google-cloud-nio:0.117.0-alpha -> 0.123.20
@droazen droazen self-assigned this Feb 25, 2022
@droazen
Copy link
Collaborator Author

droazen commented Feb 25, 2022

@lbergelson Looks like we have a failure in BigQueryUtilsUnitTest:

testQueryWithStorageAPI
java.lang.IllegalStateException: getTransportChannel() called when needsExecutor() is true
	at com.google.api.gax.grpc.InstantiatingGrpcChannelProvider.getTransportChannel(InstantiatingGrpcChannelProvider.java:194)
	at com.google.api.gax.rpc.ClientContext.create(ClientContext.java:241)
	at com.google.cloud.bigquery.storage.v1beta1.stub.EnhancedBigQueryStorageStub.create(EnhancedBigQueryStorageStub.java:108)
	at com.google.cloud.bigquery.storage.v1beta1.BigQueryStorageClient.<init>(BigQueryStorageClient.java:144)
	at com.google.cloud.bigquery.storage.v1beta1.BigQueryStorageClient.create(BigQueryStorageClient.java:125)
	at com.google.cloud.bigquery.storage.v1beta1.BigQueryStorageClient.create(BigQueryStorageClient.java:116)
	at org.broadinstitute.hellbender.utils.bigquery.StorageAPIAvroReader.<init>(StorageAPIAvroReader.java:51)
	at org.broadinstitute.hellbender.utils.bigquery.StorageAPIAvroReader.<init>(StorageAPIAvroReader.java:45)
	at org.broadinstitute.hellbender.utils.bigquery.BigQueryUtils.executeQueryWithStorageAPI(BigQueryUtils.java:394)
	at org.broadinstitute.hellbender.utils.bigquery.BigQueryUtils.executeQueryWithStorageAPI(BigQueryUtils.java:370)
	at org.broadinstitute.hellbender.utils.bigquery.BigQueryUtilsUnitTest.testQueryWithStorageAPI(BigQueryUtilsUnitTest.java:74)

I suspect we need to bump our BigQuery dependency in this PR as well -- I'll attempt it.

Fixes the failure in BigQueryUtilsUnitTest
@droazen
Copy link
Collaborator Author

droazen commented Feb 25, 2022

Pushed a commit that fixes the BigQueryUtilsUnitTest failure by upgrading several of our other Google dependencies. This may cause failures in other parts of our test suite -- we shall see.

@droazen
Copy link
Collaborator Author

droazen commented Feb 25, 2022

Looks like tests are green with the latest commit. Now we just need a test that would have caught #6179

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

👍

@jkobject
Copy link

jkobject commented Mar 4, 2022

Hi!

Thank you for making this fix.

This is a blocking error for our DepMap release starting in 1 week. Would you have an ETA for when a dockerized version of gatk will be available with the fix?

Best,

@droazen
Copy link
Collaborator Author

droazen commented Mar 4, 2022

@jkobject It turns out that to fix #6179 we need Google to patch the java-storage-nio library. Either googleapis/java-storage-nio#832 or googleapis/java-storage-nio#841 needs to be merged, and Google needs to do a release of the library before this will be fixed in GATK.

We are escalating this issue with Google support, and hope to have a fix in place soon.

@droazen
Copy link
Collaborator Author

droazen commented Mar 4, 2022

Update: Google just merged googleapis/java-storage-nio#841 and is doing a release. We'll update to the newer library version here and test it out to make sure it resolves the RP issue.

@droazen droazen changed the title Update the google-cloud-nio library to the latest version Fix requester pays access by updating the google-cloud-nio library to the latest release Mar 9, 2022
@droazen
Copy link
Collaborator Author

droazen commented Mar 9, 2022

👍 Will merge once tests pass

@droazen droazen merged commit c1190ac into master Mar 9, 2022
@droazen droazen deleted the lb_update_gnio branch March 9, 2022 19:48
@jkobject
Copy link

Thanks!
Do you know when an updated docker image with this fix will become available?
Is there an easy way to get that?

@droazen
Copy link
Collaborator Author

droazen commented Mar 10, 2022

@jkobject There will be an official release image later today when GATK 4.2.6 comes out. The latest gatk-nightly image already has the fix: https://hub.docker.com/r/broadinstitute/gatk-nightly/tags

@jkobject
Copy link

jkobject commented Mar 10, 2022

I took the 2022-03-10-4.2.5.0-13-g1c749b37f-NIGHTLY-SNAPSHOT from 18hours ago and got what looks to be the same error message.

mutect2.log

@droazen
Copy link
Collaborator Author

droazen commented Mar 10, 2022

@jkobject That appears to be a different error: "User project specified in the request is invalid" instead of "Bucket is a requester pays bucket but no user project provided", which was the error this patch fixed. Can you confirm that the broad-firecloud-ccle project exists and is authorized under your service account?

@lbergelson Can you comment further on this?

@lbergelson
Copy link
Member

That error is different than the bug we fixed. I've seen it when I have specified an incorrect project. It's possible there's a different bug though. I would check to make sure that the project name is spelled correctly AND that the service account you're running is has the necessary permissions to bill that project.

@jkobject
Copy link

yes I can confirm it. This is the same project I have always used and it worked before.

@droazen
Copy link
Collaborator Author

droazen commented Mar 10, 2022

@jkobject Thanks, we are testing with the nightly image now

@droazen
Copy link
Collaborator Author

droazen commented Mar 10, 2022

@jkobject Our testing of the nightly image with our own service account and billing project suggests that the requester pays access issue is resolved, so we're not sure what's causing it to continue to fail for you. As an experiment, could you try this: within the gatk-nightly image, try to access your requester pays file gs://cclebams/wgs_hg38/CDS-0b4jFH.wgs_ccle.bam using the gsutil command with the -u option. Eg., gsutil -u broad-firecloud-ccle cp gs://cclebams/wgs_hg38/CDS-0b4jFH.wgs_ccle.bam ., and report whether that succeeds. Thanks!

@droazen
Copy link
Collaborator Author

droazen commented Mar 10, 2022

@jkobject Actually, we just noticed that your error is triggered by the file gs://fc-secure-bd7b8bc9-f665-4269-997e-5a402088a369/5c2db926-3b1c-479c-9ed3-a99ce518de91/omics_mutect2/60955825-7723-4bc9-8202-bdd9975bb5c0/call-mutect2/Mutect2/7d737efc-c8be-4a6d-8803-4f786129521a/call-SplitIntervals/glob-0fc990c5ca95eebc97c4c204e3e303e1/0000-scattered.interval_list, not the bam. Could you attempt the gsutil test on that file instead, and let us know what happens? Eg.,

gsutil -u broad-firecloud-ccle cp gs://fc-secure-bd7b8bc9-f665-4269-997e-5a402088a369/5c2db926-3b1c-479c-9ed3-a99ce518de91/omics_mutect2/60955825-7723-4bc9-8202-bdd9975bb5c0/call-mutect2/Mutect2/7d737efc-c8be-4a6d-8803-4f786129521a/call-SplitIntervals/glob-0fc990c5ca95eebc97c4c204e3e303e1/0000-scattered.interval_list .

@jkobject
Copy link

jkobject commented Mar 10, 2022

@droazen both work fine on my laptop, will try in the image now..

@jkobject
Copy link

It works on the docker too

@droazen
Copy link
Collaborator Author

droazen commented Mar 10, 2022

@jkobject Thanks for testing that for us! After looking into this issue a bit more, it seems that there might be a problem in the latest Google libraries with handling the case of a non-requester-pays input when a requester-pays project is provided. In our tests, running a similar Mutect2 command line to yours with all inputs in requester pays buckets succeeds, but when we run with a mix of requester-pays and non-requester-pays inputs, or all non-requester-pays inputs, and do specify a requester pays project, we're able to replicate your error. We are looking in a debugger now to pin down the cause.

Can you tell us whether running Mutect2 with a mix of requester-pays and non-requester-pays inputs and --gcs-project-for-requester-pays specified succeeded for you previously? If so, with which version(s) of GATK?

@jkobject
Copy link

Hi,

I think this is the right issue. Some other files are from other buckets which are non requester pays. (this seems like the most common use case)

It always used to work before. I have used many of the GATK 4.2 dockers. The latest run was on 4.2.4.0

Thanks!

@droazen
Copy link
Collaborator Author

droazen commented Mar 11, 2022

@jkobject After further testing we think the issue might be more specific: the error seems to occur only when checking for the existence of non-existent files in non-requester-pays buckets when a requester-pays project is specified. In your case, it is checking for the existence of an index for your interval_list file and failing, since interval_list files don't have indices.

As another experiment, it would be helpful to know whether your Mutect2 command succeeds if you specify a specific interval string like "1:1000000-2000000" (doesn't matter which interval) for the -L argument instead of the interval_list file (but keeping all other inputs the same).

@droazen
Copy link
Collaborator Author

droazen commented Mar 11, 2022

I've opened #7716 to track this issue -- we should move further discussion there, since this is a separate issue from the one resolved in this PR.

@jkobject
Copy link

@droazen this seems like a bit more work on my end. I might only be able to try this next week.

@droazen
Copy link
Collaborator Author

droazen commented Mar 11, 2022

@jkobject Sure, no problem! In the meantime we'll do additional testing on our end to try to confirm this theory, and submit another patch to Google. We are considering forking the library for now in the interests of getting a fully-working version of GATK released as soon as possible.

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.

Requester pays access isn't working
3 participants