CORS-4209: ensure NodeIPFamilies field is kept as-is after transforming #426
Conversation
|
@tthvo: This pull request references CORS-4209 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. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
/jira refresh |
|
@tthvo: This pull request references CORS-4209 which is a valid jira issue. 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. |
|
/cc @sadasu |
|
@mtulio: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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. |
|
/assign @nrb |
|
Running the unit tests locally, I'm not able to get it to fail, so not sure what's going on with CI. I think this is all reasonable, just need to remove a stray comment. |
|
[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 |
|
/verified by unit test and testwith openshift/installer#9930 |
openshift-ci-robot
added
the
verified
|
@tthvo: This PR has been marked as verified by 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. |
Local unit test passed quite consistently for me too 👀 I guess the test is flaky? It passed now on CI: ci/prow/unit 😁 |
Yeah, I wasn't able to reproduce these failures, either. Looks like right now quay might be having issues. |
|
/retest-required Let's see if things are back to normal 👀 |
|
/test e2e-aws-ovn |
|
/retest |
|
/lgtm |
|
/lgtm |
| nodeIPKey := file.Section("Global").Key("NodeIPFamilies") | ||
| for i, ipFamily := range cfg.Global.NodeIPFamilies { | ||
| if i == 0 { | ||
| nodeIPKey.SetValue(ipFamily) | ||
| } else if err := nodeIPKey.AddShadow(ipFamily); err != nil { | ||
| return "", fmt.Errorf("failed to set NodeIPFamilies: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Checking the Epic https://issues.redhat.com/browse/OCPSTRAT-886, are we targeting hypershift (ROSA HCP) too?
PTAL https://github.com/openshift/enhancements/pull/1806/files#r2713056982
There was a problem hiding this comment.
Hey @mtulio, in the afternoon sync call today, PM said we are, but they will double-check. I think this PR should ensure the cloud-config in both ocp and rosa to be in correct format; and it does not break existing behaviour (ipv4).
Let's move forward with this PR?
| // NodeIPFamilies=ipv4,ipv6 | ||
| // | ||
| // Below logic ensures the original NodeIPFamilies field is kept as-is after transforming. | ||
| nodeIPKey := file.Section("Global").Key("NodeIPFamilies") |
There was a problem hiding this comment.
Does the transformation preserve the order in which NodeIPFamilies values are specified? For IPv6Primary and IPv4Primary will the order of values differ?
There was a problem hiding this comment.
It looks like the ini loading code respects the order of values in the config file.
The cloud provider type also mentions that the order matters here - whichever's primary should come first.
The loop starting on L80 looks like it's preserving order to me, being sure to set the first value at index 0 and shadowing subsequent ones.
There was a problem hiding this comment.
The cloud provider type also mentions that the order matters here - whichever's primary should come first.
That's true! The order in cloud-config will determine IP family order in the .status.addresses of nodes.
It looks like the ini loading code respects the order of values in the config file.
The loop starting on L80 looks like it's preserving order to me, being sure to set the first value at index 0 and shadowing subsequent ones.
Thanks for the reference! Yes, the order is preserved thanks to that handling logic.
There was a problem hiding this comment.
I had to adjust the changes here a bit though because of other recent changes in the CCCMO that introduces sorting section. But the idea remains the same. PTAL 🙏
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
In dual-stack environment, the CCM expects NodeIPFamilies to be in the format: NodeIPFamilies=ipv4 NodeIPFamilies=ipv6 However, iniv1 is serializing go slices as comma-separated list, for example: NodeIPFamilies=ipv4,ipv6 This commit ensures the original NodeIPFamilies field is kept as-is after transforming.
|
@tthvo: all tests passed! 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. |
7 participants
In dual-stack environment, the CCM AWS provider expects
NodeIPFamilies(kubernetes/cloud-provider-aws#638) to be in the format:However,
iniv1is serializing go slices as comma-separated list, for example:NodeIPFamilies=ipv4,ipv6This commit ensures the original NodeIPFamilies field is kept as-is after transforming.