Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NO-JIRA: network, virt: Retry virtctl console cmd on err #29520

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented Feb 3, 2025

There is a flaky on ovn-kubernetes kubevirt live migration conformance test where there is the error "expecter: No process running", this means that the command "virtctl console -n [namespace]" has die before sending login command so the golang expecter fails.

This change do a retry on that error and use a debug option to print the check status.

There is a flaky on ovn-kubernetes kubevirt live migration conformance
test where there is the error "expecter: No process running", this means
that the command "virtctl console -n [namespace]" has die before sending
login command so the golang expecter fails.

This change do a retry on that error and use a debug option to print the
check status.

Signed-off-by: Enrique Llorente <[email protected]>
@openshift-ci openshift-ci bot requested review from knobunc and trozet February 3, 2025 09:32
@qinqon
Copy link
Contributor Author

qinqon commented Feb 3, 2025

/test e2e-aws-ovn-virt-techpreview

@qinqon qinqon changed the title network, virt: Retry virtctl console cmd on err NO_JIRA: network, virt: Retry virtctl console cmd on err Feb 3, 2025
@qinqon qinqon changed the title NO_JIRA: network, virt: Retry virtctl console cmd on err NO-JIRA: network, virt: Retry virtctl console cmd on err Feb 3, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 3, 2025
@openshift-ci-robot
Copy link

@qinqon: This pull request explicitly references no jira issue.

In response to this:

There is a flaky on ovn-kubernetes kubevirt live migration conformance test where there is the error "expecter: No process running", this means that the command "virtctl console -n [namespace]" has die before sending login command so the golang expecter fails.

This change do a retry on that error and use a debug option to print the check status.

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 openshift-eng/jira-lifecycle-plugin repository.

@qinqon
Copy link
Contributor Author

qinqon commented Feb 3, 2025

/jira refresh

@openshift-ci-robot
Copy link

@qinqon: This pull request explicitly references no jira issue.

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice !

I think re-connecting the console when the process is not available, then using it will address the flakes we see now and then about this feature.

And the code looks good.

Huge thanks.

Comment on lines +195 to +199
err := retry.OnError(retry.DefaultBackoff,
func(err error) bool {
return strings.Contains(err.Error(), "expect: Process not running")
},
func() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
err := retry.OnError(retry.DefaultBackoff,
func(err error) bool {
return strings.Contains(err.Error(), "expect: Process not running")
},
func() error {
err := retry.OnError(
retry.DefaultBackoff,
func(err error) bool {
return strings.Contains(err.Error(), "expect: Process not running")
},
func() error {

expecter, _, err := newExpecter(virtctlPath, vmiNamespace, vmiName, consoleConnectionTimeout,
expect.Verbose(true),
expect.VerboseWriter(GinkgoWriter),
expect.DebugCheck(log.New(GinkgoWriter, "expect: virtctl console check", 0 /* no flags */)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do @qinqon ? Just log that we have to re-try establishing the connection ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It runs the check that of "virtctl command" running and prints it and return back the result, so if it fails we will se the stdout of the virtcl command itself.

@maiqueb
Copy link
Contributor

maiqueb commented Feb 3, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
Copy link
Contributor

openshift-ci bot commented Feb 3, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maiqueb, qinqon
Once this PR has been reviewed and has the lgtm label, please assign trozet for approval. For more information see the 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:

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

@qinqon
Copy link
Contributor Author

qinqon commented Feb 3, 2025

/retest-required

@knobunc
Copy link
Contributor

knobunc commented Feb 3, 2025

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Feb 3, 2025
@qinqon
Copy link
Contributor Author

qinqon commented Feb 3, 2025

/cherr-pick release-4.18

Copy link
Contributor

openshift-ci bot commented Feb 12, 2025

@qinqon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openstack-ovn 43c2a97 link false /test e2e-openstack-ovn
ci/prow/e2e-gcp-ovn-rt-upgrade 43c2a97 link false /test e2e-gcp-ovn-rt-upgrade
ci/prow/e2e-aws-ovn-virt-techpreview 43c2a97 link false /test e2e-aws-ovn-virt-techpreview
ci/prow/e2e-metal-ipi-ovn-ipv6 43c2a97 link true /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-aws-ovn-single-node-serial 43c2a97 link false /test e2e-aws-ovn-single-node-serial
ci/prow/e2e-aws-ovn-single-node-upgrade 43c2a97 link false /test e2e-aws-ovn-single-node-upgrade
ci/prow/okd-scos-e2e-aws-ovn 43c2a97 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-vsphere-ovn-upi 43c2a97 link true /test e2e-vsphere-ovn-upi

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants