-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update Thredds to supported version #413
base: master
Are you sure you want to change the base?
Conversation
FYI, for this change, the automated pipeline test is not enough. We have already attempted to upgrade Thredds on Ouranos side and we have |
Thanks for the info. I mostly created this PR to test the pipeline with the updated version to see what happens. I can close this if you're working on it on the ouranos side. |
Oh keep this PR, we have not yet opened a formal PR on our side, we simply override in |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2366/Result : failure BIRDHOUSE_DEPLOY_BRANCH : update-thredds-5.4 DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-20.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1482/NOTEBOOK TEST RESULTS |
A few problems found related to Thredds 5.4 Notebook changes required: |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2420/Result : failure BIRDHOUSE_DEPLOY_BRANCH : update-thredds-5.4 DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-46.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1498/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2421/Result : failure BIRDHOUSE_DEPLOY_BRANCH : update-thredds-5.4 DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-69.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1499/NOTEBOOK TEST RESULTS |
FYI @mishaschwartz we still have not done fixing the issues on our side. Besides there is a performce problem with 5.4 (Unidata/tds#406) and we hope to be fixed in 5.5 so this PR is not ready (5.5 is not released). |
…antic versioning tag
FYI, we found more problems with version 5. NCML, UDDC, ISO and NetcdfSubset link do not work with v5. Have not had time to investigate. Opendap and other links work fine. Ex: click on the NCML, UDDC, ISO and NetcdfSubset link here https://pavics.ouranos.ca/testthredds/catalog/testdatasets/testdata/catalog.html?dataset=testdatasets/testdata/ta_Amon_MRI-CGCM3_decadal1980_r1i1p1_199101-200012.nc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retracting "accept PR" given #413 (comment)
Related to Unidata/thredds-docker#310 |
NCML, UDDC, ISO links fixed by adding the missing jar, see Unidata/thredds-docker#310 (comment) No other clues found for broken NetcdfSubset link, opened an issue Unidata/tds#544 |
@@ -33,6 +33,7 @@ providers: | |||
dodsC, | |||
wcs, | |||
wms, | |||
ncss, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave the old location.
THREDDS_VERSION
can be overridden, so a server could still employ the older variant.
Given that, that will most probably cause a conflict in the data_type
resolution order if all 3 are defined, so maybe alternate paths or a dynamic variable resolution will be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh... you're right. Ok I'll figure something out.
ncss/grid, | ||
ncss/point, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test if this works?
I do not know if Magpie/Twitcher handles this properly.
The data types were not planned to include a /
, so I'm not sure if a url.split("/")
might happen somewhere in the code leading to misinterpretation of the targeted data_type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed to be fine but I didn't rigorously test by trying to read the two options with different directory permissions I guess. I'll look into that.
BUT... if these prefixes can't handle a /
we should change the service definitions to:
<service name="ncssGrid" serviceType="NetcdfSubset" base="${TWITCHER_PROTECTED_PATH}/thredds/ncss-grid/" />
<service name="ncssPoint" serviceType="NetcdfSubset" base="${TWITCHER_PROTECTED_PATH}/thredds/ncss-point/" />
or similar and that way we don't have to worry about that at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok maybe we can't change the path... the URL construction docs for this service look like point or grid needs to be a subpath under ncss:
https://docs.unidata.ucar.edu/tds/current/userguide/netcdf_subset_service_ref.html#url-construction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data types were not planned to include a /, so I'm not sure if a url.split("/") might happen somewhere in the code leading to misinterpretation of the targeted data_type.
Yup magpie splits it up (see here) so our only option at this point is to keep the magpie config as it is and treat grid/
and point/
as directory paths if we want to differentiate their magpie permissions.
That's going to cause other confusion though...
Right now we have URL paths like:
${FQDN}/twitcher/ows/proxy/thredds/ncss/datasets/...
${FQDN}/twitcher/ows/proxy/thredds/dodsC/datasets/...
So if we want to set specific permissions on the datasets/
directory we can do that by setting one directory permission rule on the datasets/
subdirectory and it will work for all services.
But if we instead have:
${FQDN}/twitcher/ows/proxy/thredds/ncss/point/datasets/...
${FQDN}/twitcher/ows/proxy/thredds/dodsC/datasets/...
Then we would need to set the same permission rule on datasets/
as well as point/datasets/
. If we don't set the same rule on point/datasets/
then the rule won't apply to resources accessed by the ncss/point
service.
Please let me know if I'm interpreting this correctly @fmigneault
If I'm right then I think we need to update Magpie to handle this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to revert my changes for now until we figure something out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright that makes sense. We should probably make an issue for this over on the Magpie repo. Do you want to do that or shall I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to duplicate the entire permission hierarchy 3 times for anything under "datasets", because the dirs for THREDDS would be defined as follows.
birdhouse datasets ... # current ncss and others points datasets grid datasets
This also causes a weird situation where an actual dir named "points" or "grid" under "birdhouse" (eg:
${FQDN}/twitcher/ows/proxy/thredds/dodsC/points
) would have a different set of permissions that might overlap/conflict or be irrelevant to what points/grid "datasets" define.
I am completely lost here. Does this means we have a problem with the current Magpie config or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mishaschwartz Yes, you can create the issue.
@tlvu Magpie cannot handle this case currently because it wasn't designed for this. It assumes THREDDS "services" don't contain a /
, so it could rely on {Twitcher-prefix}/{THREDDS_name}/{THREDDS_service}/{rest-as-dir/file}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tlvu Magpie cannot handle this case currently because it wasn't designed for this. It assumes THREDDS "services" don't contain a
/
, so it could rely on{Twitcher-prefix}/{THREDDS_name}/{THREDDS_service}/{rest-as-dir/file}
.
@fmigneault I am still lost. What case are you talking about?
Just to be clear, in the current state, with Misha's proposed Magpie config change rollback, do we have any problems?
Been running some Jenkins on the new Thredds and I am having some weird errors. Not sure if it's my test system or the code. So if you foresee some problem, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ncssGrid
and ncssPoint
services won't be protected like other services do (ie: a common browse/read/write set for all services) since the paths are not handled.
For the moment, if you want them protected, you need to duplicate the permission hierarchy.
i.e.:
A file accessed by ncssGrid
via ${FQDN}/twitcher/ows/proxy/thredds/ncss/point/datasets/sub/file.nc
will actually go through ncss
service, and point
will be considered by Magpie as if it was any other directory (like datasets
or sub
).
Therefore, the same datasets/sub/file.nc
would need 3 sets of permissions:
- datasets/sub/file.nc => (browse,read,write) for anything currently handled
- grid/datasets/sub/file.nc => (read) only if accessing via ncssGrid
- point/datasets/sub/file.nc => (read) only if accessing via ncssPoint
and those (read) need to be duplicated for every user/group/dir/file combination applicable
Overview
Unidata has dropped support for TDS versions < 5.x. This updates Thredds to version 5.4.
Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
Additional Information
Links to other issues or sources.
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false