-
Notifications
You must be signed in to change notification settings - Fork 1.5k
SPLAT-2584,OCPBUGS-69434: Added ability to use / configure different IPAM version when in TP for static IP #10158
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
Conversation
|
@vr4manta: This pull request references SPLAT-2584 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 task to target the "4.21.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.21-e2e-vsphere-static-ovn-techpreview |
|
@jcpowermac: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c0f1ab20-d695-11f0-9231-8a9bbd9a3dde-0 |
|
/payload-job periodic-ci-openshift-release-master-nightly-4.21-e2e-vsphere-static-ovn-techpreview |
|
@vr4manta: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/79244030-d697-11f0-9ff2-d00654ba5c8f-0 |
|
/retest |
|
@vr4manta: The following test 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. |
|
/payload-job-with-prs periodic-ci-openshift-release-master-nightly-4.21-e2e-vsphere-static-ovn-techpreview openshift/cluster-api#256 |
|
@damdo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/25d712e0-d9b7-11f0-915d-938ad3de1c37-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-master-nightly-4.21-e2e-vsphere-static-ovn-techpreview openshift/cluster-api#256 |
|
@damdo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/290b3bd0-d9b7-11f0-84df-7de543f5755b-0 |
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.
| logrus.Debug("updating capi ipam apiVersion from v1beta1 to v1beta2 when feature gate ClusterAPIMachineManagement is enabled") |
We tend to avoid fmt.Println where possible. Let's use standard logger instead?
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.
I am surprised golint is cool with fmt.Println 🤣 even though it was very strict in the past.
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.
Even though we set the apiVersion to ipam.cluster.x-k8s.io/v1beta2, the resulting struct is actually still ipam.cluster.x-k8s.io/v1beta1 👇
ipamv1 "sigs.k8s.io/cluster-api/api/ipam/v1beta1" //nolint:staticcheck //CORS-3563 ipclaim := &ipamv1.IPAddressClaim{
That means we are creating v1beta1 CR manifests (i.e. using v1beta1 spec/status fields), but with v1beta2 as apiVersion.
I guess v1beta2 should be backwards compatible and this is just a temporary fix, so we are fine. Ideally, we should have a conversion func to properly convert from v1beta1 struct to v1beta2 counterpart (e.g. copy the logic from webhook or the webhook should handle it :D).
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.
AFAIK, there are differences in struct presentations between the two versions (i.e. different field struct name, but same json/yaml field name). However, manifest contents are exactly the same. So, we are safe here 👍
|
/retitle SPLAT-2584,OCPBUGS-69434: Added ability to use / configure different IPAM version when in TP for static IP |
|
@vr4manta: This pull request references SPLAT-2584 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 task to target the "4.22.0" version, but no target version was set. This pull request references Jira Issue OCPBUGS-69434, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
| if config.EnabledFeatureGates().Enabled(features.FeatureGateClusterAPIMachineManagement) { | ||
| fmt.Println("Processing api update") | ||
| for index := range claim { | ||
| claim[index].APIVersion = "ipam.cluster.x-k8s.io/v1beta2" |
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.
Can we use the v1beta2 packages directly? They can be vendored as the installer is up to date with capi 1.11
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.
Or possibly continue to create the v1beta1, but call the conversion directly via scheme (i.e. no webhook involved) if we need v1beta2.
|
We'll need to backport to release-4.21 too cc. @patrickdillon @vr4manta @jcpowermac @rvanderp3 /cherry-pick release-4.21 |
|
@damdo: once the present PR merges, I will cherry-pick it on top of 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. |
|
/jira refresh |
|
@tthvo: This pull request references SPLAT-2584 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 task to target the "4.22.0" version, but no target version was set. This pull request references Jira Issue OCPBUGS-69434, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/test e2e-vsphere-static-ovn |
|
Superseded by #10169 |
|
/close In favour of #10169 |
|
@damdo: Closed this PR. 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. |
SPLAT-2584
Changes