Skip to content

Fix TestHostNetworkEndpointPublishingStrategy#738

Closed
frobware wants to merge 3 commits intoopenshift:masterfrom
frobware:temporary-fix-for-TestHostNetworkEndpointPublishingStrategy
Closed

Fix TestHostNetworkEndpointPublishingStrategy#738
frobware wants to merge 3 commits intoopenshift:masterfrom
frobware:temporary-fix-for-TestHostNetworkEndpointPublishingStrategy

Conversation

@frobware
Copy link
Contributor

The API change in openshift/api#1097 does not mark the set of configurable ports on type HostNetworkStrategy as optional. It's not clear whether that was deliberate and/or intended but without specifying these ports the e2e test TestHostNetworkEndpointPublishingStrategy() will always fail.

The e2e test `TestHostNetworkEndpointPublishingStrategy` is always
failing. We directly need openshift/api#1097
but I bumped openshift/api to current latest.

go get -u github.com/openshift/api@c3bb724c282a1ac34d7c769da99543a453ae90b9
go mod vendor
go mod tidy
The API change in openshift/api#1097 means
that the set of configurable ports on type HostNetworkStrategy are not
marked as optional. It's not clear whether that was deliberate but
without specifying these the e2e tests will always fail.
@openshift-ci openshift-ci bot requested review from gcs278 and lmzuccarelli April 12, 2022 11:07
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2022
@frobware
Copy link
Contributor Author

error: error creating buildah builder: initializing source docker://image-registry.openshift-image-registry.svc:5000/ci-op-c2k6zvk7/pipeline@sha256:30ea73ec25783d6c04b6648a2e2ae60022f1c5f71bd296e2c4e84b9de840841a: pinging container registry image-registry.openshift-image-registry.svc:5000: Get "https://image-registry.openshift-image-registry.svc:5000/v2/": dial tcp: i/o timeout

/retest

@frobware
Copy link
Contributor Author

cc @arjunrn

@arjunrn
Copy link
Contributor

arjunrn commented Apr 12, 2022

@frobware I'm not sure I understand the cause of the failures. This repo does not contain the changes I made in openshift/api. So the CRD being created in the test cluster will not have the new fields. Secondly the ports are optional implicitly IIUC. Only fields which are listed in required are actually required.

cc @alebedev87

@arjunrn
Copy link
Contributor

arjunrn commented Apr 12, 2022

Anyway, there is some defaulting for the values for the controller when it's not present(due to version upgrades) in my PR #694. If that's the cause of the issue then the defaulting should help.

@frobware
Copy link
Contributor Author

Anyway, there is some defaulting for the values for the controller when it's not present(due to version upgrades) in my PR #694. If that's the cause of the issue then the defaulting should help.

Without +optional and ,omitempty we don't get any defaults.

@frobware
Copy link
Contributor Author

@frobware I'm not sure I understand the cause of the failures. This repo does not contain the changes I made in openshift/api.

This is a good point. I currently have stacked API changes. I was trying to understand why my PR #735 was failing in CI. I have bumped to include my changes, but that will pick up yours too.

@Miciah
Copy link
Contributor

Miciah commented Apr 12, 2022

spec.endpointPublishingStrategy.hostNetwork is an optional pointer-type field, so I wouldn't expect omitting the field entirely to be a problem. Do you have an error message from a CI failure with a Create or Update call that fails because of the missing values?

@Miciah
Copy link
Contributor

Miciah commented Apr 12, 2022

TestHostNetworkEndpointPublishingStrategy did not fail in the last e2e-aws-operator run.

/test e2e-aws-operator

@frobware frobware closed this Apr 12, 2022
@frobware
Copy link
Contributor Author

2022-04-12T16:46:23.561+0100	INFO	operator.ingress_controller	controller/controller.go:114	reconciling	{"request": "openshift-ingress-operator/host"}
2022-04-12T16:46:24.259+0100	ERROR	operator.init.controller.ingress_controller	controller/controller.go:266	Reconciler error	{"name": "host", "namespace": "openshift-ingress-operator", "error": "failed to admit ingresscontroller: failed to update status: IngressController.operator.openshift.io \"host\" is invalid: [status.endpointPublishingStrategy.hostNetwork.httpsPort: Invalid value: 0: status.endpointPublishingStrategy.hostNetwork.httpsPort in body should be greater than or equal to 1, status.endpointPublishingStrategy.hostNetwork.statsPort: Invalid value: 0: status.endpointPublishingStrategy.hostNetwork.statsPort in body should be greater than or equal to 1, status.endpointPublishingStrategy.hostNetwork.httpPort: Invalid value: 0: status.endpointPublishingStrategy.hostNetwork.httpPort in body should be greater than or equal to 1]"}

This is the error I see when creating the following:

apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
  creationTimestamp: "2022-04-12T15:14:20Z"
  finalizers:
  - ingresscontroller.operator.openshift.io/finalizer-ingresscontroller
  generation: 1
  name: host
  namespace: openshift-ingress-operator
  resourceVersion: "105051"
  uid: 93e19982-6366-4345-a1fe-d3cadbd8d712
spec:
  clientTLS:
    clientCA:
      name: ""
    clientCertificatePolicy: ""
  domain: foo.host.amcdermo-2022-04-12-1246.devcluster.openshift.com
  endpointPublishingStrategy:
    type: HostNetwork
    hostNetwork:

which is, as I understand things, the reason why the test fails because we don't currently specify any values for the 3 ports.

@frobware frobware reopened this Apr 12, 2022
@frobware
Copy link
Contributor Author

@arjunrn In dff1967#diff-0c9271ea6402ecb6233e9e0a629cd72156e12bb7a2c41ef99f0d0265394d7ec4R391 we add default values if they are not specified. Why don't we allow 0 in the API and get the defaults from there?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 12, 2022

@frobware: 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-aws-single-node fdb19f1 link false /test e2e-aws-single-node
ci/prow/e2e-upgrade fdb19f1 link true /test e2e-upgrade

Full PR test history. Your PR dashboard.

Details

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.

@frobware
Copy link
Contributor Author

Obsoleted by openshift/api#1175.

@frobware frobware closed this Apr 12, 2022
@frobware frobware deleted the temporary-fix-for-TestHostNetworkEndpointPublishingStrategy branch September 5, 2022 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants