Fix bug and change cri selection procedure of cluster-sync #2155

jcanocan · 2022-11-23T15:42:20Z

The command for getting the registry port gets more than one output, which breaks following commands in the deploying scripts. The output is now parsed to pickup just the first port.

Also the container runtime selection was hardcoded, now it uses the cri selection script of kubervirtci.

Further details can be found at issue 2153

Fixes #2153

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Release note:

Fix make cluster-sync issue and changes the cri selection procedure

kubevirt-bot · 2022-11-23T15:42:29Z

Hi @jcanocan. Thanks for your PR.

I'm waiting for a kubevirt 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.

openshift-ci · 2022-11-23T15:42:31Z

Hi @jcanocan. Thanks for your PR.

I'm waiting for a kubevirt 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.

jcanocan · 2022-11-23T15:43:00Z

/cc @tiraboschi

0xFelix · 2022-11-23T15:46:39Z

cluster/sync.sh

@@ -1,15 +1,15 @@
 #!/bin/bash -ex

 source ./hack/common.sh
+source ${PWD}/_kubevirtci/cluster-up/cluster/ephemeral-provider-common.sh


Use same path style as above? . instead of ${PWD}?

0xFelix · 2022-11-23T15:46:58Z

cluster/sync.sh

-container_command=podman
-
-registry_port=$(${container_command} ps | grep -Po '(?<=0.0.0.0:)\d+(?=->5000\/tcp)' || echo "")
+container_command=$_cri_bin


Why not replace container_command with _cri_bin everywhere?

Makes sense for me too.

0xFelix · 2022-11-23T15:48:35Z

cluster/sync.sh


+registry_port=$(${container_command} ps | grep -Po '(?<=0.0.0.0:)\d+(?=->5000\/tcp)' | head -n 1)


Did you check if the regex works for both podman and docker?

Yes, I just checked it.

The command for getting the registry port gets more than one output, which breaks following commands in the deploying scripts. The output is now parsed to pickup just the first port. Also the container runtime selection was hardcoded, now it uses the cri selection script of kubervirtci. Co-authored-by: Felix Matouschek <fmatouschek@redhat.com> Signed-off-by: Javier Cano Cano <jcanocan@redhat.com>

sonarcloud · 2022-11-23T16:23:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

0xFelix · 2022-11-24T15:38:40Z

/lgtm

tiraboschi · 2022-11-25T08:56:40Z

/ok-to-test

coveralls · 2022-11-25T09:02:03Z

Pull Request Test Coverage Report for Build 3533846405

0 of 0 changed or added relevant lines in 0 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 85.419%

Files with Coverage Reduction	New Missed Lines	%
controllers/operands/operandHandler.go	1	84.34%

Totals
Change from base Build 3495979789:	-0.02%
Covered Lines:	4634
Relevant Lines:	5425

💛 - Coveralls

nunnatsa · 2022-11-28T10:31:55Z

Coverage reduction seems not related to this PR, because it does not change code nor unit tests.

/override coverage/coveralls
/approve

kubevirt-bot · 2022-11-28T10:31:57Z

@nunnatsa: Overrode contexts on behalf of nunnatsa: coverage/coveralls

In response to this:

Coverage reduction seems not related to this PR, because it does not change code nor unit tests.

/override coverage/coveralls
/approve

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.

kubevirt-bot · 2022-11-28T10:32:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nunnatsa

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~~ [nunnatsa]

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

…#2155) The command for getting the registry port gets more than one output, which breaks following commands in the deploying scripts. The output is now parsed to pickup just the first port. Also the container runtime selection was hardcoded, now it uses the cri selection script of kubervirtci. Co-authored-by: Felix Matouschek <fmatouschek@redhat.com> Signed-off-by: Javier Cano Cano <jcanocan@redhat.com> Signed-off-by: Javier Cano Cano <jcanocan@redhat.com> Co-authored-by: Felix Matouschek <fmatouschek@redhat.com>

* [release-1.8] Align github actions from newer branches Backport a few configuration changes in github actions. Signed-off-by: stirabos <stirabos@redhat.com> * Fix bug and change cri selection procedure of cluster-sync. (#2155) The command for getting the registry port gets more than one output, which breaks following commands in the deploying scripts. The output is now parsed to pickup just the first port. Also the container runtime selection was hardcoded, now it uses the cri selection script of kubervirtci. Co-authored-by: Felix Matouschek <fmatouschek@redhat.com> Signed-off-by: Javier Cano Cano <jcanocan@redhat.com> Signed-off-by: Javier Cano Cano <jcanocan@redhat.com> Co-authored-by: Felix Matouschek <fmatouschek@redhat.com> * cluster-up: Fix tag updating (#2189) In case the _kubevirtci folder exists, changing the tag won't reclone the folder. The cluster-up folder won't be updated and it can lead to bugs in case the folder is changed. Fix it by deleting the folder in case of tag mismatch. It will enforce a reclone. Signed-off-by: Or Shoval <oshoval@redhat.com> Signed-off-by: Or Shoval <oshoval@redhat.com> * Bump the default kubevirtci provider Signed-off-by: stirabos <stirabos@redhat.com> * Workaround for SELinux boolean for chardev Workaround for kubevirt/kubevirt#9434 we started setting SELinux boolean for chardev access with kubevirt/kubevirtci#968 but it got reverted with kubevirt/kubevirtci#975 altough it's still needed with Kubevirt v0.58.1 TODO: let's remove this once kubevirt/kubevirt#9434 is fixed Signed-off-by: stirabos <stirabos@redhat.com> --------- Signed-off-by: stirabos <stirabos@redhat.com> Signed-off-by: Javier Cano Cano <jcanocan@redhat.com> Signed-off-by: Or Shoval <oshoval@redhat.com> Co-authored-by: Javier Cano Cano <jcanocan@redhat.com> Co-authored-by: Felix Matouschek <fmatouschek@redhat.com> Co-authored-by: oscollabus <oshoval@redhat.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 23, 2022

kubevirt-bot requested review from nunnatsa and sradco November 23, 2022 15:42

kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 23, 2022

kubevirt-bot added the size/S label Nov 23, 2022

kubevirt-bot requested a review from tiraboschi November 23, 2022 15:43

0xFelix reviewed Nov 23, 2022

View reviewed changes

jcanocan force-pushed the fix-cluster-sync branch from c5dd3ec to e2b19e2 Compare November 23, 2022 16:22

kubevirt-bot assigned 0xFelix Nov 24, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 24, 2022

kubevirt-bot 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 25, 2022

nunnatsa approved these changes Nov 28, 2022

View reviewed changes

kubevirt-bot assigned nunnatsa Nov 28, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2022

kubevirt-bot merged commit 09fd80e into kubevirt:main Nov 28, 2022

xpivarc mentioned this pull request Jan 23, 2023

Add jcanocan tot the Kubevirt org kubevirt/project-infra#2565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug and change cri selection procedure of cluster-sync #2155

Fix bug and change cri selection procedure of cluster-sync #2155

jcanocan commented Nov 23, 2022 •

edited by nunnatsa

Loading

kubevirt-bot commented Nov 23, 2022

openshift-ci bot commented Nov 23, 2022

jcanocan commented Nov 23, 2022

0xFelix Nov 23, 2022

jcanocan Nov 23, 2022

0xFelix Nov 23, 2022

jcanocan Nov 23, 2022

0xFelix Nov 23, 2022

jcanocan Nov 23, 2022

sonarcloud bot commented Nov 23, 2022

0xFelix commented Nov 24, 2022

tiraboschi commented Nov 25, 2022

coveralls commented Nov 25, 2022

nunnatsa commented Nov 28, 2022

kubevirt-bot commented Nov 28, 2022

kubevirt-bot commented Nov 28, 2022


		registry_port=$(${container_command} ps \| grep -Po '(?<=0.0.0.0:)\d+(?=->5000\/tcp)' \| head -n 1)

Fix bug and change cri selection procedure of cluster-sync #2155

Fix bug and change cri selection procedure of cluster-sync #2155

Conversation

jcanocan commented Nov 23, 2022 • edited by nunnatsa Loading

kubevirt-bot commented Nov 23, 2022

openshift-ci bot commented Nov 23, 2022

jcanocan commented Nov 23, 2022

0xFelix Nov 23, 2022

Choose a reason for hiding this comment

jcanocan Nov 23, 2022

Choose a reason for hiding this comment

0xFelix Nov 23, 2022

Choose a reason for hiding this comment

jcanocan Nov 23, 2022

Choose a reason for hiding this comment

0xFelix Nov 23, 2022

Choose a reason for hiding this comment

jcanocan Nov 23, 2022

Choose a reason for hiding this comment

sonarcloud bot commented Nov 23, 2022

0xFelix commented Nov 24, 2022

tiraboschi commented Nov 25, 2022

coveralls commented Nov 25, 2022

Pull Request Test Coverage Report for Build 3533846405

💛 - Coveralls

nunnatsa commented Nov 28, 2022

kubevirt-bot commented Nov 28, 2022

kubevirt-bot commented Nov 28, 2022

jcanocan commented Nov 23, 2022 •

edited by nunnatsa

Loading