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.

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

Jump to bottom

vendor: Bump OpenShift API to Add Alibaba Platform #5216

Merged
merged 1 commit into from
Sep 29, 2021
Merged

vendor: Bump OpenShift API to Add Alibaba Platform #5216

merged 1 commit into from
Sep 29, 2021

Conversation

patrickdillon
Copy link
Contributor

@patrickdillon patrickdillon commented Sep 15, 2021 •
edited
Loading 

go mod edit -require=github.com/openshift/api@master \
            -replace=k8s.io/client-go=k8s.io/[email protected] \
            -replace=k8s.io/kube-openapi=k8s.io/[email protected]

Bumps the OpenShift & k8s APIs. There seems to be a breaking change
in kube-openshift which breaks our kubevirt.io dependencies. I'm pinning
this to the earlier kube-openapi until these dependencies can be
updated.

cc @kwoodson

I believe this is a functional solution. I'm marking this as a work in progress as I am still looking into this breaking change from the kube-openapi, which were introduced, I believe in kubernetes/kube-openapi#234

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2021
@patrickdillon
Copy link
Contributor Author

patrickdillon commented Sep 15, 2021 •
edited
Loading

@sttts I wonder if you could take a look, as you were involved with the upstream PRs:

It seems like the upstream API change to remove go-openapi, kubernetes/kube-openapi#234 has broken one of our dependencies, kubervirt/containerized-data-importer.

When I update the API & client-go

$ go mod edit -require=github.com/openshift/api@master -replace=k8s.io/client-go=k8s.io/[email protected]

I start getting compilation errors all similar to this:

# kubevirt.io/containerized-data-importer/pkg/apis/core/v1alpha1
vendor/kubevirt.io/containerized-data-importer/pkg/apis/core/v1alpha1/openapi_generated.go:56:3: cannot use "github.com/go-openapi/spec".Schema{...} (type "github.com/go-openapi/spec".Schema) as type "k8s.io/kube-openapi/pkg/validation/spec".Schema in field value

So it seems this API change and removal of go-openapi is causing a failure in this generated code here as it is attempting to use go-openapi instead of kube-openapi: https://github.com/kubevirt/containerized-data-importer/blob/release-v1.12/pkg/apis/core/v1alpha1/openapi_generated.go#L26

Are issues like this expected? I have resolved the issue in this PR by replacing the kube-openapi version with one that is compatible with our dependency. Presumably this will be pinned until the dependency is fixed (the installer does not directly depend on kube-openapi). Do you think this is an acceptable fix or have any further suggestions?

@patrickdillon
Copy link
Contributor Author

/retitle vendor: Bump OpenShift API to Add Alibaba Platform

Removing this from WIP as it seems like this is an accepted way to deal with these dependencies. @jhixson74 PTAL

I do need to add a comment in the code explaining the dependency

Replaces #5198

@openshift-ci openshift-ci bot changed the title [wip] vendor: Bump OpenShift API to Add Alibaba Platform vendor: Bump OpenShift API to Add Alibaba Platform Sep 20, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2021
@kwoodson kwoodson mentioned this pull request Sep 20, 2021
Closed
@kwoodson
Copy link
Contributor

@patrickdillon Thanks for the help with this. I was able to build the Alibaba PR #5018 on top of this PR and get the code to compile.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2021
@clnperez clnperez mentioned this pull request Sep 20, 2021
Closed
@Karthik-K-N Karthik-K-N mentioned this pull request Sep 21, 2021
Merged
@kwoodson
Copy link
Contributor

/rerun

@patrickdillon
Copy link
Contributor Author

/retest

2 similar comments
@rna-afk
Copy link
Contributor

rna-afk commented Sep 21, 2021

/retest

@fabianofranz
Copy link
Member

/retest

@staebler
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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:

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 Sep 21, 2021
@fabianofranz
Copy link
Member

/retest

@kwoodson
Copy link
Contributor

@patrickdillon
Test e2e-aws failed with this. This seems unrelated to this PR.

