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

Updating the prowjob CRD to v1 #21805

Merged
merged 4 commits into from
Apr 26, 2021

Conversation

bradmwilliams
Copy link
Contributor

The prowjob api has been available at v1 for quite some time. This PR bumps the existing CRDs from v1beta1 to v1.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 14, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @bradmwilliams!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @bradmwilliams. Thanks for your PR.

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

@k8s-ci-robot k8s-ci-robot requested review from chases2 and dims April 14, 2021 20:15
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/config Issues or PRs related to code in /config area/prow/bump Updates to the k8s prow cluster sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 14, 2021
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/cc @chaodaiG
At what kube version is the prow.k8s.io cluster? This need 1.16+ : https://www.openshift.com/blog/a-look-into-the-technical-details-of-kubernetes-1-16

🙃

@k8s-ci-robot k8s-ci-robot requested a review from chaodaiG April 14, 2021 20:24
@chaodaiG
Copy link
Contributor

It's already 1.16+, all our prow instances as well, as prometheus operator wants 1.16+ too.
/cc @fejta
Who is oncall

@k8s-ci-robot k8s-ci-robot requested a review from fejta April 14, 2021 20:27
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@k8s-ci-robot k8s-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 Apr 14, 2021
@bradmwilliams
Copy link
Contributor Author

I've signed the CLA

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 14, 2021
@k8s-ci-robot k8s-ci-robot added the area/prow Issues or PRs related to prow label Apr 14, 2021
@bradmwilliams
Copy link
Contributor Author

/retest

@alvaroaleman
Copy link
Member

/retest

Weird, it fails waiting for sinker to delete the pod and prowjob, but sinker does that, according to the log

@bradmwilliams
Copy link
Contributor Author

@alvaroaleman It appears that the issue is the pod is not getting flagged for deletion before the 60 second time limit expires.

@bradmwilliams bradmwilliams force-pushed the prowjobs-crd-bump branch 2 times, most recently from 85f5833 to efa9b34 Compare April 16, 2021 20:01
@bradmwilliams
Copy link
Contributor Author

/retest

@chaodaiG
Copy link
Contributor

/hold
I would be concerned if the integration test passed with so many retries

@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 Apr 19, 2021
@chaodaiG
Copy link
Contributor

Trying to get you unblocked, sorry for being late here. I'm able to reproduce this locally, for some reason sinker is not cleaning the pods with your change, I'll take a deeper look to see what's going on.

What's weird is that sinker logs says it's deleting these pods, but nothing really happened

@chaodaiG
Copy link
Contributor

There were several weird things happening in my local testing:

  • Added a log at
    if k8serrors.IsNotFound(err) {
    , and it always returns k8serrors.IsNotFound(err) for all test cases, including running-pod, orphaned-pod etc.
  • The prowjobs that were not found by sinker were discoverable by kubectl.

Compared prow jobs logs:

And the sinker logs from the artifacts of the spyglass links above:

One noticeable difference is that this log line:

{"component":"sinker","file":"prow/cmd/sinker/main.go:324","func":"main.(*controller).clean","job":"","level":"info","msg":"Deleted prowjob.","name":"max-prow-job-age-0dfcc0aebfc0b5e6adb2e4aa72ed9eb8","severity":"info","time":"2021-04-21T01:41:20Z","type":""}

Only exists in the passed run but not in this PR, feels like sinker is not able to delete prowjob that exceeds max prow job age.

@fejta
Copy link
Contributor

fejta commented Apr 21, 2021

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from fejta April 21, 2021 22:24
@bradmwilliams
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2021
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/label tide/merge-method-squash
/hold cancel
Chao will need to approve though, I am not a root approver

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 23, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2021
@chaodaiG
Copy link
Contributor

LGTM, would defer to @cjwagner who is oncall

@cjwagner
Copy link
Member

LGTM as well, but I'll hold off on merging this until Monday in case we encounter unexpected issues.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, bradmwilliams, cjwagner

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 Apr 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit 354125e into kubernetes:master Apr 26, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Apr 26, 2021
@bradmwilliams bradmwilliams deleted the prowjobs-crd-bump branch April 26, 2021 19:10
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/config Issues or PRs related to code in /config area/prow/bump Updates to the k8s prow cluster area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants