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

simplify early-boot-config dependencies #3890

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

bcressey
Copy link
Contributor

@bcressey bcressey commented Apr 11, 2024

Issue number:
N/A

Description of changes:
Take advantage of the new metadata provides to simplify the early-boot-config dependencies.

Testing done:
Build log for aws-dev:

#15 0.807  bottlerocket-early-boot-config         x86_64   0.0-0             repo   919 k
#15 0.807  bottlerocket-early-boot-config-aws     x86_64   0.0-0             repo   5.1 M
#15 0.807  bottlerocket-early-boot-config-local   x86_64   0.0-0             repo   2.4 M

Build log for metal-dev:

#15 0.920  bottlerocket-early-boot-config         x86_64   0.0-0             repo   919 k
#15 0.920  bottlerocket-early-boot-config-local   x86_64   0.0-0             repo   2.4 M
#15 0.920  bottlerocket-early-boot-config-metal   x86_64   0.0-0             repo   6.3 k

Build log for vmware-dev:

#15 0.823  bottlerocket-early-boot-config          x86_64   0.0-0            repo   919 k
#15 0.823  bottlerocket-early-boot-config-local    x86_64   0.0-0            repo   2.4 M
#15 0.823  bottlerocket-early-boot-config-vmware   x86_64   0.0-0            repo   1.7 M

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Use RPM's shorthand syntax for subpackages names that are a suffix of
the main package's name.

Signed-off-by: Ben Cressey <[email protected]>
Some form of early-boot-config is required for the OS to function, so
add that dependency to the release package, which collects the other
mandatory dependencies.

Switch to boolean dependencies in the early-boot-config package, so
that the expected providers are installed for the variant's platform.

Remove the explicit dependency from all variant definitions, as an
exercise in boilerplate reduction.

Signed-off-by: Ben Cressey <[email protected]>
Copy link
Contributor

@sam-berning sam-berning left a comment

Choose a reason for hiding this comment

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

Changes look correct to me. My only concern is that it's adding back in some variant-aware conditional compilation to packages/, but this does seem like the logical end of the packaging setup we landed on with early-boot-config-${PLATFORM}.

Just reasoning through this for my own sake, in full-OOTB world it seems like we'll want the early-boot-config package to just install the binary and data-providers.d directory (and maybe the local file providers?). And then OOTBuilders should package in any data providers they want to use themselves. Does that sound right to you @bcressey ?

@bcressey
Copy link
Contributor Author

My only concern is that it's adding back in some variant-aware conditional compilation to packages/, but this does seem like the logical end of the packaging setup we landed on with early-boot-config-${PLATFORM}.

It's admittedly variant-aware, but the appeal of this approach is that the RPMs are actually not conditionally compiled: they are always built the same way, but with a boolean dependency that only triggers if a condition is met at install time. We do still have the one bottlerocket-metadata RPM that is conditionally compiled when building the variant, which is what ultimately drives the different install behaviors we want.

In concrete terms, if you inspect the package you can see that all the requirements are present on the same artifact:

❯ rpm -qp --requires build/rpms/bottlerocket-early-boot-config-0.0-0.x86_64.rpm
(bottlerocket-early-boot-config-aws if bottlerocket-variant-platform(aws))
(bottlerocket-early-boot-config-metal if bottlerocket-variant-platform(metal))
(bottlerocket-early-boot-config-vmware if bottlerocket-variant-platform(vmware))
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsZstd) <= 5.4.18-1
rpmlib(RichDependencies) <= 4.12.0-1

Just reasoning through this for my own sake, in full-OOTB world it seems like we'll want the early-boot-config package to just install the binary and data-providers.d directory (and maybe the local file providers?). And then OOTBuilders should package in any data providers they want to use themselves.

Yes, although the behavior change from PR is that if they name their OOTB variant "aws-*" then it will now automatically pull in early-boot-config-aws. I feel like this is almost always going to be desirable. If it turns out not to be, we would need to shift the binary and directories to a subpackage (e.g. early-boot-config-core) that could be installed separately from any provider.

The OOTBuilder would also need to avoid installing the release package, since that will now pull in early-boot-config and give them the matching provider. However, they will likely need to do that regardless for anything that's deeply customized: release covers a lot of ground, and is quite a bit more opinionated than just the "in EC2, we want to support reading user data from IMDS" opinion that this encodes.

@sam-berning
Copy link
Contributor

Got it, thanks for the explanation!

Yes, although the behavior change from PR is that if they name their OOTB variant "aws-*" then it will now automatically pull in early-boot-config-aws. I feel like this is almost always going to be desirable.

Yeah agreed, I think this is really nice, although it feels a bit "magic". We should probably make a note somewhere in a future OOTB guide that the variant name does matter (i.e. it changes behavior) in some ways.

@bcressey bcressey merged commit 2b71061 into bottlerocket-os:develop Apr 12, 2024
35 checks passed
@bcressey bcressey deleted the simplify-ebc branch April 12, 2024 00:15
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.

5 participants