Don't require registry during reboot and upgrade #1483

jhernand · 2023-09-21T15:16:34Z

This patch adds an enhancement that describes the need to be able to boot and upgrade clusters without a registry server when all the required images have already been pulled.

Related: https://issues.redhat.com/browse/OCPBUGS-13219
Related: #1481
Related: openshift/cluster-network-operator#1803

This patch adds an enhancement that describes the need to be able to boot and upgrade clusters without a registry server when all the required images have already been pulled. Related: https://issues.redhat.com/browse/OCPBUGS-13219 Related: openshift#1481 Related: openshift/cluster-network-operator#1803 Signed-off-by: Juan Hernandez <[email protected]>

bparees · 2023-09-22T20:23:27Z

enhancements/update/dont-require-registry-during-boot-and-upgrade.md

+approvers:
+- "@sdodson"
+- "@zaneb"
+- "@LalatenduMohanty"


as with the other EP, can we get this down to a single approver? the other two can be reviewers

Yes, who should be that approver?

well since you've got 3 candidates listed, i suggest you ask them who amongst them would like to take point and if none of them feel they are the right person hopefully they can make another suggestion.

@sdodson @zaneb @LalatenduMohanty do you want to be the approver for this?

I can not be approver for this enhancement as this does not need any changes in clusterversion object or cluster version operator.

Having read through the EP am I correct in summarizing this as a CI testing effort only? We put in place CI jobs that ensure that e2e tests pass without any remote registry access and we're done?

Do we expect a future coordination EP between CVO and PinnedImageSet implementation that would allow something like the PinnedImageSet to assert that a given release's pullspec has been mirrored and then a webhook or pre-apply step could be added to the CVO to go check the PinnedImageSet for it to assert that all nodes have mirrored the desired content?

Yes, this is mostly about testing in CI that we can reboot nodes and do upgrades without a registry server. Note that it is not about ensuring that existing end to end tests are able to run without a registry server, rather introducing new tests to verify these specific cases: reboot and upgrade. For example, we may have an end to end test that runs a workload with images that don't exist in the cluster yet. That will require a registry server to pull those images. We are not trying to address that.

Yes, once this and #1481 are accepted we will work on a new EP (based on the now closed #1432) that will introduce the orchestration of an upgrade without a registry server.

enhancements/update/dont-require-registry-during-boot-and-upgrade.md

This patch adds YAML comments explaining what is the reason to add specific reviewers. Signed-off-by: Juan Hernandez <[email protected]>

This patch removes the mention of the test to scan source code for the `Always` pull policy, as that would be hard to implement and unreliable for pods that are dynamically generated by operators. Signed-off-by: Juan Hernandez <[email protected]>

This patch adds a paragraph to the non-goals section to explicitly state that eliminating the requirement for a registry for other operations is a non-goal. Signed-off-by: Juan Hernandez <[email protected]>

This patch adds a paragraph to the open questions section about image streams. Do they block upgrades if there is no registry server available? Signed-off-by: Juan Hernandez <[email protected]>

oourfali · 2023-10-10T08:55:56Z

@bparees @jhernand any items left here?
@bparees can you please re-review?

bparees

just a couple nits, otherwise it lgtm.

enhancements/update/dont-require-registry-during-boot-and-upgrade.md

Signed-off-by: Juan Hernandez <[email protected]>

oourfali · 2023-10-11T06:10:32Z

@bparees Thanks! I see Juan addressed the comments.
Can we merge this one?

bparees · 2023-10-11T13:39:46Z

@bparees Thanks! I see Juan addressed the comments.
Can we merge this one?

that's up to approver(s) you listed in the EP

oourfali · 2023-10-11T13:45:55Z

@sdodson @mrunalp do you approve as well?

This patch changes the document to use `reboot` instead of `boot` to make it clear that this is about reboots of existing nodes, not about initial boots of new nodes. Signed-off-by: Juan Hernandez <[email protected]>

oourfali · 2023-10-18T12:41:29Z

@sdodson @mrunalp can you please review (and approve?).

mrunalp · 2023-10-20T02:03:53Z

enhancements/update/dont-require-registry-during-reboot-and-upgrade.md

+upgrades less fragile we should have a test that gates the OpenShift release and
+that verifies that reboots of nodes and upgrades can be performed without a
+registry server. One way to ensure this is to run in CI an admission hook that
+rejects/warns about any spec that uses the `Always` pull policy.


It may be worth thinking about adding something to the test that blocks access to the registry till the update is done.

Yeah that was the only way I could think of to test this. Blackhole registry.access.rdhat.com registry.redhat.io and quay.io after images are pinned to all nodes, rolling restart of all nodes or upgrade.

mrunalp · 2023-10-20T02:04:09Z

/lgtm

sdodson · 2023-10-20T19:39:39Z

/approve
I don't mind approving this, but my approval should not imply agreement to complete the implementation of these jobs by any specific team. Who builds them and who monitors them needs to be a specific follow up discussion.

openshift-ci · 2023-10-20T19:43:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson

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:

~~enhancements/update/OWNERS~~ [sdodson]

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

openshift-ci · 2023-10-20T20:01:40Z

@jhernand: 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 bot requested review from LalatenduMohanty and wking September 21, 2023 15:19

jhernand mentioned this pull request Sep 21, 2023

Upgrade without registry enhancement #1432

Closed

bparees mentioned this pull request Sep 22, 2023

Pin and pre-load images #1481

Merged

bparees reviewed Sep 22, 2023

View reviewed changes

jhernand added 3 commits September 25, 2023 16:24

Add reviewers comments

bc6bfd4

This patch adds YAML comments explaining what is the reason to add specific reviewers. Signed-off-by: Juan Hernandez <[email protected]>

Remove test to scan source code

6a6e6ec

This patch removes the mention of the test to scan source code for the `Always` pull policy, as that would be hard to implement and unreliable for pods that are dynamically generated by operators. Signed-off-by: Juan Hernandez <[email protected]>

Removing a registry for other operations is a non-goal

c77ae4a

This patch adds a paragraph to the non-goals section to explicitly state that eliminating the requirement for a registry for other operations is a non-goal. Signed-off-by: Juan Hernandez <[email protected]>

jhernand force-pushed the dont_require_registry_during_boot_and_upgrade branch from 3425309 to c77ae4a Compare September 26, 2023 07:58

Add open question about image streams

79211b8

This patch adds a paragraph to the open questions section about image streams. Do they block upgrades if there is no registry server available? Signed-off-by: Juan Hernandez <[email protected]>

bparees reviewed Oct 10, 2023

View reviewed changes

enhancements/update/dont-require-registry-during-boot-and-upgrade.md Outdated Show resolved Hide resolved

enhancements/update/dont-require-registry-during-boot-and-upgrade.md Outdated Show resolved Hide resolved

Fix some typos and wording

969d2b7

Signed-off-by: Juan Hernandez <[email protected]>

Use reboot instead of boot

49b0079

This patch changes the document to use `reboot` instead of `boot` to make it clear that this is about reboots of existing nodes, not about initial boots of new nodes. Signed-off-by: Juan Hernandez <[email protected]>

jhernand changed the title ~~Don't require registry during boot and upgrade~~ Don't require registry during reboot and upgrade Oct 13, 2023

mrunalp reviewed Oct 20, 2023

View reviewed changes

openshift-ci bot assigned mrunalp Oct 20, 2023

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2023

openshift-ci bot merged commit 955c042 into openshift:master Oct 20, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't require registry during reboot and upgrade #1483

Don't require registry during reboot and upgrade #1483

jhernand commented Sep 21, 2023

bparees Sep 22, 2023

jhernand Sep 25, 2023

bparees Sep 25, 2023

jhernand Sep 25, 2023 •

edited

Loading

LalatenduMohanty Sep 27, 2023

sdodson Oct 12, 2023

jhernand Oct 13, 2023

oourfali commented Oct 10, 2023

bparees left a comment

oourfali commented Oct 11, 2023

bparees commented Oct 11, 2023

oourfali commented Oct 11, 2023

oourfali commented Oct 18, 2023

mrunalp Oct 20, 2023

sdodson Oct 20, 2023

mrunalp commented Oct 20, 2023

sdodson commented Oct 20, 2023 •

edited

Loading

openshift-ci bot commented Oct 20, 2023

openshift-ci bot commented Oct 20, 2023

Don't require registry during reboot and upgrade #1483

Don't require registry during reboot and upgrade #1483

Conversation

jhernand commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhernand Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oourfali commented Oct 10, 2023

bparees left a comment

Choose a reason for hiding this comment

oourfali commented Oct 11, 2023

bparees commented Oct 11, 2023

oourfali commented Oct 11, 2023

oourfali commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Oct 20, 2023

sdodson commented Oct 20, 2023 • edited Loading

openshift-ci bot commented Oct 20, 2023

openshift-ci bot commented Oct 20, 2023

jhernand Sep 25, 2023 •

edited

Loading

sdodson commented Oct 20, 2023 •

edited

Loading