refactor : Use strongunits.B, strongunits.MiB and strongunits.GiB to store memory size (#1642)

[rohanKanojia]

Description

Fixes: #1642

Currently, we're using memory values in uint64 variable. This works but sometimes it can be confusing to figure out what unit we're using if variable is not named correctly.

Instead, we can rely on containers/strongunits types for these values for byte,MegaByte and GigaByte.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Proposed changes

Please note that at the moment, I've only made changes at CRC levels. I haven't updated driver code. I plan to do it in follow up PRs.

• Replace integer types used for strong memory values with strongunits.B, strongunits.MiB and strongunits.GiB
• Since strongunit types are unsigned integers underneath adjust some places in codebase where we use -1 as a value for memory size to indicate null values. Use 0 instead of -1.

Testing

I made sure all unit tests were passing and verified that cluster was getting started. I also ran basic e2e test locally on my machine to check if these changes were not breaking anything.

Contribution Checklist

• I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Which platform have you tested the code changes on?
  • Linux
  • Windows
  • MacOS

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[praveenkumar]

@rohanKanojia I think that is one part of the #1642 but iirc what @cfergeau wanted for the user to provide memory/disk size in MB and GB like if user know that he need 10GB ram he can use memory setting as 10G instead converting it to MB and using it.

[rohanKanojia]

@praveenkumar : Are you talking about CRC config memory values?

[cfergeau]

@rohanKanojia I think that is one part of the #1642 but iirc what @cfergeau wanted for the user to provide memory/disk size in MB and GB like if user know that he need 10GB ram he can use memory setting as 10G instead converting it to MB and using it.

Ah right, I say in #1642 "to the option parsing code", so it must have been what I meant in this issue. However, having clear units throughout the code base is also very useful from a maintainance perspective, so imo both are good to have.

[rohanKanojia]

Okay, I'm going to implement parsing and validation logic for these crc config values:

• memory
• disk-size
• persistent-volume-size

I have some doubts regarding the implementation:

• I'm not able to see any parsing support in strongunits codebase, Shall I implement a simple parsing utility method based on a simple regex ^(?i)(\d+)\s*(B|GB|MB|KB)?$ in CRC code itself?
• Shall we preserve old behavior or not? Currently CRC expects an integer value for memory that's automatically assumed to be in MiB. After these changes, if user passes an integer value shall we treat it as MiB or raw bytes?
  • If we don't want this change to be breaking, I can add the storage unit parsing logic as a fallback option. If it fails to parse as integer, it would try parsing string as storage unit.

[praveenkumar]

Okay, I'm going to implement parsing and validation logic for these crc config values:

* `memory`

* `disk-size`

* `persistent-volume-size`

I have some doubts regarding the implementation:

* I'm not able to see any parsing support in [strongunits](https://github.com/containers/common/blob/03380ecd72b0f33c0eff90b186ed1e06c19df80b/pkg/strongunits/config.go#L4) codebase, Shall I implement a simple parsing utility method based on a simple regex `^(?i)(\d+)\s*(B|GB|MB|KB)?$` in CRC code itself?

* Shall we preserve old behavior or not? Currently CRC expects an integer value for memory that's automatically assumed to be in `MiB`. After these changes, if user passes an integer value shall we treat it as `MiB` or raw bytes?
  
  * If we don't want this change to be breaking, I can add the storage unit parsing logic as a fallback option. If it fails to parse as integer, it would try parsing string as storage unit.

As of now our call back functions handles misconfiguration around config and currently if user specify different value for ram/disk-size/persistent-volume then what we expect then we do error out. I don't mind to have this as it is because now atleast we have clear units through out code. Breaking changes is something we dont' want to do because lot of folks now use those config as part of scripts.

[rohanKanojia]

I don't mind to have this as it is because now atleast we have clear units through out code. Breaking changes is something we dont' want to do because lot of folks now use those config as part of scripts.

I see. Understood that we don't want to add any breaking changes. So if user would provide integer value, CRC would consider it as currently configured MiB/GiB value.

Would you like me to add the parsing only as a fallback option? Or maybe drop this idea altogether?

[vyasgun]

Looks good to me. Just one minor comment.

[vyasgun]

You can use variables in this line for clarity:

Suggested change

	return strongunits.B(ram.([]uint64)[0]), strongunits.B(ram.([]uint64)[1])
	used, total := ram.([]int64)[0], ram.([]int64)[1]
	return strongunits.B(used), strongunits.B(total)

[rohanKanojia]

Thanks for taking the time to review! I've updated it as per your suggestion.

[openshift-ci]

@rohanKanojia: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	269f5f8	link	true	/test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[praveenkumar]

Would you like me to add the parsing only as a fallback option? Or maybe drop this idea altogether?

@rohanKanojia I don't think this is very high priority work as of now so I am going to merge this as it is .