fix ptp labels to skip virtual machines #122

yuvalk · 2020-03-22T08:29:51Z

this is currently done by manually pre-labeling those with 'virtual-machine'

Signed-off-by: Yuval Kashtan yuvalkashtan@gmail.com

in our env this will be done by cluster deployment

note: I tried to use ironic introspection data, but that is included only for workers ...

openshift-ci-robot · 2020-03-22T08:29:59Z

Hi @yuvalk. Thanks for your PR.

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

cynepco3hahue · 2020-03-22T09:35:47Z

/ok-to-test

yanirq · 2020-03-22T10:09:35Z

@yuvalk is this visible enough for others to know that they need to manual label a machine with virtualmachine ? (or at least consider it) maybe worth changing to a variable that defaults to virtual machine but you can change it (exporting etc..) ?

yuvalk · 2020-03-22T11:02:26Z

@yuvalk is this visible enough for others to know that they need to manual label a machine with virtualmachine ? (or at least consider it) maybe worth changing to a variable that defaults to virtual machine but you can change it (exporting etc..) ?

I'll add a variable
maybe also worth mentioning in the readme

hack/setup-test-cluster.sh

yuvalk · 2020-03-22T14:49:19Z

@SchSeba - tnx, was going to ask you to take a look

slintes

Good idea to add a README 👍
I have some comments about it though

hack/README.md

slintes · 2020-03-23T07:37:22Z

hack/README.md

+The explanations and descriptions of these sciprts are helpful for contributors.
+
+## setup-test-cluster.sh
+This script does all the CNF installation parts that are expected to be done by users, beside installation related ones. like labeling nodes and creating needed MachineConfiguration object, etc.


This is a bit confusing IMHO: "it does all the installation parts beside installation".
What about:
This script does all the cluster setup parts that are expected to be done by admins, like labelling nodes and creating the MachineConfigPool resource, etc.

I wanted to convey that it does not install CNF just do the pre/post setup
still not sure this is good enough

maybe use your phrasing and add after the dot. "It does not install CNF itself"

hack/README.md

yuvalk · 2020-03-23T08:52:14Z

Good idea to add a README +1
I have some comments about it though

tnx for reading,
fixed.

slintes · 2020-03-23T09:50:21Z

hack/setup-test-cluster.sh

@@ -15,11 +17,11 @@ do
 done

 echo "[INFO]: Labeling first node as the ptp grandmaster"
-node=$(${OC_TOOL} get nodes -o name | sed -n 1p)
+node=$(${OC_TOOL} get nodes -o name --selector '!${NON_PTP_LABEL}' | sed -n 1p)


'!${NON_PTP_LABEL}' -> the single quotes prevent using the actual value, please use double quotes. See failure in e2e test.

slintes · 2020-03-23T09:50:28Z

hack/setup-test-cluster.sh

 ${OC_TOOL} label --overwrite $node ptp/grandmaster=""

 echo "[INFO]: Labeling all the other nodes as ptp slaves"
-nodes=$(${OC_TOOL} get nodes -o name | sed 1d)
+nodes=$(${OC_TOOL} get nodes -o name --selector '!${NON_PTP_LABEL}' | sed 1d)


this is currently done by manually pre-labeling those with 'virtual-machine' Signed-off-by: Yuval Kashtan <yuvalkashtan@gmail.com>

with reference to PTP setup environment variable Signed-off-by: Yuval Kashtan <yuvalkashtan@gmail.com>

slintes · 2020-03-23T13:29:50Z

/retest

openshift-ci-robot · 2020-03-23T14:18:08Z

@yuvalk: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-origin	`881bac8`	link	`/test e2e-gcp-origin`
ci/prow/e2e-gcp-ovn	`881bac8`	link	`/test e2e-gcp-ovn`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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

slintes

/approve

openshift-ci-robot · 2020-03-23T15:44:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SchSeba, slintes, yuvalk

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

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

yuvalk · 2020-03-23T19:03:12Z

/cherrypick release-4.4

openshift-cherrypick-robot · 2020-03-23T19:03:19Z

@yuvalk: only openshift-kni org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherrypick release-4.4

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.

yuvalk · 2020-03-23T19:04:16Z

@yuvalk: only openshift-kni org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherrypick release-4.4

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.

@slintes - that shouldnt have happened, now that I am a member. can you check?

yuvalk · 2020-03-23T19:05:47Z

maybe it take some time for the cherrypick robot to pick new members up or something?

slintes · 2020-03-23T20:15:57Z

maybe it take some time for the cherrypick robot to pick new members up or something?

you need to make your membership public on https://github.com/orgs/openshift-kni/people

yuvalk · 2020-03-23T21:18:47Z

/cherrypick release-4.4

openshift-cherrypick-robot · 2020-03-23T21:18:53Z

@yuvalk: new pull request created: #126

In response to this:

/cherrypick release-4.4

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-robot requested review from cynepco3hahue and yanirq March 22, 2020 08:29

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 22, 2020

openshift-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 Mar 22, 2020

yuvalk force-pushed the fix_setup_labeling branch from 663d97b to be87dc0 Compare March 22, 2020 11:00

yuvalk force-pushed the fix_setup_labeling branch 2 times, most recently from bfcdc2e to 7c8ba89 Compare March 22, 2020 11:18

yanirq reviewed Mar 22, 2020

View reviewed changes

hack/setup-test-cluster.sh Outdated Show resolved Hide resolved

yuvalk force-pushed the fix_setup_labeling branch 2 times, most recently from 5922340 to d86c143 Compare March 22, 2020 12:09

SchSeba approved these changes Mar 22, 2020

View reviewed changes

openshift-ci-robot assigned SchSeba Mar 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2020

slintes requested changes Mar 23, 2020

View reviewed changes

openshift-ci-robot assigned slintes Mar 23, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020

yuvalk force-pushed the fix_setup_labeling branch from d86c143 to 68a0c38 Compare March 23, 2020 08:52

slintes requested changes Mar 23, 2020

View reviewed changes

yuvalk added 2 commits March 23, 2020 13:51

fix ptp labels to skip virtual machines

9965b3c

this is currently done by manually pre-labeling those with 'virtual-machine' Signed-off-by: Yuval Kashtan <yuvalkashtan@gmail.com>

add hack/README.md

881bac8

with reference to PTP setup environment variable Signed-off-by: Yuval Kashtan <yuvalkashtan@gmail.com>

yuvalk force-pushed the fix_setup_labeling branch from 68a0c38 to 881bac8 Compare March 23, 2020 11:51

slintes approved these changes Mar 23, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2020

openshift-merge-robot merged commit 8e278f0 into openshift-kni:master Mar 23, 2020

openshift-cherrypick-robot mentioned this pull request Mar 23, 2020

[release-4.4] fix ptp labels to skip virtual machines #126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ptp labels to skip virtual machines #122

fix ptp labels to skip virtual machines #122

yuvalk commented Mar 22, 2020

openshift-ci-robot commented Mar 22, 2020

cynepco3hahue commented Mar 22, 2020

yanirq commented Mar 22, 2020

yuvalk commented Mar 22, 2020

yuvalk commented Mar 22, 2020

slintes left a comment

slintes Mar 23, 2020

yuvalk Mar 23, 2020

yuvalk Mar 23, 2020

yuvalk commented Mar 23, 2020

slintes Mar 23, 2020

slintes Mar 23, 2020

slintes commented Mar 23, 2020

openshift-ci-robot commented Mar 23, 2020 •

edited

Loading

slintes left a comment

openshift-ci-robot commented Mar 23, 2020

yuvalk commented Mar 23, 2020

openshift-cherrypick-robot commented Mar 23, 2020

yuvalk commented Mar 23, 2020

yuvalk commented Mar 23, 2020

slintes commented Mar 23, 2020

yuvalk commented Mar 23, 2020

openshift-cherrypick-robot commented Mar 23, 2020

fix ptp labels to skip virtual machines #122

fix ptp labels to skip virtual machines #122

Conversation

yuvalk commented Mar 22, 2020

openshift-ci-robot commented Mar 22, 2020

cynepco3hahue commented Mar 22, 2020

yanirq commented Mar 22, 2020

yuvalk commented Mar 22, 2020

yuvalk commented Mar 22, 2020

slintes left a comment

Choose a reason for hiding this comment

slintes Mar 23, 2020

Choose a reason for hiding this comment

yuvalk Mar 23, 2020

Choose a reason for hiding this comment

yuvalk Mar 23, 2020

Choose a reason for hiding this comment

yuvalk commented Mar 23, 2020

slintes Mar 23, 2020

Choose a reason for hiding this comment

slintes Mar 23, 2020

Choose a reason for hiding this comment

slintes commented Mar 23, 2020

openshift-ci-robot commented Mar 23, 2020 • edited Loading

slintes left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 23, 2020

yuvalk commented Mar 23, 2020

openshift-cherrypick-robot commented Mar 23, 2020

yuvalk commented Mar 23, 2020

yuvalk commented Mar 23, 2020

slintes commented Mar 23, 2020

yuvalk commented Mar 23, 2020

openshift-cherrypick-robot commented Mar 23, 2020

openshift-ci-robot commented Mar 23, 2020 •

edited

Loading