k8snetworkplumbingwg / ovs-cni

Open vSwitch CNI plugin
Apache License 2.0
224 stars 71 forks source link

Fix make cluster-sync failure with latest kubevirtci #204

Closed masap closed 3 years ago

masap commented 3 years ago

make cluster-sync fails with continuous deletion confirmation process.

++ ./cluster/kubectl.sh get --ignore-not-found -f ./examples/ovs-cni.yml
++ wc -l
+ [[ 1 -eq 0 ]]
+ sleep 1
++ ./cluster/kubectl.sh get --ignore-not-found -f ./examples/ovs-cni.yml
++ wc -l
+ [[ 1 -eq 0 ]]
+ sleep 1
...

This process expects ./cluster/kubectl.sh get --ignore-not-found -f ./examples/ovs-cni.yml returns no line. But currently it returns "selecting docker as container runtime". So suppress this message by specifying container runtime with KUBEVIRTCI_RUNTIME.

Signed-off-by: Masashi Honma masashi.honma@gmail.com

kubevirt-bot commented 3 years ago

Hi @masap. Thanks for your PR.

I'm waiting for a k8snetworkplumbingwg 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
masap commented 3 years ago

/release-note-none

phoracek commented 3 years ago

Odd, I have tried this locally and the output it still "" without the message about Docker/Podman.

/ok-to-test

masap commented 3 years ago

@phoracek On the CI result, it appears to be returning the expected value.

+ expecting_message='selecting docker as container runtime'
++ ./cluster/kubectl.sh get --ignore-not-found -f ./examples/ovs-cni.yml
+ [[ selecting docker as container runtime = selecting docker as container runtime ]]
++ ./cluster/kubectl.sh get --ignore-not-found ds ovs-cni-plugin-amd64
+ [[ selecting docker as container runtime = selecting docker as container runtime ]]
++ ./cluster/kubectl.sh get --ignore-not-found ds ovs-vsctl-amd64
+ [[ selecting docker as container runtime = selecting docker as container runtime ]]
phoracek commented 3 years ago

My bad, I have not deleted the previous _kubevirtci directory. Now I can reproduce it. Waiting for this specific message seems little awkward though. Would it make sense to instead perform wc only on stdout? Currently we do it across both stdout and stderr, the docker messages is I believe taken from stderr (https://github.com/kubevirt/kubevirtci/blob/1ad0cbd6f8621e0d8583c921581960a46155098b/cluster-up/cluster/ephemeral-provider-common.sh#L31). What do you think?

masap commented 3 years ago

@phoracek I do not like to watch only stdout because I would like to ensure the stderr does not show error message other than that...

The message selecting docker as container runtime is shown by container runtime auto detection functionality. I fixed this PR to avoid the message by specifying container runtime with KUBEVIRTCI_RUNTIME.

phoracek commented 3 years ago

Then maybe we could check that the return code is 0 and stdout is empty?

My worry with the proposed approach is that it favors docker and does not leverage the container runtime auto-detection mechanism: https://github.com/kubevirt/kubevirtci/blob/1ad0cbd6f8621e0d8583c921581960a46155098b/cluster-up/cluster/ephemeral-provider-common.sh#L24, and so it may be a regression for people using Podman.

masap commented 3 years ago

Ok, fixed.

kubevirt-bot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: masap, phoracek

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](https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/OWNERS)~~ [phoracek] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment