Fix: install calico-kube-controller on kdd #9358

wayfrro · 2022-10-04T07:48:15Z

What type of PR is this?

This partially reverts 2de5c48#diff-c94700ab11e60a92e9706fb87b3a61796ff8b2a7a36e18fd465e173203d6574dL14 - which makes kubespray not installing policy controller when using calico with kubernetes data store.

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #9300

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[Calico] Install calico-kube-controller also when using kdd datastore

k8s-ci-robot · 2022-10-04T07:48:24Z

Hi @wayfrro. 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/test-infra repository.

wayfrro · 2022-10-04T08:34:47Z

FYI @cristicalin

oomichi

/cc @oomichi

roles/kubernetes-apps/policy_controller/meta/main.yml

oomichi · 2022-10-05T02:26:44Z

roles/kubernetes-apps/policy_controller/meta/main.yml

    tags:
      - policy-controller

  - role: policy_controller/calico
    when:
      - kube_network_plugin == 'canal'
-      - calico_datastore != "kdd"
+      - calico_datastore != "kdd" or calico_policy_version is version('v3.6.0', '>=')


Shouldn't we just leave one dependency on this role for both calico and canal with enable_network_policy set to true? It makes little sense to me otherwise. Something like:

--- dependencies: - role: policy_controller/calico when: - kube_network_plugin in ['calico', 'canal'] - enable_network_policy tags: - policy-controller

@wayfrro haha exactly what I wrote yesterday too #9358 (comment)

Indeed it's cleaner that way but the original intent of the PR you are referencing was a clean up which accidentally triggered this bug.

@wayfrro please update the PR with the code you proposed above which better communicates the intention of having the policy controller dependency on both calico and canal.

@cristicalin Done.

floryut · 2022-10-05T11:58:25Z

Not sure I fully understand why we don't check enable_network_policy for canal but well 🤷

oomichi · 2022-10-05T21:23:14Z

Not sure I fully understand why we don't check enable_network_policy for canal but well 🤷

Yeah, that seems strange.
But it would be good enough to investigate/solve it later to concentrate on fixing the corresponding issue with this pr.

/ok-to-test
/lgtm

yankay · 2022-10-08T09:55:44Z

Great Fix， it's very useful.
Thank you @wayfrro.

cyclinder · 2022-10-08T11:14:53Z

Thanks @wayfrro

/lgtm

floryut

@wayfrro Thanks for the update 👍

k8s-ci-robot · 2022-10-10T08:26:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, wayfrro

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:

~~OWNERS~~ [floryut]

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

oomichi · 2022-10-11T01:59:30Z

/lgtm

* Fix: install policy controller on kdd too * Remove the calico_policy_version condition altogether * Install policy controller both on canal and calico under same condition

* Fix: install policy controller on kdd too * Remove the calico_policy_version condition altogether * Install policy controller both on canal and calico under same condition Co-authored-by: Piotr Kowalczyk <7711184+wayfrro@users.noreply.github.com>

* Fix: install policy controller on kdd too * Remove the calico_policy_version condition altogether * Install policy controller both on canal and calico under same condition

Fix: install policy controller on kdd too

0a7b4d1

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 4, 2022

k8s-ci-robot requested review from holmsten and jayonlau October 4, 2022 07:48

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 4, 2022

wayfrro mentioned this pull request Oct 4, 2022

kubespray don't install calico-kube-controller #9300

Closed

oomichi reviewed Oct 5, 2022

View reviewed changes

k8s-ci-robot requested a review from oomichi October 5, 2022 02:26

Remove the calico_policy_version condition altogether

589619e

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 Oct 5, 2022

k8s-ci-robot assigned oomichi Oct 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2022

k8s-ci-robot assigned cyclinder Oct 8, 2022

Install policy controller both on canal and calico under same condition

e10992f

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 10, 2022

floryut approved these changes Oct 10, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2022

k8s-ci-robot merged commit 3b99d24 into kubernetes-sigs:master Oct 11, 2022

azuwis mentioned this pull request Nov 7, 2022

[2.20] Fix: install calico-kube-controller on kdd (#9358) #9470

Merged

floryut mentioned this pull request Jan 4, 2023

Release Proposal v2.21 #9638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: install calico-kube-controller on kdd #9358

Fix: install calico-kube-controller on kdd #9358

wayfrro commented Oct 4, 2022 •

edited by floryut

Loading

k8s-ci-robot commented Oct 4, 2022

wayfrro commented Oct 4, 2022

oomichi left a comment

oomichi Oct 5, 2022

wayfrro Oct 5, 2022

floryut Oct 6, 2022

cristicalin Oct 8, 2022

wayfrro Oct 10, 2022

floryut commented Oct 5, 2022

oomichi commented Oct 5, 2022 •

edited by floryut

Loading

yankay commented Oct 8, 2022

cyclinder commented Oct 8, 2022

floryut left a comment

k8s-ci-robot commented Oct 10, 2022

oomichi commented Oct 11, 2022

Fix: install calico-kube-controller on kdd #9358

Fix: install calico-kube-controller on kdd #9358

Conversation

wayfrro commented Oct 4, 2022 • edited by floryut Loading

k8s-ci-robot commented Oct 4, 2022

wayfrro commented Oct 4, 2022

oomichi left a comment

Choose a reason for hiding this comment

oomichi Oct 5, 2022

Choose a reason for hiding this comment

wayfrro Oct 5, 2022

Choose a reason for hiding this comment

floryut Oct 6, 2022

Choose a reason for hiding this comment

cristicalin Oct 8, 2022

Choose a reason for hiding this comment

wayfrro Oct 10, 2022

Choose a reason for hiding this comment

floryut commented Oct 5, 2022

oomichi commented Oct 5, 2022 • edited by floryut Loading

yankay commented Oct 8, 2022

cyclinder commented Oct 8, 2022

floryut left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 10, 2022

oomichi commented Oct 11, 2022

wayfrro commented Oct 4, 2022 •

edited by floryut

Loading

oomichi commented Oct 5, 2022 •

edited by floryut

Loading