Support installing extensions and rework OS update #1941

sinnykumari · 2020-07-21T11:03:09Z

Earlier to perform OS update we were pulling OS image
and mounting the content using podman. We were performing
OS update in host context because of selinux constraints
on mounted container.

Also, for rt-kernel switch we were pulling OS image again.
With coreos extensions support, we require extensions rpm
which is available in os container.

We tried different approach to solve problems
like minimizing container image pull, host/container
context switch, selinux permission on mounted
container and rpm-ostreed behavior.
See #1850

Finally, using oc image extract and skopeo to inspect container
image solves our problems. With this we are also getting
rid of using mco-pivot systemd service to rebase OS.

sinnykumari · 2020-07-21T11:16:34Z

Note that with this PR we don't need to do m-c-d pivot . So, if we require tunable args then we can either update update()->updateKernelArguments() accordingly or update our docs to perform tunable args via MachineConfigs.

runcom · 2020-07-21T11:40:22Z

pkg/daemon/update.go

+		return fmt.Errorf("error creating extensions repo %s: %v", extensionsRepo, err)
+	}
+	defer repo.Close()
+	if _, err := repo.WriteString("[coreos-extensions]\nenabled=1\nmetadata_expire=1m\nbaseurl=" + osImageContentDir + "/extensions/\ngpgcheck=0\nskip_if_unavailable=False\n"); err != nil {


you could use writeFileAtomically

runcom · 2020-07-21T11:41:29Z

pkg/daemon/update.go

+	args := generateExtensionsArgs(oldConfig, newConfig)
+	glog.Infof("Applying extensions : %+q", args)
+	if err := exec.Command("rpm-ostree", args...).Run(); err != nil {
+		return fmt.Errorf("Failed to execute rpm-ostree %+q : %v", args, err)


nit: errors start lowercase (glog uppercase)

pkg/daemon/rpm-ostree.go

sinnykumari · 2020-07-21T14:00:39Z

/retest

cgwalters · 2020-07-21T15:26:54Z

pkg/daemon/rpm-ostree.go

 	var output []byte
-	output, err = runGetOut("podman", inspectArgs...)
+	output, err = runGetOut("skopeo", inspectArgs...)


This is going to re-introduce openshift/pivot#51

I went skopeo route to save pulling container image as it is required by podman inspect. Is skopeo inspect more expensive than doing podman pull containerImg && podman inspect ?

I was also wondering if we can just use https://github.com/openshift/machine-config-operator/blob/master/pkg/daemon/rpm-ostree.go#L257,L280 to get checksum and rebase. It works but not sure if we hit issue in some situation.

I think we actually have the containers/image stack vendored into the MCO for other reasons, so we could just directly do that from our process. Another alternative looks like oc image info -o json.

Or we could try to figure out if there's a way to get skopeo to not try to enumerate all the tags.

By looking at skopeo code, it doesn't seem like we can ask skopeo to skip looking for tags(have also asked on skopeo irc channel to get a second opinion)
Not very comfortable with us maintaining the code to inspect images, feels like it is better to use already tested tool oc image info

After discussing with skopeo team it appears skipping to fetch RepoTags is not possible right now. With inspiration from skopeo inspect code, wrote an independent program which uses containers/image stack(already vendored in MCO) https://github.com/sinnykumari/container-inspect/blob/master/container_inspect.go and doesn't fetch RepoTags . If this looks ok I will integrate this in MCO, otherwise we can go for oc image info

Pushed changes to use containers/image stack directly for inspecting image.

cgwalters · 2020-07-21T15:48:40Z

pkg/daemon/rpm-ostree.go

-	cid := strings.TrimSpace(string(cidBuf))
-	// Use the container ID to find its mount point
-	var mntBuf []byte
-	mntBuf, err = runGetOut("podman", "mount", cid)


Aren't we missing a call to the extract function here?

Hopefully I understood your question correctly.
We are now not mounting container image. https://github.com/openshift/machine-config-operator/pull/1941/files#diff-06961b075f1753956d802ba954d2cfb5R251 is where we extract OS container content in /run/mco-machine-os-content/ and use same for rpm-ostree rebase.

kikisdeliveryservice

Some thoughts

kikisdeliveryservice · 2020-07-21T23:40:12Z

pkg/daemon/update.go

+		}
+	}
+
+	if mcDiff.osUpdate {


This is confusing bc the if stmt right before this is also checking for mcDiff.osUpdate. Why not nest this inside the above or not possible?

I feel moving this with earlier section is going to be tricky and may lead to un-necessary bugs. Both section are doing different things and right order is really important here.

Actually if is not needed here as we also compare OSImageURL in updateOS()

kikisdeliveryservice · 2020-07-21T23:42:09Z

pkg/daemon/update.go

-				}
+			dn.logSystem("Updating rt-kernel packages on host: %+q", args)
+			if err := exec.Command("rpm-ostree", "update").Run(); err != nil {
+				return fmt.Errorf("Failed to execute rpm-ostree %+q : %v", args, err)


kikisdeliveryservice · 2020-07-21T23:43:41Z

pkg/daemon/update.go

-			pivotErr, err2 := ioutil.ReadFile(pivottypes.PivotFailurePath)
-			if err2 != nil || len(pivotErr) == 0 {
-				glog.Warningf("pivot error file doesn't contain anything, it was never written or an error occurred: %v", err2)
+	// Before rebase make sure that osImageContentDir exists. This is to continue with OS update in case we came here directly


❤️ this comment!

kikisdeliveryservice · 2020-07-21T23:46:45Z

pkg/daemon/update.go

+	return nil
+}
+
+func extractOSImage(imgURL string) error {


I feel like this file has so many helper funcs it would be easier to move them into their own file for readability

true. will look at moving helper function in a separate file

I feel that this is one of the things which we can discuss collectively to cleanup and refactor MCO code.

kikisdeliveryservice · 2020-07-21T23:59:33Z

pkg/daemon/update.go

+		return err
+	}
+
+	if mcDiff.osUpdate || mcDiff.extensions || mcDiff.kernelType {


feels like this should be 1 func call that evals this if inside and then calls extract, addExtensions & updateOS

Definitely, there is scope of moving OS update/layering section into a function.

sinnykumari · 2020-07-22T11:58:57Z

Thank you all for reviewing!
Have pushed changes which should address all comments other than skopeo issue(digging into it)

jlebon · 2020-07-22T14:14:42Z

pkg/daemon/daemon.go

@@ -1004,7 +1004,10 @@ func (dn *Daemon) checkStateOnFirstRun() error {
 		if !osMatch {
 			glog.Infof("Bootstrap pivot required to: %s", targetOSImageURL)
 			// This only returns on error


Should move this comment down to the dn.finalizeAndReboot now I suppose?

umm, we will return back here also when updateOS() fails?

jlebon · 2020-07-22T14:16:41Z

pkg/daemon/update.go

+// addExtensionsRepo adds a repo into /etc/yum.repos.d/ which we use later to
+// install extensions and rt-kernel
+func addExtensionsRepo() error {
+	repoConetnt := "[coreos-extensions]\nenabled=1\nmetadata_expire=1m\nbaseurl=" + osImageContentDir + "/extensions/\ngpgcheck=0\nskip_if_unavailable=False\n"


Why gpgcheck=0 here? Shouldn't all the RPMs we ship via extensions be signed?

Ah yes, good catch. Don't know why I kept it like that, most likely copy paste error.

We had an offline discussion, but putting a comment here for future reference:
It is possible that some packages may not be signed, especially when dealing with nightly images. We are anyway shipping extensions packages in machine-os-content(which is source of truth for us). So it should be ok to install them with gpgcheck off.

I don't know that it's ok to turn off signature verification. It seems like it leaves open a pretty significant security hole that attackers could exploit to run untrusted code on the host. While we check the machine-os-content image signature, we don't have any checks on the integrity of the files in the extensions directory ensuring that the RPMs that get installed are same ones shipped in the machine-os-image without alteration. There are likely other security gaps that could be enabled by installing unverified RPMs.

At the very least, I'm pretty sure many customers in highly-regulated industries require every package to be signed and validated to run on a host.

While we check the machine-os-content image signature, we don't have any checks on the integrity of the files in the extensions directory ensuring that the RPMs that get installed are same ones shipped in the machine-os-image without alteration.

We do, because we pull-by-digest and the container stack verifies that the digest matches.

jlebon · 2020-07-22T14:18:48Z

pkg/daemon/update.go

+// addExtensionsRepo adds a repo into /etc/yum.repos.d/ which we use later to
+// install extensions and rt-kernel
+func addExtensionsRepo() error {
+	repoConetnt := "[coreos-extensions]\nenabled=1\nmetadata_expire=1m\nbaseurl=" + osImageContentDir + "/extensions/\ngpgcheck=0\nskip_if_unavailable=False\n"


Minor typo: repoContent

sinnykumari · 2020-07-23T09:55:41Z

/retest

sinnykumari · 2020-07-23T17:28:51Z

/test cluster-bootimages

sinnykumari · 2020-07-27T09:56:43Z

@mtrmac Thank you for the help with implementing container inspect. 28742e3 is how currently I have implemented in MCO, let us know if you have any additional feedback.

sinnykumari · 2020-07-27T11:33:23Z

/retest

sinnykumari · 2020-07-27T12:39:06Z

/test cluster-bootimages

sinnykumari · 2020-07-27T13:36:03Z

Tests seems to be flaky
/retest

mtrmac

(Reading only the image inspect part.) ACK, just trivial nits.

mtrmac · 2020-07-27T13:49:58Z

pkg/daemon/container-inspect.go

@@ -0,0 +1,77 @@
+package daemon


Shouldn’t the file be named image-inspect.go?

mtrmac · 2020-07-27T13:51:10Z

pkg/daemon/container-inspect.go

+	return err
+}
+
+// parseImageSource converts image URL-like string to an ImageSource.


With docker.ParseReference it is no longer very URL-like; and I would expect this function to handle the "//" prefix instead of callers having to care. Ideally, this should “convert an image reference”, or “create an ImageSource from an image reference”.

mtrmac · 2020-07-27T13:51:27Z

pkg/daemon/container-inspect.go

+
+// parseImageSource converts image URL-like string to an ImageSource.
+// The caller must call .Close() on the returned ImageSource.
+func parseImageSource(ctx context.Context, name string) (types.ImageSource, error) {


Absolutely non-blocking: newDockerImageSource maybe; the “parse” name was used in context of a fairly general CLI that is not applicable in this call tree.

mtrmac · 2020-07-27T13:52:23Z

pkg/daemon/container-inspect.go

+	if err != nil {
+		return nil, err
+	}
+	sys := &types.SystemContext{AuthFilePath: kubeletAuthFile}


The caller can pass this down instead of having two copies that can get out of sync.

mtrmac · 2020-07-27T13:55:53Z

go.mod

@@ -8,23 +8,20 @@ require (
 	github.com/Masterminds/goutils v1.1.0 // indirect
 	github.com/Masterminds/semver v1.4.2 // indirect
 	github.com/Masterminds/sprig v2.20.0+incompatible
-	github.com/Microsoft/go-winio v0.4.14 // indirect
 	github.com/OpenPeeDeeP/depguard v1.0.1 // indirect
 	github.com/apparentlymart/go-cidr v1.0.0
 	github.com/ashcrow/osrelease v0.0.0-20180626175927-9b292693c55c
 	github.com/clarketm/json v1.14.1
 	github.com/containers/image v3.0.2+incompatible


Hum, container-runtime-config (and openshift/runtime-utils) still use the old version. That should probably be updated to decrease duplication… but it’s also rather out of scope of this PR.

yeah, we can clean this up separately.

mtrmac · 2020-07-27T13:57:59Z

pkg/daemon/rpm-ostree.go

-	var imagedataArray []imageInspection
-	err = json.Unmarshal(output, &imagedataArray)
+	var imageData *types.ImageInspectInfo
+	imageData, err = imageInspect(fmt.Sprintf("//%s", container))


As mentioned elsewhere, I’d expect the "//" syntax to be hidden in the implementation.

sinnykumari · 2020-07-27T14:55:23Z

(Reading only the image inspect part.) ACK, just trivial nits.

Thanks for the review, pushed changes addressing the feedback.

sinnykumari · 2020-07-27T14:55:45Z

/test cluster-bootimages

Vendor latest containers/image v5.5.1 which contains utilities needed to inspect container image in MCO.

Also rename pkg/daemon/container-inspect.go to pkg/daemon/image-inspect.go to fit naming better with what it does.

Also, explicitly delete the extracted OS image content once done

…iner

sinnykumari · 2020-07-31T17:52:30Z

Rebased to get e2e-gcp-op fixes from #1969 included

sinnykumari · 2020-07-31T18:05:16Z

needs lgtm blessing :)

cgwalters · 2020-07-31T18:05:28Z

/lgtm

openshift-ci-robot · 2020-07-31T18:05:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom, sinnykumari

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,runcom,sinnykumari]

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

sinnykumari · 2020-07-31T20:03:06Z

e2e-aws is failing consistently today across various PRs.
If it makes sense let's override it.

kikisdeliveryservice · 2020-07-31T20:43:12Z

e2e-aws is failing consistently today across various PRs.
If it makes sense let's override it.

The PR is already approved by the cutoff, we usually don't override tests just to merge, they usually end up merging over the weekend. This PR hasn't passed this test since its initial run on Tuesday so I think we should just let the bot retest so it can pass.

openshift-bot · 2020-08-01T00:59:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-01T02:17:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-01T03:22:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-01T04:14:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-01T06:37:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-08-01T07:17:24Z

@sinnykumari: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-proxy	26b51d2abc9a3842d062eede1321dbce2012522c	link	`/test e2e-aws-proxy`
ci/prow/e2e-metal-ipi	`5b44766`	link	`/test e2e-metal-ipi`
ci/prow/okd-e2e-aws	`5b44766`	link	`/test okd-e2e-aws`

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/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2020-08-01T07:29:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

With the introduction of extensions and reworking of OS updates in openshift#1941, we now run `oc/podman` directly in the MCD to fetch the OS image. Thus we need to inject the proxy env vars into the container via the daemonset definition, otherwise image pulls will fail. Signed-off-by: Yu Qi Zhang <[email protected]>

openshift-ci-robot requested review from cgwalters and ericavonb July 21, 2020 11:03

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2020

sinnykumari mentioned this pull request Jul 21, 2020

Support installing extensions shipped by RHCOS #1850

Closed

sinnykumari requested a review from runcom July 21, 2020 11:08

sinnykumari requested a review from LorbusChris July 21, 2020 11:17

runcom reviewed Jul 21, 2020

View reviewed changes

pkg/daemon/rpm-ostree.go Show resolved Hide resolved

cgwalters reviewed Jul 21, 2020

View reviewed changes

kikisdeliveryservice reviewed Jul 22, 2020

View reviewed changes

sinnykumari force-pushed the coreos-extensions branch from 26b51d2 to 0ae69c8 Compare July 22, 2020 07:29

jlebon reviewed Jul 22, 2020

View reviewed changes

sinnykumari force-pushed the coreos-extensions branch from 6cdcc06 to a714530 Compare July 23, 2020 07:39

sinnykumari force-pushed the coreos-extensions branch from a714530 to 990e561 Compare July 23, 2020 12:35

sinnykumari force-pushed the coreos-extensions branch 2 times, most recently from 2123350 to 33c3ce8 Compare July 27, 2020 09:52

mtrmac reviewed Jul 27, 2020

View reviewed changes

sinnykumari force-pushed the coreos-extensions branch from 26c0487 to 966d8b8 Compare July 27, 2020 14:59

sinnykumari added 6 commits July 31, 2020 23:20

vendor: update vendor to use github.com/containers/image/ v5.5.1

16771e3

Vendor latest containers/image v5.5.1 which contains utilities needed to inspect container image in MCO.

daemon: refactor image inspect logic

549f7b1

Also rename pkg/daemon/container-inspect.go to pkg/daemon/image-inspect.go to fit naming better with what it does.

daemon: extract OS image in a temporary directory

6cdfc2d

Also, explicitly delete the extracted OS image content once done

daemon: reuse existing InSlice() from common package

e34463f

test/e2e: use infra pool to test extensions

662bf1d

test/e2e: check for installed package on node instead of in the conta…

5b44766

…iner

sinnykumari force-pushed the coreos-extensions branch from 0cd99b9 to 5b44766 Compare July 31, 2020 17:51

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2020

openshift-ci-robot assigned cgwalters Jul 31, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2020

sinnykumari mentioned this pull request Aug 1, 2020

Bug 1852047: daemon: Remove "compare digest" code for OS updates #1971

Merged

openshift-merge-robot merged commit 17b8d09 into openshift:master Aug 1, 2020

sinnykumari mentioned this pull request Aug 27, 2020

Bug 1861026: daemon: perform other rpm-ostree operations after OS rebase #2029

Merged

yuqi-zhang mentioned this pull request Aug 28, 2020

Bug 1857162: daemon: inject proxy vars into MCD container #2037

Merged

yuqi-zhang mentioned this pull request Sep 2, 2021

update: Always use podman pull+cp #2751

Closed

Support installing extensions and rework OS update #1941

Support installing extensions and rework OS update #1941

Conversation

sinnykumari commented Jul 21, 2020

sinnykumari commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinnykumari commented Jul 21, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikisdeliveryservice 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

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

sinnykumari commented Jul 22, 2020

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

Choose a reason for hiding this comment

sinnykumari commented Jul 23, 2020

sinnykumari commented Jul 23, 2020

sinnykumari commented Jul 27, 2020

sinnykumari commented Jul 27, 2020

sinnykumari commented Jul 27, 2020

sinnykumari commented Jul 27, 2020

mtrmac 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

Choose a reason for hiding this comment

sinnykumari commented Jul 27, 2020

sinnykumari commented Jul 27, 2020

sinnykumari commented Jul 31, 2020

sinnykumari commented Jul 31, 2020

cgwalters commented Jul 31, 2020

openshift-ci-robot commented Jul 31, 2020

sinnykumari commented Jul 31, 2020

kikisdeliveryservice commented Jul 31, 2020

openshift-bot commented Aug 1, 2020

openshift-bot commented Aug 1, 2020

openshift-bot commented Aug 1, 2020

openshift-bot commented Aug 1, 2020

openshift-bot commented Aug 1, 2020

openshift-ci-robot commented Aug 1, 2020

openshift-bot commented Aug 1, 2020