could not resolve inputs: could not determine inputs for step [input:rhel-7]: could not resolve base image from ocp/4.10:base-7: imagestreamtags.image.openshift.io "4.10:base-7" not found

Test okd-unit failed with similar message but I believe the test might be out-of-date?

could not resolve inputs: could not determine inputs for step [input:rhel-7]: could not resolve base image from ocp/4.9:base-7: imagestreamtags.image.openshift.io "4.9:base-7" not found

There must be an issue with the base-7 image tag.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@patrickdillon
Copy link
Contributor Author

/hold
need to look at failing okd-unit test

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2021
@patrickdillon
Copy link
Contributor Author

/test okd-unit

@kwoodson kwoodson mentioned this pull request Sep 27, 2021
Merged
@kwoodson
Copy link
Contributor

@patrickdillon Looks like a vendor issue in the okd unit test?

vendor/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go:21:2: cannot find package "." in:
	/go/src/github.com/openshift/installer/vendor/io/fs
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-09-27T15:46:16Z"}
2/3 Tests Passed!
expand_more

@patrickdillon
Copy link
Contributor Author

@patrickdillon Looks like a vendor issue in the okd unit test?

vendor/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go:21:2: cannot find package "." in:
	/go/src/github.com/openshift/installer/vendor/io/fs
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-09-27T15:46:16Z"}
2/3 Tests Passed!
expand_more

Correct but I don't understand the issue. Why is it only failing on the okd-unit test? It looks like this okd-unit test is using a different image with go 1-14 but I'm not that familiar with these tests.

@patrickdillon
Copy link
Contributor Author

@patrickdillon Looks like a vendor issue in the okd unit test?

vendor/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics.go:21:2: cannot find package "." in:
	/go/src/github.com/openshift/installer/vendor/io/fs
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-09-27T15:46:16Z"}
2/3 Tests Passed!
expand_more

Correct but I don't understand the issue. Why is it only failing on the okd-unit test? It looks like this okd-unit test is using a different image with go 1-14 but I'm not that familiar with these tests.

Looks like that package may require go 1-16, so maybe we need to update the okd-unit images

@patrickdillon
Copy link
Contributor Author

/test e2e-azurestack

@patrickdillon
Copy link
Contributor Author

/test e2e-azurestack-upi

2 similar comments
@patrickdillon
Copy link
Contributor Author

/test e2e-azurestack-upi

@patrickdillon
Copy link
Contributor Author

/test e2e-azurestack-upi

@Lyt99 Lyt99 mentioned this pull request Sep 28, 2021
Merged
@fabianofranz
Copy link
Member

/test e2e-azurestack-upi

@patrickdillon
Copy link
Contributor Author

let's retry this once openshift/release#22357 lands

@patrickdillon
Copy link
Contributor Author

/hold cancel
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2021 •
edited
Loading

@patrickdillon: 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 f7f93ba link false /test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel7 f7f93ba link false /test e2e-aws-workers-rhel7
ci/prow/e2e-ovirt f7f93ba link false /test e2e-ovirt
ci/prow/e2e-aws-workers-rhel8 f7f93ba link false /test e2e-aws-workers-rhel8
ci/prow/e2e-libvirt f7f93ba link false /test e2e-libvirt
ci/prow/e2e-crc f7f93ba link false /test e2e-crc
ci/prow/e2e-azurestack f7f93ba link false /test e2e-azurestack
ci/prow/e2e-azurestack-upi f7f93ba link false /test e2e-azurestack-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/test-infra repository. I understand the commands that are listed here.

@patrickdillon
Copy link
Contributor Author

/test okd-unit

@patrickdillon
Copy link
Contributor Author

/skip

@openshift-merge-robot openshift-merge-robot merged commit 9e844a7 into openshift:master Sep 29, 2021
@patrickdillon patrickdillon mentioned this pull request Oct 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e-tienne e-tienne Awaiting requested review from e-tienne

@rna-afk rna-afk Awaiting requested review from rna-afk

Assignees

@kwoodson kwoodson

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@patrickdillon @kwoodson @rna-afk @fabianofranz @staebler @openshift-bot @clnperez @openshift-merge-robot