update kube-ovn to use v1.12.28 #11728

bobz965 · 2024-11-19T07:26:06Z

What type of PR is this?

/kind feature

update kube-ovn 1.12 to latest
expose more process args
fix yaml lint
separate k8s CRD, role, role binding, service account into different yamls

What this PR does / why we need it:
[kube-ovn] update to v1.12.28

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[kube-ovn] update to v1.12.28

linux-foundation-easycla · 2024-11-19T07:26:13Z

The committers listed above are authorized under a signed CLA.

✅ login: bobz965 / name: bobz965 (8e816c4, d8e06e4)

k8s-ci-robot · 2024-11-19T07:26:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bobz965
Once this PR has been reviewed and has the lgtm label, please assign ant31 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-11-19T07:26:15Z

Welcome @bobz965!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-19T07:26:16Z

Hi @bobz965. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

yankay · 2024-11-19T08:01:27Z

/ok-to-test

VannTen · 2024-11-19T08:27:28Z

/uncc

yankay · 2024-11-26T01:58:43Z

HI @bobz965
The commits need to be Squashed .

bobz965 · 2024-11-26T02:42:35Z

HI @bobz965 The commits need to be Squashed .

done

yankay · 2024-12-02T02:26:53Z

HI @bobz965 The commits need to be Squashed .

done

HI @bobz965

The PR changes lots of work than upgrading,
So why not separate it into multi-PRs?
It's better to fill in the PR description and release note.

roles/network_plugin/kube-ovn/defaults/main.yml

roles/reset/tasks/main.yml

bobz965 · 2024-12-02T02:52:16Z

HI @bobz965 The commits need to be Squashed .

done

HI @bobz965

The PR changes lots of work than upgrading, So why not separate it into multi-PRs? It's better to fill in the PR description and release note.

I will keep this PR containing kube-ovn only

VannTen · 2024-12-13T10:49:23Z

roles/network_plugin/kube-ovn/defaults/main.yml

-kube_ovn_ic_autoroute: true
-kube_ovn_ic_dbhost: "127.0.0.1"
+kube_ovn_ic_auto_route: true
+kube_ovn_ic_db_host: "127.0.0.1"


Changing variable names is a breaking change => we should probably leave those alone

j2 will render the value into a file. It will not break anything. it is a key value in Ansible, but, nothing changes in the k8s yaml. right?

That's not it.

Files in defaults can be overridden by users in their inventory (that's their point). So if we change the variable name and use the new one, someone who had defined a value different from the default would silently reset to the default on upgrading.

It's ok to have breaking changes when there is a sufficiently ™️ benefit (or we just merged and it was not in a released version). But changing spacing is below the bar of usefulness for making a breaking change, IMO.

ok,will roll back

VannTen · 2024-12-13T10:52:21Z

roles/network_plugin/kube-ovn/templates/cni-kube-ovn.yml.j2

The deleted stuff here is just moving, right ? Best leave that for a separate PR, here it just make the PR bigger to review and obscures the actual changes

Right, But I think using a small separate PR should be easier to maintain in the future.

using a small separate PR

You meant a small separate file here I guess ?
Yes, why not, but there is no reason to do that split in this PR, it inflates it's size and make the diff unnecessary big

I get your point, but in the upstream kube-ovn, these files have already been split. I do not know when to split these files here.

Hum, yeah, keeping in sync with upstream.
Ok let's keep it in the PR, but could you put the file split in a separate commit (preferably before the rest if possible).

This will make our history more readable for future readers.

ok, will be in a a separate commit

VannTen · 2024-12-13T10:53:12Z

roles/network_plugin/kube-ovn/templates/cni-kube-ovn-crd.yml.j2

(Side note, not mandatory for this PR):
This isn't actually a template, so it should probably move to files/ instead.

VannTen · 2024-12-13T10:54:10Z

roles/network_plugin/kube-ovn/templates/cni-kube-ovn.yml.j2

+          - --default-provider-name={{ kube_ovn_default_provider_name | default('')}}
+          - --default-interface-name={{ kube_ovn_default_vlan_interface_name | default('') }}
+          - --default-vlan-id={{ kube_ovn_default_vlan_id | default('') }}
+          - --default-vlan-name={{ kube_ovn_default_vlan_name | default('') }}


Avoid the use of default where possible, just have the value defined in roles defaults/main.yml

VannTen · 2024-12-13T10:55:23Z

roles/network_plugin/kube-ovn/templates/cni-kube-ovn.yml.j2

@@ -580,7 +434,7 @@ spec:
        type: infra
    spec:
      priorityClassName: system-node-critical
-      serviceAccountName: ovn
+      serviceAccountName: kube-ovn-app
      hostPID: true


Is this safe to upgrade this way on running cluster ? If the sa changes, I suppose that will leave a dangling SA, right ?

Not tested the upgrade before, it should work. I test the deployment in the new env. actually it changed in the upsteam kube-ovn.

ovn SA is still in use, so it is not dangling SA.

Ah ok so it was using one before and now it's two ? Understood.

Ah ok so it was using one before and now it's two ? Understood.

yes

Signed-off-by: bobz965 <[email protected]>

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 19, 2024

k8s-ci-robot requested review from mzaian and VannTen November 19, 2024 07:26

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 19, 2024

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 19, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2024

bobz965 marked this pull request as draft November 19, 2024 08:08

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2024

k8s-ci-robot removed the request for review from VannTen November 19, 2024 08:27

bobz965 marked this pull request as ready for review November 19, 2024 10:24

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2024

k8s-ci-robot requested a review from MrFreezeex November 19, 2024 10:24

bobz965 force-pushed the master branch from 313ed48 to 7271624 Compare November 26, 2024 02:41

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 2, 2024

yankay changed the title ~~update kube-ovn to use 1.12~~ update kube-ovn to use v1.12.28 Dec 2, 2024

yankay reviewed Dec 2, 2024

View reviewed changes

roles/network_plugin/kube-ovn/defaults/main.yml Outdated Show resolved Hide resolved

roles/reset/tasks/main.yml Outdated Show resolved Hide resolved

bobz965 force-pushed the master branch 2 times, most recently from c363ed5 to ede011d Compare December 2, 2024 02:46

VannTen requested changes Dec 13, 2024

View reviewed changes

update kube-ovn to use 1.12

d8e06e4

Signed-off-by: bobz965 <[email protected]>

bobz965 force-pushed the master branch from ede011d to d8e06e4 Compare December 18, 2024 06:29

split kube-ovn manifests

8e816c4

Signed-off-by: bobz965 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update kube-ovn to use v1.12.28 #11728

update kube-ovn to use v1.12.28 #11728

bobz965 commented Nov 19, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 19, 2024 •

edited

Loading

k8s-ci-robot commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

yankay commented Nov 19, 2024

VannTen commented Nov 19, 2024 via email

yankay commented Nov 26, 2024

bobz965 commented Nov 26, 2024

yankay commented Dec 2, 2024

bobz965 commented Dec 2, 2024

VannTen Dec 13, 2024

bobz965 Dec 17, 2024 •

edited

Loading

VannTen Dec 17, 2024

bobz965 Dec 18, 2024

VannTen Dec 13, 2024

bobz965 Dec 16, 2024

VannTen Dec 16, 2024

bobz965 Dec 17, 2024 •

edited

Loading

VannTen Dec 17, 2024

bobz965 Dec 18, 2024

VannTen Dec 13, 2024

VannTen Dec 13, 2024

VannTen Dec 13, 2024

bobz965 Dec 17, 2024

VannTen Dec 17, 2024

bobz965 Dec 18, 2024

update kube-ovn to use v1.12.28 #11728

Are you sure you want to change the base?

update kube-ovn to use v1.12.28 #11728

Conversation

bobz965 commented Nov 19, 2024 • edited Loading

linux-foundation-easycla bot commented Nov 19, 2024 • edited Loading

k8s-ci-robot commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

yankay commented Nov 19, 2024

VannTen commented Nov 19, 2024 via email

yankay commented Nov 26, 2024

bobz965 commented Nov 26, 2024

yankay commented Dec 2, 2024

bobz965 commented Dec 2, 2024

Choose a reason for hiding this comment

bobz965 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobz965 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobz965 commented Nov 19, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 19, 2024 •

edited

Loading

bobz965 Dec 17, 2024 •

edited

Loading

bobz965 Dec 17, 2024 •

edited

Loading