Skip to content

NE-1951: Pre-upgrade Admin Gate for Gateway API CRD Management Succession #1196

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 24, 2025
Merged

NE-1951: Pre-upgrade Admin Gate for Gateway API CRD Management Succession #1196

merged 1 commit into from
Mar 24, 2025

Conversation

@Thealisyed
Copy link
Contributor

@Thealisyed Thealisyed commented Feb 26, 2025 •
edited
Loading

This PR introduces the following:

Initialize the GatewayAPI upgradeable controller

  • Added the required imports and constants.
  • Defined predicates for CRD and ConfigMap events to manage the admin gate.
  • Configured watches in the controller for specific changes in the ConfigMap.

Implement the reconciler logic

  • Added the Reconcile function to manage the admin gate addition and removal based on conditions

Enhance the operator initialization

  • Conditionally initialize the gatewayapi_upgradeable controller based on the GatewayAPI feature gate.

End-to-End Tests

  • Added the TestGatewayAPIUpgradeable end-to-end test.
  • Included the new test case in the test suite.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 26, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Thealisyed
Thealisyed commented Feb 26, 2025
View reviewed changes
@Thealisyed Thealisyed changed the title Add GatewayAPI upgradeable controller with ConfigMap and CRD watches [WIP] NE-1951: Pre-upgrade Admin Gate for Gateway API CRD Management Succession Feb 26, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 26, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 26, 2025 •
edited by openshift-ci bot
Loading

@Thealisyed: This pull request references NE-1951 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.18.z" version, but no target version was set.

In response to this:

  • Initialize the GatewayAPI upgradeable controller with required imports and constants.
  • Set up predicates and watches for ConfigMap and CRD updates.
  • Implement the reconciler logic to manage the admin gate addition and removal.
  • Add placeholder methods for future logic implementation.
  • Ensure proper logging and error handling throughout the code.

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.

@Thealisyed
Copy link
Contributor Author

Thealisyed commented Feb 26, 2025

Used Grant's PR as reference to implement admin gate in this manner.

@Miciah
Copy link
Contributor

Miciah commented Feb 26, 2025

/assign
/assign @shaneutt

Thealisyed
Thealisyed commented Feb 27, 2025
View reviewed changes
Thealisyed
Thealisyed commented Feb 27, 2025
View reviewed changes
Thealisyed
Thealisyed commented Feb 27, 2025
View reviewed changes
alebedev87
alebedev87 reviewed Feb 27, 2025
View reviewed changes
Copy link
Contributor

@alebedev87 alebedev87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! Keep it up! Some comments to steer the direction of the controller's implementation.

Thealisyed
Thealisyed commented Mar 3, 2025
View reviewed changes
@Thealisyed Thealisyed force-pushed the gwapi-admingate-4.18 branch from 56318ed to 5edb547 Compare March 4, 2025 13:56
candita
candita reviewed Mar 5, 2025
View reviewed changes
candita
candita reviewed Mar 5, 2025
View reviewed changes
candita
candita reviewed Mar 5, 2025
View reviewed changes
candita
candita reviewed Mar 5, 2025
View reviewed changes
candita
candita reviewed Mar 5, 2025
View reviewed changes
Miciah
Miciah reviewed Mar 6, 2025
View reviewed changes
@Thealisyed Thealisyed force-pushed the gwapi-admingate-4.18 branch 3 times, most recently from 5ed9569 to f81b817 Compare March 7, 2025 20:11
@Thealisyed Thealisyed marked this pull request as ready for review March 7, 2025 20:15
@Thealisyed Thealisyed force-pushed the gwapi-admingate-4.18 branch from 844683f to bd93c36 Compare March 20, 2025 12:38
@alebedev87
Copy link
Contributor

alebedev87 commented Mar 20, 2025

@Thealisyed: Thanks for addressing all the comments!

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2025
@alebedev87
Copy link
Contributor

alebedev87 commented Mar 20, 2025

/retest

grzpiotrowski
grzpiotrowski reviewed Mar 20, 2025
View reviewed changes
@Miciah Miciah added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Mar 20, 2025
NE-1951: Pre-upgrade Admin Gate for Gateway API CRD Management Succes…
c22c21d 
…sion

This PR introduces a pre-upgrade admin gate for Gateway API CRD management
succession. It implements an operator which, upon the detection of any
Gateway API CRDs will apply an admin gate to inform the cluster admin.
There is also an e2e implemented and a conditional check for the
operator to skip if the featuregate is enabled.

This ensures to block upgrades until a cluster admin explicitly provides
consent for the platform to start taking over management of the
Gateway API CRDs
@Thealisyed Thealisyed force-pushed the gwapi-admingate-4.18 branch from bd93c36 to c22c21d Compare March 20, 2025 17:15
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2025
@alebedev87
Copy link
Contributor

alebedev87 commented Mar 20, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2025
@candita
Copy link
Contributor

candita commented Mar 20, 2025

@Thealisyed I see many comments resolved with no answer from you. Can you please take a quick look through and add responses that explain what you did to address the comment? Even just a "done", or thumbs-up would suffice. Our team generally does not resolve an un-answered comment.

@candita
Copy link
Contributor

candita commented Mar 20, 2025

/test e2e-aws-operator

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 20, 2025

/retest-required

