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

plugins: Introduce phased plugin #3103

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Conversation

oshoval
Copy link
Contributor

@oshoval oshoval commented Dec 4, 2023

Phased plugin allows running jobs that will be considered as "phase 2"
only once both lgtm / approve are presented.

We are running 3 k8s versions for each sig on kubevirt, for each PR.
With this plugin we can declare the non latest one as "phase 2"
and run it only when the PR is entering the merge queue,
This will save CI resources.

A job will be declared as "phase 2", if it has always_run=false, optional=false,
without additional conditions such as run_if_changed, skip_if_only_changed.

Since the jobs are always_run=false it means that a potential race condition might happen
between running the job and merging.
For this we use require_manually_triggered_jobs which will block the PR entering
the merge queue / being merge by tide before all "phase 2" jobs finish.
It will also cause tide to auto run phase 2 when rerunning batches.

Notes:

  • Doesn't support branches, since for branches we consider everything as stable phase 1.
    A follow-up PR will update the job forking to update phase 2 to be considered phase 1 on new branches.
  • Works only for kubevirt/kubevirt.
  • pull-kubevirt-e2e-k8s-1.27-ipv6-sig-network was chosen as phase 2 candid.
  • Assuming both lgtm / approve were not added when PR was draft (else the plugin won't kick
    in once undrafted).

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Dec 4, 2023
@oshoval
Copy link
Contributor Author

oshoval commented Dec 4, 2023

/cc @brianmcarey @EdDev @dhiller

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2023
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2023
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2023
Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Thanks @oshoval for this - it could be very useful. I just have a couple of questions inline.

@oshoval
Copy link
Contributor Author

oshoval commented Dec 12, 2023

Rebased only

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023
@oshoval oshoval force-pushed the phased_pr branch 2 times, most recently from 77b285e to 08df60d Compare December 12, 2023 08:41
@oshoval
Copy link
Contributor Author

oshoval commented Dec 12, 2023

Addressed comments, post submit is as bot-review now
and built latest image (updated manifest)
Thanks

@oshoval
Copy link
Contributor Author

oshoval commented Dec 12, 2023

Removed branch support

on a follow PR we should update new branch forking to change phase 2 to be phase 1 for branches

@oshoval
Copy link
Contributor Author

oshoval commented Dec 12, 2023

pull-project-infra-prow-deploy-test fails
i dont think its my PR ? it passed before small changes / rebase
or is it ?

@oshoval
Copy link
Contributor Author

oshoval commented Dec 12, 2023

/retest

@oshoval
Copy link
Contributor Author

oshoval commented Dec 12, 2023

worked now, there was a flake twice today

@oshoval oshoval force-pushed the phased_pr branch 2 times, most recently from d623253 to 5a8b17c Compare December 13, 2023 08:23
@oshoval
Copy link
Contributor Author

oshoval commented Dec 13, 2023

first push - cosmetics: remove unused code
2nd push - rebase

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2024
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2024
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

One thing I missed: you need to enable this plugin in the external_plugins config like here:

@oshoval
Copy link
Contributor Author

oshoval commented Jan 14, 2024

One thing I missed: you need to enable this plugin in the external_plugins config like here:

It is here isn't it ?
https://github.com/kubevirt/project-infra/pull/3103/files#diff-3a088ffbe61dd30f930cd1855ab72a652fcea4a33c4b047eeb6df38f7a79ad51R531

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2024
@oshoval oshoval force-pushed the phased_pr branch 2 times, most recently from 8794293 to 0883ae5 Compare January 14, 2024 10:55
@oshoval
Copy link
Contributor Author

oshoval commented Jan 14, 2024

Pushed (separated):

  • addressed the marked comments (commented on the rest) , added few tests cases, and refactor as table with some BeforeEach so it can be updated easier if needed
  • rebased
  • updated image

Thanks

/hold cancel
(we can do the rest in a follow up as mentioned)

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2024
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

Sorry, I overlooked the external plugin misconfiguration in first pass. That needs to get fixed.

Other than than just a spelling mistake.

Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval oshoval force-pushed the phased_pr branch 2 times, most recently from b04789a to f5b1287 Compare January 15, 2024 11:48
@oshoval
Copy link
Contributor Author

oshoval commented Jan 15, 2024

Thanks

fixed comments
rebased
updated image

Signed-off-by: Or Shoval <oshoval@redhat.com>
… main

Signed-off-by: Or Shoval <oshoval@redhat.com>
Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval
Copy link
Contributor Author

oshoval commented Jan 15, 2024

created kubevirt/kubevirt for the plugin

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2024
@kubevirt-bot kubevirt-bot merged commit 0c8c659 into kubevirt:main Jan 15, 2024
7 checks passed
@kubevirt-bot
Copy link
Contributor

@oshoval: Updated the following 3 configmaps:

  • job-config configmap in namespace kubevirt-prow at cluster default using the following files:
    • key kubevirt-presubmits.yaml using file github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml
    • key project-infra-postsubmits.yaml using file github/ci/prow-deploy/files/jobs/kubevirt/project-infra/project-infra-postsubmits.yaml
  • config configmap in namespace kubevirt-prow at cluster default using the following files:
    • key config.yaml using file github/ci/prow-deploy/kustom/base/configs/current/config/config.yaml
  • plugins configmap in namespace kubevirt-prow at cluster default using the following files:
    • key plugins.yaml using file github/ci/prow-deploy/kustom/base/configs/current/plugins/plugins.yaml

In response to this:

Phased plugin allows running jobs that will be considered as "phase 2"
only once both lgtm / approve are presented.

We are running 3 k8s versions for each sig on kubevirt, for each PR.
With this plugin we can declare the non latest one as "phase 2"
and run it only when the PR is entering the merge queue,
This will save CI resources.

A job will be declared as "phase 2", if it has always_run=false, optional=false,
without additional conditions such as run_if_changed, skip_if_only_changed.

Since the jobs are always_run=false it means that a potential race condition might happen
between running the job and merging.
For this we use require_manually_triggered_jobs which will block the PR entering
the merge queue / being merge by tide before all "phase 2" jobs finish.
It will also cause tide to auto run phase 2 when rerunning batches.

Notes:

  • Doesn't support branches, since for branches we consider everything as stable phase 1.
    A follow-up PR will update the job forking to update phase 2 to be considered phase 1 on new branches.
  • Works only for kubevirt/kubevirt.
  • pull-kubevirt-e2e-k8s-1.27-ipv6-sig-network was chosen as phase 2 candid.

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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants