NE-2138: Bump cluster-dns-operator to Kubernetes 1.33 for 4.21 #448
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@davidesalerno: This pull request references NE-2138 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.21.0" version, but no target version was set. In 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. |
| pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ | ||
| type: string | ||
| required: | ||
| - lastTransitionTime |
There was a problem hiding this comment.
this is a breaking change. Can we confirm that our intention was always to have these as "required" and that this won't cause any issue?
There was a problem hiding this comment.
(or at least track where this change came from)
There was a problem hiding this comment.
This change come from the update into the OpenShift APIs version and a similar change was applied to the CIO in this commit.
If we would like to be compliant to that API version we cannot avoid this change.
I checked that at the moment we are setting everytime there is a change this field and the Status too.
There was a problem hiding this comment.
Can we confirm that our intention was always to have these as "required" and that this won't cause any issue?
(or at least track where this change came from)
The change was added in openshift/api#2037 then reverted by TRT openshift/api#2045 (for the reason of breaking some cluster operators, good catch!) and then re-added again in openshift/api#2048. Looking at the bug created by the TRT team seems like the apiserver and the authentication were impacted by the change. David Eads tried to make sure these gaps covered in his unrevert PR.
We did a similar change in CIO and it seemed to go unnoticed there (I don't see any comment about it in the PR).
OperatorCondition is used mostly by cluster operators to report Degraded, Progressing and Available statuses. Also, I know that we use this condition in some operands like IngressController whose status is also managed by a cluster operator (ingress).
Overall, I think it's unlikely the end user will face a compatibility issue for a cluster operator (or a cluster operand) statuses. But we can follow up with David Eads on 1) how we can make sure we don't break the compatibility (maybe Davide's check about setting newly required fields is enough), 2) how incompatible changes are managed in openshift/api module (taking into account that the module has no semver).
There was a problem hiding this comment.
@deads2k Could you help us in verifying that these changes won't cause any issue to the impacted systems?
There was a problem hiding this comment.
As discussed with David Eads, we (CDO as part of OCP payload) are consumers of the CRDs generated in openshift/api from the API types. The compatibility requirements are more relaxed for us (compared to clients). David suggested to keep in sync with latest openshift/api updates which includes code changes. I think that we can move on with this change in the Condition type taking into account the CI signal (status updates should go smooth without API server errors).
| type: string | ||
| status: | ||
| description: status of the condition, one of True, False, Unknown. | ||
| enum: |
There was a problem hiding this comment.
We need to verify if we are trying to set something other than this enum as this apparently was added recently on some API change
| github.com/openshift/client-go v0.0.0-20231024221206-506d798bc61c | ||
| github.com/go-logr/logr v1.4.2 | ||
| github.com/google/go-cmp v0.7.0 | ||
| github.com/openshift/api v0.0.0-20250710004639-926605d3338b |
There was a problem hiding this comment.
btw the breaking changes I am pointing below may be coming from this bump.
tbh I didn't tried to re-generate the manifests on openshift/cluster-ingress-operator#1279 and seeing the breaking changes below kind of concerns me we may be missing something.
There was a problem hiding this comment.
yes, these changes are coming from that bump after executing a make
There was a problem hiding this comment.
Changes are due to this update into OperatorCondition into the OpenShift API https://github.com/openshift/api/blob/master/operator/v1/types.go#L190
I think that if we would like to bump to that version the API we cannot avoid it even if it could be a breaking change.
|
/test okd-scos-e2e-aws-ovn |
|
/test e2e-aws-ovn-serial |
|
/approve From my perspective, this looks fine, but I will leave final comments to Andrey and Miciah regarding this "breaking change" and if it already happened on past, we should be fine :) /cc @alebedev87 @Miciah |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/assign |
There was a problem hiding this comment.
I commented on the point about the breaking change. I don't think we are going to impact end users. The cluster dns operator's e2e checks the operator conditions which means that the operator is setting the status condition during the test. This should give us a signal if any required field is not set or if any unexpected value is used.
/lgtm
/hold
@davidesalerno: LGTM, just keeping a small hold in case you would like to follow up with David Eads.
openshift-ci
bot
added
do-not-merge/hold
|
/test okd-scos-e2e-aws-ovn |
|
/retest |
|
/verified by @lihongan |
openshift-ci-robot
added
the
verified
|
@lihongan: This PR has been marked as verified by In 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. |
Signed-off-by: Davide Salerno <[email protected]>
Signed-off-by: Davide Salerno <[email protected]>
davidesalerno
force-pushed
the
NE-2138
branch
from
e71bd37 to
84d742b
Compare
openshift-ci-robot
removed
the
verified
|
/retest-required |
1 similar comment
|
/retest-required |
|
As discussed with David Eads, we should check the CI signal about the status updates. No status update errors should be seen in the CDO logs. @davidesalerno : I let you unhold the PR once the CDO logs are checked ^. |
|
/retest |
|
@davidesalerno: The following tests failed, say
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. |
This change bumps Kubernetes libraries to 1.33.4 and controller-runtime to 0.21.0.
Due to some breaking changes, additional modifications were required: