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

Mariner Image Customizer boilerplate #5982

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

cwize1
Copy link
Contributor

@cwize1 cwize1 commented Aug 10, 2023

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

This creates the boilerplate for Mariner Image Customizer (imgcustomizer) tool. In addition, it implements AdditionalFiles since that is one of the simplest features to implement.

Change Log
  • Added feature to chroot to allow partitions to be mounted before the default partitions.
  • Create imgcustomizer tool boilerplate.
  • Implement AdditionalFiles in imgcustomizer.
  • Added tests for:
    • Unmarshalling of AdditionalFiles JSON/YAML.
    • imgcustomizer's top level functions.
    • AdditionalFiles logic.
Does this affect the toolchain?

NO

Test Methodology
  • Tests added.

@cwize1 cwize1 requested a review from a team as a code owner August 10, 2023 22:51
@microsoft-github-policy-service microsoft-github-policy-service bot added the main PR Destined for main label Aug 10, 2023
@@ -0,0 +1,16 @@
# Mariner Image Customizer API
Copy link
Member

Choose a reason for hiding this comment

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

I think, we should probably keep all the libraries in pkg directory.
The reason, I request this is because, I have proposed that we have a distinction between main packages and lib packages.

Reference:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I am fond of that code layout. Having Go directories with the same name (e.g. specreader and pkg/specreader) can make writing Go code kind of annoying as it forces you to rename one of the packages in the imports.

I also want to avoid the case where the package name is something overly generic. Hence, why I have imagecustomizerapi and not imagecustomizer/api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved imagecustomizerlib into pkg but kept imagecustomizer and imagecustomizerapi here.

Copy link
Contributor

@dmcilvaney dmcilvaney Aug 28, 2023

Choose a reason for hiding this comment

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

Only request is that we track any "internal" code via something like we do for the imager, scheduler, etc.

go_internal_files = $(shell find $(TOOLS_DIR)/internal/ -type f -name '*.go')
go_pkg_files = $(shell find $(TOOLS_DIR)/pkg/ -type f -name '*.go')
go_imagegen_files = $(shell find $(TOOLS_DIR)/imagegen/ -type f -name '*.go')
go_scheduler_files = $(shell find $(TOOLS_DIR)/scheduler -type f -name '*.go')
go_common_files = $(go_module_files) $(go_internal_files) $(go_imagegen_files) $(go_pkg_files) $(go_scheduler_files) $(STATUS_FLAGS_DIR)/got_go_deps.flag $(BUILD_DIR)/tools/internal.test_coverage

Its not particularly elegant but it makes sure we rebuild all the go tools if any of the shared libraries change.

toolkit/tools/imagecustomizerlib/imagecustomizer.go Outdated Show resolved Hide resolved
"github.com/microsoft/CBL-Mariner/toolkit/tools/internal/safechroot"
)

func doCustomizations(baseConfigPath string, config *imagecustomizerapi.SystemConfig, imageChroot *safechroot.Chroot) error {
Copy link
Member

Choose a reason for hiding this comment

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

Would like to open a discussion on a possibility of Trident calling into this function more directly. Trident will be also setting up chroot during image deployment and could take advantage of this function. We can of course create a more suitable wrapper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Had another comment above, but maybe in the future we can hook into these functions as part of the normal image build flow as well.

toolkit/tools/imagecustomizer/main.go Show resolved Hide resolved
toolkit/tools/imagecustomizer/main.go Show resolved Hide resolved
released and doesn't provide any backwards compatibility guarantees.

While currently the new image config and imgcustomizer config are very similar, in the
future there is the possibility they will diverge.
Copy link
Contributor

Choose a reason for hiding this comment

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

Few of us had a idle thought about splitting the existing imager tool in two. What are your thoughts on taking the "customization" parts of the imager tool out, and using this tool as an extra step even for offline buidls? (Or at least the same libraries)
That would couple this tool more closely with the image config.json format, although which direction that coupling happens in isn't clear to me.

toolkit/tools/imagecustomizerapi/README.md Show resolved Hide resolved
"github.com/microsoft/CBL-Mariner/toolkit/tools/internal/safechroot"
)

func doCustomizations(baseConfigPath string, config *imagecustomizerapi.SystemConfig, imageChroot *safechroot.Chroot) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Had another comment above, but maybe in the future we can hook into these functions as part of the normal image build flow as well.

toolkit/tools/imagecustomizerlib/imagecustomizer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@romoh romoh left a comment

Choose a reason for hiding this comment

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

LGTM.

You may want to consider adding a --version option to the boilerplate.

@cwize1 cwize1 force-pushed the user/chrisgun/ImageCustomizer branch from bfb52fc to 370979b Compare September 20, 2023 22:14
@cwize1 cwize1 merged commit c1dc869 into microsoft:main Sep 20, 2023
cwyborny pushed a commit to cwyborny/CBL-Mariner that referenced this pull request Sep 29, 2023
cwyborny pushed a commit to cwyborny/CBL-Mariner that referenced this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
main PR Destined for main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants