-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MCO-497: Prep node for RHCOS 9 upgrade #3553
MCO-497: Prep node for RHCOS 9 upgrade #3553
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi 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:
Approvers can indicate their approval by writing |
235b52e
to
e7b4613
Compare
@cheesesashimi: This pull request references MCO-497 which is a valid jira issue. In response to this:
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. |
65d6d64
to
166663d
Compare
PR to add hotfixes up at coreos/coreos-assembler#3352 and openshift/os#1162 Demonstration container up at
|
7a53c11
to
ac92a76
Compare
9dda49f
to
6697fc4
Compare
6697fc4
to
5d97232
Compare
} else if err := dn.NodeUpdaterClient.RebaseLayered(newURL); err != nil { | ||
} | ||
|
||
if err := dn.prepForRHCOS9(newURL); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain this is correct for the applyLayeredOSChanges()
path. However, I'm not as certain for the applyLegacyOSChanges()
path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think applyLegacyOSChanges()
may be fully dead code now? cc @jkyros
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that path is dead, I'd like to remove it in a follow-up PR. No sense in keeping it around if it's not being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applyLegacyOSChanges()
can only be reached if someone feeds us an "old format" image. I'm assuming the only way to get to 4.13 is through a 4.12 version (like, we don't support someone forcing an upgrade from 4.11 to 4.13), so yes, if that assumption is true, I think we can safely take applyLegacyOSChanges()
out for 4.13, which is awesome, because that was an unpleasant bit of code. 😄
} | ||
|
||
// Only FCOS and SCOS set this field. | ||
if osName, osNameOK := imageLabels["io.openshift.build.version-display-names"]; osNameOK { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd that only FCOS and SCOS set this field and that RHCOS simply doesn't. It is possible that I examined the wrong container labels when I was setting up the tests for this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 😢 This is because of the machine-os-content
zombie issue...some links for this in openshift/os#1048
Basically oc adm release new
barfs and dies if there are two container images in the release which claim to be an operating system. OKD already set things up so that machine-os-content
is an alias, so they don't have this problem.
func TestOSReleaseFromImageLabels(t *testing.T) { | ||
t.Parallel() | ||
|
||
// $ skopeo inspect --no-tags "docker://$(oc adm release info --pullspecs "quay.io/openshift-release-dev/ocp-release:4.12.3-x86_64" -o json | jq -r '.references.spec.tags[] | select(.name == "rhel-coreos-8") | .from.name')" | jq '.Labels' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included the commands I used to obtain the image labels for a sanity check. If I was looking at the wrong source image, please let me know.
func TestOSDetection(t *testing.T) { | ||
cs := framework.NewClientSet("") | ||
nodes, err := cs.CoreV1Interface.Nodes().List(context.TODO(), metav1.ListOptions{}) | ||
require.NoError(t, err) | ||
|
||
osImageRelease, osImageReleaseSrc := getOSReleaseInfoForOSImage(t, cs, nodes.Items[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an additional check to this e2e test that gets the OS image labels given the current release on the test cluster. This way, we can get the same early warning if something changes unexpectedly.
|
||
// Upgrades rpm-ostree. | ||
// TODO(zzlotnik): Is this the right command to upgrade rpm-ostree? | ||
if _, err := runGetOut("rpm", "-Uvh", "rpm-ostree"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right way to upgrade rpm-ostree with the extensions container repo set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tricky bit is we'll likely need to run this code in the host mount namespace. So something like:
systemd-run -qP /bin/sh -c 'rpm-ostree usroverlay; rpm -Uvh /path/to/extensions/hotfixes/8/*.rpm && systemctl restart rpm-ostreed'
or so.
The actual path to the mounted hotfixes will be a hotfixes
subdirectory of the extensions path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Yeah, that may take a bit to get right or I'll have to drop it in a temp file first.
/test unit |
@cheesesashimi: all tests passed! 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. |
Zack and I had a realtime chat about this. Summary from my PoV:
or so? |
pkg/daemon/osrelease/osrelease.go
Outdated
|
||
version := "" | ||
versionOK := false | ||
if version, versionOK = imageLabels["version"]; !versionOK { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also try to handle the standard OCI label for this. See ostreedev/ostree-rs-ext#454
Basically the standard key is org.opencontainers.image.version
so let's look for that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that key in any of the labels I used to set up the tests. Did I not get the right labels?
|
||
// Upgrades rpm-ostree. | ||
// TODO(zzlotnik): Is this the right command to upgrade rpm-ostree? | ||
if _, err := runGetOut("rpm", "-Uvh", "rpm-ostree"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tricky bit is we'll likely need to run this code in the host mount namespace. So something like:
systemd-run -qP /bin/sh -c 'rpm-ostree usroverlay; rpm -Uvh /path/to/extensions/hotfixes/8/*.rpm && systemctl restart rpm-ostreed'
or so.
The actual path to the mounted hotfixes will be a hotfixes
subdirectory of the extensions path.
} else if err := dn.NodeUpdaterClient.RebaseLayered(newURL); err != nil { | ||
} | ||
|
||
if err := dn.prepForRHCOS9(newURL); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think applyLegacyOSChanges()
may be fully dead code now? cc @jkyros
- What I did
- How to verify it
- Description for the changelog