CORS-3652: RHCOS Stream Marketplace Images#9329
CORS-3652: RHCOS Stream Marketplace Images#9329openshift-merge-bot[bot] merged 8 commits intoopenshift:mainfrom
Conversation
|
/cc @bryan-cox @bennerv |
patrickdillon
force-pushed
the
az-mkt-imgs
branch
from
28ac065 to
6944b1b
Compare
openshift-merge-robot
added
the
needs-rebase
patrickdillon
force-pushed
the
az-mkt-imgs
branch
from
6944b1b to
9618db4
Compare
openshift-merge-robot
removed
the
needs-rebase
patrickdillon
force-pushed
the
az-mkt-imgs
branch
2 times, most recently
from
5db7f58 to
8727a60
Compare
patrickdillon
force-pushed
the
az-mkt-imgs
branch
from
8727a60 to
79291bb
Compare
| }, | ||
| "ocp-emea": { | ||
| "hyperVGen1": { | ||
| "publisher": "redhat-limited", |
There was a problem hiding this comment.
Generally looks good from MCO POV; just a quick question here:
Is the publisher redhat-limited the only difference between the regions for paid marketplace images? Is there something else in the providerSpec the MCO could use to determine regions, or would this be the most deterministic way to do this?
There was a problem hiding this comment.
Exactly this. AFAIK the only difference is the publisher. Does that work for MCO?
There was a problem hiding this comment.
ack, we can make that work; as long as this hasn't changed historically - we might be updating fairly old machinesets! 😅
patrickdillon
force-pushed
the
az-mkt-imgs
branch
from
79291bb to
a41c0d0
Compare
Adds a standalone go program that generates data/data/coreos/marketplace-rhcos.json This file is combined with rhcos.json to create the coreos stream, but the files are updated at different cadences by different teams.
Updates the generation of the coreos stream manifest, which runs during container building, to merge the contents of the marketplace-rhcos.json file with rhcos.json when creating the coreos stream configmap.
patrickdillon
force-pushed
the
az-mkt-imgs
branch
from
a41c0d0 to
0391ea7
Compare
|
oops forgot to fix linting, still need to do that |
patrickdillon
force-pushed
the
az-mkt-imgs
branch
2 times, most recently
from
1e5eb81 to
35f81b5
Compare
|
I have tested this on version 4.16+ and this should be ready for review |
|
/test ? |
|
@patrickdillon: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn 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-sigs/prow repository. |
|
/test e2e-gcp-ovn e2e-nutanix-ovn e2e-openstack-ovn e2e-metal-ipi-ovn |
go mod tidy && go mod vendor
patrickdillon
force-pushed
the
az-mkt-imgs
branch
from
35f81b5 to
0fb323e
Compare
| return "coreos/fcos.json" | ||
| } | ||
|
|
||
| func getMarketplaceStreamFileName() string { |
There was a problem hiding this comment.
Is this name not causing issues? There are 3 defined in the same package. They may not be exported but they are all in the same package.
There was a problem hiding this comment.
oh maybe this is ok because of the build tags at the top of the files
There was a problem hiding this comment.
correct, the problem I fixed was that this was only defined for fcos and also needed to be defined for scos
|
wtf build failed due to virtctl build failure /retest-required |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
LGTM (go-code wise) 😅 I have some small questions.
| // TODO(padillon): dumb question time, git is still complaining \ No newline at end of file | ||
| // what am I doing wrong? | ||
| contents = append(contents, []byte("\n")...) |
There was a problem hiding this comment.
Yes! I thought i removed that. 🤦
| func checkIfNewer(candidate, release string) bool { | ||
| img, err := strconv.Atoi(strings.Split(semver.MajorMinor(candidate), ".")[1]) | ||
| if err != nil { | ||
| logrus.Infof("Error converting minor version to int with version %s", candidate) | ||
| return true | ||
| } | ||
| rel, err := strconv.Atoi(strings.Split(release, ".")[1]) | ||
| if err != nil { | ||
| logrus.Infof("Error converting minor version to int with version %s", release) | ||
| return true | ||
| } | ||
| return img > rel | ||
| } |
There was a problem hiding this comment.
Can we use semver.Compare here too without having to split out the minor version? That should be able to compare 4.Y0 and 4.Y1?
There was a problem hiding this comment.
💭
return semver.Compare(candidate, release) > 0There was a problem hiding this comment.
For the release image override i have it just required x.y, e.g. 4.19. When you convert that to a semver it becomes 4.19.0 so you end up excluding all 4.19 images, which was not the intent…
however i think this would work if we changed the semantics so that when users specify the version override they are specifying the minimum version they want to exclude. For example if you are populating 4.18, you set the env var to 4.19–not 4.18. Does that make sense? WDYT?
There was a problem hiding this comment.
Ah opps! Now, I understand the intention of the function (i.e. the name caught be off guard), thanks! The current way is good to me :D
Maybe, lets name it checkIfNewerYStream/checkIfNewerMinorVersion or similar to note that it only cares about minor version segment?
|
/retitle CORS-3652: RHCOS Stream Marketplace Images There is an extra space in the title... I think the questions i have are pretty much cosmetics so no concerns at all :D |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-3652 which is a valid jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/override e2e-azure-ovn these are just random e2e failures at this point |
|
@patrickdillon: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn 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-sigs/prow repository. |
|
/override ci/prow/e2e-azure-ovn |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-azure-ovn DetailsIn 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-sigs/prow repository. |
openshift-merge-bot
bot
merged commit d0aabcc
into
openshift:main
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-baremetal-installer |
|
[ART PR BUILD NOTIFIER] Distgit: ose-installer |
|
[ART PR BUILD NOTIFIER] Distgit: ose-installer-artifacts |
| "publisher": "azureopenshift", | ||
| "offer": "aro4", | ||
| "sku": "aro_419", | ||
| "version": "419.6.20250523" |
There was a problem hiding this comment.
Hmm, what's up with the version string here? This should just be the same 9.6.xxx version string inherited from the non-marketplace image.
There was a problem hiding this comment.
Note the versioning of RHCOS changed in 4.19 and up to reflect that fact that it's the same bootimages used across multiple OCP releases. See also https://github.com/openshift/enhancements/blob/master/enhancements/rhcos/split-rhcos-into-layers.md#etcos-release.
There was a problem hiding this comment.
Hmm, what's up with the version string here? This should just be the same 9.6.xxx version string inherited from the non-marketplace image.
That is the version ARO chose when publishing the image:
% az vm image list --publisher azureopenshift --offer aro4 --sku aro_419 --all -o table
Architecture Offer Publisher Sku Urn Version
-------------- ------- -------------- ------- ------------------------------------------ --------------
x64 aro4 azureopenshift aro_419 azureopenshift:aro4:aro_419:419.6.20250523 419.6.20250523So we should work with them (and Software Production, who publishes the paid images) to cohere the way images are published.
@bennerv any suggestions on how we could get this on ARO's radar? As Jonathan said, the version should just be the release from rhcos string, truncated as necessary.
There was a problem hiding this comment.
This is not a huge issue for the MCO in the short term, but could be a confusing to users when we perform boot image updates(machineset says RHCOS-like values, but node was born on RHEL-like values). Matching version formats would definitely come in handy for when the MCO implements skew enforcement in the not so distant future, too. Just my 2 cents 😄
There was a problem hiding this comment.
I'll talk to the other ARO TLs. I don't see a problem matching it 4.20+. It changed in 4.18 and i'm guessing we didn't notice it or change it in 4.19.
There was a problem hiding this comment.
Note that we can't change a version once it's out, only publish new ones. Also we won't be able to deprecate images until MCO implements skew enforcement and we can confirm no one is running older marketplace images.
There was a problem hiding this comment.
Note that we can't change a version once it's out
+1. Just wanted to standardize/align for future images.
9 participants
This is an implementation of openshift/enhancements#1655
./hack/rhcos/populate-marketplace-imagestream.gowhich generatesdata/data/coreos/marketplace-rhcos.jsonmarketplace-rhcos.jsonwithrhcos.jsonwhen:openshift-install coreos print-stream-json./hack/build-coreos-manifest.go(as part of the Dockerfile build) to generate the RHCOS stream configmapopenshift-install create clusteropenshift/enhancements#1655 is still a WIP and needs to be updated to reflect this work, but enough consensus has been reached that the work itself can help guide the enhancement.
How it works:
Users can run this standalone command (using authentication libs of the openshift-installer) to generate/update the marketplace stream:
By default, the command will look for (Azure unpaid) images in the current branch (discovered from the local
rhcos.json). I added an override to force searching for a specific release. In this case, the override value is hacky with zero-padding because I could not determine the RHCOS build which was used to publish the ARO images (they truncate the timestamp).The results of this command produce the
marketplace-rhcos.jsonfile, checked in here.When using the print-stream-json command, the marketplace-rhcos.json results are merged with rhcos.json:
Similarly, the configmap generated from ./hack/build-coreos-manifest.go contains the merged json.
Finally, the poc ipi azure install demonstrates the installation time gains we achieve when not needing to upload an image as part of the install process.