Remove use of sgdisk as sfdisk already supports GPT partitions

[ani-sinha]

Proposed Commit Message

sfdisk already fully supports GPT partitions. Remove redundant code that uses sgdisk for GPT.

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[ani-sinha]

The failed check is also failing for other PRs.

[holmanb]

@ani-sinha Is sgdisk not used anywhere?

[ani-sinha]

@ani-sinha Is sgdisk not used anywhere?

It's much less used these days. We would want to get rid of it as sfdisk is more common.

[ani-sinha]

@ani-sinha Is sgdisk not used anywhere?

It's much less used these days. We would want to get rid of it as sfdisk is more common.

Another thing is, I have not actually tested this change beyond the usual initial tests and integration tests. We need to make sure that sfdisk can handle EFI partitions with the same options or if something different needs to be passed. I didn't have much time today but maybe I can test something later next week if needed.

[holmanb]

@ani-sinha Is sgdisk not used anywhere?

It's much less used these days. We would want to get rid of it as sfdisk is more common.

I'm in agreement that getting rid of unused code is a good thing.

My question is whether you are confident that no distro is using it currently. This change would break anybody that's using sgdisk and doesn't have one of the alternate tools available. Also we need to be sure that the version of sfdisk used in all supported distros is capable of GPT partitions. I don't want to break compatibility with, say, a BSD if this capability isn't available everywhere.

[ani-sinha]

@ani-sinha Is sgdisk not used anywhere?

It's much less used these days. We would want to get rid of it as sfdisk is more common.

I'm in agreement that getting rid of unused code is a good thing.

My question is whether you are confident that no distro is using it currently. This change would break anybody that's using sgdisk and doesn't have one of the alternate tools available. Also we need to be sure that the version of sfdisk used in all supported distros is capable of GPT partitions. I don't want to break compatibility with, say, a BSD if this capability isn't available everywhere.

Well currently if MBR is used, we need sfdisk. So the question is, is there any distro that does not support MBR? Unlikely. Besides sfdisk support GPT from 2017 onwards. Its much more widely known IMHO than sgdisk. So it is highly unlikely that anyone would have a version of sfdisk that does not support GPT and we would need sgdisk. Another issue is that downstream distros generally use their package management tools (rpm, apt etc) to make sure that the dependencies are listed correctly so that when cloud-init is installed, all the dependent packages are installed . I am finding it hard to believe that they would not list sfdisk as one of the dependencies for cloud-init. If they did not, its a matter of updating cloud-init docs to make sure that people know we made this change and they need to make updates for the dependent packages in the downstream specific package management scripts.

I think without our ability to make changes like this (even sometimes when it comes at the cost of temporarily breaking some cases) we cannot make progress. You guys have argued this in the context of another change (when you introduced a new status code 2). I am making the same argument here.

[holmanb]

@ani-sinha Is sgdisk not used anywhere?

It's much less used these days. We would want to get rid of it as sfdisk is more common.

I'm in agreement that getting rid of unused code is a good thing.
My question is whether you are confident that no distro is using it currently. This change would break anybody that's using sgdisk and doesn't have one of the alternate tools available. Also we need to be sure that the version of sfdisk used in all supported distros is capable of GPT partitions. I don't want to break compatibility with, say, a BSD if this capability isn't available everywhere.

Well currently if MBR is used, we need sfdisk. So the question is, is there any distro that does not support MBR? Unlikely. Besides sfdisk support GPT from 2017 onwards. Its much more widely known IMHO than sgdisk. So it is highly unlikely that anyone would have a version of sfdisk that does not support GPT and we would need sgdisk.
Another issue is that downstream distros generally use their package management tools (rpm, apt etc) to make sure that the dependencies are listed correctly so that when cloud-init is installed, all the dependent packages are installed . I am finding it hard to believe that they would not list sfdisk as one of the dependencies for cloud-init. If they did not, its a matter of updating cloud-init docs to make sure that people know we made this change and they need to make updates for the dependent packages in the downstream specific package management scripts.

This reasoning would make sense if only one sfdisk project existed.

I think without our ability to make changes like this (even sometimes when it comes at the cost of temporarily breaking some cases) we cannot make progress. You guys have argued this in the context of another change (when you introduced a new status code 2). I am making the same argument here.

We agree that these breaking changes are needed to make progress. I don't object to breaking compatibility with old unsupported OSes. I'm concerned that the proposed code is not just a temporarily breaking change. That is why I pointed to the BSDs in my earlier response. At a glance, it doesn't look like Freebsd's sfdisk supports GPT partitioning. I think that at least FreeBSD currently still requires sgdisk, and maybe others too (@igalic thoughts?). We can still drop sgdisk from use on Linux, but the change should use the proper abstractions to avoid breaking other OSes.

To make this change without breaking the BSDs, one could shift this OS-specific code into the Distro definitions in cloudinit/distros/. Then just pass the instance of Distro that we have in the handler( function (cloud.distro) through to the proper call sites.

Going forward in this codebase we should consider putting calls to external tools into the abstractions under distros/ so that we can express behavior without tying non-distro code to specific tools and tool versions. This pattern will ease maintenance in the long term anyways with respect to changes in external tools.

[ani-sinha]

After looking at cgdisk output and sfdisk output and how they are used in cloud-init, it seems the output from the two tools are not compatible. In particular, the ID values for GPT partitions are different - fdisk returns UUID, gdisk returns hex MBR-type values (see https://github.com/Shihta/gdisk/blob/master/parttypes.cc#L70) . The config for GPT uses the MBR type values currently and hence in order to not break existing setups, we need to translate UUID values to MBR type values like in the table in the link. This is more complicated than I thought and has potential to break customers existing setups. So after internal discussions, we decided not to pursue this approach.

[holmanb]

@ani-sinha That sounds reasonable. Thanks for contributing!