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

resindataexpander: expand data part to max usable size on MBR #2243

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jakogut
Copy link
Contributor

@jakogut jakogut commented Jul 7, 2021

MBR formatted disks are only capable of 32-bit addressing of sectors,
limiting disk usage to 2 TB. In cases where the full disk cannot be
utilized, expand the data partition to fill the usable space, and print
a message.

Change-type: patch
Signed-off-by: Joseph Kogut [email protected]

Related to: #2241

@jakogut jakogut requested a review from alexgg July 7, 2021 20:41
@resin-jenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@alexgg alexgg left a comment

Choose a reason for hiding this comment

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

Could you also please make separate previous commits to:

  • Fix all previous shellcheck errors
  • Reformat to use tabs instead of spaces (I use the following vim setting)
:set filetype=sh noexpandtab shiftwidth=4 tabstop=4 autoindent

jakogut added 5 commits July 8, 2021 17:46
Change-type: patch
Signed-off-by: Joseph Kogut <[email protected]>
Change-type: patch
Signed-off-by: Joseph Kogut <[email protected]>
Change-type: patch
Signed-off-by: Joseph Kogut <[email protected]>
Reformat to use tabs instead of spaces, according to the following vim
config:

:set filetype=sh noexpandtab shiftwidth=4 tabstop=4 autoindent

Change-type: patch
Signed-off-by: Joseph Kogut <[email protected]>
MBR formatted disks are only capable of 32-bit addressing of sectors,
limiting disk usage to 2 TB. In cases where the full disk cannot be
utilized, expand the data partition to fill the usable space, and print
a message.

Change-type: patch
Signed-off-by: Joseph Kogut <[email protected]>
@jakogut jakogut force-pushed the mbr-data-resize branch from 2aef1a8 to 7957fca Compare July 8, 2021 17:54
@jakogut jakogut requested a review from alexgg July 8, 2021 17:55
@jakogut
Copy link
Contributor Author

jakogut commented Jul 8, 2021

@balena-ci rebase

@ghost ghost force-pushed the mbr-data-resize branch from 7957fca to 72dfad4 Compare July 8, 2021 17:56
@jakogut
Copy link
Contributor Author

jakogut commented Jul 8, 2021

@alexgg Good review, sorry I didn't catch those things before pushing!

@jakogut
Copy link
Contributor Author

jakogut commented Jul 9, 2021

@balena-ci rebase

@ghost ghost force-pushed the mbr-data-resize branch from 72dfad4 to 8e346d7 Compare July 9, 2021 02:15
@jakogut
Copy link
Contributor Author

jakogut commented Jul 20, 2021

@balena-ci rebase

@ghost ghost force-pushed the mbr-data-resize branch from 8e346d7 to 6daa96e Compare July 20, 2021 15:48
@jakogut
Copy link
Contributor Author

jakogut commented Aug 11, 2021

@alexgg Do we want to close this in favor of failing to resize the data partition on MBR partitioned disks > 2 TB? If so, I can rebase and push the shellcheck fixups separately.

@alexgg
Copy link
Contributor

alexgg commented Aug 13, 2021

@jakogut separating the linter fixes is a good idea. With regards to this PR, I think the fix is a technical improvement, but it does not help the user realize they are using a DT with an MBR partition and that disks bigger than 2TB are not supported.
At least without the fix they would realize as the disk is unusable, while with the fix they might finish up in production with the wrong DT.
Maybe a middle ground is to apply the fix but to add a check that warns the user about the situation? I haven't thought through how this check would work though. Might be an arch call discussion.

@jakogut
Copy link
Contributor Author

jakogut commented Aug 23, 2021

@balena-ci rebase

@ghost ghost force-pushed the mbr-data-resize branch from 6daa96e to 53ee274 Compare August 23, 2021 20:33
@jakogut
Copy link
Contributor Author

jakogut commented Aug 23, 2021

@resin-jenkins retest this please

@ab77
Copy link
Contributor

ab77 commented Sep 24, 2021

@jakogut @alexgg just wondering if this is blocked by anything?

@alexgg
Copy link
Contributor

alexgg commented Sep 24, 2021

@ab77 it's just the discussion above about this patch not helping - the user has the wrong DT and the sooner they realize the better. So if we are going to expand anyway to make it work we better find a way to warn the user. We don't want this on a production device on the field.

@ab77
Copy link
Contributor

ab77 commented Sep 24, 2021

@alexgg so this is superseded then by the new generic-amd64 DT?

I am the user by the way, at least one of them :)

@alexgg
Copy link
Contributor

alexgg commented Sep 24, 2021

@ab77 yes, the generic-amd64 has a GPT partition table so it's the alternative for x86 architectures. In ARM, it would probably be generic-aarch64. I only expect to see >2TB in the server space, not on the embedded space for some years at least. Anyway, all new DTs should be GPT.

@ab77
Copy link
Contributor

ab77 commented Sep 24, 2021

@alexgg perfect, do you have a sense when we will have a generic-amd64 image to deploy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants