pkg/daemon: implement OS version validation #60

jlebon · 2018-09-13T20:58:32Z

Teach the MCD to check the current OS version and upgrade the host. This
essentially implements the OSImageURL part of the MachineConfig
spec.

Since we're in a chroot anyway, I just took the easy path of exec'ing
rpm-ostree and pivot directly. Which is good, because right now only
pivot knows how to upgrade RHCOS from an oscontainer image pull spec.

jlebon · 2018-09-13T21:00:06Z

I'm going to hold this for now. It works fine though I haven't reprovisioned my cluster in a while, and the installer and m-c-o move at the speed of light...

/hold

sdemos · 2018-09-13T21:24:04Z

pkg/daemon/rpm-ostree.go

+
+	// just make it a hard error if we somehow don't have any deployments
+	if len(rosState.Deployments) == 0 {
+		return nil, fmt.Errorf("Not currently booted in a deployment")


since this is entirely the same error as the one at the end of the function, and the one at the end of the function will be hit if the length is zero and this isn't here, do we need this if statement at all?

Oh yeah, I meant to drop this but forgot!

sdemos · 2018-09-13T21:26:10Z

pkg/daemon/update.go

@@ -400,6 +403,11 @@ func getFileOwnership(file ignv2_2types.File) (int, int, error) {
 	return uid, gid, nil
 }

+func (dn *Daemon) updateOS(oldConfig, newConfig *mcfgv1.MachineConfig) error {
+	glog.Infof("Updating OS to %s", newConfig.Spec.OSImageURL)
+	return runPivot(newConfig.Spec.OSImageURL)


is pivot idempotent? this is going to get run times when there is not an actual os update, because everything gets refreshed if anything is different (eg if we add a file, we will run pivot with our currently booted os image url even though it's not different)

Yes, it's idempotent (see https://github.com/ashcrow/pivot/pull/14), but I should add a check here as well so we don't waste time running it.

sdemos · 2018-09-13T21:26:47Z

pkg/daemon/update.go

@@ -400,6 +403,11 @@ func getFileOwnership(file ignv2_2types.File) (int, int, error) {
 	return uid, gid, nil
 }

+func (dn *Daemon) updateOS(oldConfig, newConfig *mcfgv1.MachineConfig) error {


even though this isn't exported, it's probably worth adding a comment describing it's functionality to keep it in line with the other functions that perform parts of an update.

sdemos · 2018-09-13T21:29:39Z

pkg/daemon/rpm-ostree.go

+)
+
+// Subset of `rpm-ostree status --json`
+// https://github.com/projectatomic/rpm-ostree/blob/bce966a9812df141d38e3290f845171ec745aa4e/src/daemon/rpmostreed-deployment-utils.c#L227


love this ref

abhinavdahiya · 2018-09-13T22:27:11Z

@jlebon can you run ./hack/update-generated-bindata.sh to update /manifests to pkg/operator/assets/bindata.go

ashcrow · 2018-09-14T13:49:09Z

pkg/daemon/daemon.go

 	if dn.checkFiles(desiredConfig.Spec.Config.Storage.Files) &&
-		dn.checkUnits(desiredConfig.Spec.Config.Systemd.Units) {
+		dn.checkUnits(desiredConfig.Spec.Config.Systemd.Units) &&
+		isDesiredOS {


nit: My brain does not like how this if statement is structured, but it's not problematic.

ashcrow · 2018-09-14T13:49:49Z

pkg/daemon/daemon.go

 		return true, nil
 	}

 	// error is nil, as we successfully decided that validate is false
 	return false, nil
 }

+func (dn *Daemon) checkOS(osImageURL string) (bool, error) {


nit: It would be helpful to document what the non-error return signifies.

jlebon · 2018-09-14T21:22:05Z

Alrighty, comments addressed! Tested on top of the latest everything. Dropping hold.
/hold cancel

jlebon · 2018-09-14T21:23:42Z

I will note the daemon cannot yet go through with rebooting the node due to some missing role bindings:

I0914 21:07:35.208898    6711 update.go:44] Update completed. Draining the node.
E0914 21:07:35.215929    6711 daemon.go:85] Marking degraded due to: pods is forbidden: User "system:serviceaccount:openshift-machine-config-operator:machine-config-daemon" cannot list pods at the cluster scope: no RBAC policy matched

Though let's keep that separate.

jlebon · 2018-09-14T21:24:40Z

I filed #66 so we remember it.

jlebon · 2018-09-14T21:28:29Z

Oh one more thing. Once this merges, the MCD in a fresh cluster will error out because the osImageURL currently embedded in machine configs is set to ://dummy. So we're essentially at the point where the installer actually needs to use https://github.com/ashcrow/pivot.

If we're not ready for that yet, we could work around this for now by considered a MC with ://dummy a match on nodes which haven't yet been updated.

abhinavdahiya · 2018-09-14T22:44:53Z

@jlebon everything regarding the MachineConfig generation for masters and worker, is controller by this repo.

https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/template/render.go#L208

abhinavdahiya · 2018-09-14T22:47:50Z

Also #66 (comment)

ashcrow

Will defer to a second review by @sdemos for merging.

jlebon · 2018-09-17T13:53:27Z

@jlebon everything regarding the MachineConfig generation for masters and worker, is controller by this repo.

It's not that simple though. This is dependent on the installer moving over from using the RHCOS AMI/qcow2 directly to using the OS container image and pivot.

jlebon · 2018-09-17T14:49:08Z

If we're not ready for that yet, we could work around this for now by considered a MC with ://dummy a match on nodes which haven't yet been updated.

OK, I pushed another commit for now which does this! ⬆️

ashcrow · 2018-09-17T17:34:55Z

/lgtm

jlebon · 2018-09-17T17:36:10Z

Sorry for the race condition there. I just updated that last commit to use backticks to avoid escaping the double quotes.

ashcrow · 2018-09-17T17:38:56Z

@jlebon not a problem 👍

ashcrow · 2018-09-17T17:41:20Z

/lgtm

sdemos · 2018-09-17T18:21:32Z

looks like we need a person with approver on the repo at large to /lgtm as well for the changes to the manifest.

ashcrow · 2018-09-17T18:25:19Z

Opened #73 as I think it may make sense to let us modify MCD manifest files.

jlebon · 2018-09-17T21:03:11Z

Oh one more thing. Once this merges, the MCD in a fresh cluster will error out because the osImageURL currently embedded in machine configs is set to ://dummy. So we're essentially at the point where the installer actually needs to use https://github.com/ashcrow/pivot.

I opened openshift/installer#267 to raise visibility on this.

ashcrow · 2018-09-18T15:48:20Z

/lgtm

jlebon · 2018-09-18T15:52:10Z

Hmm, I think it might be blocking on the fact that we're adding a new pkg/utils?

ashcrow · 2018-09-18T15:56:42Z

/lgtm

abhinavdahiya · 2018-09-18T16:16:20Z

seems like daemon is the only one calling the pkg/utils and i personally don't like packages that are utils. I would recommend you created one under daemon and name it something like Executor or Runner etc..

Teach the MCD to check the current OS version and upgrade the host. This essentially implements the 'OSImageURL` part of the `MachineConfig` spec. Since we're in a chroot anyway, I just took the easy path of exec'ing `rpm-ostree` and `pivot` directly. Which is good, because right now only `pivot` knows how to upgrade RHCOS from an oscontainer image pull spec.

One subtle thing that might be hard to appreciate in the previous patch is that we're running `pivot` from inside the container, which in turn runs `podman` to access the contents of the oscontainer. For podman to work right, we need `hostPID`. Also need `hostNetwork` for the `rpm-ostree` client to talk to the daemon over D-Bus. Another way to see this: make the MCD feel even more like it's running directly on the host. (Though ideally, if `rpm-ostree` learns to pull content from the oscontainer itself, this would get easier).

Until the installer learns to pivot ➰ and we have an accurate starting `OSImageURL` entry.

jlebon · 2018-09-18T17:04:10Z

OK, updated to just keep those helper functions under pkg/daemon. ⬆️

ashcrow · 2018-09-18T19:33:50Z

/lgtm

openshift-ci-robot · 2018-09-18T19:34:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, jlebon

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:

~~manifests/machineconfigdaemon/OWNERS~~ [ashcrow,jlebon]
~~pkg/daemon/OWNERS~~ [ashcrow,jlebon]
~~pkg/operator/assets/OWNERS~~ [ashcrow,jlebon]

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

update readme with details on config fields, behavior, troubleshooting

openshift-ci-robot requested review from abhinavdahiya and ashcrow September 13, 2018 20:58

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 13, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2018

sdemos reviewed Sep 13, 2018

View reviewed changes

ashcrow reviewed Sep 14, 2018

View reviewed changes

jlebon force-pushed the pr/rpm-ostree branch from bae10d0 to c59c08a Compare September 14, 2018 21:21

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2018

ashcrow approved these changes Sep 17, 2018

View reviewed changes

openshift-ci-robot assigned ashcrow Sep 17, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2018

jlebon force-pushed the pr/rpm-ostree branch from b35665b to 3eb24cd Compare September 17, 2018 17:35

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2018

sdemos approved these changes Sep 17, 2018

View reviewed changes

ashcrow mentioned this pull request Sep 17, 2018

Add MCD OWNERS to manifest files #73

Closed

jlebon mentioned this pull request Sep 17, 2018

Pivot installer to pivot ➰ into latest RHCOS openshift/installer#267

Closed

jlebon mentioned this pull request Sep 18, 2018

manifests/mcd: add OWNERS file #75

Merged

jlebon force-pushed the pr/rpm-ostree branch from 3eb24cd to bb96f5d Compare September 18, 2018 15:56

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

jlebon added 3 commits September 18, 2018 13:03

pkg/daemon: work around ://dummy OS image URL for now

a5dc94b

Until the installer learns to pivot ➰ and we have an accurate starting `OSImageURL` entry.

jlebon force-pushed the pr/rpm-ostree branch from bb96f5d to a5dc94b Compare September 18, 2018 17:03

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 18, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

openshift-merge-robot merged commit a91ab52 into openshift:master Sep 18, 2018

osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021

Merge pull request openshift#60 from gabemontero/upd-readme

3269e28

update readme with details on config fields, behavior, troubleshooting

jlebon deleted the pr/rpm-ostree branch May 1, 2023 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/daemon: implement OS version validation #60

pkg/daemon: implement OS version validation #60

jlebon commented Sep 13, 2018 •

edited

Loading

jlebon commented Sep 13, 2018

sdemos Sep 13, 2018

jlebon Sep 13, 2018

sdemos Sep 13, 2018

jlebon Sep 13, 2018

sdemos Sep 13, 2018

sdemos Sep 13, 2018

abhinavdahiya commented Sep 13, 2018

ashcrow Sep 14, 2018

ashcrow Sep 14, 2018

jlebon commented Sep 14, 2018

jlebon commented Sep 14, 2018

jlebon commented Sep 14, 2018

jlebon commented Sep 14, 2018

abhinavdahiya commented Sep 14, 2018 •

edited

Loading

abhinavdahiya commented Sep 14, 2018

ashcrow left a comment

jlebon commented Sep 17, 2018

jlebon commented Sep 17, 2018

ashcrow commented Sep 17, 2018

jlebon commented Sep 17, 2018

ashcrow commented Sep 17, 2018

ashcrow commented Sep 17, 2018

sdemos commented Sep 17, 2018

ashcrow commented Sep 17, 2018

jlebon commented Sep 17, 2018

ashcrow commented Sep 18, 2018

jlebon commented Sep 18, 2018

ashcrow commented Sep 18, 2018

abhinavdahiya commented Sep 18, 2018

jlebon commented Sep 18, 2018

ashcrow commented Sep 18, 2018

openshift-ci-robot commented Sep 18, 2018

pkg/daemon: implement OS version validation #60

pkg/daemon: implement OS version validation #60

Conversation

jlebon commented Sep 13, 2018 • edited Loading

jlebon commented Sep 13, 2018

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

abhinavdahiya commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Sep 14, 2018

jlebon commented Sep 14, 2018

jlebon commented Sep 14, 2018

jlebon commented Sep 14, 2018

abhinavdahiya commented Sep 14, 2018 • edited Loading

abhinavdahiya commented Sep 14, 2018

ashcrow left a comment

Choose a reason for hiding this comment

jlebon commented Sep 17, 2018

jlebon commented Sep 17, 2018

ashcrow commented Sep 17, 2018

jlebon commented Sep 17, 2018

ashcrow commented Sep 17, 2018

ashcrow commented Sep 17, 2018

sdemos commented Sep 17, 2018

ashcrow commented Sep 17, 2018

jlebon commented Sep 17, 2018

ashcrow commented Sep 18, 2018

jlebon commented Sep 18, 2018

ashcrow commented Sep 18, 2018

abhinavdahiya commented Sep 18, 2018

jlebon commented Sep 18, 2018

ashcrow commented Sep 18, 2018

openshift-ci-robot commented Sep 18, 2018

jlebon commented Sep 13, 2018 •

edited

Loading

abhinavdahiya commented Sep 14, 2018 •

edited

Loading