add "dependency" provisioning mode #1105

pendo324 · 2022-10-14T21:45:49Z

Adds a new provisioning script mode that allows users to customize the workflow of pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh.

This can be useful in the case where a user wants to override the default behavior to (for example) add repositories before attempting to install system dependencies.

Another option is to add a flag to limayaml to allow running dependency provisioning scripts in addition to the default logic (instead of bypassing the default logic), please comment if that's more preferable.

More details in #1093

jandubois · 2022-10-14T22:34:57Z

Another option is to add a flag to limayaml to allow running dependency provisioning scripts in addition to the default logic (instead of bypassing the default logic), please comment if that's more preferable.

I may need to think about this some more, but my gut feeling would be that the dependencies scripts should always be run in addition to, and not instead of the default installation.

I don't know if there is a need to potentially disable the default package installation, but that would then also require a mechanism to disable the corresponding requirements checks (pkg/hostagent/requirements.go), doesn't it?

pendo324 · 2022-10-14T23:16:04Z

I don't know if there is a need to potentially disable the default package installation, but that would then also require a mechanism to disable the corresponding requirements checks (pkg/hostagent/requirements.go), doesn't it?

I think that the essential requirements should still be the same, but I do see the benefit in having another flag in limayaml to disable default requirement checks.

The most customizable approach would probably be something like:

dependency mode provisioning scripts to run in addition to the default 30-install-packages.sh
- these scripts actually don't have to be limited to the "dependency" scope, naming them something like "early" or "pre-installation" mode would also make sense
new option like skipDefaultDependencyInstallation to skip the default dependency installation
new option like skipDefaultRequirements to skip the default requirements in pkg/hostagent/requirements.go

skipDefaultDependencyInstallation could also be overloaded to skip default requirements too, if we think that two new options would be too much/unnecessary

AkihiroSuda · 2022-11-02T02:07:16Z

Please squash commits

AkihiroSuda · 2022-11-07T07:45:34Z

pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh

-		apk add ${pkgs}
-	fi
+else
+	echo "LIMA_CIDATA_USE_DEFAULT_DEPENDENCY_RESOLUTION set, skipping regular dependency installation"


Did you mean

Suggested change

echo "LIMA_CIDATA_USE_DEFAULT_DEPENDENCY_RESOLUTION set, skipping regular dependency installation"

echo "LIMA_CIDATA_USE_DEFAULT_DEPENDENCY_RESOLUTION != 1, skipping regular dependency installation"

Yeah, good catch. I think changing it to "skip" also makes more sense, like your other comment suggested.

The default value of "false" is easier to reason about, since most users won't interact with this option. Just pushed an update to rebase, address both comments, and also update the default example yaml

AkihiroSuda · 2022-11-07T07:46:32Z

pkg/limayaml/defaults.go

