fix(clusterpools): cluster-pool-ipv4-cidr/cluster-pool-ipv6-cidr

[cndoit18]

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Thanks for contributing!

`cluster-pool-ipv4-cidr` and `cluster-pool-ipv6-cidr` options now accept string slices and not just string

[gandro]

Thanks for the contribution.

I believe however changing this value breaks upgrades. If users use "--reuse-values" when performing "helm upgrade", Helm will likely fail trying to apply join to a string, no?

My proposal therefore would be to introduce a new Helm value (e.g. operator.clusterPoolIPv6PodCIDRList), modify the chart such that it prefers the new list-based one (if set), but fails back to the old string-based one if not. And then we can deprecate the old one eventually. At least we should mention this change it the update notes.

While less elegant, there is also an option where we don't change the Helm value at all. I believe if you only changed flags.String to flags.StringSlice that you can specify multiple CIDRs via Helm by just comma-separating them, e.g. "ipam.operator.clusterPoolIPv4PodCIDR=10.0.0.0/8,172.16.0.0/12" should work too.

[cndoit18]

clusterPoolIPv6PodCIDRList

Thanks for the contribution.

I believe however changing this value breaks upgrades. If users use "--reuse-values" when performing "helm upgrade", Helm will likely fail trying to apply join to a string, no?

My proposal therefore would be to introduce a new Helm value (e.g. operator.clusterPoolIPv6PodCIDRList), modify the chart such that it prefers the new list-based one (if set), but fails back to the old string-based one if not. And then we can deprecate the old one eventually. At least we should mention this change it the update notes.

While less elegant, there is also an option where we don't change the Helm value at all. I believe if you only changed flags.String to flags.StringSlice that you can specify multiple CIDRs via Helm by just comma-separating them, e.g. "ipam.operator.clusterPoolIPv4PodCIDR=10.0.0.0/8,172.16.0.0/12" should work too.

Thank you very much, I agree with you. I'll think about how to modify it

[cndoit18]

Thanks for the contribution.

I believe however changing this value breaks upgrades. If users use "--reuse-values" when performing "helm upgrade", Helm will likely fail trying to apply join to a string, no?

My proposal therefore would be to introduce a new Helm value (e.g. operator.clusterPoolIPv6PodCIDRList), modify the chart such that it prefers the new list-based one (if set), but fails back to the old string-based one if not. And then we can deprecate the old one eventually. At least we should mention this change it the update notes.

While less elegant, there is also an option where we don't change the Helm value at all. I believe if you only changed flags.String to flags.StringSlice that you can specify multiple CIDRs via Helm by just comma-separating them, e.g. "ipam.operator.clusterPoolIPv4PodCIDR=10.0.0.0/8,172.16.0.0/12" should work too.

done

[errordeveloper]

Please see my comments

[joestringer]

Thanks for the PR. At a glance this seems like it should be safe for upgrades from existing clusterpool configuration, although I have not tested it out.

There's a few references to the old name for this field in the docs, could you fix them up?

$ git grep clusterPoolIPv4
Documentation/gettingstarted/ipam-cluster-pool.rst:   * ``--set ipam.operator.clusterPoolIPv4PodCIDR=<IPv4CIDR>``
Documentation/gettingstarted/ipam-cluster-pool.rst:   * ``--set ipam.operator.clusterPoolIPv4MaskSize=<IPv4MaskSize>``
Documentation/gettingstarted/k8s-install-openshift-okd.rst:reflect those changes. Namely ``clusterNetwork`` should match ``nativeRoutingCIDR``, ``clusterPoolIPv4PodCIDR`` and ``clusterPoolIPv4MaskSize``.
...

Also, should this be added into the Documentation/operations/upgrade.rst upgrade guide to inform users that they should migrate their configurations to the new settings?

[joestringer]

I suspect that some of the failures in CI are because this PR has not been rebased against the latest master branch recently. Could you also rebasing against master while addressing the above feedback?

[cndoit18]

ABC was deprecated in favor of BBC

Thanks for the PR. At a glance this seems like it should be safe for upgrades from existing clusterpool configuration, although I have not tested it out.

There's a few references to the old name for this field in the docs, could you fix them up?

$ git grep clusterPoolIPv4
Documentation/gettingstarted/ipam-cluster-pool.rst:   * ``--set ipam.operator.clusterPoolIPv4PodCIDR=<IPv4CIDR>``
Documentation/gettingstarted/ipam-cluster-pool.rst:   * ``--set ipam.operator.clusterPoolIPv4MaskSize=<IPv4MaskSize>``
Documentation/gettingstarted/k8s-install-openshift-okd.rst:reflect those changes. Namely ``clusterNetwork`` should match ``nativeRoutingCIDR``, ``clusterPoolIPv4PodCIDR`` and ``clusterPoolIPv4MaskSize``.
...

Also, should this be added into the Documentation/operations/upgrade.rst upgrade guide to inform users that they should migrate their configurations to the new settings?

Thx, updated

[joestringer]

LGTM, thanks.

[cndoit18]

Hi, Is there anything else I need to do next?

[gandro]

Hi, Is there anything else I need to do next?

Let's give @errordeveloper some time to re-review. Other than that, this should be good to go.

I'll also re-run CI again to incorporate the last changes.

[gandro]

/test

Job 'Cilium-PR-K8s-1.21-kernel-4.19' hit: #17881 (98.48% similarity)

[cndoit18]

Hi, Is there anything else I need to do next?

Let's give @errordeveloper some time to re-review. Other than that, this should be good to go.

I'll also re-run CI again to incooperate the last changes.

thank you

[cndoit18]

Hi, do I need to do something to solve this CI can not succeed?

[cndoit18]

/test

[gandro]

Hi, do I need to do something to solve this CI can not succeed?

Pushing new commits unfortunately invalidates previous CI runs and their links, so I can't tell you what went wrong the last time besides one known-flaky test that got flagged (which is unrelated to your changes) here: #17780 (comment)

I'll run the tests again (this can only be done by team members with commit access unfortunately), please avoid pushing to the branch after the tests have run to keep the result validated. There is usually no need to rebase.

[gandro]

/test

[cndoit18]

Hi, do I need to do something to solve this CI can not succeed?

Pushing new commits unfortunately invalidates previous CI runs and their links, so I can't tell you what went wrong the last time besides one known-flaky test that got flagged (which is unrelated to your changes) here: #17780 (comment)

I'll run the tests again (this can only be done by team members with commit access unfortunately), please avoid pushing to the branch after the tests have run to keep the result validated. There is usually no need to rebase.

Thank you very much

[cndoit18]

/test

@gandro Sorry, it seems that the rebase is a past commit, I need to fix it.

[cndoit18]

/test

Can you retrigger it? Thank you

[gandro]

/test

Can you retrigger it? Thank you

Yes, will re-trigger it as soon as the Image CI Build stage has finished. Please note that it is usually fine if your branch is not up to date with the master branch, there is no need to rebase, unless there is a known bug somewhere.

[gandro]

/test

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Encapsulation Check connectivity with sockops and VXLAN encapsulation

Failure Output

FAIL: Kubernetes DNS did not become ready in time

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.