[mcbs] Validate rpm-ostree version is new enough #2859

cgwalters · 2021-12-03T16:56:13Z

This is mainly to validate that
openshift/release#24225
worked.

But, this code may be useful as a sanity check going forward.

(I also may try to expose e.g. ex-container as a feature flag
that we can query instead of version-parsing)

cgwalters · 2021-12-03T17:07:44Z

pkg/daemon/rpm-ostree.go

@@ -100,6 +115,43 @@ func (r *RpmOstreeClient) loadStatus() (*rpmOstreeState, error) {
 	return &rosState, nil
 }

+func parseVer(s string) ([]int, error) {


Rust has totally spoiled me. I find it soooo hard to write code like this. In Rust it's just that simple and elegant: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3709150890266cfee1f09a35c1f7391f

colin we're not rewriting openshift in rust. at least... not yet. ;)

kikisdeliveryservice

just a question =)

kikisdeliveryservice · 2021-12-03T18:43:30Z

pkg/daemon/rpm-ostree.go

@@ -20,8 +22,21 @@ const (
 	numRetriesNetCommands = 5
 	// Pull secret.  Written by the machine-config-operator
 	kubeletAuthFile = "/var/lib/kubelet/config.json"
+
+	// rpmOstreeVersionMinimum is the minimum required version
+	rpmOstreeVersionMinimum = "2021.14"


nice idea for sanity check (aside from immediate mcbs use) 1 q from that pov: would hardcoding directly in file might make it go stale over time? how often would we expect this to change?

I think we wouldn't change this too often. In practice, we'll just know out of band when we're using a new feature - it'll have landed in rhel 8.z or fast tracked into OCP.

I honestly don't have a really strong opinion on actually merging this patch. We can drop it too when we look at merging the mcbs branch.

But for now it'll be a sanity check.

makes sense to me!

cgwalters · 2021-12-03T20:20:45Z

Capacity issues
/test e2e-aws

cgwalters · 2021-12-03T20:22:16Z

Hmm. The gcp-op failure looks much like node join failure. We really need coreos/ignition#585

Short term, I'll try moving this to be daemonset only, so we can more easily see failures.

cgwalters · 2021-12-03T20:30:49Z

Oh right, in fact duh - we can't do this check during node join, because my test payload didn't override the boot images.

cgwalters · 2021-12-03T21:06:18Z

Oh right, in fact duh - we can't do this check during node join, because my test payload didn't override the boot images.

But actually, in order for this to be really useful I'm now realizing we really will want it on the boot images. But doing that escalates the level of difficulty, since it's harder to upload and manage e.g. GCP and AWS images than it is to just ship a container image.

Hmm. Or...we accept a short term hack where we either double reboot, or we pull the machine-os-content, then live-apply to get the new rpm-ostree, then rebase again from there.

This is *mainly* to validate that openshift/release#24225 worked. But, this code may be useful as a sanity check going forward. See also coreos/rpm-ostree#3251 (I also may try to expose e.g. `ex-container` as a feature flag that we can query instead of version-parsing)

cgwalters · 2021-12-06T23:20:34Z

OK rebased 🏄 now that #2860 merged

sinnykumari · 2021-12-08T15:14:26Z

/lgtm

sinnykumari · 2021-12-08T17:42:34Z

Don't know what's wrong with the tide. retrying lgtm
/lgtm

openshift-ci · 2021-12-08T17:45:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, 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~~ [sinnykumari]

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

sinnykumari · 2021-12-08T17:58:00Z

/retest

openshift-bot · 2021-12-08T18:56:29Z

/retest-required