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

Integrate Hero Version: Disable usage of hardlinks, but allow enabling via settings #678

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Jun 20, 2024

Changelog Description

Integrate Hero Version: Disable usage of hardlinks, but allow enabling via settings

Additional info

There are 'known issues' with hardlinks on Windows where Windows disallows deleting any of the links if ANY of the files are in use. As such, when e.g. hero version and latest v010 publish are hardlinked, a machine actively has v010 in use (e.g. on renderfarm) and we try to delete an swap the hero version with a v011 hardlink when publishing a new version then Windows disallows the removal of the hero version's files - even if actually that particular file isn't in use.


Historically there have been issues with hardlinks when publishing looks. More details scattered across:

And yes, hardlinks have bitten our asses before as you can see.

Testing notes:

  1. Integrating hero version should not use hardlinks by default
  2. Setting to enable hardlink usage should work.

On Windows you should be able to 'detect' hardlinks using fsutil hardlink:

fsutil hardlink list MyFileName.txt

@ynbot ynbot added size/XS type: enhancement Improvement of existing functionality or minor addition labels Jun 20, 2024
@m-u-r-p-h-y m-u-r-p-h-y added the sponsored This is directly sponsored by a client or community member label Jun 20, 2024
@m-u-r-p-h-y
Copy link
Member

just to show different ways of finding the hardlinks on Windows

fsutil hardlink list <fileName>
image

using Linux subsystem on Windows

find . -links +1
image

@LiborBatek
Copy link
Member

Not sure how to approach this PR and its testing... could you elaborate more on exact testing steps pls?

I was trying to render animation on DL and also publish animation products while rendering and never happened to me that hero version integration failed (tested outside of this PR). So Im not sure how I should prove this PR fixes something.

So more elaborate test steps might help me a lot...thx

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jun 21, 2024

Not sure how to approach this PR and its testing... could you elaborate more on exact testing steps pls?

I was trying to render animation on DL and also publish animation products while rendering and never happened to me that hero version integration failed (tested outside of this PR). So Im not sure how I should prove this PR fixes something.

So more elaborate test steps might help me a lot...thx

The bug the client is facing may be hard to reproduce - I think the best test for you here, outside of their production enviromment, is to ensure the setting does what it is intended to do and that is "choose" between writing hero versions as hardlinks or regular copies.

So I suppose, with the setting on and off do a few hero publishes, all should work fine.
Then each time check whether the files are hardlinks or not - using e.g. the techniques @m-u-r-p-h-y showed in the screenshots (for which a windows variant is also in the testing notes). Those will have to be done through command line most likely since I don't know a way on windows to detect whether a file is hardlink or not without command line.

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

I have tested the defaults with Use Hardlinks Disabled

Screenshot 2024-06-24 091632

... and it seems all working fine...as seen in the Publish Report below

Screenshot 2024-06-24 092438

but when Use Hardlinks being ON it seems it stills uses copy instead and not hardlinks...

as I have inspected my Publish Report...still the same report details using copy.

@LiborBatek
Copy link
Member

...it seems that the Use Hardlinks knob does not have any effect atm

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jun 24, 2024

@LiborBatek your test screenshot with the forcing copy versus hardlink seems to be for Look publishing from Maya - which is unrelated to this PR. The only thing this PR changes is the Integration of Hero Versions

Then, the logic currently was always reporting it's copying the file instead of reporting it's hardlinking - which may be confusing since you then can't really check the log but must check the files on disk itself to confirm whether it hardlinked or not. I've changed that with 0ab3653

It should now debug log "Hardlinking file {source} to {destination}" if hardlink is enabled for Integrate Hero Versions and will debug log "Copying file {source} to {destination}". If for whatever reason the hardlink failed and it falls back to copying the file it will now debug log that as well.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jun 24, 2024

Test run on my side. Setting enabled:

// pyblish.IntegrateHeroVersion : Hardlinking file "C:\projects\ayontest\asset\char_hero\publish\pointcache\pointcacheMain\v022\ynts_char_hero_pointcacheMain_v022.abc" to "C:/projects/ayontest/asset/char_hero/publish/pointcache/pointcacheMain/hero/ynts_char_hero_modeling_hero.abc"

Setting disabled:

// pyblish.IntegrateHeroVersion : Copying file "C:\projects\ayontest\asset\char_hero\publish\pointcache\pointcacheMain\v023\ynts_char_hero_pointcacheMain_v023.abc" to "C:/projects/ayontest/asset/char_hero/publish/pointcache/pointcacheMain/hero/ynts_char_hero_modeling_hero.abc"

@BigRoy BigRoy requested a review from LiborBatek June 24, 2024 09:17
@m-u-r-p-h-y
Copy link
Member

should not the hardlinking be more generic setting (the storage either supports it or not)

having this option scattered across different families and DCCs does not sound right to me.
The solution should not be part of this PR just thinking aloud . . .

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jun 24, 2024

should not the hardlinking be more generic setting (the storage either supports it or not)

having this option scattered across different families and DCCs does not sound right to me. The solution should not be part of this PR just thinking aloud . . .

I understand your point. But at the same time I don't see an issue with a studio wanting looks to NOT be hardlinked, but hero versions they want. Which is a customization option you would lose then. These are now two options - that's not too bad. If this was scattered all over for 10s of options.

It could be worth a separate issue if you think it's very problematic in design currently.

@LiborBatek LiborBatek removed their assignment Jun 26, 2024
Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Works like charm...also checked the published hero version with fsutil command and all good

when hardlinks:

Screenshot 2024-06-26 120114

when hardlinks disabled it doesnt list any hard links...

LGTM!

@iLLiCiTiT iLLiCiTiT merged commit 9a65275 into ynput:develop Jun 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants