Fix checksum for SAS3IRCU resources to use 64 bits

[gabrielcocenza]

• resources were using the checksum for 32 bits

How to test it:

• download the binaries. E.g: https://docs.broadcom.com/docs/SAS3IRCU_P14.zip and extract it
• go to the sas3ircu_linux_x64_rel and run sha256sum sas3ircu

Fix: #324

[chanchiwai-ray]

Need to double check P11

[Vultaire]

Some bits of feedback are for lines outside of the diff, i.e. additional changes which are needed, so I'll provide my overall advice here:

1. While the changes to the sas3ircu version info data seem likely correct (I haven't actually checked checksums myself; I'm assuming this), I do have a concern over whether deployed hardware observer units with attached sas3ircu resources will continue working (without rechecking the resource), or whether upgrading the charm might break them. The data structures are for Architecture.X86_64 so clearly they should be pointing at the 64-bit binaries, so that correction is the right thing to do - but, we should be careful to avoid breaking existing deployments unless absolutely necessary.
2. While I think the bug I rose was for sas3ircu, I think the same applies to sas2ircu. Those are also broadcom binaries and I think they also have x86 and x86-64 versions. We should also correct those, so that if someone pulls the 64-bit binaries and tries to attach them, they should be accepted by the charm.

Marking as "request changes" because the bug also appears to apply to sas2ircu, and I'd like to see that fixed as well.

[jneo8]

Some bits of feedback are for lines outside of the diff, i.e. additional changes which are needed, so I'll provide my overall advice here:

1. While the changes to the sas3ircu version info data seem likely correct (I haven't actually checked checksums myself; I'm assuming this), I do have a concern over whether deployed hardware observer units with attached sas3ircu resources will continue working (without rechecking the resource), or whether upgrading the charm might break them. The data structures are for Architecture.X86_64 so clearly they should be pointing at the 64-bit binaries, so that correction is the right thing to do - but, we should be careful to avoid breaking existing deployments unless absolutely necessary.
2. While I think the bug I rose was for sas3ircu, I think the same applies to sas2ircu. Those are also broadcom binaries and I think they also have x86 and x86-64 versions. We should also correct those, so that if someone pulls the 64-bit binaries and tries to attach them, they should be accepted by the charm.

Marking as "request changes" because the bug also appears to apply to sas2ircu, and I'd like to see that fixed as well.

Hi Paul,

Thank you for sharing your thoughtful insights on the proposed change. I’d like to provide some feedback as well.

For Point 1:
In the hw-observer's lifecycle, the checksum only runs when the HWToolHelper.install method is executed. This is triggered during Juju's install or upgrade events (reference).

As a result, if any hw-observer unit is deployed with an incorrect resource, it will enter a blocked status once the Juju application revision, including this patch, is applyed. On the other hand, if the resource is correct, it will pass the check as expected.

The blocked status error message will be: Fail strategy checks: sas3ircu.

For Point 2:
I agree with your perspective. However, I also believe it would be beneficial to make the PR changes more atomic. Splitting the changes into separate PRs and issues would likely be a more effective approach.

[jneo8]

Based on information from @Pjack, this checksum 7fa299a36254c582cf579d197463d6e59ffa9270b7241d98d0e477f05235be26 works for both architectures, which explains why we didn’t initially notice the mismatch.

Hi @Vultaire,

I understand your approach to supporting backward compatibility, and we could achieve this in a somewhat tricky way. Specifically, by adding another ToolVersionInfo entry for SAS3IRCU with the checksum 7fa299a36254c582cf579d197463d6e59ffa9270b7241d98d0e477f05235be26, and defining it to support both x86 and x64.

However, I personally prefer asking users to attach the correct version of the tool as specified by the official documentation, if backward compatibility will not cause critical issue on production. This approach seems cleaner and aligns better with the intended usage.

[Pjack]

@gabrielcocenza Could you also handle the sas2 case? I am okay to patch it within the same PR or different PR. It's a small change. Thanks!

[gabrielcocenza]

@Pjack @Vultaire I took a look at sas2 from version 20 to 16 and on all cases we have just two linux folders:

• sas2ircu_linux_ppc64_rel
• sas2ircu_linux_x86_rel

So there is no arm64 and the amd version apparently is for both 32 and 64 bits, so there is no need to change the checksum 👍

[Vultaire]

Thanks for your feedback and investigating the sas2ircu issue as well.

I downloaded and did a quick check of the sas3ircu binaries' sha256sums; I believe this is correct. And based upon the description of what happens for existing deployments, I'm +1. Approved.