overlay node image before bootstrapping if necessary #899

jlebon · 2024-09-06T19:40:54Z

As per openshift/enhancements#1637, we're trying
to get rid of all OpenShift-versioned components from the bootimages.

This means that there will no longer be oc, kubelet, or crio
binaries for example, which bootstrapping obviously relies on.

To adapt to this, the OpenShift installer now ships a new
node-image-overlay.service in its bootstrap Ignition config. This
service takes care of pulling down the node image and overlaying it,
effectively updating the system to the node image version.

Here, we accordingly also adapt assisted-installer so that we run
node-image-overlay.service before starting e.g. kubelet.service and
bootkube.service.

See also: openshift/installer#8742

openshift-ci · 2024-09-06T19:41:02Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

src/installer/installer.go

openshift-bot · 2024-12-10T01:00:27Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

jlebon · 2024-12-13T17:33:07Z

/remove-lifecycle stale

Now that openshift/enhancements#1637 has merged, we will need this. I'll leave it in draft for now until I can rebase and retest it, but feel free to start reviewing.

Because that's effectively what it is. Also use that in another case that was manually calling `stat`.

As per openshift/enhancements#1637, we're trying to get rid of all OpenShift-versioned components from the bootimages. This means that there will no longer be oc, kubelet, or crio binaries for example, which bootstrapping obviously relies on. To adapt to this, the OpenShift installer now ships a new `node-image-overlay.service` in its bootstrap Ignition config. This service takes care of pulling down the node image and overlaying it, effectively updating the system to the node image version. Here, we accordingly also adapt assisted-installer so that we run `node-image-overlay.service` before starting e.g. `kubelet.service` and `bootkube.service`. See also: openshift/installer#8742

openshift-ci · 2024-12-21T17:22:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jlebon
Once this PR has been reviewed and has the lgtm label, please assign jhernand for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jlebon · 2024-12-21T17:23:11Z

Rebased and verified this still works! Requires openshift/installer#8742.

codecov · 2024-12-21T17:41:11Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 52.35%. Comparing base (34b971a) to head (33d9f10).

Files with missing lines	Patch %	Lines
src/ops/ops.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
- Coverage   55.20%   52.35%   -2.85%     
==========================================
  Files          15       14       -1     
  Lines        3286     2695     -591     
==========================================
- Hits         1814     1411     -403     
+ Misses       1292     1159     -133     
+ Partials      180      125      -55

Files with missing lines	Coverage Δ
src/ops/ops.go	`43.39% <0.00%> (ø)`

... and 1 file with indirect coverage changes

openshift-ci · 2024-12-21T20:04:39Z

@jlebon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/edge-unit-test	`33d9f10`	link	true	`/test edge-unit-test`
ci/prow/okd-scos-e2e-aws-ovn	`33d9f10`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/edge-e2e-oci-assisted-4-18	`33d9f10`	link	false	`/test edge-e2e-oci-assisted-4-18`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

tsorya · 2025-01-09T14:05:03Z

src/installer/installer.go

@@ -669,6 +685,25 @@ func (i *installer) waitForBootkube(ctx context.Context) {
 	}
 }

+func (i *installer) waitForNodeImageOverlay(ctx context.Context) {


Probably it can be change to just call utils.WaitForPredicate with
i.ops.SystemctlAction("is-active", "node-image-overlay.service")
WDYT?

tsorya · 2025-01-09T14:06:49Z

src/ops/ops.go

-// DryRebootHappened checks if a reboot happened according to a particular reboot marker path
-// The dry run installer creates this file on "Reboot" (instead of actually rebooting)
-// We use this function to check whether the given node in the cluster have already rebooted
-func (o *ops) DryRebootHappened(markerPath string) bool {


IMO lets leave this one as it helps to understand that we are looking for dry reboot logic, it can though call for FileExists

tsorya · 2025-01-09T14:07:13Z

src/installer/installer.go

+
+		i.waitForNodeImageOverlay(context.Background())
+
+		if !i.ops.FileExists("/usr/bin/oc") {


lets make /usr/bin/oc to be a const?

tsorya · 2025-01-09T14:10:56Z

src/installer/installer.go

+	// If we're in a pure RHEL/CentOS environment, we need to overlay the node image
+	// first to have access to e.g. oc, kubelet, cri-o, etc...
+	// https://github.com/openshift/enhancements/pull/1637
+	if !i.ops.FileExists("/usr/bin/oc") {


Shouldn't we pull overlay the node image as a first thing? I mean we already applied bootstrap ignition at this point and this can be a little bit late for us to pull another image layer, we probably should do it before

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2024

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 6, 2024

jlebon mentioned this pull request Sep 6, 2024

overlay node image before bootstrapping if necessary openshift/installer#8742

Open

paul-maidment reviewed Sep 8, 2024

View reviewed changes

src/installer/installer.go Show resolved Hide resolved

jlebon force-pushed the pr/no-more-oc branch from 23f9c3f to 48c6cc1 Compare September 10, 2024 15:01

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 10, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 13, 2024

jlebon added 2 commits December 20, 2024 13:30

ops: rename DryRebootHappened to FileExists

95fa603

Because that's effectively what it is. Also use that in another case that was manually calling `stat`.

jlebon force-pushed the pr/no-more-oc branch from 48c6cc1 to 33d9f10 Compare December 21, 2024 17:21

jlebon changed the title ~~WIP: overlay node image before bootstrapping if necessary~~ overlay node image before bootstrapping if necessary Dec 21, 2024

jlebon marked this pull request as ready for review December 21, 2024 17:21

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2024

openshift-ci bot requested review from jhernand and omertuc December 21, 2024 17:22

tsorya reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlay node image before bootstrapping if necessary #899

overlay node image before bootstrapping if necessary #899

jlebon commented Sep 6, 2024

openshift-ci bot commented Sep 6, 2024

openshift-bot commented Dec 10, 2024

jlebon commented Dec 13, 2024

openshift-ci bot commented Dec 21, 2024

jlebon commented Dec 21, 2024

codecov bot commented Dec 21, 2024 •

edited

Loading

openshift-ci bot commented Dec 21, 2024

tsorya Jan 9, 2025

tsorya Jan 9, 2025

tsorya Jan 9, 2025

tsorya Jan 9, 2025


		i.waitForNodeImageOverlay(context.Background())

		if !i.ops.FileExists("/usr/bin/oc") {

overlay node image before bootstrapping if necessary #899

Are you sure you want to change the base?

overlay node image before bootstrapping if necessary #899

Conversation

jlebon commented Sep 6, 2024

openshift-ci bot commented Sep 6, 2024

openshift-bot commented Dec 10, 2024

jlebon commented Dec 13, 2024

openshift-ci bot commented Dec 21, 2024

jlebon commented Dec 21, 2024

codecov bot commented Dec 21, 2024 • edited Loading

Codecov Report

openshift-ci bot commented Dec 21, 2024

tsorya Jan 9, 2025

Choose a reason for hiding this comment

tsorya Jan 9, 2025

Choose a reason for hiding this comment

tsorya Jan 9, 2025

Choose a reason for hiding this comment

tsorya Jan 9, 2025

Choose a reason for hiding this comment

codecov bot commented Dec 21, 2024 •

edited

Loading