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

OCPBUGS-28647: deferred updates: cleanups #1119

Merged

Conversation

ffromani
Copy link
Contributor

@ffromani ffromani commented Jul 23, 2024

cleanups suggested late in the #1019 cycle or emerged after the first nightly CI run

@openshift-ci openshift-ci bot requested review from jmencak and yanirq July 23, 2024 06:00
Copy link
Contributor

openshift-ci bot commented Jul 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2024
@ffromani ffromani force-pushed the tuned-deferred-updates-fixes branch from e34363c to 113da9f Compare July 23, 2024 06:00
@jmencak
Copy link
Contributor

jmencak commented Jul 23, 2024

Thank you for the cleanup. This looks good to me, but letting other reviewers comment. Only one super-nit:

diff --git a/pkg/tuned/controller.go b/pkg/tuned/controller.go
index cce3334a..561a60dc 100644
--- a/pkg/tuned/controller.go
+++ b/pkg/tuned/controller.go
@@ -403,7 +403,7 @@ type ExtractedProfiles struct {
 
 // ProfilesExtract extracts TuneD daemon profiles to tunedProfilesDirCustom directory.
 // Returns:
-//   - ExtractedProfile with the details of the operation performed
+//   - ExtractedProfiles with the details of the operation performed.
 //   - Error if any or nil.
 func ProfilesExtract(profiles []tunedv1.TunedProfile, recommendedProfile string) (ExtractedProfiles, error) {
 	klog.Infof("profilesExtract(): extracting %d TuneD profiles (recommended=%s)", len(profiles), recommendedProfile)

xref: openshift#1019 (comment)

Signed-off-by: Francesco Romani <fromani@redhat.com>
Simplify the flow aligning code to the left.
No intended changes in behavior.

xref: openshift#1019 (comment)

Signed-off-by: Francesco Romani <fromani@redhat.com>
Simplify the code with no intended changes in behavior.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani ffromani force-pushed the tuned-deferred-updates-fixes branch from 113da9f to d480083 Compare July 23, 2024 06:38
@ffromani
Copy link
Contributor Author

Thank you for the cleanup. This looks good to me, but letting other reviewers comment. Only one super-nit:

diff --git a/pkg/tuned/controller.go b/pkg/tuned/controller.go
index cce3334a..561a60dc 100644
--- a/pkg/tuned/controller.go
+++ b/pkg/tuned/controller.go
@@ -403,7 +403,7 @@ type ExtractedProfiles struct {
 
 // ProfilesExtract extracts TuneD daemon profiles to tunedProfilesDirCustom directory.
 // Returns:
-//   - ExtractedProfile with the details of the operation performed
+//   - ExtractedProfiles with the details of the operation performed.
 //   - Error if any or nil.
 func ProfilesExtract(profiles []tunedv1.TunedProfile, recommendedProfile string) (ExtractedProfiles, error) {
 	klog.Infof("profilesExtract(): extracting %d TuneD profiles (recommended=%s)", len(profiles), recommendedProfile)

thanks, fixed

@ffromani
Copy link
Contributor Author

/cc @Tal-or
as he did most of the comments I'm addressing here

@openshift-ci openshift-ci bot requested a review from Tal-or July 23, 2024 06:38
test/e2e/deferred/restart.go Outdated Show resolved Hide resolved
test/e2e/deferred/restart.go Outdated Show resolved Hide resolved
@ffromani ffromani force-pushed the tuned-deferred-updates-fixes branch from d480083 to 9794263 Compare July 23, 2024 07:51
@Tal-or
Copy link
Contributor

Tal-or commented Jul 23, 2024

@ffromani I don't see where the actual skip is done?
We should have something like this:
https://github.com/openshift/cluster-node-tuning-operator/blob/master/Makefile#L260C135-L260C162
Am I missing something here?

@ffromani
Copy link
Contributor Author

@ffromani I don't see where the actual skip is done? We should have something like this: https://github.com/openshift/cluster-node-tuning-operator/blob/master/Makefile#L260C135-L260C162 Am I missing something here?

because is not yet :) I'll still need to do the skipping part consuming the labels

@ffromani
Copy link
Contributor Author

/hold

incomplete PR

@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 Jul 23, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2024
@Tal-or
Copy link
Contributor

Tal-or commented Jul 23, 2024

/lgtm
Thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@ffromani
Copy link
Contributor Author

/hold

@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 Jul 23, 2024
@ffromani ffromani force-pushed the tuned-deferred-updates-fixes branch from 9afed2e to 897339c Compare July 23, 2024 09:06
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@ffromani
Copy link
Contributor Author

/unhold

before the latest update:

$ make test-e2e
for d in deferred; do \
  KUBERNETES_CONFIG="/home/fromani/clusters/cnflab/kubeconfig" GOOS=linux GO111MODULE=on GOFLAGS=-mod=vendor go test -v -timeout 40m ./test/e2e/$d -ginkgo.v -ginkgo.no-color -ginkgo.fail-fast -ginkgo.label-filter=\"!flaky\" || exit; \
done
=== RUN   TestNodeTuningOperatorDeferred
Ginkgo detected configuration issues:
Syntax Error Parsing Label Filter
  "!flaky"
  Invalid token '!'.

  Learn more at: http://onsi.github.io/ginkgo/#spec-labels
FAIL	github.com/openshift/cluster-node-tuning-operator/test/e2e/deferred	0.021s
FAIL
make: *** [Makefile:103: test-e2e] Error 1

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2024
@ffromani ffromani force-pushed the tuned-deferred-updates-fixes branch from 897339c to e4ce506 Compare July 23, 2024 09:07
@ffromani
Copy link
Contributor Author

/retest

@Tal-or
Copy link
Contributor

Tal-or commented Jul 23, 2024

The label is Flaky (begin with capital letter) but the filter is !flaky (small letter).
Are you sure it would match the label?

@ffromani
Copy link
Contributor Author

The label is Flaky (begin with capital letter) but the filter is !flaky (small letter). Are you sure it would match the label?

good catch. Partial update. Fixing.

@ffromani
Copy link
Contributor Author

uhm, kube standard is to use Capitalized nouns, but the rest of the tests is using lowercase-dash-separated

and update the tags to match. This way we can easily
skip the flaky tests

The naming schema is made consistent with performanceprofile
labels: test/e2e/performanceprofile/functests/utils/label/label.go

note the recommended kube model is to use `CapitalizedNouns`,
while we do `lowercase-dash-separated`.

Signed-off-by: Francesco Romani <fromani@redhat.com>
nightlies call `make test-e2e` so skip the
flaky tests here. Presubmit lanes (= running pre-merge)
should still run all tests

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani ffromani force-pushed the tuned-deferred-updates-fixes branch from e4ce506 to 0413618 Compare July 23, 2024 11:38
@Tal-or
Copy link
Contributor

Tal-or commented Jul 23, 2024

/lgtm
Thank you for the fixes

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@ffromani
Copy link
Contributor Author

thank you for the careful review! low caffeine intake today

Fix the runtime directory to be forward compatible
xref: openshift#1019 (comment)

Signed-off-by: Francesco Romani <fromani@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@jmencak
Copy link
Contributor

jmencak commented Jul 23, 2024

New changes are detected. LGTM label has been removed.

Thank you @ffromani , the last change was requested by me.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@ffromani ffromani changed the title deferred updates: cleanups OCPBUGS-28647: deferred updates: cleanups Jul 23, 2024
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Jul 23, 2024
@openshift-ci-robot
Copy link
Contributor

@ffromani: This pull request references Jira Issue OCPBUGS-28647, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.17.0) matches configured target version for branch (4.17.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @shajmakh

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

cleanups suggested late in the #1019 cycle or emerged after the first nightly CI run

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 openshift-eng/jira-lifecycle-plugin repository.

@ffromani
Copy link
Contributor Author

/jira refresh

@openshift-ci openshift-ci bot requested a review from shajmakh July 23, 2024 16:00
@openshift-ci-robot
Copy link
Contributor

@ffromani: This pull request references Jira Issue OCPBUGS-28647, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.17.0) matches configured target version for branch (4.17.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @shajmakh

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Jul 23, 2024

@ffromani: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit a869242 into openshift:master Jul 23, 2024
16 checks passed
@openshift-ci-robot
Copy link
Contributor

@ffromani: Jira Issue OCPBUGS-28647: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-28647 has not been moved to the MODIFIED state.

In response to this:

cleanups suggested late in the #1019 cycle or emerged after the first nightly CI run

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 openshift-eng/jira-lifecycle-plugin repository.

@ffromani ffromani deleted the tuned-deferred-updates-fixes branch July 23, 2024 17:01
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: cluster-node-tuning-operator
This PR has been included in build cluster-node-tuning-operator-container-v4.18.0-202407232010.p0.ga869242.assembly.stream.el9.
All builds following this will include this PR.

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. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants