fix multus include #10105

darkobas2 · 2023-05-18T17:03:59Z

"msg": "Failed to template loop_control.label: 'ansible.utils.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'item'. 'ansible.utils.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'item'", "skip_reason": "Conditional result was False"}

case when multus should NOT be included.

/kind bug

What this PR does / why we need it:

if multus is not enabled, unable to deploy/upgrade.

Which issue(s) this PR fixes:

Special notes for your reviewer:

someone with multus should test this.

Does this PR introduce a user-facing change?:

NONE

linux-foundation-easycla · 2023-05-18T17:04:03Z

The committers listed above are authorized under a signed CLA.

✅ login: darkobas2 (e41b7f3)

k8s-ci-robot · 2023-05-18T17:04:07Z

Welcome @darkobas2!

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 · 2023-05-18T17:04:08Z

Hi @darkobas2. 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.

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

roles/network_plugin/meta/main.yml

roles/kubernetes-apps/network_plugin/multus/tasks/main.yml

oomichi · 2023-05-19T08:07:43Z

/ok-to-test

MrFreezeex · 2023-05-19T11:32:03Z

If the | default('None') is not necessary anymore I would remove it otherwise (or if it still is necessary) lgtm.

darkobas2 · 2023-05-19T12:17:50Z

If the | default('None') is not necessary anymore I would remove it otherwise (or if it still is necessary) lgtm.

well on my setup I did not need it.. but when i uploaded the first patch i noticed the CI job was failing without it.

MrFreezeex · 2023-05-19T13:57:44Z

If the | default('None') is not necessary anymore I would remove it otherwise (or if it still is necessary) lgtm.

well on my setup I did not need it.. but when i uploaded the first patch i noticed the CI job was failing without it.

Could you try again without this line changed? Anyway the CI is a bit flaky these days when there are too much tasks running so you would have to push force to relaunch the CI anyway :x 🙈...

darkobas2 · 2023-05-19T14:07:08Z

i understand your concern... in case multus is needed and these wars are needed playbook should fail... i can look into how to maybe implement this logic where the multus_manifest_x vars are defined... meanwhile the CI should be running

MrFreezeex · 2023-05-19T14:25:54Z

Sorry could you rebase your PR and push force to relaunch the pipeline again 🙏 😬

MrFreezeex · 2023-05-19T17:09:04Z

roles/kubernetes-apps/network_plugin/multus/tasks/main.yml

@@ -9,9 +9,12 @@
    state: "latest"
  delegate_to: "{{ groups['kube_control_plane'][0] }}"
  run_once: true
-  with_items: "{{ multus_manifest_1.results }} + {{ multus_nodes_list|map('extract', hostvars, 'multus_manifest_2')|list|json_query('[].results') }}"
+  with_items: "{{ multus_manifest_1.results + multus_nodes_list|map('extract', hostvars, 'multus_manifest_2.results')|list }}"


So you were indeed right the CI fails with this change :(.

I think something like that should work though:

Suggested change

with_items: "{{ multus_manifest_1.results + multus_nodes_list|map('extract', hostvars, 'multus_manifest_2.results')|list }}"

with_items: "{{ multus_manifest_1.results + (multus_nodes_list|map('extract', hostvars, 'multus_manifest_2.results') | default([]))|list }}"

sorry that didnt work.

"msg": "Failed to template loop_control.label: 'None' has no attribute 'item'. 'None' has no attribute 'item'"

I could not get it to work otherwise, so I added a task after this one to fail if this task resulted with item.name 'None'

i even tried setting

default([{'item': {'name': 'None', 'type': 'None', 'file': 'None'}}]

Hmmm actually I am thinking that the extract might simply not works... Does this works better?

Suggested change

with_items: "{{ multus_manifest_1.results + multus_nodes_list|map('extract', hostvars, 'multus_manifest_2.results')|list }}"

with_items: "{{ multus_manifest_1.results + (multus_nodes_list|map('extract', hostvars, 'multus_manifest_2')|list|json_query('[].results')) }}"

If this works I believe it's the only change that we would really need to have it working consistently...

sorry i cant test locally in the next day or two.... CI is failing too.. will push again later to retry

No worries, thanks for all your OSS works ❤️

`` "msg": "Failed to template loop_control.label: 'ansible.utils.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'item'. 'ansible.utils.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'item'", "skip_reason": "Conditional result was False"} `` fixes case when multus should NOT be included.

floryut

@darkobas2 Thank you for the PR 👍

MrFreezeex

Thanks for the contribution and bearing with me and all the research that went into this! ❤️
/lgtm

k8s-ci-robot · 2023-05-23T08:10:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkobas2, floryut, MrFreezeex

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

`` "msg": "Failed to template loop_control.label: 'ansible.utils.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'item'. 'ansible.utils.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'item'", "skip_reason": "Conditional result was False"} `` fixes case when multus should NOT be included.

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 18, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 18, 2023

k8s-ci-robot requested review from liupeng0518 and yankay May 18, 2023 17:04

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 18, 2023

k8s-ci-robot added size/S Denotes a PR that changes 10-29 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 May 18, 2023

MrFreezeex suggested changes May 18, 2023

View reviewed changes

roles/kubernetes-apps/network_plugin/meta/main.yml Outdated Show resolved Hide resolved

roles/network_plugin/meta/main.yml Outdated Show resolved Hide resolved

roles/kubernetes-apps/network_plugin/multus/tasks/main.yml Outdated Show resolved Hide resolved

MrFreezeex reviewed May 19, 2023

View reviewed changes

yankay mentioned this pull request May 23, 2023

Release Proposal v2.22 #10075

Closed

floryut approved these changes May 23, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2023

MrFreezeex approved these changes May 23, 2023

View reviewed changes

k8s-ci-robot assigned MrFreezeex May 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2023

k8s-ci-robot merged commit 4014a1c into kubernetes-sigs:master May 23, 2023

blackluck mentioned this pull request Jul 20, 2023

multus - mixed type of container engine support commit fails #10305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix multus include #10105

fix multus include #10105

darkobas2 commented May 18, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 18, 2023 •

edited

Loading

k8s-ci-robot commented May 18, 2023

k8s-ci-robot commented May 18, 2023

oomichi commented May 19, 2023

MrFreezeex commented May 19, 2023

darkobas2 commented May 19, 2023

MrFreezeex commented May 19, 2023 •

edited

Loading

darkobas2 commented May 19, 2023

MrFreezeex commented May 19, 2023

MrFreezeex May 19, 2023 •

edited

Loading

darkobas2 May 19, 2023

darkobas May 19, 2023

MrFreezeex May 21, 2023 •

edited

Loading

darkobas2 May 22, 2023

MrFreezeex May 22, 2023

floryut left a comment

MrFreezeex left a comment •

edited

Loading

k8s-ci-robot commented May 23, 2023

	with_items: "{{ multus_manifest_1.results + multus_nodes_list\|map('extract', hostvars, 'multus_manifest_2.results')\|list }}"
	with_items: "{{ multus_manifest_1.results + (multus_nodes_list\|map('extract', hostvars, 'multus_manifest_2.results') \| default([]))\|list }}"

fix multus include #10105

fix multus include #10105

Conversation

darkobas2 commented May 18, 2023 • edited Loading

linux-foundation-easycla bot commented May 18, 2023 • edited Loading

k8s-ci-robot commented May 18, 2023

k8s-ci-robot commented May 18, 2023

oomichi commented May 19, 2023

MrFreezeex commented May 19, 2023

darkobas2 commented May 19, 2023

MrFreezeex commented May 19, 2023 • edited Loading

darkobas2 commented May 19, 2023

MrFreezeex commented May 19, 2023

MrFreezeex May 19, 2023 • edited Loading

Choose a reason for hiding this comment

darkobas2 May 19, 2023

Choose a reason for hiding this comment

darkobas May 19, 2023

Choose a reason for hiding this comment

MrFreezeex May 21, 2023 • edited Loading

Choose a reason for hiding this comment

darkobas2 May 22, 2023

Choose a reason for hiding this comment

MrFreezeex May 22, 2023

Choose a reason for hiding this comment

floryut left a comment

Choose a reason for hiding this comment

MrFreezeex left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented May 23, 2023

darkobas2 commented May 18, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 18, 2023 •

edited

Loading

MrFreezeex commented May 19, 2023 •

edited

Loading

MrFreezeex May 19, 2023 •

edited

Loading

MrFreezeex May 21, 2023 •

edited

Loading

MrFreezeex left a comment •

edited

Loading