MCO-707: Strip registry transport from os image URL for comparison #3857

ori-amizur · 2023-08-15T10:05:57Z

One of the conditions to reboot after firstboot is different os image
URLs. In order to correctly compare the URLs, the booted OS image URL
has to be stripped from container image reference and transport.
This commit utilizes a recent change in rpm-ostree client to do just
that.

- What I did
Strip registry transport from os image URLs before checking if reboot is needed.
- How to verify it

Regression
Verify if URLs are considered equal when booted os URL is with a registry transport, and the required is without.
- Description for the changelog

Strip registry transport from OS Image URLs for comparison

openshift-ci-robot · 2023-08-15T10:06:00Z

@ori-amizur: This pull request references MCO-707 which is a valid jira issue.

In response to this:

When comparing os image URLs in or to decide if reboot is needed, sometimes the URL starts with a scheme (i.e docker://), and sometimes without. In order to compare correctly, the scheme is removed before comparison is done.

- What I did
Canonized os image URLs before checking if reboot is needed.
- How to verify it

Regression

Verify if URLs are considered equal when booted os URL is with scheme, and the required is without.
- Description for the changelog

Canonize OS Image URLs for comparison

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.

ori-amizur · 2023-08-15T10:06:26Z

/assign @sinnykumari

ori-amizur · 2023-08-15T10:07:12Z

/uncc @cdoern

ori-amizur · 2023-08-16T15:15:32Z

/retest

ori-amizur · 2023-08-16T15:16:11Z

/test e2e-hypershift

ori-amizur · 2023-08-21T12:57:59Z

/test e2e-hypershift

cgwalters · 2023-08-21T13:09:23Z

pkg/daemon/daemon.go

@@ -2075,6 +2075,10 @@ func (dn *Daemon) validateOnDiskState(currentConfig *mcfgv1.MachineConfig) error
 	return nil
 }

