Use Adler32 algorithm to compute string checksums#48812
Conversation
terminalmage
added
the
pending-discussion
terminalmage
left a comment
terminalmage
left a comment
There was a problem hiding this comment.
I am open to this change, but if we are going to make it then I think we need to at minimum be very explicit about the change in the release notes, due to the fact that this will change a grain on which users may be relying.
@thatch45 @cachedout @rallytime thoughts? Should we gate this behind a config item and make it the new default method after a couple release cycles? Or, should we make it the default right away?
|
I really don't like doing this sort of thing. We're going to change all of these hashes out from under people but for no reason other than it's only just a fraction of a bit faster and the code is slightly cleaner and I just don't feel like that's worth the disruption to users. |
|
I agree with @cachedout here. I don't think this is something that we want to disrupt upgrades for with users, IMO. |
|
We didn't get a response from @isbm so I'm closing this for now. Bo, if you like we can discuss this more on our next call. :) |
|
@cachedout @rallytime I was on the vacations in the woods off the grid, and now this PR is just closed. 😢 The part that changing hashes probably isn't that much problematic, but I've got the point. So why we indeed do not gate this behind the option and retire then it later, just like @terminalmage said? In the meanwhile firing up another interpreter for just hash is really what has to be done due to impossibility of doing better. Which is fine (-ish, because on the Mainframes with the VM of Ferrari-price RAM), but still how we can go away from this hack in the future clean? Another problem with the hack is that Adler32 is fixed, finished algorithm. It won't change. But are we sure Python next versions won't turn off that environment variable at all and we have them this problem back? I would still look for the option to gating this behind the option & press release this. |
|
I can re-open this for now, certainly. We can discuss this tomorrow in our meeting. :) |
|
As per conversation, a valid concern from @cachedout for not to update defaults abruptly has been addressed as per suggestion from @terminalmage . I also added choices between Adler32 (going to be default) and CRC32. The latter is slower (the slowness is not any significant for this task though) but much more reliable for collisions. However, returns a bigger number then Adler32. Current default is OFF and the Python shell-out workaround is used. In two versions this workaround will be retired. This way concern has been solved. @rallytime what are the next steps here? @terminalmage I think I probably/likely still need to add some better docs in press-release, or? Any suggestions? |
Generate the same numeric value across all Python versions and platforms Re-add getting hash by Python shell-out method Add an option to choose between default hashing, Adler32 or CRC32 algorithms Set default config option for server_id hashing to False on minion Choose CRC method, default to faster but less reliable "adler32", if crc is in use Add warning for Sodium. Move server_id deprecation warning to reduce log spamming (bsc#1135567) (bsc#1135732) Remove deprecated warning that breaks miniion execution when "server_id_use_crc" opts are missing BACKPORT-UPSTREAM=saltstack#48812 DOWNSTREAM-REF=openSUSE/salt#159 DOWNSTREAM-REF=openSUSE/salt@73e357d
6 participants
What does this PR do?
An alternative to #46649
Previous Behavior
server_idis changing due to Python 3 is saltinghashfunction. A workaround is to shell-out Python with environment variablePYTHONHASHSEED=0. Unfortunately, this is the only option.However, what if we use Adler-32 instead of hash? This would avoid shell-out Python.
@terminalmage what do you think?
Tests written?
Should apply existing