+		y.UseDefaultDependencyResolution = o.UseDefaultDependencyResolution
+	}
+	if y.UseDefaultDependencyResolution == nil {
+		y.UseDefaultDependencyResolution = pointer.Bool(true)


Wondering this should be rather inverted to SkipDefaultDependencyPackages with the default value false

AkihiroSuda

Thanks

AkihiroSuda · 2022-11-09T01:24:11Z

@jandubois LGTY?

AkihiroSuda · 2022-11-18T00:56:36Z

Sorry, needs another rebase

AkihiroSuda

Thanks

AkihiroSuda · 2023-05-04T11:11:02Z

@lima-vm/maintainers Let me know if we can merge this.
Do we want to treat this as an experimental feature?

jandubois

I'm generally fine with this PR, but have some stylistic feedback.

I think marking it as experimental gives us some wiggle-room to still change how we configure it in the future.

jandubois · 2023-05-05T00:04:45Z

examples/default.yaml

+# the regular dependency resolution workflow in pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh
+# and provide a fully customizable replacement
+# 🟢 Builtin default: false
+# skipDefaultDependencyResolution: true


This is quite a mouthful, and also feels somewhat disconnected here. I'm wondering if this would make more sense to make this an option on the dependency provisioning script.

I don't have strong opinions, it just feels a bit clunky to have this weirdly-named option at the top level, that only makes sense when you also have some other bits of configuration defined.

Good point, addressed in latest revision. I added a new field to the Provisioning scripts in general, but invalidate it when used on non-dependency scripts.

jandubois · 2023-05-05T00:07:05Z

pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh

+	for f in "${LIMA_CIDATA_MNT}"/provision.dependency/*; do
+		echo "Executing $f"
+		if ! "$f"; then
+			echo "Failed to execute $f"


Is there a reason we don't want to exit 1 here? The rest of the script runs under set -e, so any error return would immediately exit the script.

On the other hand, in boot.sh, we set CODE=1 when a provisioning script fails, and then exit $CODE at the end.

Makes sense. Changed it to collect all errors in a CODE var and exit if its 1 after all dependency scripts, to match boot.sh behavior

jandubois · 2023-05-05T00:10:10Z

pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh

+if [ "${LIMA_CIDATA_SKIP_DEFAULT_DEPENDENCY_RESOLUTION}" = 1 ]; then
+	echo "LIMA_CIDATA_SKIP_DEFAULT_DEPENDENCY_RESOLUTION is set, skipping regular dependency installation"
+else


The conditionals are already nested too deeply in this file for my taste, so adding a 140 line else clause does not feel appealing.

I would rather do an early exit here, and then move the 2 actions at the bottom of the script (setting up DNS, and updating fuse_conf) into a separate script.

I wasn't a big fan of the nesting either. Split the file into two and exit early now

Signed-off-by: Justin Alvarez <alvajus@amazon.com>

AkihiroSuda

Thanks

AkihiroSuda · 2023-05-18T15:40:30Z

@jandubois LGTY?

jandubois

Thanks, LGTM.

I think having the comment on the error exit would be nice, but I'm also fine with merging as-is.

jandubois · 2023-05-20T23:11:21Z

pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh

+			CODE=1
+		fi
+	done
+	if [ $CODE != 0 ]; then


I think adding a log message that we are exiting early would be nice to have (as we do in boot.sh):

Suggested change

if [ $CODE != 0 ]; then

if [ $CODE -ne 0 ]; then

echo "Exiting with code $CODE"

pendo324 mentioned this pull request Oct 19, 2022

add "additionalArchives" config option #1123

Open

AkihiroSuda added this to the v0.14 (tentative) milestone Nov 2, 2022

pendo324 force-pushed the dependency-provisioning-mode branch from 775657e to 3eb8a5d Compare November 2, 2022 17:09

AkihiroSuda requested a review from jandubois November 2, 2022 23:24

AkihiroSuda reviewed Nov 7, 2022

View reviewed changes

pendo324 force-pushed the dependency-provisioning-mode branch 2 times, most recently from 6c15c5f to 575e0f1 Compare November 8, 2022 19:29

AkihiroSuda previously approved these changes Nov 9, 2022

View reviewed changes

AkihiroSuda modified the milestones: v0.14, v0.15 (tentative) Nov 25, 2022

AkihiroSuda mentioned this pull request Dec 23, 2022

Allow local repo directories in containerd archives #1263

Open

AkihiroSuda modified the milestones: v0.15.0, v0.16 (tentative) Feb 21, 2023

AkihiroSuda removed this from the v0.15.1 milestone Apr 5, 2023

pendo324 dismissed AkihiroSuda’s stale review via e128e1e April 13, 2023 20:56

pendo324 force-pushed the dependency-provisioning-mode branch from 575e0f1 to e128e1e Compare April 13, 2023 20:56

pendo324 requested a review from AkihiroSuda April 13, 2023 20:57

AkihiroSuda added this to the v0.16.0 (tentative) milestone Apr 14, 2023

AkihiroSuda previously approved these changes May 4, 2023

View reviewed changes

jandubois reviewed May 5, 2023

View reviewed changes

add "dependency" provisioning mode

8e7efed

Signed-off-by: Justin Alvarez <alvajus@amazon.com>

pendo324 dismissed AkihiroSuda’s stale review via 8e7efed May 15, 2023 19:23

pendo324 force-pushed the dependency-provisioning-mode branch from e128e1e to 8e7efed Compare May 15, 2023 19:23

pendo324 requested review from jandubois and AkihiroSuda May 15, 2023 19:49

AkihiroSuda approved these changes May 16, 2023

View reviewed changes

jandubois approved these changes May 20, 2023

View reviewed changes

AkihiroSuda merged commit 28cf48d into lima-vm:master May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "dependency" provisioning mode #1105

add "dependency" provisioning mode #1105

pendo324 commented Oct 14, 2022

jandubois commented Oct 14, 2022

pendo324 commented Oct 14, 2022 •

edited

Loading

AkihiroSuda commented Nov 2, 2022

AkihiroSuda Nov 7, 2022

pendo324 Nov 8, 2022 •

edited

Loading

AkihiroSuda Nov 7, 2022 •

edited

Loading

AkihiroSuda left a comment

AkihiroSuda commented Nov 9, 2022

AkihiroSuda commented Nov 18, 2022

AkihiroSuda left a comment

AkihiroSuda commented May 4, 2023

jandubois left a comment

jandubois May 5, 2023

pendo324 May 15, 2023

jandubois May 5, 2023

pendo324 May 15, 2023

jandubois May 5, 2023

pendo324 May 15, 2023

AkihiroSuda left a comment

AkihiroSuda commented May 18, 2023

jandubois left a comment

jandubois May 20, 2023

	echo "LIMA_CIDATA_USE_DEFAULT_DEPENDENCY_RESOLUTION set, skipping regular dependency installation"
	echo "LIMA_CIDATA_USE_DEFAULT_DEPENDENCY_RESOLUTION != 1, skipping regular dependency installation"

	if [ $CODE != 0 ]; then
	if [ $CODE -ne 0 ]; then
	echo "Exiting with code $CODE"

add "dependency" provisioning mode #1105

add "dependency" provisioning mode #1105

Conversation

pendo324 commented Oct 14, 2022

jandubois commented Oct 14, 2022

pendo324 commented Oct 14, 2022 • edited Loading

AkihiroSuda commented Nov 2, 2022

Choose a reason for hiding this comment

pendo324 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Nov 9, 2022

AkihiroSuda commented Nov 18, 2022

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented May 4, 2023

jandubois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented May 18, 2023

jandubois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pendo324 commented Oct 14, 2022 •

edited

Loading

pendo324 Nov 8, 2022 •

edited

Loading

AkihiroSuda Nov 7, 2022 •

edited

Loading