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.

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

Revert boskos client changes #16259

Closed
wants to merge 2 commits into from

Conversation

alvaroaleman
Copy link
Member

/assign @ixdy
/cc @stevekuznetsov

…cache"

This reverts commit 1668e26, reversing
changes made to 87c0597.
…ctrl-client"

This reverts commit 767a53b, reversing
changes made to b253986.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/boskos Issues or PRs related to code in /boskos sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 12, 2020
@ixdy
Copy link
Member

ixdy commented Feb 12, 2020

/hold

I'd like to see if there's a quick and easy way to roll-forward with the CRD config changes first.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2020
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/hold

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, fejta

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2020
@ixdy
Copy link
Member

ixdy commented Feb 12, 2020

The issue is when we upgrade an existing deployment; the existing CRDs use ResourceCollection and DRLCCollection for the listKinds, while the new code uses ResourceObjectList and DRLCObjectList, and so the caching client fails to initialize properly:

E0212 22:31:57.049418       1 reflector.go:125] external/io_k8s_client_go/tools/cache/reflector.go:98: Failed to list *crds.DRLCObject: no kind "DRLCCollection" is registered for version "boskos.k8s.io/v1" in scheme "pkg/runtime/scheme.go:101"
E0212 22:31:57.803985       1 reflector.go:125] external/io_k8s_client_go/tools/cache/reflector.go:98: Failed to list *crds.ResourceObject: no kind "ResourceCollection" is registered for version "boskos.k8s.io/v1" in scheme "pkg/runtime/scheme.go:101"
{"component":"boskos","error":"timeout waiting for cache sync","file":"boskos/cmd/boskos/boskos.go:96","func":"main.main","level":"fatal","msg":"unable to get client","time":"2020-02-12T22:31:57Z"}

From some simple local experiments, I think it's possible to update the listKinds in the CRD specs without affecting the existing CRs. Maybe it would be worth extracting the CRD specs from the code (into separate YAML specs that can be applied) so that we can easily update the specs at the same time we update the code?

@ixdy
Copy link
Member

ixdy commented Feb 13, 2020

We definitely can't roll forward without #16270, but I think #16267 works otherwise.

@ixdy
Copy link
Member

ixdy commented Feb 13, 2020

/close

We've fixed the issue by pulling the CRDs out of boskos code and then updating and applying them to the cluster.

@k8s-ci-robot
Copy link
Contributor

@ixdy: Closed this PR.

In response to this:

/close

We've fixed the issue by pulling the CRDs out of boskos code and then updating and applying them to the cluster.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2020
@k8s-ci-robot
Copy link
Contributor

@alvaroaleman: PR needs rebase.

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.

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. area/boskos Issues or PRs related to code in /boskos cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants