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

CMP-1097 FIO 1.3.1 bug fix and enhancement #61261

Closed
wants to merge 2 commits into from

Conversation

GroceryBoyJr
Copy link
Contributor

@GroceryBoyJr GroceryBoyJr commented Jun 14, 2023

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 14, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jun 14, 2023

🤖 Updated build preview is available at:
https://61261--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/21775

@GroceryBoyJr
Copy link
Contributor Author

/label hold

@openshift-ci
Copy link

openshift-ci bot commented Jun 14, 2023

@GroceryBoyJr: The label(s) /label hold cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, rebase/manual, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, cnv, dev-tools, distributed-tracing, ims, jira/valid-bug, merge-review-in-progress, merge-review-needed, mtc, multi-arch, oadp, peer-review-done, peer-review-in-progress, peer-review-needed, rhacs, rhv, sd-docs, serverless, service-mesh, sme-review-done, sme-review-needed, staff-eng-approved, telco. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label hold

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.

Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

Looks like a great start! I have a few comments.

@GroceryBoyJr GroceryBoyJr force-pushed the CMP-2039 branch 2 times, most recently from 9fd14a6 to 26a0e3b Compare June 15, 2023 17:12
Copy link

@Vincent056 Vincent056 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jun 15, 2023
@GroceryBoyJr GroceryBoyJr force-pushed the CMP-2039 branch 2 times, most recently from 05e16a1 to 7ec8011 Compare June 15, 2023 18:27
@GroceryBoyJr
Copy link
Contributor Author

@xiaojiey PTAL

Copy link
Contributor

@rhmdnd rhmdnd left a comment

Choose a reason for hiding this comment

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

/lgtm

@GroceryBoyJr
Copy link
Contributor Author

GroceryBoyJr commented Jun 16, 2023 via email

@sheriff-rh
Copy link
Contributor

@xiaojiey PTAL at FIO as well please, thanks!

@GroceryBoyJr GroceryBoyJr changed the title CMP-1097 FIO 1.3.0 bug fix and enhancement CMP-1097 FIO 1.3.1 bug fix and enhancement Aug 9, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023
@openshift-ci
Copy link

openshift-ci bot commented Aug 9, 2023

New changes are detected. LGTM label has been removed.

@GroceryBoyJr
Copy link
Contributor Author

Hello @xiaojiey or @wangke19 seeking your QE LGTM. PTAL? TY!

@GroceryBoyJr
Copy link
Contributor Author

@rhmdnd your SME approval requested please. PTAL, TY!

Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

LGTM, adding peer review done label for now.

@sheriff-rh sheriff-rh added the peer-review-done Signifies that the peer review team has reviewed this PR label Aug 9, 2023

* Previously, FIO would not clean up node status CRDs when nodes are removed from the cluster. FIO would also erroneously indicate that new nodes failed integrity checks. FIO has been updated to correctly clean up node status CRDs on node removal or when adding new nodes to the cluster. This provides correct node status notifications. (link:https://issues.redhat.com/browse/OCPBUGS-8502[*OCPBUGS-8502*])

* Previously, when FIO was reconciling File Integrity CRDs, it would pause scanning until the reconciliation was done. This caused an overly aggressive re-initiatization process on nodes not impacted by the reconciliation. This problem also resulted in unnecessary daemonsets for machine config pools which are unrelated to the file integrity being changed. FIO correctly handles these cases and only pauses AIDE scanning for nodes that are affected by file integrity changes. (link:https://issues.redhat.com/browse/CMP-1097[*CMP-1097*])

Choose a reason for hiding this comment

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

Better to update "reconciling File Integrity CRDs" to " FileIntegrity CRDs"

Choose a reason for hiding this comment

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

I was wondering if you could change "Previously, when FIO was reconciling File Integrity CRDs, it would pause scanning until the reconciliation was done. " to something like "Previously, during a node updates causing by MCP updates, it would pause scanning on all nodes until the update was done."

@xiaojiey
Copy link

One more comment, could you please address https://issues.redhat.com/browse/OCPBUGS-8502? Thanks.

@sheriff-rh
Copy link
Contributor

@GroceryBoyJr is out of the office between now and the release date. I will be closing this PR in favor of #63648 because I added a known issues section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.10 branch/enterprise-4.11 branch/enterprise-4.12 branch/enterprise-4.13 branch/enterprise-4.14 peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants