Skip to content

ovirt e2e: fix nodeport issue on CI bug 1794714 #7614

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.

Sign up for GitHub

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

Closed
Gal-Zaidman wants to merge 1 commit into from
Closed

ovirt e2e: fix nodeport issue on CI bug 1794714 #7614

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions ci-operator/templates/openshift/installer/cluster-launch-installer-ovirt-e2e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,10 @@ objects:
# This can be used if we just want to check the installer exits 0
echo "WARNING: No tests were run against the installed cluster"
}

${TEST_COMMAND}

# Runs an install
- name: setup
# A midstep till we have the installer work merged, then we
# can use the CI artifact
Copy link
Member

@wking wking Mar 18, 2020

Choose a reason for hiding this comment

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

Looks like this is your comment from #4340. Can you explain (ideally in the commit message), why you're removing it here?

Copy link
Contributor Author

@Gal-Zaidman Gal-Zaidman Mar 18, 2020

Choose a reason for hiding this comment

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

I will edit the commit message

image: ${IMAGE_INSTALLER}
terminationMessagePolicy: FallbackToLogsOnError
volumeMounts:
Expand Down Expand Up @@ -416,9 +413,9 @@ objects:
function update_image_registry() {
while true; do
sleep 10;
oc get configs.imageregistry.operator.openshift.io/cluster > /dev/null && break
oc --insecure-skip-tls-verify get configs.imageregistry.operator.openshift.io/cluster > /dev/null && break
done
oc patch configs.imageregistry.operator.openshift.io cluster --type merge --patch '{"spec":{"managementState":"Managed","storage":{"emptyDir":{}}}}'
oc --insecure-skip-tls-verify patch configs.imageregistry.operator.openshift.io cluster --type merge --patch '{"spec":{"managementState":"Managed","storage":{"emptyDir":{}}}}'
Copy link
Member

@wking wking Mar 18, 2020

Choose a reason for hiding this comment

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

Why are you adding these? Don't we have a kubeconfig with the CA for the Kube API? If that CA doesn't match what the cluster serves us, I'd rather error out here instead of ignoring the mismatch.

Copy link
Contributor Author

@Gal-Zaidman Gal-Zaidman Mar 18, 2020

Choose a reason for hiding this comment

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

+1
I will remove it, it worked fine before.
I have added it because for some reason the "get nodes" fail for me locally without it.

}

#change the masters igntion , to use tempfs for etcd IOPS optimization
Expand All @@ -441,7 +438,11 @@ objects:
wait "$!"
install_exit_status=$?
sleep 10m
oc get co/image-registry
# This is a workaround for https://bugzilla.redhat.com/show_bug.cgi?id=1794714
for n in $(oc --insecure-skip-tls-verify get nodes|awk '{print $1}'|grep worker);do
Copy link
Member

@wking wking Mar 18, 2020

Choose a reason for hiding this comment

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

I don't like operating on the table output of get nodes. Can you use -o jsonpath=... to have it spit out the field you want?

Copy link
Contributor Author

@Gal-Zaidman Gal-Zaidman Mar 18, 2020

Choose a reason for hiding this comment

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

sure didn't know the option, I will try it

oc -n default --insecure-skip-tls-verify debug node/$n --image=centos/tools -- ethtool --offload vxlan_sys_4789 tx off
Copy link
Member

@wking wking Mar 18, 2020

Choose a reason for hiding this comment

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

I feel like this is the sort of change that should be handled by a MachineConfig, although I don't know what you'd put in the config to apply this specific ethtool tweak. Dropping into a debug session on each node and bumping things yourself seems very brittle.

Copy link
Contributor Author

@Gal-Zaidman Gal-Zaidman Mar 18, 2020

Choose a reason for hiding this comment

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

When we talked about it we said that we don't want to apply it by default on a cluster because it is a tweak that disables checksum and it seems to us like a decision that we don't want by default, plus it is a high priority bug which affects most platforms, happens only when using OpenshiftSDN , and has network people working on it.
We wanted to apply the fix in the CI because it made network test flaky during conformance.

done
oc --insecure-skip-tls-verify get co/image-registry
exit $install_exit_status

# Performs cleanup of all created resources
Expand Down