Skip to content

Replacing Jobs with Pods in the building/signing features. #504

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

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

ybettan
Copy link
Contributor

@ybettan ybettan commented Aug 6, 2023

We are not using any of the Job's features and we are reconciling the operator artifacts (jobs/pods) anyway, so it is better to reconcile pods directly.


Fixes #465

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 6, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ybettan

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

@netlify
Copy link

netlify bot commented Aug 6, 2023

Deploy Preview for kubernetes-sigs-kmm ready!

Name Link
🔨 Latest commit afdb6c1
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/64d1f28da3b1520008ad0d87
😎 Deploy Preview https://deploy-preview-504--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 6, 2023
@ybettan
Copy link
Contributor Author

ybettan commented Aug 6, 2023

/cc @yevgeny-shnaidman

@yevgeny-shnaidman @qbarrand @mresvanis Do you think we should also rename the job packages?

@ybettan ybettan force-pushed the jobs-to-pods branch 2 times, most recently from 7f85166 to c163c64 Compare August 6, 2023 14:04
@yevgeny-shnaidman
Copy link
Contributor

/cc @yevgeny-shnaidman

@yevgeny-shnaidman @qbarrand @mresvanis Do you think we should also rename the job packages?

yes, otherwise it is confusing. We should rename job package under build and sign, and rename the "jobhelper" file to "podhelper"

Copy link
Contributor

@qbarrand qbarrand left a comment

Choose a reason for hiding this comment

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

Minor suggestions; lgtm otherwise.

@ybettan ybettan force-pushed the jobs-to-pods branch 2 times, most recently from 0c1a25d to c432517 Compare August 7, 2023 08:19
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 87.50% and project coverage change: -0.11% ⚠️

Comparison is base (37217d6) 81.58% compared to head (15353b0) 81.48%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
- Coverage   81.58%   81.48%   -0.11%     
==========================================
  Files          42       42              
  Lines        3758     3748      -10     
==========================================
- Hits         3066     3054      -12     
- Misses        564      566       +2     
  Partials      128      128              
Files Changed Coverage Δ
api/v1beta1/module_types.go 100.00% <ø> (ø)
controllers/hub/managedclustermodule_reconciler.go 73.56% <0.00%> (ø)
controllers/preflightvalidation_reconciler.go 71.65% <0.00%> (ø)
controllers/module_reconciler.go 85.97% <40.00%> (ø)
internal/build/job/manager.go 76.92% <85.71%> (ø)
internal/build/job/maker.go 90.27% <89.65%> (-0.27%) ⬇️
internal/sign/job/manager.go 87.67% <90.00%> (ø)
internal/sign/job/signer.go 84.11% <90.00%> (-0.37%) ⬇️
internal/utils/jobhelper.go 94.38% <92.00%> (-2.25%) ⬇️
internal/cluster/cluster.go 91.61% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ybettan ybettan force-pushed the jobs-to-pods branch 2 times, most recently from 33d5519 to 0048bba Compare August 7, 2023 08:58
@ybettan
Copy link
Contributor Author

ybettan commented Aug 7, 2023

yes, otherwise it is confusing. We should rename job package under build and sign, and rename the "jobhelper" file to "podhelper"

Done.

@ybettan ybettan force-pushed the jobs-to-pods branch 4 times, most recently from 6fc4379 to 6cb7ee7 Compare August 7, 2023 12:55
@qbarrand
Copy link
Contributor

qbarrand commented Aug 7, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023
@yevgeny-shnaidman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2023
We are not using any of the Job's features and we are reconciling the
operator artifacts (jobs/pods) anyway, so it is better to reconcile pods
directly.

Signed-off-by: Yoni Bettan <[email protected]>
@yevgeny-shnaidman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit 32b7182 into kubernetes-sigs:main Aug 8, 2023
@ybettan ybettan deleted the jobs-to-pods branch August 8, 2023 07:58
qbarrand added a commit to qbarrand/kernel-module-management that referenced this pull request Nov 8, 2023
…netes-sigs#504)

This PR add the following 3 labels, that will be used during ordered
update process:
1) kmm.node.kubernetes.io/version-label-module_<namespace>_<name>
2) kmm.node.kubernetes.io/version-label-module-loader_<namespace>_<name>
3) kmm.node.kubernetes.io/version-label-device-plugin_<namespace>_<name>

The first label is set by the user and signifies on which node which version
should be run. The second and the third labels are managed by KMMO and are used as
node selectors for module loader and device plugin damoensets accordingly.
This PR also include utils functions for generating the full labels' names and
for extracting namespace and name data from labels

Upstream-Commit: a5ebe9b

Co-authored-by: Yevgeny Shnaidman <[email protected]>
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. 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Pods for builds & signing instead of Jobs
5 participants