search

openshift / cluster-node-tuning-operator

Manage node-level tuning by orchestrating the tuned daemon.
Apache License 2.0
102 stars 105 forks source link

OCPBUGS-28647: deferred updates: cleanups #1119

Closed ffromani closed 4 months ago

ffromani commented 4 months ago

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

openshift-ci[bot] commented 4 months ago

[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: - ~~[OWNERS](https://github.com/openshift/cluster-node-tuning-operator/blob/master/OWNERS)~~ [ffromani] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
jmencak commented 4 months ago

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)
ffromani commented 4 months ago

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 commented 4 months ago

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

Tal-or commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

/hold

incomplete PR

ffromani commented 4 months ago

/hold cancel

xref: https://github.com/openshift/release/blob/master/ci-operator/config/openshift/cluster-node-tuning-operator/openshift-cluster-node-tuning-operator-master.yaml#L119

Tal-or commented 4 months ago

/lgtm Thanks!

ffromani commented 4 months ago

/hold

ffromani commented 4 months ago

/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
ffromani commented 4 months ago

/retest

Tal-or commented 4 months ago

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

ffromani commented 4 months ago

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 commented 4 months ago

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

Tal-or commented 4 months ago

/lgtm Thank you for the fixes

ffromani commented 4 months ago

thank you for the careful review! low caffeine intake today

jmencak commented 4 months ago

New changes are detected. LGTM label has been removed.

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

/lgtm

openshift-ci-robot commented 4 months ago

@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](https://github.com/openshift/cluster-node-tuning-operator/pull/1119): >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](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
ffromani commented 4 months ago

/jira refresh

openshift-ci-robot commented 4 months ago

@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](https://github.com/openshift/cluster-node-tuning-operator/pull/1119#issuecomment-2245638655): >/jira refresh Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 4 months ago

@ffromani: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-ci-robot commented 4 months ago

@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](https://github.com/openshift/cluster-node-tuning-operator/pull/1119): >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](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-node-tuning-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-bot commented 4 months ago

[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.