-
Notifications
You must be signed in to change notification settings - Fork 462
daemon: Accept osImageURL with matching digests #463
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
daemon: Accept osImageURL with matching digests #463
Conversation
|
Not tested locally though beyond new unit tests; looks like the UBI work broke |
|
/approve |
jlebon
left a comment
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.
Some minor comments, otherwise LGTM!
|
/approve |
b8ac65b to
a02917b
Compare
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Just for general interest, here's a workflow I have for testing PRs like this locally. As part of submitting a PR, a release image will be generated. You can log into the CI namespace (see In those logs you'll see a message like: Now, because the CI namespaces only last ~1h, if you want to do testing you'll need to mirror it, e.g.:
Now, you can pass it to the installer to create a fresh cluster with it:
Or you can use it as a target for |
|
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Hmm, that job failed with: Hmm. In non-ancient systemd looks like that error has a more useful message which would have told us what was conflicting. I feel like this is another case where we should have retried instead of going degraded maybe? But clearly need to figure out why this happened. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Not 100% certain but I think what happened here is since we don't have this pivot commit we ended up in a race where the system was shutting down for reboot, but the MCD had come up and tried to start pivot again. /retest |
|
/retest |
|
/retest |
|
(Any reason not to drop the |
|
/lgtm @jlebon ptal |
|
/retest |
Yup, this is still
Yeah, that sounds plausible to me. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jlebon, runcom 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 |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
aws limit hit /retest |
|
I'll retest in a bit, we keep hitting aws rate limits errors |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
There is a reason we do this - we will almost certainly migrate our content to another registry location in the future. It’s good to be smart, but don’t assume that if a new url comes in that it will always be at that location in the future. |
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
This is a lowering of openshift/machine-config-operator#463 to pivot. We need it for the case of doing an early pivot before the MCD comes up.
The way CI works, a release payload is generated in the CI namespace
but reusing the digested pull of existing components. This means
the pull specs for everything change format - but the actual
content stays the same.
Let's handle this for the OS image at least.
In practice...I feel like we're going to want higher level special
handling for this - maybe the CVO does pull-through to the registry
and rewrites to an internal canonical form, but this should help
us for now.
Closes: #462