+func canonizeImageURL(url string) string {


This could use a doc comment (and also "canonicalize" is more canonical here) - canonization is a different thing and I don't think this function quite qualifies for sainthood yet 😄 . But even "canonicalize" would make someone wonder "canonicalize how?" I think.

Further this is going to be a bit fragile because in theory we could (and in fact have a TODO to) change it. So how about combining these two:

Suggested change

func canonizeImageURL(url string) string {

// stripRegistryTransport removes the registry transport prefix, see https://docs.rs/ostree-ext/latest/ostree_ext/container/enum.Transport.html

func stripRegistryTransport(url string) string {

return strings.Relpace(strings.Replace(url, "docker://", "", 1), "registry:", "", 1))

or so?

Could also use a unit test.

Do we know that it will be always "docker://" or we should handle rest of the transport replacement as well that are supported by OSTree ?

I think in OpenShift today we effectively hardcode docker:// - i.e. we only support fetching from a registry in osImageURL.

So...yes. Note openshift/os#657 would actually switch things so that the initial state is from an oci-archive, but that's OK because it will today always be older anyways.

(A whole other problem here is that today we deploy as OCI, but when we later push the container image it gets converted to d2s2, which changes the digest...)

yeah, makes sense.

sinnykumari

One question, other than that I agree with changes already suggested by Colin.

sinnykumari · 2023-08-23T20:29:36Z

pkg/daemon/update_test.go

+	assert.Nil(t, err)
+	assert.False(t, diff.isEmpty())
+	assert.True(t, diff.osUpdate)
+


Can we also add a test case where oldConfig has empty OSImageURL and newConfig has a valid OSImageURL. something like oldConfig.Spec.OSImageURL = " " and newConfig.Spec.OSImageURL = ""quay.io/example/foo@sha256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c" .

This usually is the case today during initial node bootstrap for a cluster.

Doesn't this already exist on line 222?

ah yes, should have looked at existing test cases.

sinnykumari · 2023-08-23T20:37:30Z

Also, will be good to reword commit message based on latest changes that we have.

openshift-ci-robot · 2023-08-28T09:28:54Z

@ori-amizur: This pull request references MCO-707 which is a valid jira issue.

In response to this:

When comparing os image URLs in or to decide if reboot is needed,
sometimes the URL starts with a prefix (i.e docker:// or registry:), and sometimes
without. In order to compare correctly, the prefix is removed before
comparison is done.

- What I did
Strip registry transport os image URLs before checking if reboot is needed.
- How to verify it

Regression

Verify if URLs are considered equal when booted os URL is with scheme, and the required is without.
- Description for the changelog

Canonize OS Image URLs for comparison

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.

openshift-ci-robot · 2023-08-28T09:30:10Z

@ori-amizur: This pull request references MCO-707 which is a valid jira issue.

In response to this:

When comparing os image URLs in or to decide if reboot is needed,
sometimes the URL starts with a prefix (i.e docker:// or registry:), and sometimes
without. In order to compare correctly, the prefix is removed before
comparison is done.

- What I did
Strip registry transport from os image URLs before checking if reboot is needed.
- How to verify it

Regression

Verify if URLs are considered equal when booted os URL is with a registry transport, and the required is without.
- Description for the changelog

Strip registry transport from OS Image URLs for comparison

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.

sinnykumari · 2023-08-28T20:06:48Z

Thanks Ori for working on this.
/lgtm
This should get merged once 4.14 branching has happened and we no longer require valid-bz label.

sergiordlr · 2023-09-07T14:19:52Z

Verifying using IPI on GCP

Create a custom osImage (we used the internal registry)
image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/tc-54056-new-os-image@sha256:5ccada2081859a95414cd3f6a96a5f871e9ef8b5463ad28e4e8128bfe503faa3
Deploy a mc to use this custom osImage

kind: MachineConfig
apiVersion: machineconfiguration.openshift.io/v1
metadata:
  labels:
    machineconfiguration.openshift.io/role: "worker"
  name: "test-new-os-image"
spec:
  osImageURL: "image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/tc-54056-new-os-image@sha256:5ccada2081859a95414cd3f6a96a5f871e9ef8b5463ad28e4e8128bfe503faa3"

The new osImage is applied and the nodes rebooted.

Edit the test-new-os-image MC create in step 2 to prepend "docker://" to the osIMage. Like this:

spec:
  osImageURL: "docker://image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/tc-54056-new-os-image@sha256:5ccada2081859a95414cd3f6a96a5f871e9ef8b5463ad28e4e8128bfe503faa3"

A new MC is rendered and, when it is applied to the nodes, it is considered to be already applied and the nodes are not rebooted nor drained, as expected.

Nevertheless, we need to be aware that "docker://..." is not the right format for osImageURL. So we are accepting a wrong format in osImageURL if the image is the same as the one already present in the cluster. Imho, It's not a big deal, but we need to be aware of it. A customer could wonder why "docker://" is accepted in the MCs' osImageURL only sometimes.

Regarding the agent-install, we can't verify this PR using agent-install until openshift/os#657 is merged. Is it right?

/cc @rioliu-rh

rioliu-rh · 2023-09-07T23:58:27Z

/hold

rioliu-rh · 2023-09-08T00:04:37Z

hold this PR according to Sergio's comment, another issue about url format validation, if we create a mc to update os image with a new one w/o transport prefix docker://, the pool will be degraded with message like docker://... is not the right format

Sergio can provide more details about it.

cgwalters · 2023-09-08T17:43:54Z

Nevertheless, we need to be aware that "docker://..." is not the right format for osImageURL. So we are accepting a wrong format in osImageURL if the image is the same as the one already present in the cluster. Imho, It's not a big deal, but we need to be aware of it. A customer could wonder why "docker://" is accepted in the MCs' osImageURL only sometimes.

Hmm yes. I think we should actually continue to reject docker:// being present in osImageURL. I don't see what we gain by allowing it there.

So this PR started with:

sometimes the URL starts with a prefix (i.e docker:// or registry:), and sometimes without

Let's try to identify more precisely the code flows involved here.

I'm actually a bit confused here because I thought we were parsing the output from rpm-ostree status --json, which has e.g.:
"container-image-reference" : "ostree-unverified-registry:quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:cd29255bd4e6634404ef8e0f4a7f19b4ae5a57a3afc7d67cbe00934ba998d6ac"

And then we sort of hackily scrape that off...

Ohh, wait hmm...I think I may understand now! Yes, the bug is in that code in the MCO that's incorrectly parsing ostree-container image references. When we use ostree container image deploy, it ends up as e.g.:

$ rpm-ostree status --json | jq -r '.deployments[0]."container-image-reference"'
ostree-unverified-image:docker://quay.io/fedora/fedora-coreos:stable

So the core bug is here

machine-config-operator/pkg/daemon/rpm-ostree.go

Line 155 in 5b821a2

tokens := strings.SplitN(bootedDeployment.ContainerImageReference, ":", 2)

Now an immediate practical problem is that the code for parsing these strings today lives in Rust, not Go. (And even for using Rust, we don't really want to vendor all of ostree-rs-ext, which is its own distinct bug).

Here's what I'd propose:

Add a minimal Go reimplementation of the logic in https://github.com/ostreedev/ostree-rs-ext/blob/8b04520d8e0d3ffb0c6ba72098d4114083ee7124/lib/src/container/mod.rs#L183 into https://github.com/coreos/rpmostree-client-go/
Bump that dependency and use it here instead of just stripping from the first :

cgwalters · 2023-09-08T17:47:33Z

I can also look at "canonicalizing" ostree-unverified-image:docker:// back to ostree-unverified-registry: on the ostree side, which would also implicitly fix things.

cgwalters · 2023-09-09T15:58:22Z

ostreedev/ostree-rs-ext#533

This is needed in the MCO, see openshift/machine-config-operator#3857

cgwalters · 2023-09-13T21:22:33Z

PR in coreos/rpmostree-client-go#21

cgwalters · 2023-09-19T20:22:48Z

So next we need #3916
and then we can update this PR to execute on the bottom half of #3857 (comment)

sinnykumari · 2023-09-21T08:50:49Z

#3916 has been merged, we should be good to utilize bootedDeployment.RequireContainerImage() to get bootedOSImageURL without worrying about transport prefix.

ori-amizur · 2023-09-22T15:15:52Z

/retest

openshift-ci-robot · 2023-09-22T15:37:07Z

@ori-amizur: This pull request references MCO-707 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

One of the conditions to reboot after firstboot is different os image
URLs. In order to correctly compare the URLs, the booted OS image URL
has to be stripped from container image reference and transport.
This commit utilizes a recent change in rpm-ostree client to do just
that.

- What I did
Strip registry transport from os image URLs before checking if reboot is needed.
- How to verify it

Regression

Verify if URLs are considered equal when booted os URL is with a registry transport, and the required is without.
- Description for the changelog

Strip registry transport from OS Image URLs for comparison

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.

…erence for booted OS image URL One of the conditions to reboot after firstboot is different os image URLs. In order to correctly compare the URLs, the booted OS image URL has to be stripped from container image reference and transport. This commit utilizes a recent change in rpm-ostree client to do just that.

sinnykumari · 2023-09-25T08:05:13Z

LGTM
/approve
@sergiordlr This should be ready for qe testing now
@cgwalters PTAL

cgwalters

Thanks for this!

ori-amizur · 2023-09-26T08:15:56Z

/unhold

sergiordlr · 2023-09-26T08:16:58Z

Hello! @sinnykumari this PR depends on openshift/os#657

We need that PR to be merged before we can execute the verification here.

We have verified that the new change does not have the problem reported above, and it fails properly when we use osImage values with "docker://".

The rest of the verification (agent-installer skipping the first boot) will be executed once the PR linked above is merged.

sinnykumari · 2023-09-26T09:02:18Z

Thanks Sergio for testing it. Agree it is difficult to test complete functionality without openshift/os#657 . With current testing, it should be sufficient for the PR to get merged as it doesn't break any existing functionalities.
/lgtm

openshift-ci · 2023-09-26T09:02:58Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-ci-robot · 2023-09-26T11:52:47Z

/retest-required

Remaining retests: 0 against base HEAD 30c28f3 and 2 for PR HEAD b19dc30 in total

openshift-ci · 2023-09-26T14:43:32Z

@ori-amizur: 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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 15, 2023

openshift-ci bot requested review from cdoern and cgwalters August 15, 2023 10:06

openshift-ci bot assigned sinnykumari Aug 15, 2023

openshift-ci bot removed the request for review from cdoern August 15, 2023 10:07

cgwalters reviewed Aug 21, 2023

View reviewed changes

sinnykumari reviewed Aug 22, 2023

View reviewed changes

ori-amizur force-pushed the MCO-707 branch from 014a636 to 8d9466d Compare August 23, 2023 09:29

sinnykumari reviewed Aug 23, 2023

View reviewed changes

ori-amizur force-pushed the MCO-707 branch from 8d9466d to d2b7638 Compare August 28, 2023 09:27

ori-amizur changed the title ~~MCO-707: Canonize os image URL for comparison~~ MCO-707: Strip registry transport from os image URL for comparison Aug 28, 2023

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 28, 2023

cgwalters mentioned this pull request Aug 28, 2023

rpm-ostree status shows error fetching image metadata after ostree container image deploy ostreedev/ostree#3002

Closed

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

cgwalters mentioned this pull request Sep 11, 2023

COS-2611: Use deploy-via-container openshift/os#657

Merged

cgwalters added a commit to cgwalters/rpmostree-client-go-fork that referenced this pull request Sep 13, 2023

Add imgref module that parses ostree-container image references

f0aecc4

This is needed in the MCO, see openshift/machine-config-operator#3857

cgwalters mentioned this pull request Sep 13, 2023

Add imgref module that parses ostree-container image references coreos/rpmostree-client-go#21

Merged

cgwalters mentioned this pull request Sep 13, 2023

mod: Update rpmostree-client-go #3916

Merged

ori-amizur force-pushed the MCO-707 branch from d2b7638 to 6d45db5 Compare September 19, 2023 21:09

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

ori-amizur force-pushed the MCO-707 branch from 6d45db5 to 238165c Compare September 19, 2023 21:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

ori-amizur force-pushed the MCO-707 branch from 238165c to 8dd6ed6 Compare September 20, 2023 08:08

ori-amizur force-pushed the MCO-707 branch from 8dd6ed6 to 45371d8 Compare September 22, 2023 13:52

ori-amizur force-pushed the MCO-707 branch from 45371d8 to 78343d0 Compare September 22, 2023 15:35

ori-amizur force-pushed the MCO-707 branch from 78343d0 to b19dc30 Compare September 22, 2023 15:37

cgwalters approved these changes Sep 25, 2023

View reviewed changes

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

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2023

openshift-merge-robot merged commit 119b164 into openshift:master Sep 26, 2023
13 checks passed

-func canonizeImageURL(url string) string {
+// stripRegistryTransport removes the registry transport prefix, see https://docs.rs/ostree-ext/latest/ostree_ext/container/enum.Transport.html
+func stripRegistryTransport(url string) string {
+        return strings.Relpace(strings.Replace(url, "docker://", "", 1), "registry:", "", 1))

MCO-707: Strip registry transport from os image URL for comparison #3857

MCO-707: Strip registry transport from os image URL for comparison #3857

Conversation

ori-amizur commented Aug 15, 2023 • edited

openshift-ci-robot commented Aug 15, 2023 • edited by openshift-ci bot

ori-amizur commented Aug 15, 2023

ori-amizur commented Aug 15, 2023

ori-amizur commented Aug 16, 2023

ori-amizur commented Aug 16, 2023

ori-amizur commented Aug 21, 2023

cgwalters Aug 21, 2023 • edited

Choose a reason for hiding this comment

sinnykumari Aug 22, 2023

Choose a reason for hiding this comment

cgwalters Aug 22, 2023

Choose a reason for hiding this comment

sinnykumari Aug 23, 2023

Choose a reason for hiding this comment

sinnykumari left a comment

Choose a reason for hiding this comment

sinnykumari Aug 23, 2023

Choose a reason for hiding this comment

ori-amizur Aug 28, 2023

Choose a reason for hiding this comment

sinnykumari Aug 28, 2023

Choose a reason for hiding this comment

sinnykumari commented Aug 23, 2023

openshift-ci-robot commented Aug 28, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Aug 28, 2023 • edited by openshift-ci bot

sinnykumari commented Aug 28, 2023

sergiordlr commented Sep 7, 2023 • edited

rioliu-rh commented Sep 7, 2023

rioliu-rh commented Sep 8, 2023 • edited

cgwalters commented Sep 8, 2023 • edited

cgwalters commented Sep 8, 2023

cgwalters commented Sep 9, 2023

cgwalters commented Sep 13, 2023

cgwalters commented Sep 19, 2023

sinnykumari commented Sep 21, 2023

ori-amizur commented Sep 22, 2023

openshift-ci-robot commented Sep 22, 2023 • edited by openshift-ci bot

sinnykumari commented Sep 25, 2023

cgwalters left a comment

Choose a reason for hiding this comment

ori-amizur commented Sep 26, 2023

sergiordlr commented Sep 26, 2023 • edited

sinnykumari commented Sep 26, 2023

openshift-ci bot commented Sep 26, 2023

openshift-ci-robot commented Sep 26, 2023

openshift-ci bot commented Sep 26, 2023

ori-amizur commented Aug 15, 2023 •

edited

openshift-ci-robot commented Aug 15, 2023 •

edited by openshift-ci bot

cgwalters Aug 21, 2023 •

edited

openshift-ci-robot commented Aug 28, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Aug 28, 2023 •

edited by openshift-ci bot

sergiordlr commented Sep 7, 2023 •

edited

rioliu-rh commented Sep 8, 2023 •

edited

cgwalters commented Sep 8, 2023 •

edited

openshift-ci-robot commented Sep 22, 2023 •

edited by openshift-ci bot

sergiordlr commented Sep 26, 2023 •

edited