Remaining retests: 0 against base HEAD b0222ed and 2 for PR HEAD c22c21d in total

@candita
Copy link
Contributor

candita commented Mar 20, 2025

== NAME TestAll/parallel/TestAWSEIPAllocationsForNLB
util_test.go:1068: service openshift-ingress/openshift-ingress UID has not changed yet, retrying...
lb_eip_test.go:149: failed to delete and recreate service: context deadline exceeded

/test e2e-aws-operator-techpreview

alebedev87
alebedev87 approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@alebedev87 alebedev87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing "change requested" (change).

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 21, 2025

/retest-required

Remaining retests: 0 against base HEAD b0222ed and 2 for PR HEAD c22c21d in total

@Thealisyed
Copy link
Contributor Author

Thealisyed commented Mar 21, 2025

/retest

Static nodes not created and other unrelated to changes

@alebedev87
Copy link
Contributor

alebedev87 commented Mar 21, 2025

Build of the CIO image took too much time:

Build cluster-ingress-operator-amd64 succeeded after 1h4m8s

Due to some problems with the internal image registry:

Pushing image image-registry.openshift-image-registry.svc:5000/ci-op-0s543lfi/pipeline:cluster-ingress-operator-amd64 ...
Warning: Push failed, retrying in 5s ...
Getting image source signatures
Warning: Push failed, retrying in 5s ...
Getting image source signatures
Warning: Push failed, retrying in 5s ...
Registry server Address: 
Registry server User Name: <token>
Registry server Email: 
Registry server Password: <<non-empty>>
error: build error: Failed to push image: trying to reuse blob sha256:25c75c34b2e2b68ba9245d9cddeb6b8a0887371ed30744064f85241a75704d87 at destination: unable to retrieve auth token: invalid username/password: authentication required

The CI job was forcibly stopped because the default 4h timeout was reached.

/retest-required

@Thealisyed
Copy link
Contributor Author

Thealisyed commented Mar 21, 2025 •
edited
Loading

@candita Thanks for the feedback and my apologies for missing that. I’ve been resolving the comments without adding responses to the reviewer, which is something Miciah and I had discussed earlier. I’ll go through the PR now and make sure to address the remaining comments.

@Thealisyed
Copy link
Contributor Author

Thealisyed commented Mar 21, 2025

/test e2e-aws-ovn-serial

Due to some problems with the internal image registry:

error: build error: Failed to push image: trying to reuse blob sha256:25c75c34b2e2b68ba9245d9cddeb6b8a0887371ed30744064f85241a75704d87 at destination: unable to retrieve auth token: invalid username/password: authentication required

{"component":"entrypoint","file":"sigs.k8s.io/prow/pkg/entrypoint/run.go:169","func":"sigs.k8s.io/prow/pkg/entrypoint.Options.ExecuteProcess","level":"error","msg":"Process did not finish before 4h0m0s timeout","severity":"error","time":"2025-03-21T04:34:06Z"}

The CI job was forcibly stopped because the default 4h timeout was reached.
Nothing to suggest issues with test so will retest now

@Thealisyed
Copy link
Contributor Author

Thealisyed commented Mar 21, 2025

/test e2e-aws-ovn-serial

auth error: failed to push image

error: build error: Failed to push image: trying to reuse blob sha256:25c75c34b2e2b68ba9245d9cddeb6b8a0887371ed30744064f85241a75704d87 at destination: unable to retrieve auth token: invalid username/password: authentication required

Failed at 4 hour timeout:

Process did not finish before 4h0m0s timeout

It has failed before in Job history for this reason so retesting

@Thealisyed
Copy link
Contributor Author

Thealisyed commented Mar 22, 2025

/test e2e-aws-ovn-serial

Same 4 hour timeout failure, no on-going or known issues with test.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 22, 2025 •
edited
Loading

@Thealisyed: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-single-node c22c21d link false /test e2e-aws-ovn-single-node
ci/prow/okd-scos-e2e-aws-ovn c22c21d link false /test okd-scos-e2e-aws-ovn

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-sigs/prow repository. I understand the commands that are listed here.

@alebedev87
Copy link
Contributor

alebedev87 commented Mar 24, 2025

/retest-required

@openshift-merge-bot openshift-merge-bot bot merged commit d967a73 into openshift:release-4.18 Mar 24, 2025
18 of 20 checks passed
@openshift-bot
Copy link
Contributor

openshift-bot commented Mar 24, 2025

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-ingress-operator
This PR has been included in build ose-cluster-ingress-operator-container-v4.18.0-202503241334.p0.gd967a73.assembly.stream.el9.
All builds following this will include this PR.

@Thealisyed Thealisyed deleted the gwapi-admingate-4.18 branch May 12, 2025 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Miciah Miciah Miciah left review comments

@alebedev87 alebedev87 alebedev87 approved these changes

@frobware frobware Awaiting requested review from frobware

+4 more reviewers

@wking wking wking left review comments

@candita candita candita left review comments

@shaneutt shaneutt shaneutt left review comments

@grzpiotrowski grzpiotrowski grzpiotrowski left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Miciah Miciah

@shaneutt shaneutt

@alebedev87 alebedev87

@lihongan lihongan

@melvinjoseph86 melvinjoseph86

@ShudiLi ShudiLi

@rhamini3 rhamini3

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.