Skip to content

[release-4.18] nrt: log: return updated generation value for cache #301

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

Conversation

shajmakh
Copy link
Member

cherry-picked from u/s. please check the commits details.

@openshift-ci openshift-ci bot requested review from ffromani and swatisehgal May 21, 2025 15:24
Copy link

openshift-ci bot commented May 21, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shajmakh
Once this PR has been reviewed and has the lgtm label, please assign swatisehgal for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@shajmakh shajmakh force-pushed the update-release-4.18-20250521 branch from d520f0c to 9448d3f Compare May 21, 2025 15:28
@ffromani
Copy link
Member

/hold

I'm not sure the backport is fine - this is on me for unclear instructions

@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 May 22, 2025
@@ -101,17 +101,16 @@ func NewOverReserve(ctx context.Context, lh logr.Logger, cfg *apiconfig.NodeReso
func (ov *OverReserve) GetCachedNRTCopy(ctx context.Context, nodeName string, pod *corev1.Pod) (*topologyv1alpha2.NodeResourceTopology, CachedNRTInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

this is a u/s commit right? if so why it has the [KNI] prefix? https://github.com/openshift-kni/scheduler-plugins/blob/master/RESYNC.md#patching-openshift-kni-specific-commits this is not a d/s specific change (e.g a change which u/s cannot accept or don't care about)

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, I think the verify-commits could use some enhancement to pass on cherry-picks that were authored by the same member. I'll check that.

Copy link
Member

Choose a reason for hiding this comment

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

totally, but is not so easy to implement so this is why it keeps getting deferred. I think you can pass the check by adding a top-level commit updating our RESYNC.log.md. That commit will have the [KNI] prefix so it can pass the checks (and is not wrong either). See: 780ea84

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunatly that didn't work, we have to either update each cherry-picked commit or skip the check completely like we do for resyncs:
#357

Copy link
Member

Choose a reason for hiding this comment

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

ok, let's check what we did in the past

@shajmakh shajmakh force-pushed the update-release-4.18-20250521 branch 4 times, most recently from aabd89a to 049ebc3 Compare June 16, 2025 12:54
k8s-ci-robot and others added 2 commits June 16, 2025 15:55
nrt: log: return updated generation value for cache
track upstream sync by updating the cherrypicked commits in RESYNC.log.md.

Signed-off-by: Shereen Haj <[email protected]>
@shajmakh shajmakh closed this Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants